Repository navigation
feat(engine): add pull request review command with full-diff parsing - #52
Merged
Merged
Conversation
Repository as a TypeScript workspace: an engine package and an extension package, strict type checks, ESLint, Vitest, and GitHub Actions CI running install, build, lint and tests on every pull request and on master. The engine gets a review command: given a GitHub pull request URL it fetches the metadata (description kept in full, never truncated) and the full diff through the diff media type (so a large lockfile keeps its patch), parses the diff into files and hunks — renames, deletions, binary files, mode changes and missing final newlines included — and prints a typed, versioned review result as JSON with one part per changed file. A coverage validator proves every changed line belongs to exactly one part before anything is printed. The GitHub token is passed in by the caller (flag or environment), is used only for the request, and is never written to disk or logs. The extension consumes the shared, versioned protocol types and validates the engine's JSON before use. Tests run against recorded fixtures and responses; no test touches the network. Closes #17
…drop redundant devDependency
…-path escape table
…est code in check
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Intent
This is the first change of the v1 milestone: it turns the repository from a design-only project into a working TypeScript codebase and gives the engine its first command. The project is a VS Code companion for human pull request review; its glossary is CONTEXT.md and its decisions are recorded in docs/adr/0001-0006. ADR 0005 sets the engine and the extension in TypeScript, with the engine a separate local process behind a language-neutral protocol. This change implements the v1 step-0 ticket: #17.
What to build
Sets up the repository as a TypeScript project with an engine package and an extension package, strict type checks, lint, unit tests, and GitHub Actions CI on every pull request. The engine gets a review command: given a GitHub pull request URL it fetches the metadata, the full description (never truncated) and the full diff (from the diff media type, so a large lockfile keeps its patch), parses the diff into files and hunks, and prints a typed, versioned review result as JSON with one part per file and no importance yet.
How to try it by hand: give the engine a GitHub login (for example a token from the GitHub CLI in the environment) and run the review command on any public pull request. Compare the file list and line counts with the GitHub page; try a pull request whose lockfile is too large for GitHub to show inline.
Acceptance criteria
What Changed
npm run checkcommand on every pull request and on master.packages/enginereview command: given a pull request URL and a caller-supplied token, it fetches the full PR metadata (description never truncated) and the complete diff via the diff media type (so oversized files like lockfiles keep their patch), parses renames, deletions, binary files, mode changes, and missing final newlines into files and hunks, and prints a typed, versionedReviewResultJSON with one part per changed file after a coverage validator proves every changed line belongs to exactly one part.packages/extensionguards that validate and parse the engine's JSON output against the shared, versioned protocol types, with fixture-backed tests for the diff parser, coverage validator, GitHub client, CLI, and extension reader that never touch the network.Risk Assessment
🚨 High: The core review command crashes on any pull request that adds or deletes a binary file — a very common shape — which directly violates the acceptance criterion that binary files parse correctly, so the change needs that fix (or an explicit decision) before merge; the remaining findings (broken documented npx entry point, beyond-scope diff shapes, type-check and redundancy nits) are bounded follow-ups.
Testing
Drove the real built engine CLI live against four public GitHub PRs with the caller-supplied token, exactly as the intent's manual trial prescribes: file lists, per-file line counts, totals, and full untruncated descriptions match GitHub's REST data in every case, including the lockfile PR whose patch the REST file list omits. Parser acceptance shapes (rename, copy, binary add/delete/modify, mode change, missing final newline, quoted non-ASCII path, multi-digit hunk counts, header-shaped body lines) were generated with real git and verified against git's own numstat accounting on identical diff text. Token secrecy held under three adversarial error paths with no disk writes. Build, full type-check (src + test code), and the 54 unit tests pass; the lint and CI legs of 'npm run check' were not run because this phase forbids running linters and CI requires a push owned by the outer executor. No UI surface exists in this change, so evidence is CLI transcripts and JSON outputs rather than screenshots.
Evidence: Live CLI vs GitHub REST summary (4 PRs)
Evidence: Full review-result JSON for second-look#16 (typed, versioned, 8 parts)
~/.no-mistakes/evidence/01M3WAQP6ZCNE2HGG2BZJGKVEX/live-lockfile-pr-review.json)Evidence: Proof REST files list omits the lockfile patch for npm/cli#9734
Evidence: Review-result JSON for the added-binary PR (Logo.png, matchpoint-mark.png)
~/.no-mistakes/evidence/01M3WAQP6ZCNE2HGG2BZJGKVEX/live-rename-pr-review.json)Evidence: Real-git diff shapes parsed by the built engine, cross-checked against git apply --numstat
Evidence: Adversarial coverage-validator check (dropped part rejected)
Evidence: Adversarial token-secrecy checks (sentinel never echoed; no disk writes)
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
packages/engine/src/diff.ts:27- The parser crashes on git's canonical output for ADDED and DELETED binary files. BINARY_FILES (/^Binary files a\/(.*) and b\/(.*) differ$/) requiresa/on the left andb/on the right, but git emitsBinary files /dev/null and b/shot.png differfor any newly added binary file (e.g. a screenshot added to a README) andBinary files a/old.png and /dev/null differfor a deleted one. Verified by running the built parser on both shapes: parseDiff throwsunexpected line outside any hunk, so reviewPullRequest and the whole review command fail for such a pull request. This contradicts the required acceptance criterion: 'The full diff is used even when the REST file list omits a patch; renames, deletions, binary files, mode changes and missing final newlines parse correctly' — added/deleted binary files do not parse at all, and the test fixtures only cover the modified-binary and GIT-binary-patch shapes. The fix direction is mechanical and mirrors what OLD_PATH/NEW_PATH already do at lines 28-29 (accept/dev/nullon either side): accept/dev/nullin BINARY_FILES, set changeKind to addition/deletion accordingly, and make finishFile (diff.ts:289) derive the path from the non-null side; the affected consumer is packages/engine/src/review.ts (the review command fails entirely). Quoting the criterion for the author's decision as the intent-conformance rules require.README.md:35- The README's primary invocationnpx second-look-engine review …does not work on a fresh clone following the README's own order.npm ciruns while the bin targetpackages/engine/dist/main.jsis a gitignored build output that does not yet exist, so npm never creates thenode_modules/.bin/second-look-enginelink;npm run buildafterwards creates dist/main.js but not the link, andnpxthen falls back to the registry, which 404s (verified: fresh clone → npm ci → build →npx --no-install second-look-enginefinds no local bin and the unscoped name does not exist on npm). The documented alternativenode packages/engine/dist/main.js review …(README.md:39) does work, as does re-runningnpm ciafter a build. Remedy: make thenode packages/engine/dist/main.jsform the primary documented command (or commit a small bin wrapper script so the link target exists at install time).packages/engine/src/diff.ts:20- The parser hard-crashes on two other shapes git really emits, both verified against the built parser: (1) C-quoted paths — any filename with non-ASCII or special bytes is printed asdiff --git "a/\303\274ber.md" "b/\303\274ber.md"/--- "a/…"(core.quotePath default), which matches neither DIFF_GIT (line 20) nor OLD_PATH/NEW_PATH (lines 28-29), so the whole review fails withunexpected line before any 'diff --git' header; (2)copy from/copy toheaders, which no branch handles (crash:unexpected line outside any hunk). The intent's manual trial is 'run the review command on any public pull request', and non-ASCII filenames are realistic, but these forms are not among the acceptance criteria's enumerated cases, so the remedy (quoted-path octal decoding plus copy handling) extends the change beyond its stated intent and needs the author's authorization rather than an automated fix. All callers are affected equally (reviewPullRequest, runCli, the future extension).packages/engine/src/review.ts:44- The coverage proof is wired as validateCoverage(parsed, parsed.files): parts are exactly the parsed files, so both sides of the comparison come from the same parse and the check can never fail today (it also cannot detect a parse that dropped lines, since diffLines are built from the same parse). At this milestone the invariant 'every changed line belongs to exactly one part' holds by construction because part ≡ file, and the validator itself is real and well-tested against manipulated inputs; it becomes load-bearing when later steps group several files into one part. Informational note of that tradeoff, no action needed now.packages/engine/tsconfig.json:9-include: ["src"](same in packages/extension/tsconfig.json:10) meanstsc -bnever type-checks the ~800 lines of test code, and vitest's esbuild transform does not type-check either, sonpm run check— which the acceptance criterion says 'type-checks … the whole repository' — leaves test files untype-checked. Remedy: add a noEmit tsconfig coveringtest(e.g. tsconfig.test.json wired into the build or atsc -pstep incheck).package.json:20- Simplification: the root devDependency on@second-look/engineis a second, redundant definition of a dependency the repository already declares once — the extension package depends on it (which is what creates the workspace symlink), vitest aliases it to engine source, and tsc resolves it through the extension's own dependency. No intent requirement needs it, and it does not make thenpx second-look-engineentry point reliable (see the bin-linking finding). Recommend removing it as the remedy.🔧 Fix applied.
7 issues (2 errors, 3 warnings, 2 infos) still open:
packages/engine/src/diff.ts:27- The parser crashes on git's canonical output for ADDED and DELETED binary files. BINARY_FILES (/^Binary files a\/(.*) and b\/(.*) differ$/) requiresa/on the left andb/on the right, but git emitsBinary files /dev/null and b/shot.png differfor any newly added binary file (e.g. a screenshot added to a README) andBinary files a/old.png and /dev/null differfor a deleted one. Verified by running the built parser on both shapes: parseDiff throwsunexpected line outside any hunk, so reviewPullRequest and the whole review command fail for such a pull request. This contradicts the required acceptance criterion: 'The full diff is used even when the REST file list omits a patch; renames, deletions, binary files, mode changes and missing final newlines parse correctly' — added/deleted binary files do not parse at all, and the test fixtures only cover the modified-binary and GIT-binary-patch shapes. The fix direction is mechanical and mirrors what OLD_PATH/NEW_PATH already do at lines 28-29 (accept/dev/nullon either side): accept/dev/nullin BINARY_FILES, set changeKind to addition/deletion accordingly, and make finishFile (diff.ts:289) derive the path from the non-null side; the affected consumer is packages/engine/src/review.ts (the review command fails entirely). Quoting the criterion for the author's decision as the intent-conformance rules require.README.md:35- The README's primary invocationnpx second-look-engine review …does not work on a fresh clone following the README's own order.npm ciruns while the bin targetpackages/engine/dist/main.jsis a gitignored build output that does not yet exist, so npm never creates thenode_modules/.bin/second-look-enginelink;npm run buildafterwards creates dist/main.js but not the link, andnpxthen falls back to the registry, which 404s (verified: fresh clone → npm ci → build →npx --no-install second-look-enginefinds no local bin and the unscoped name does not exist on npm). The documented alternativenode packages/engine/dist/main.js review …(README.md:39) does work, as does re-runningnpm ciafter a build. Remedy: make thenode packages/engine/dist/main.jsform the primary documented command (or commit a small bin wrapper script so the link target exists at install time).packages/engine/src/diff.ts:20- The parser hard-crashes on two other shapes git really emits, both verified against the built parser: (1) C-quoted paths — any filename with non-ASCII or special bytes is printed asdiff --git "a/\303\274ber.md" "b/\303\274ber.md"/--- "a/…"(core.quotePath default), which matches neither DIFF_GIT (line 20) nor OLD_PATH/NEW_PATH (lines 28-29), so the whole review fails withunexpected line before any 'diff --git' header; (2)copy from/copy toheaders, which no branch handles (crash:unexpected line outside any hunk). The intent's manual trial is 'run the review command on any public pull request', and non-ASCII filenames are realistic, but these forms are not among the acceptance criteria's enumerated cases, so the remedy (quoted-path octal decoding plus copy handling) extends the change beyond its stated intent and needs the author's authorization rather than an automated fix. All callers are affected equally (reviewPullRequest, runCli, the future extension).packages/engine/src/review.ts:44- The coverage proof is wired as validateCoverage(parsed, parsed.files): parts are exactly the parsed files, so both sides of the comparison come from the same parse and the check can never fail today (it also cannot detect a parse that dropped lines, since diffLines are built from the same parse). At this milestone the invariant 'every changed line belongs to exactly one part' holds by construction because part ≡ file, and the validator itself is real and well-tested against manipulated inputs; it becomes load-bearing when later steps group several files into one part. Informational note of that tradeoff, no action needed now.packages/engine/tsconfig.json:9-include: ["src"](same in packages/extension/tsconfig.json:10) meanstsc -bnever type-checks the ~800 lines of test code, and vitest's esbuild transform does not type-check either, sonpm run check— which the acceptance criterion says 'type-checks … the whole repository' — leaves test files untype-checked. Remedy: add a noEmit tsconfig coveringtest(e.g. tsconfig.test.json wired into the build or atsc -pstep incheck).packages/engine/src/diff.ts:33- The fix round (commit 05197d3) accidentally regressed HUNK_HEADER's old-side count capture from(\d+)to(\d)(exactly one digit), while the new-side count kept(\d+). Any hunk header whose old-side line count is 10 or more — e.g.@@ -1,15 +1,20 @@, which any PR deleting ~10 lines or with a 10-line hunk context produces — fails to match, falls through every branch, and throwsunexpected line outside any hunk, so parseDiff, reviewPullRequest and the whole review command fail. Verified against the built parser:@@ -1,9 +1,20 @@parses,@@ -1,10 +1,20 @@and@@ -1,99 +1,1 @@throw. Every fixture's old-side count is a single digit (@@ -4388,9 +4388,2507 @@in large-lock.diff and pull-42.diff,@@ -12,3 +12,5 @@, etc.), including the four fixtures the fix round itself added, so the test suite cannot catch this — this is collateral damage in code adjacent to the F1/F3 fixes, not part of either finding. Remedy: restore(\d+)on the old-side count (and add a fixture with a multi-digit old count). No sibling sites: OLD_MODE/NEW_MODE/NEW_FILE_MODE/DELETED_FILE_MODE (diff.ts:22-25) all already use(\d+); this is the only single-digit capture. This defect was introduced by the pipeline's fix round.packages/engine/src/diff.ts:36- ESCAPED_BYTES, added by the fix round for decodePathToken, covers only",\\,n,r,t, but git's quote_c_style also emits\a(0x07),\b(0x08),\v(0x0b),\f(0x0c) for control bytes in paths. For those, decodePathToken hits the non-octal branch and returns undefined (diff.ts:73-74), so the header carrying the quoted path falls through and parseDiff throws — the review command crashes (fail-loud, not silently wrong) on any PR with a control character in a filename, which is legal on macOS/Linux and realistic for machine-generated paths. Remedy: add the four mappings (a: 0x07, b: 0x08, v: 0x0b, f: 0x0c) to ESCAPED_BYTES. Incompleteness introduced by the fix round inside the decoding machinery the user authorized for F3; the octal-escape path itself reassembles UTF-8 correctly.🔧 Fix applied.
3 issues (1 warning, 2 infos) still open:
README.md:35- The README's primary invocationnpx second-look-engine review …does not work on a fresh clone following the README's own order.npm ciruns while the bin targetpackages/engine/dist/main.jsis a gitignored build output that does not yet exist, so npm never creates thenode_modules/.bin/second-look-enginelink;npm run buildafterwards creates dist/main.js but not the link, andnpxthen falls back to the registry, which 404s (verified: fresh clone → npm ci → build →npx --no-install second-look-enginefinds no local bin and the unscoped name does not exist on npm). The documented alternativenode packages/engine/dist/main.js review …(README.md:39) does work, as does re-runningnpm ciafter a build. Remedy: make thenode packages/engine/dist/main.jsform the primary documented command (or commit a small bin wrapper script so the link target exists at install time).packages/engine/src/review.ts:44- The coverage proof is wired as validateCoverage(parsed, parsed.files): parts are exactly the parsed files, so both sides of the comparison come from the same parse and the check can never fail today (it also cannot detect a parse that dropped lines, since diffLines are built from the same parse). At this milestone the invariant 'every changed line belongs to exactly one part' holds by construction because part ≡ file, and the validator itself is real and well-tested against manipulated inputs; it becomes load-bearing when later steps group several files into one part. Informational note of that tradeoff, no action needed now.packages/engine/tsconfig.json:9-include: ["src"](same in packages/extension/tsconfig.json:10) meanstsc -bnever type-checks the ~800 lines of test code, and vitest's esbuild transform does not type-check either, sonpm run check— which the acceptance criterion says 'type-checks … the whole repository' — leaves test files untype-checked. Remedy: add a noEmit tsconfig coveringtest(e.g. tsconfig.test.json wired into the build or atsc -pstep incheck).🔧 Fix applied.
2 issues (1 warning, 1 info) still open:
packages/engine/src/review.ts:44- The coverage proof is wired as validateCoverage(parsed, parsed.files): parts are exactly the parsed files, so both sides of the comparison come from the same parse and the check can never fail today (it also cannot detect a parse that dropped lines, since diffLines are built from the same parse). At this milestone the invariant 'every changed line belongs to exactly one part' holds by construction because part ≡ file, and the validator itself is real and well-tested against manipulated inputs; it becomes load-bearing when later steps group several files into one part. Informational note of that tradeoff, no action needed now.packages/engine/src/diff.ts:380- The---/+++header matchers run even while the current hunk still expects body lines, so a deletion whose content begins-- a/(realistic as a Lua comment or vendored-patch text naming a path) or-- /dev/nullis misread as the---header. Verified against the built parser: mid-hunk it resets the hunk and the next body line throwsunexpected line outside any hunk; when it is the final body line the deletion is silently dropped (parse returns deletions=0, the line absent from hunk.lines) and the part's addition/deletion counts are wrong — a wrong result without an error from a tool whose core job is exact line accounting, and the coverage validator cannot catch it (both sides come from the same parse, cf. previously-noted F4). Sibling site with the same collision shape: the NEW_PATH matcher at packages/engine/src/diff.ts:391 (an addition of++ b/…content parses as+++ b/…). No other matcher can collide: hunk body lines begin with+,-, space or\, and only-+-- …→---and++++ …→+++reproduce header shapes; DIFF_GIT, HUNK_HEADER, mode/rename/copy/index/Binary-files lines all require leading characters a body prefix cannot produce. Remedy: mirror the existing body gate at packages/engine/src/diff.ts:421 — whileoldRemaining > 0 || newRemaining > 0, consume the line as hunk body before the OLD_PATH/NEW_PATH matchers; git never emits these headers inside a hunk body, so well-formed diffs (every recorded fixture) parse identically. This defect is in the author's original parser (commit 2fc5e20), not introduced by the fix rounds; no prior round reported it.🔧 Fix applied.
1 info still open:
packages/engine/src/review.ts:44- The coverage proof is wired as validateCoverage(parsed, parsed.files): parts are exactly the parsed files, so both sides of the comparison come from the same parse and the check can never fail today (it also cannot detect a parse that dropped lines, since diffLines are built from the same parse). At this milestone the invariant 'every changed line belongs to exactly one part' holds by construction because part ≡ file, and the validator itself is real and well-tested against manipulated inputs; it becomes load-bearing when later steps group several files into one part. Informational note of that tradeoff, no action needed now.✅ **Test** - passed
✅ No issues found.
npm run build (tsc -b) — cleannpx tsc -p tsconfig.test.json — test code type-checksnpx vitest run — 54/54 tests pass (diff parser fixtures, recorded-response GitHub client, no live network)GITHUB_TOKEN="$(gh auth token)" node packages/engine/dist/main.js review https://github.com/lbildzinkas/second-look/pull/16 — 8 parts, +146/-2, description 981 chars, identical to REST metadata and file listGITHUB_TOKEN="$(gh auth token)" node packages/engine/dist/main.js review https://github.com/npm/cli/pull/9734 — 18 parts, +7422/-8052, description 20850 chars; per-file diff against gh api pulls/9734/files (paginated): all files and counts match; package-lock.json parsed +7405/-8035 in 61 hunks despite REST omitting its patchGITHUB_TOKEN="$(gh auth token)" node packages/engine/dist/main.js review https://github.com/AjAzE-dev/Hotters_hackaton/pull/12 — two added binary PNGs parsed as addition/isBinary:trueGITHUB_TOKEN="$(gh auth token)" node packages/engine/dist/main.js review https://github.com/pmndrs/zustand/pull/1305 — 35 parts, 30 renames with previousPath, all files/flags/counts match paginated RESTDisposable git repo: generated real diffs (pure rename, rename with edits, copy via -C, added/deleted/modified binary, mode change, missing final newline both sides, C-quoted non-ASCII path 'src/über.md', @@ -1,40 +1,40 @@ multi-digit counts, body lines shaped like '--- a/x'/'+++ b/x') and parsed them with packages/engine/dist parseDiff, cross-checking every part against 'git apply --numstat' on the identical diff textAdversarial coverage check: validateCoverage rejects a parse with a dropped part, accepts the intact parseAdversarial token checks: sentinel token via env, via --token flag, and with a malformed URL — 0 occurrences of the sentinel in any output; grep sweep confirms the real gh keyring token is absent from every evidence file; git status stayed clean (nothing written to disk)✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.
Closes #17