Repository navigation
feat: mark parts reviewed, kept locally by content hash - #88
Merged
Merged
Conversation
Each part in the tree gets a reviewed checkbox. Marks live in the pull request's cache folder, keyed by the part's content hash and covering the hashes of its pieces (each hunk without its line numbers, or a hunkless file with its blob ids), so a part whose content changes is unmarked and says it changed since it was marked. The view badge counts the parts left. An opt-in setting, off by default, marks a file Viewed on GitHub once every part in it is reviewed. Closes #45
…n groupingProblems with retry/fallback tests
…os/ubuntu/windows) had one root cause: the CI-only real-host and package-smoke tests still asserted that a clean review round trip sends exactly 2 engine requests (initialize, review), but this PR's reviewed-marks feature legitimately sends a third — the extension calls engine.reviewedMarks(url) immediately after dispatching the review request (extension.ts engineReview → readMarks), and the fake engine logs every request, so the assertion failed with 3 !== 2. Invariant: a test pinning the engine's request log must enumerate every request the extension legitimately sends in the flow it drives. Both sibling sites (packages/extension/test/real-host/run.ts and packages/extension/test/package-smoke/run.ts) got the same smallest correction: expect 3 requests and assert the third is reviewedMarks with params { url }, mirroring the order already proven deterministically by the integration tests (['initialize', 'review', 'reviewedMarks']; stdin writes are synchronous and the review command resolves only after marksRead). No production code changed; later sendReview/draftComment lookups use find over the log and are unaffected, and tree expectations are unchanged (no marks exist in the fake store; renderedItem does not read checkboxState). Verified locally per the intent's constraints (no VS Code launch, no GitHub writes): npm run check passes fully (build, typecheck, lint, 1234 tests including the integration tests for the same request sequence), and tsc -p on both changed test projects compiles. The real VS Code runs remain CI-only as designed
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
1 participant
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
Closes #45. Each part in the review tree gets a reviewed checkbox. Marks are stored locally in the pull request's own cache folder, keyed by the part's content hash, so a part whose content changes is unmarked automatically and says "changed since you marked it", while untouched parts keep their marks. The tree view's badge counts the parts left to review. An opt-in setting, off by default because the GitHub Pull Requests extension syncs the same field, marks a file "Viewed" on GitHub once every part in it is reviewed.
The acceptance criteria are that marks persist across restarts in the local per-pull-request store, that content-hash invalidation has tests, and that the GitHub mirror is off by default and only marks whole files. Validation runs through the engine protocol and unit tests without launching VS Code or writing to GitHub; tests exercise the mirror only against fake GitHub responses.
What Changed
reviewed-marks.json) keyed by the part's content hash (per-hunk pieces without line numbers, blob ids for binary files), so marks survive restarts and a part whose content changed is unmarked and flagged "changed since you marked it"; the tree view's badge counts the parts left. NewreviewedMarksandmarkReviewedJSON-RPC methods read and update the store.second-look.mirrorViewedToGitHubsetting (off by default, since the GitHub Pull Requests extension syncs the same field) mirrors fully-reviewed files to GitHub's "Viewed" through a newmarkViewedRPC and a GraphQLmarkFileAsViewedmutation; files only partly reviewed are never marked, and nothing is ever unmarked.Risk Assessment
✅ Low: Both user-prescribed fixes verify in source, the identity invariant now holds across every reachable part source I could construct, tests are behavioral and match the acceptance criteria, and no new defect survived adversarial tracing of the pipeline-authored commits.
Testing
Exercised the reviewed-marks feature at every layer available without launching VS Code or touching real GitHub, as the intent prescribes: the full unit suite passes; the real engine binary and real engine server processes were driven live over their stdio JSON-RPC protocol, proving restart-persistent local marks, content-hash invalidation that unmarks only a pushed part, coexisting marks for same-named parts (declaration merging), and a mirror that marks only whole files with the request's token and never fires on its own; the mirror's off-by-default was checked only non-live (declarative contribution boolean default false plus zero-mutation logs, with the extension-side gate owned by the CI integration test), and the badge and agent-fallback scenarios were likewise not driven live. Lint was not run (phase rule); the extension UI surface (checkbox rendering, badge display) is covered by unit tests and the CI-owned integration suite because the intent forbids launching VS Code in validation.
Evidence: Live drive A transcript: real engine binary, marks persist across restart
engine process 1 (real binary, isolated cache dir) ok: initialize answers the version handshake ok: a pull request nobody marked holds no marks ok: ticking a part answers with one mark keyed by its content hash ok: the mark carries the part identity and its pieces ok: reading the marks back answers the same engine process 2 (a restart, same cache dir) ok: marks wait for the version handshake ok: the mark survived the restart in the local store ok: clearing the checkbox empties the store ok: a part whose pieces are not sha256 hashes is refused ok: a request whose url is not a pull request is refused ok: the part ticks again after being refused and cleared ok: the store on disk holds the mark under its content hash LIVE A PASSEDEvidence: Live drive B transcript: review, whole-file mirror, same-named parts, pushed change
Evidence: Persisted local store after the push: two same-named Config parts hold distinct kind-discriminated marks
{ "version": 1, "marks": { "3363…": { "name": "["Config in src/config.ts",["src/config.ts"],["class Config"]]", … }, "2274…": { "name": "["Config in src/config.ts",["src/config.ts"],["interface Config"]]", … } } }Evidence: Fake-GitHub request log, pull 7 drive: markFileAsViewed mutations only from explicit markViewed, each with the request token
Evidence: Fake-GitHub request log, pull 99 drive: zero mutations during review and marking, mirror only after every part is reviewed
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 1 issue found → auto-fixed ✅
packages/engine/src/reviewed-marks.ts:118- The invariant the round-1 fix established — part names are unique within one review, becauseapplyMarkreplaces every mark of the same name (reviewed-marks.ts:118) andreviewedStatetreats a name match as a prior mark (reviewed-marks.ts:75) — is still violated by the plain grouping, which is the default path when no agent is configured. Round 1's analysis claimed the plain grouping guarantees unique names, but it unions hunks byentityKey=${kind} ${name}(packages/engine/src/parts.ts:9, used at parts.ts:79) whilepartNamerenders onlyentity.name(parts.ts:27-38), dropping the kind. Concrete sequence during intended usage: a TypeScript file declares bothinterface Configandclass Config(declaration merging — kinds 'interface' and 'class' both exist in TYPESCRIPT_ENTITIES, languages.ts:29-43); a pull request edits the interface in one hunk and the class in another; the two hunks share no entity key, sosplitFilemakes two parts, both named "Config in src/config.ts". The reviewer ticks part A — mark stored; ticks part B — A's mark is deleted by the name filter; A reads 'changed since marked' via the name-match clause; re-ticking A deletes B's mark: the badge never reaches 0 andwholeFilesReviewed(reviewed-marks.ts:91) keeps the file out of the GitHub mirror — the same failure mode as the round-1 error the user chose to fix, left behind by the fix round (2181c17) which closed only the agent-answer path in groupingProblems. Remaining sibling site of the same invariant: packages/engine/src/grouping.ts:296 concatenates the agent's parts with the sinking files' plain parts ([...parts, ...noise]) without checking names across the two sources, so an agent part named exactly like a noise part's plain name (e.g. a bare lockfile path) collides identically. Earliest supported boundary: make the names unique where they are generated — inpartName, include the entity's kind (or another discriminator) when parts of one file would otherwise share a name, and reserve the noise parts' names in the agent answer check (or dedupe where the review's parts are assembled) — so the name key holds for every source of parts, not just agent answers.🔧 Fix applied.
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
npm ci (dependencies from package-lock.json into the worktree)npm run buildnpx tsc -p tsconfig.test.jsonnpx vitest run (1234 unit tests, 69 files; extension integration excluded as CI-owned)npx vitest run packages/engine/test/reviewed-marks.test.ts packages/engine/test/grouping.test.ts packages/extension/test/tree.test.ts packages/extension/test/engine-client.test.ts (91 tests)npx vitest run packages/engine/test/server.test.ts -t "reviewed marks" (5 tests)node .tmp-live/drive-a.mjs — live: real engine binary (packages/engine/dist/main.js serve --cache-dir) over stdio JSON-RPC; initialize/reviewedMarks/markReviewed tick, read-back, restart persistence, clear, malformed-part and bad-URL refusal; persisted reviewed-marks.json inspectednode .tmp-live/drive-b.mjs — live: real runRpcServer engine child processes over stdio with fixture-backed fetch (repo fixtures for pull 7, custom declaration-merging pull 99); full review, tick all parts, markViewed whole-files-only with per-request token, stranger-path and no-finished-review refusals with zero mutations, both same-named parts marked at once, pushed change unmarking only the interface part, re-tick restoring the filenode --input-type=module semantic parse of packages/extension/package.json: second-look.mirrorViewedToGitHub contributed as boolean with default false🔧 **Document** - 1 issue found → auto-fixed ✅
CONTEXT.md:1- The change coins "reviewed mark" as core domain vocabulary — used throughout README.md, the second-look.mirrorViewedToGitHub setting description, and the reviewedMarks/markReviewed protocol methods — but the project glossary (CONTEXT.md), the authoritative vocabulary document, has no entry for it, while comparable acting-on-the-review terms (Comment, Draft comment) do. No existing glossary entry was made stale, so per scope discipline I did not add one; whether to add a Reviewed mark entry (e.g. under "Acting on the review", covering the local per-pull-request store, content-hash invalidation, and the opt-in GitHub Viewed mirror) is a vocabulary decision for the owner.🔧 Fix applied.
✅ Re-checked - no issues remain.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.