feat(review): render a branch review as a local HTML page

pkgmgr review writes one self-contained HTML page for a repository: commits ahead of the base, the staged, unstaged and untracked diff per file, and the checks of the change set. Repository text reaches the packaged Jinja template only as autoescaped data, git runs in scan mode (now also without replace objects), and the page is written atomically through mkstemp and os.replace.

🎯 Fix: 92 %
💥 Break: 3 %
💸 Cost: new
⏱️ Time: new
🛡️ Security: 100 %
🏆 Score: 89
🚦 Gate: cost new, time new: cleared by the operator's commit word for C (2026-11-07)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
Kevin Veen-BirkenbachandClaude Opus 5.5 committed 2026-11-07 10:39:52 +00:00
1 parent 54f0cbb37b
commit 1b31b10df6
20 files changed
+1949 -2

No files matched your search

+237
View File
@@ -0,0 +1,237 @@
from __future__ import annotations
import io
import os
import subprocess
import tempfile
import unittest
from contextlib import redirect_stderr, redirect_stdout
from pathlib import Path
from unittest.mock import patch
from urllib.parse import quote
from pkgmgr.actions.review import ReviewError, write_review
from pkgmgr.cli.commands.review import handle_review
from pkgmgr.cli.parser import create_parser
GIT_ENV = {
**os.environ,
"GIT_AUTHOR_NAME": "t",
"GIT_AUTHOR_EMAIL": "t@example.org",
"GIT_COMMITTER_NAME": "t",
"GIT_COMMITTER_EMAIL": "t@example.org",
}
COMMIT_MESSAGE = (
'feat: <script>alert("subject")</script>\n\nBody <script>alert("body")</script>'
)
def git(cwd: str, *args: str) -> str:
return subprocess.run(
["git", "-c", "commit.gpgsign=false", *args],
cwd=cwd,
env=GIT_ENV,
check=True,
capture_output=True,
text=True,
).stdout.strip()
def write(path: str, content: bytes) -> None:
os.makedirs(os.path.dirname(path), exist_ok=True)
with open(path, "wb") as handle:
handle.write(content)
class TestReview(unittest.TestCase):
def setUp(self) -> None:
self._tmp = tempfile.TemporaryDirectory()
self.root = os.path.realpath(self._tmp.name)
self.repo = os.path.join(self.root, "my repo")
git(self.root, "init", "-b", "main", self.repo)
app = "".join(f"line {n}\n" for n in range(1, 21))
write(self._path("app.py"), app.encode())
write(self._path("notes.md"), b"# Notes\n")
git(self.repo, "add", "-A")
git(self.repo, "commit", "-m", "init")
git(self.repo, "checkout", "-b", "feature")
write(self._path("notes.md"), b"# Notes\nfeature\n")
git(self.repo, "commit", "-am", COMMIT_MESSAGE)
write(self._path("app.py"), app.replace("line 5\n", "line five\n").encode())
big = "".join(f"big {n}\n" for n in range(1, 31))
write(self._path("dir with space", "big.txt"), big.encode())
write(self._path("image.png"), b"\x89PNG\x00\x01\x02")
git(self.repo, "mv", "notes.md", "renamed.md")
git(self.repo, "add", "-A")
unstaged = app.replace("line 5\n", "line five\n")
write(
self._path("app.py"), unstaged.replace("line 15\n", "line 15!\n").encode()
)
write(
self._path("todo.txt"),
b"first\n<script>alert('file')</script>\nlast\n",
)
write(self._path("blob.bin"), b"\x00\xffdata")
self.note = os.path.join(self.root, "score.txt")
write(self.note, b"<b>score</b> 90/5\n")
def tearDown(self) -> None:
self._tmp.cleanup()
def _path(self, *parts: str) -> str:
return os.path.join(self.repo, *parts)
def _render(self, out: str, base: str | None, notes: list[str]) -> str:
path = write_review(
cwd=self.repo,
out=os.path.join(self.root, "out", out),
max_file_lines=10,
base=base,
notes=notes,
)
return Path(path).read_text(encoding="utf-8")
def _link(self, rel: str, line: int) -> str:
return f"vscode://file{quote(os.path.join(self.repo, rel))}:{line}"
def test_sections_and_counts(self) -> None:
html = self._render("page.html", None, [])
self.assertIn(
'id="staged" data-files="4" data-added="31" data-removed="1"', html
)
self.assertIn(
'id="unstaged" data-files="1" data-added="1" data-removed="1"', html
)
self.assertIn(
'id="untracked" data-files="2" data-added="3" data-removed="0"', html
)
self.assertNotIn('id="commits"', html)
self.assertIn(
'notes.md</span><span class="arrow"> &rarr; </span>renamed.md', html
)
self.assertIn('<span class="tag">binary</span>', html)
self.assertIn('data-path="blob.bin"', html)
self.assertIn("Binary file, contents not shown.", html)
self.assertIn(git(self.repo, "write-tree"), html)
self.assertIn(git(self.repo, "rev-parse", "--short", "HEAD"), html)
def test_base_adds_commits_section(self) -> None:
html = self._render("page.html", "main", [])
self.assertIn(
'id="commits" data-commits="1" data-files="1" data-added="1" '
'data-removed="0"',
html,
)
self.assertIn("t &lt;t@example.org&gt;", html)
self.assertIn(git(self.repo, "rev-parse", "--short", "feature"), html)
def test_untrusted_text_is_escaped(self) -> None:
html = self._render("page.html", "main", [self.note])
self.assertEqual(html.count("<script"), 1)
self.assertNotIn("<script>alert", html)
self.assertIn("&lt;script&gt;alert(&#39;file&#39;)&lt;/script&gt;", html)
self.assertIn("feat: &lt;script&gt;alert(&#34;subject&#34;)", html)
self.assertIn("Body &lt;script&gt;alert(&#34;body&#34;)", html)
self.assertIn("<figcaption>score.txt</figcaption>", html)
self.assertIn("&lt;b&gt;score&lt;/b&gt; 90/5", html)
def test_rendering_is_deterministic(self) -> None:
first = self._render("one.html", "main", [self.note])
second = self._render("two.html", "main", [self.note])
self.assertEqual(first.encode("utf-8"), second.encode("utf-8"))
def test_vscode_links_and_truncation(self) -> None:
html = self._render("page.html", None, [])
self.assertIn(f'class="open" href="{self._link("app.py", 5)}"', html)
self.assertIn(f'class="open" href="{self._link("app.py", 15)}"', html)
self.assertIn(f'<a href="{self._link("app.py", 5)}">5</a>', html)
self.assertIn("my%20repo/dir%20with%20space/big.txt:1", html)
self.assertIn("big 10</td>", html)
self.assertNotIn("big 11</td>", html)
self.assertIn("20 more lines not shown", html)
def test_errors_exit_two(self) -> None:
plain = os.path.join(self.root, "plain")
os.makedirs(plain)
with (
patch.dict(os.environ, {"GIT_CEILING_DIRECTORIES": self.root}),
self.assertRaisesRegex(ReviewError, "not inside a git working copy"),
):
write_review(cwd=plain, out="x.html", max_file_lines=5, base=None, notes=[])
with self.assertRaisesRegex(ReviewError, "unknown base ref"):
write_review(
cwd=self.repo,
out="x.html",
max_file_lines=5,
base="no-such-ref",
notes=[],
)
args = create_parser("test").parse_args(
["review", "--out", "x.html", "--max-file-lines", "5", "--base", "nope"]
)
stderr = io.StringIO()
with (
patch("pkgmgr.cli.commands.review.os.getcwd", return_value=self.repo),
redirect_stderr(stderr),
self.assertRaises(SystemExit) as exit_ctx,
):
handle_review(args, ctx=None)
self.assertEqual(exit_ctx.exception.code, 2)
self.assertEqual(len(stderr.getvalue().splitlines()), 1)
self.assertFalse(os.path.exists(self._path("x.html")))
def test_cli_forwards_every_option(self) -> None:
args = create_parser("test").parse_args(
[
"review",
"--out",
"page.html",
"--max-file-lines",
"50",
"--base",
"origin/main",
"--note",
"score.txt",
"--note",
"message.txt",
]
)
target = os.path.join(self.root, "page.html")
stdout = io.StringIO()
with (
patch("pkgmgr.cli.commands.review.os.getcwd", return_value=self.repo),
patch(
"pkgmgr.cli.commands.review.write_review", return_value=target
) as run_review,
redirect_stdout(stdout),
):
handle_review(args, ctx=None)
run_review.assert_called_once_with(
cwd=self.repo,
out="page.html",
max_file_lines=50,
base="origin/main",
notes=["score.txt", "message.txt"],
)
self.assertEqual(stdout.getvalue(), f"file://{target}\n")
bare = create_parser("test").parse_args(
["review", "--out", "p.html", "--max-file-lines", "3"]
)
self.assertIsNone(bare.base)
self.assertEqual(bare.note, [])
if __name__ == "__main__":
unittest.main()
@@ -0,0 +1,148 @@
from __future__ import annotations
import os
import unittest
from pkgmgr.actions.review import ReviewError
from tests.integration.test_review import git, write
from tests.integration.test_review_hardening import MODULE, ReviewRepoCase
INVISIBLE = "".join(
map(
chr,
(
*range(0x202A, 0x202F),
*range(0x2066, 0x206A),
*range(0x200B, 0x2010),
0x061C,
0xFEFF,
),
)
)
RLO, LRI, ZWSP, BOM = map(chr, (0x202E, 0x2066, 0x200B, 0xFEFF))
HIDDEN = "<li>Index entry {} is marked {}: its worktree changes are not shown.</li>"
ATTRIBUTE_BINARY = (
"{} is shown as binary because {}, not because of its content: "
"run git diff --text to see it."
)
UNSET = "a gitattributes entry unsets its diff attribute"
DRIVER = "its gitattributes diff driver {0} sets diff.{0}.binary"
def marks(text: str) -> str:
return "".join(f"&lt;U+{ord(char):04X}&gt;" for char in text)
class TestReviewConcealment(ReviewRepoCase):
def test_replace_objects_cannot_mask_the_index(self) -> None:
evil = self._object("blob", "print('evil')\n")
benign = self._object("blob", "print('staged')\n")
git(self.repo, "update-index", "--cacheinfo", f"100644,{evil},app.py")
git(self.repo, "replace", evil, benign)
git(self.repo, "update-index", "--skip-worktree", "app.py")
git(self.repo, "update-index", "--assume-unchanged", "lib.py")
write(self._path("lib.py"), b"print('hidden')\n")
html = self._render(None, [])
self.assertIn("print(&#39;evil&#39;)", html)
self.assertNotIn("print(&#39;staged&#39;)", html)
self.assertIn(HIDDEN.format("app.py", "skip-worktree"), html)
self.assertIn(HIDDEN.format("lib.py", "assume-unchanged"), html)
def test_ignored_submodule_change_is_shown(self) -> None:
sub = self._submodule(self.repo, "sub", "\tignore = all\n")
git(sub, "commit", "--allow-empty", "-m", "two")
new = git(sub, "rev-parse", "HEAD")
git(self.repo, "update-index", "--cacheinfo", f"160000,{new},sub")
gitmodules = self._render(None, [])
write(self._path(".gitmodules"), MODULE.format("sub", "").encode())
git(self.repo, "config", "diff.ignoreSubmodules", "all")
config = self._render(None, [])
for source, html in (("gitmodules", gitmodules), ("config", config)):
with self.subTest(source=source):
self.assertIn(f"Subproject commit {new}", html)
def test_attribute_binary_is_warned(self) -> None:
write(self._path(".gitattributes"), b"*.txt -diff\n")
write(self._path(".git", "info", "attributes"), b"*.md binary\n")
write(self._path("plain.txt"), b"plain text\n")
write(self._path("readme.md"), b"# readme\n")
write(self._path("image.bin"), b"\x00\x01binary\n")
git(self.repo, "add", ".gitattributes", "plain.txt", "readme.md", "image.bin")
html = self._render(None, [])
self.assertIn('data-path="image.bin"', html)
for path in ("plain.txt", "readme.md"):
self.assertIn(f"<li>{ATTRIBUTE_BINARY.format(path, UNSET)}</li>", html)
for path in ("image.bin", "new.txt"):
self.assertNotIn(f"{path} is shown as binary", html)
def test_binary_diff_driver_is_warned(self) -> None:
write(self._path(".gitattributes"), b"*.dat diff=opaque\n*.bin diff=plain\n")
git(self.repo, "config", "diff.opaque.binary", "yes")
write(self._path("data.dat"), b"text data\n")
write(self._path("image.bin"), b"\x00\x01binary\n")
git(self.repo, "add", ".gitattributes", "data.dat", "image.bin")
html = self._render(None, [])
self.assertIn('data-path="image.bin"', html)
warning = ATTRIBUTE_BINARY.format("data.dat", DRIVER.format("opaque"))
self.assertIn(f"<li>{warning}</li>", html)
self.assertNotIn("image.bin is shown as binary", html)
def test_base_resolves_the_branch_not_a_shadow(self) -> None:
head = git(self.repo, "rev-parse", "HEAD")
git(self.repo, "update-ref", "refs/tags/main", head)
tag_shadow = self._render("main", [])
write(self._path(".git", "main"), f"{head}\n".encode())
dir_shadow = self._render("main", [])
for html in (tag_shadow, dir_shadow):
self.assertIn('id="commits" data-commits="1"', html)
self.assertIn("<dt>Base</dt><dd>refs/heads/main @ ", html)
def test_base_refuses_ambiguous_and_other_refs(self) -> None:
head = git(self.repo, "rev-parse", "HEAD")
git(self.repo, "update-ref", "refs/remotes/main", head)
git(self.repo, "update-ref", "refs/tags/v1", head)
cases = {
"main": "ambiguous base ref",
"refs/tags/v1": "unknown base ref",
"HEAD~1": "unknown base ref",
head: "unknown base ref",
}
for base, message in cases.items():
with (
self.subTest(base=base),
self.assertRaisesRegex(ReviewError, message),
):
self._render(base, [])
def test_invisible_characters_are_marked(self) -> None:
tree = git(self.repo, "rev-parse", "HEAD^{tree}")
message = f"fix{RLO}: subject\n\nbody{LRI}text"
commit = git(self.repo, "commit-tree", tree, "-p", "HEAD", "-m", message)
git(self.repo, "update-ref", "HEAD", commit)
write(self._path(f"evil{RLO}txt.py"), f"code{ZWSP}line{BOM}\n".encode())
note = os.path.join(self.root, "note.txt")
write(note, f"note{INVISIBLE}\n".encode())
html = self._render("main", [note])
for char in INVISIBLE:
self.assertNotIn(char, html, f"U+{ord(char):04X}")
self.assertIn(f"fix{marks(RLO)}: subject", html)
self.assertIn(f"body{marks(LRI)}text", html)
self.assertIn(f"evil{marks(RLO)}txt.py", html)
self.assertIn(f"code{marks(ZWSP)}line{marks(BOM)}", html)
self.assertIn(f"note{marks(INVISIBLE)}", html)
if __name__ == "__main__":
unittest.main()
+229
View File
@@ -0,0 +1,229 @@
from __future__ import annotations
import os
import stat
import subprocess
import tempfile
import unittest
from pathlib import Path
from unittest.mock import patch
from pkgmgr.actions.review import ReviewError, write_review
from tests.integration.test_review import GIT_ENV, git, write
SECRET = b"SECRET-KEY-MATERIAL\n"
SIGNATURE = (
"gpgsig -----BEGIN PGP SIGNATURE-----\n \n iQEzBAABCAAdFiEE\n"
" -----END PGP SIGNATURE-----"
)
MODULE = '[submodule "{0}"]\n\tpath = {0}\n\turl = ./{0}\n{1}'
class ReviewRepoCase(unittest.TestCase):
def setUp(self) -> None:
self._tmp = tempfile.TemporaryDirectory()
self.root = os.path.realpath(self._tmp.name)
self.repo = os.path.join(self.root, "repo")
self.marker = os.path.join(self.root, "marker")
self.secret = os.path.join(self.root, "secret", "id_ed25519")
write(self.secret, SECRET)
git(self.root, "init", "-b", "main", self.repo)
write(self._path("app.py"), b"print('base')\n")
write(self._path("lib.py"), b"print('lib')\n")
git(self.repo, "add", "-A")
git(self.repo, "commit", "-m", "init")
git(self.repo, "checkout", "-b", "feature")
write(self._path("app.py"), b"print('feature')\n")
git(self.repo, "commit", "-am", "feature")
write(self._path("app.py"), b"print('staged')\n")
git(self.repo, "add", "app.py")
write(self._path("app.py"), b"print('unstaged')\n")
write(self._path("new.txt"), b"untracked\n")
def tearDown(self) -> None:
self._tmp.cleanup()
def _path(self, *parts: str) -> str:
return os.path.join(self.repo, *parts)
def _render(self, base: str | None, notes: list[str]) -> str:
path = write_review(
cwd=self.repo,
out=os.path.join(self.root, "out", "page.html"),
max_file_lines=50,
base=base,
notes=notes,
)
return Path(path).read_text(encoding="utf-8")
def _trap(self, path: str, tail: str) -> str:
write(path, f"#!/bin/sh\ntouch '{self.marker}'\n{tail}\n".encode())
os.chmod(path, 0o755)
return path
def _assert_fires(self, *args: str) -> None:
subprocess.run(
["git", *args],
cwd=self.repo,
env=GIT_ENV,
stdin=subprocess.DEVNULL,
capture_output=True,
check=False,
)
self.assertTrue(os.path.exists(self.marker), f"plain git {args[0]} fires")
os.remove(self.marker)
def _submodule(self, parent: str, name: str, extra: str = "") -> str:
path = os.path.join(parent, name)
git(parent, "init", "-b", "main", path)
write(os.path.join(path, "f.txt"), b"one\n")
git(path, "add", "-A")
git(path, "commit", "-m", "one")
head = git(path, "rev-parse", "HEAD")
write(os.path.join(parent, ".gitmodules"), MODULE.format(name, extra).encode())
git(parent, "update-index", "--add", "--cacheinfo", f"160000,{head},{name}")
git(parent, "add", ".gitmodules")
git(parent, "commit", "-m", name)
return path
def _object(self, kind: str, content: str) -> str:
return subprocess.run(
["git", "hash-object", "-t", kind, "-w", "--stdin"],
cwd=self.repo,
env=GIT_ENV,
input=content,
capture_output=True,
text=True,
check=True,
).stdout.strip()
class TestReviewHardening(ReviewRepoCase):
def test_fsmonitor_never_runs(self) -> None:
monitor = self._trap(os.path.join(self.root, "fsmonitor"), "exit 1")
git(self.repo, "config", "core.fsmonitor", monitor)
self._assert_fires("diff")
self._render("main", [])
self.assertFalse(os.path.exists(self.marker))
def test_repository_filter_never_runs(self) -> None:
write(self._path(".git", "info", "attributes"), b"* filter=trap\n")
clean = self._trap(os.path.join(self.root, "clean"), "exec cat")
git(self.repo, "config", "filter.trap.clean", clean)
self._assert_fires("diff")
html = self._render("main", [])
self.assertFalse(os.path.exists(self.marker))
self.assertIn('id="unstaged" data-files="0"', html)
self.assertIn("filter.trap.clean", html)
self.assertNotIn("print(&#39;unstaged&#39;)", html)
self.assertIn("<dd>(not computed, see the warnings)</dd>", html)
def test_submodule_filter_never_runs(self) -> None:
sub = self._submodule(self.repo, "sub")
inner = self._submodule(sub, "inner")
clean = self._trap(os.path.join(self.root, "clean"), "exec cat")
for path in (sub, inner):
write(os.path.join(path, ".git", "info", "attributes"), b"* filter=x\n")
write(os.path.join(path, "f.txt"), b"two\n")
git(path, "config", "filter.x.clean", clean)
self._assert_fires("diff")
html = self._render(None, [])
self.assertFalse(os.path.exists(self.marker))
self.assertIn('id="unstaged" data-files="0"', html)
self.assertIn("the config of submodule sub defines filter.x.clean", html)
self.assertIn("the config of submodule sub/inner defines filter.x.clean", html)
self.assertIn("<dd>(not computed, see the warnings)</dd>", html)
def test_signature_program_never_runs(self) -> None:
header, message = git(self.repo, "cat-file", "commit", "HEAD").split("\n\n", 1)
signed = self._object("commit", f"{header}\n{SIGNATURE}\n\n{message}\n")
git(self.repo, "update-ref", "HEAD", signed)
program = self._trap(os.path.join(self.root, "gpg"), "exit 1")
git(self.repo, "config", "gpg.program", program)
self._assert_fires("log", "-1", "--format=%G?")
html = self._render("main", [])
self.assertFalse(os.path.exists(self.marker))
self.assertIn('id="commits" data-commits="1"', html)
self.assertNotIn('<span class="sig', html)
def test_hooks_never_run(self) -> None:
hooks = os.path.join(self.root, "hooks")
self._trap(os.path.join(hooks, "post-index-change"), "exit 0")
git(self.repo, "config", "core.hooksPath", hooks)
self._assert_fires("update-index", "--force-write-index")
self._render("main", [])
self.assertFalse(os.path.exists(self.marker))
def test_note_symlinks_are_refused(self) -> None:
link = os.path.join(self.root, "score.txt")
os.symlink(self.secret, link)
parent = os.path.join(self.root, "notes")
os.symlink(os.path.dirname(self.secret), parent)
for note in (link, os.path.join(parent, "id_ed25519")):
with (
self.subTest(note=note),
self.assertRaisesRegex(ReviewError, "cannot read note"),
):
self._render(None, [note])
def test_untracked_parent_swap_is_not_followed(self) -> None:
write(self._path("sub", "id_ed25519"), b"harmless\n")
real_run = subprocess.run
swapped: list[list[str]] = []
def swap(cmd, *args, **kwargs):
result = real_run(cmd, *args, **kwargs)
if "--others" in cmd and not swapped:
os.rename(self._path("sub"), os.path.join(self.root, "sub-real"))
os.symlink(os.path.dirname(self.secret), self._path("sub"))
swapped.append(cmd)
return result
with patch("subprocess.run", swap):
html = self._render(None, [])
self.assertTrue(swapped)
self.assertNotIn(SECRET.decode().strip(), html)
self.assertIn("Not a regular file, contents not shown.", html)
def test_out_symlink_is_replaced_not_followed(self) -> None:
keys = b"ssh-ed25519 AAAA operator\n"
victim = os.path.join(self.root, "authorized_keys")
write(victim, keys)
missing = os.path.join(self.root, "missing_keys")
out = os.path.join(self.root, "out")
os.makedirs(out)
os.symlink(victim, os.path.join(out, "page.html"))
os.symlink(missing, os.path.join(out, "dangling.html"))
for name in ("page.html", "dangling.html"):
with self.subTest(out=name):
path = write_review(
cwd=self.repo,
out=os.path.join(out, name),
max_file_lines=50,
base=None,
notes=[],
)
info = os.lstat(path)
self.assertTrue(stat.S_ISREG(info.st_mode))
self.assertEqual(stat.S_IMODE(info.st_mode), 0o600)
self.assertEqual(Path(victim).read_bytes(), keys)
self.assertFalse(os.path.lexists(missing))
self.assertEqual(sorted(os.listdir(out)), ["dangling.html", "page.html"])
if __name__ == "__main__":
unittest.main()