Repository navigation
feat(engine): agent grouping pass groups related hunks across files - #67
Merged
Merged
Conversation
lbildzinkas
force-pushed
the
fm/second-look-grouping-pass
branch
from
October 2, 2026 20:17
8e9a192 to
89ca8fd
Compare
The reviewer's installed agent groups related hunks across files into parts named by the entities they touch, behind a versioned grouping prompt. The engine checks every answer: an invalid one is retried once and then the plain grouping stays, and hunks a valid answer leaves out go to a part marked not grouped by the agent, so coverage holds. A part can now span files (review result version 4), and a review arrives in stages over the protocol: the plain result first in a review/stage notification, then the agent's parts. The extension shows the plain tree first with a status line naming the running stage, then regroups in place and keeps the reviewer's selected part. The evaluation scores grouping by pairwise hunk agreement with hand labels, gates on 100% coverage, runs the prompt through Pi with --agent pi, traces each call, and keeps one baseline per agent and model. The prompt's cases are the two canaries, example-7 and three recorded public pull requests.
…fallbacks in baseline
lbildzinkas
force-pushed
the
fm/second-look-grouping-pass
branch
from
October 2, 2026 22:10
89ca8fd to
a5d7311
Compare
The reviewer's installed agent groups related hunks across files into parts named by the entities they touch, behind a versioned grouping prompt. The engine checks every answer: an invalid one is retried once and then the plain grouping stays, and hunks a valid answer leaves out go to a part marked not grouped by the agent, so coverage holds. A part can now span files (review result version 4), and a review arrives in stages over the protocol: the plain result first in a review/stage notification, then the agent's parts. The extension shows the plain tree first with a status line naming the running stage, then regroups in place and keeps the reviewer's selected part. The evaluation scores grouping by pairwise hunk agreement with hand labels, gates on 100% coverage, runs the prompt through Pi with --agent pi, traces each call, and keeps one baseline per agent and model. The prompt's cases are the two canaries, example-7 and three recorded public pull requests.
…fallbacks in baseline
…ping-pass # Conflicts: # README.md # packages/engine/src/rpc.ts # packages/engine/src/server.ts # packages/engine/test/server.test.ts # packages/extension/src/engine-client.ts # packages/extension/src/extension.ts # packages/extension/src/tree.ts # packages/extension/test/integration/extension.test.ts
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 change builds the agent grouping pass for the Second Look review companion, issue #28 of the v1 plan. The plain pass groups hunks into parts within one file; the reviewer's installed coding agent now proposes parts that group related hunks across files, such as a function, its caller and its test, named by the entities they touch.
The engine checks every agent answer before showing it. Hunks the agent leaves out go to a part marked "not grouped by the agent", so every changed line still belongs to exactly one part, and an invalid answer falls back to the plain grouping. In VS Code the tree appears first from the plain pass and updates in place when the agent's parts arrive, with a status line naming the stage still running, without losing the reviewer's place.
The grouping prompt is versioned and lands with its evaluation cases: the two canary cases plus three recorded public pull requests with hand-labelled groupings. The evaluation scores coverage as a hard gate at 100% and agreement with the hand labels as pairwise hunk agreement, and records the baseline per agent and model tried. Tests cover the coverage fallback and the invalid-answer fallback.
Validation does not launch the VS Code application locally: the extension integration tests run in CI, and local validation goes through the engine protocol and unit tests. Model calls go only through the locally installed Pi agent with its own sign-in, at modest volume, without reading any credential file and without paid API keys.
Closes #28.
What Changed
groupingfield (schema v4 also gives parts anoriginandotherFiles).reviewandservegain--agent/--model/--effort/--agent-timeout, plain parts are announced on stderr while the agent works, and the engine stops its agent children on SIGINT/SIGTERM.grouping-agreementscore (pairwise hunk agreement against hand-labelled groups), made coverage a hard gate, stamped agent runs per agent/model with fallbacks listed, and recorded three public pull requests (httpx #3690, click #3781, ky #880) with hand-labelled groupings and licenses, beside the refreshed baseline.Risk Assessment
✅ Low: Both pipeline-authored fix commits minimally and faithfully implement the recorded human decisions (concurrent request answering in the serve loop; agent children stopped on SIGINT/SIGTERM with tests exercising real processes), and this pass found no reachable defect in them or in the previously reviewed authored change beyond the user-decided items, so the branch is safe to merge pending the pipeline's own test step.
Testing
Drove the real engine process over its JSON-RPC protocol and CLI against a real public pull request (encode/httpx#3690) with a disposable fake-pi PATH shim and temp caches, plus a real-model evaluation through the locally installed Pi (4 calls, within the intent's volume cap): plain parts always arrived first in a review/stage notification, the agent's cross-file parts then replaced them with left-out hunks collected into 'not grouped by the agent', invalid answers retried once and fell back to the plain grouping with the reason stamped, coverage stayed 100% everywhere (offline and agent runs, exit 0), a sendReview was answered while an agent-stage review still ran and the review completed afterwards, SIGTERM stopped the engine's agent child, and the CLI refused agent misconfiguration. The extension tree's place-keeping was exercised only by the repository's unit tests; the intent forbids launching VS Code on this machine, so that surface could not be driven live and is reported untested. No failures; no findings.
Evidence: Serve protocol: plain stage then agent grouping with left-out hunks
Evidence: Serve protocol: invalid answer retried once, falls back to plain grouping
Evidence: Serve protocol: sendReview answered during the agent stage (1.50s vs 13.65s)
Evidence: SIGTERM to the engine stops its running agent child
Evidence: Review CLI with --agent pi: stderr plain-parts announcement and final agent result
Evidence: Engine CLI agent guards (unknown --agent, tuning without --agent)
Evidence: Offline evaluation report: coverage 1 everywhere, baseline 0 drops
Evidence: Live agent evaluation report through Pi (coverage 1, agreement 0.8482)
Evidence: Agent evaluation results.json (stamped rows per agent and model)
~/.no-mistakes/evidence/01M42NEJEXTP669SNQX5PPM2ME/eval-agent-trace.jsonl)Evidence: Delivered grouping prompt v2 excerpt: hunk headers inside untrusted blocks
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 2 issues found → auto-fixed (2) ✅
packages/engine/src/server.ts:104- The serve loop handles one request at a time (await review(...)before reading the next line), and the agent grouping stage now keeps a review request open for minutes (groupingStageTimeoutMs = 2×5 min + 60 s) by design — the reviewer is invited to read the plain tree and write comments during it. Concrete sequence: reviewer starts a review (engine spawned with --agent), the plain stage arrives, they write comments and press Submit while the agent stage still runs;submitReview→engineSend(extension.ts:463) reuses the same engine, the sendReview line is buffered unread behind the running review, and after SEND_REVIEW_TIMEOUT_MS = 60 s (engine-client.ts:80)expire()rejects the send and callsthis.dispose()(engine-client.ts:259), which SIGTERMs the engine and fails the in-flight review request too — the reviewer gets 'the engine did not answer in time', the agent's grouping is lost, and only the comments survive. Remedy needs a product/protocol decision, hence ask-user: either the engine answers a send while a review runs (concurrent request handling at this shared boundary), or the extension holds a send until the running review settles (serialize in ReviewSession). Sibling paths that must hold the same invariant: a second review request on the same engine (the extension side-steps it by disposing first at extension.ts:239), and the stage-notification deadline restart (engine-client.ts onResponse) which correctly extends only the review's own deadline.packages/extension/src/extension.ts:239- Killing the engine mid-agent-run orphans the running agent child. The new replaced-review flow disposes the engine while the grouping agent works (this.engine?.dispose()at extension.ts:239, and the same mid-run kill is reachable via the send-timeout dispose at engine-client.ts:259 and deactivate at extension.ts:515), but the engine installs no SIGTERM handler (packages/engine/src/main.ts) and the adapters kill their child only through the engine's own timeout timer (pi.ts:246, claude-code.ts:298), so thepi/claudeprocess survives its parent and runs to completion on the reviewer's subscription after the engine died. Bounded impact: the one-shot run finishes by itself; the cost is one wasted agent run per killed review. Remedy (mechanical): the engine kills its running agent children on SIGTERM/SIGINT, or the adapters tie the child to the parent's exit.🔧 Fix applied.
2 issues (1 warning, 1 info) still open:
packages/engine/src/server.ts:104- The serve loop handles one request at a time (await review(...)before reading the next line), and the agent grouping stage now keeps a review request open for minutes (groupingStageTimeoutMs = 2×5 min + 60 s) by design — the reviewer is invited to read the plain tree and write comments during it. Concrete sequence: reviewer starts a review (engine spawned with --agent), the plain stage arrives, they write comments and press Submit while the agent stage still runs;submitReview→engineSend(extension.ts:463) reuses the same engine, the sendReview line is buffered unread behind the running review, and after SEND_REVIEW_TIMEOUT_MS = 60 s (engine-client.ts:80)expire()rejects the send and callsthis.dispose()(engine-client.ts:259), which SIGTERMs the engine and fails the in-flight review request too — the reviewer gets 'the engine did not answer in time', the agent's grouping is lost, and only the comments survive. Remedy needs a product/protocol decision, hence ask-user: either the engine answers a send while a review runs (concurrent request handling at this shared boundary), or the extension holds a send until the running review settles (serialize in ReviewSession). Sibling paths that must hold the same invariant: a second review request on the same engine (the extension side-steps it by disposing first at extension.ts:239), and the stage-notification deadline restart (engine-client.ts onResponse) which correctly extends only the review's own deadline.packages/extension/src/extension.ts:239- Killing the engine mid-agent-run orphans the running agent child. The new replaced-review flow disposes the engine while the grouping agent works (this.engine?.dispose()at extension.ts:239, and the same mid-run kill is reachable via the send-timeout dispose at engine-client.ts:259 and deactivate at extension.ts:515), but the engine installs no SIGTERM handler (packages/engine/src/main.ts) and the adapters kill their child only through the engine's own timeout timer (pi.ts:246, claude-code.ts:298), so thepi/claudeprocess survives its parent and runs to completion on the reviewer's subscription after the engine died. Bounded impact: the one-shot run finishes by itself; the cost is one wasted agent run per killed review. Remedy (mechanical): the engine kills its running agent children on SIGTERM/SIGINT, or the adapters tie the child to the parent's exit.🔧 Fix applied.
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
npx vitest run packages/engine/test/grouping.test.ts packages/engine/test/review.test.ts packages/engine/test/server.test.ts packages/engine/test/agent-children.test.ts packages/engine/test/cli.test.ts (64 tests pass: coverage/invalid-answer fallbacks, concurrent serve, signal handling, CLI guards)npx vitest run packages/extension/test/tree.test.ts packages/extension/test/review-result.test.ts packages/extension/test/engine-client.test.ts (52 tests pass, incl. 'finds the part that now holds the first hunk of the part the reviewer was on' and groupingStatus tests)npm run eval -- --cache-dir/--runs in temp (offline evaluation over all 10 cases: exit 0, coverage 1 on every row, grouping-agreement computed for example-7 + 3 public PRs, baseline 0 dropped/0 missing/66 unchanged)node packages/evaluation/dist/main.js run --agent pi (live evaluation through locally installed Pi 0.86.1 / zai-coding-cn/glm-5.3: 4 model calls, coverage 1 on all agent rows, agreement 0.8482 overall, no fallbacks; trace stamped promptVersion 2)Live serve-protocol drives against node packages/engine/dist/main.js serve on https://github.com/encode/httpx/pull/3690 with a PATH-shimmed fake pi and temp cache: stage notification then agent grouping; left-out hunks -> 'not grouped by the agent'; two invalid answers -> retry once -> plain fallback; sendReview answered at 1.50s while the review finished at 13.65s; SIGTERM to the engine stopped its SIGTERM-ignoring agent childnode packages/engine/dist/main.js review .../pull/3690 --agent pi (fake pi): stderr announces 'plain parts ready; grouping related hunks with pi', stdout result grouping.by=agent with cross-file part + not-grouped partCLI guards: serve --agent bogus -> 'unknown agent' exit 1; review --model/--effort without --agent -> 'tunes the agent; pass --agent' exit 1; serve --model reached the agent child's argsVerified the delivered grouping prompt (recorded in the real-Pi trace and the fake agent's stdin) carries PR title, description, file paths and entity names inside <untrusted-input> blocks with only hunk id and @@ range outside✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.