Repository navigation
feat: show the linked issues' acceptance criteria - #84
Merged
Merged
Conversation
The engine reads the issues a pull request links — the closing references
GitHub returns, which cover the description's closing keywords and the
sidebar's "will close" links in this repository or another, and the issues
the pull request's own timeline shows referencing it — with one GraphQL
query through the same client, and lists each condition from the
checklist under a configurable heading ("Acceptance criteria" by default),
quoted and not checked. GitHub returns no closing references for a pull
request into a non-default branch, and the result says so plainly.
The overview panel lists each criterion with its quote, a button that
opens its issue on GitHub and its "not checked" verdict; an issue read
but listing no checklist under the heading is named. Issue text is
untrusted: it is parsed and never followed, and its hidden content stays
in the quotes, shown and flagged on the page. The heading travels with
each review request from the editor's new second-look.criteriaHeading
setting, and the review command takes it as --criteria-heading. No model
is involved.
The review result's version is 13.
… and overview section
…Ubuntu/Windows) shared one root cause: this PR's extension now reads the new `second-look.criteriaHeading` setting (contributed default "Acceptance criteria") and sends it with every `review` JSON-RPC request, but the two CI-only tests still asserted the old exact params shape, so their deepStrictEqual failed on the extra `criteriaHeading: 'Acceptance criteria'` (the real settings layer returns the contributed default; the stub-based tests correctly omit it because the stub falls back to ''). Fixed by updating the expected review params in packages/extension/test/real-host/run.ts and packages/extension/test/package-smoke/run.ts to include `criteriaHeading: 'Acceptance criteria'`, and adding the field to those tests' logged-request type shapes; the explanatory comment now names the heading alongside the agent choice. Verified: npm run build, both CI compile steps (tsc -p for each test project), tsconfig.test.json type-check, eslint, and the full unit suite (64 files, 1102 tests) all pass; the sendReview params assertion and the stub-driven integration/engine-client expectations were confirmed unaffected siblings. The real-host/package-smoke runners themselves execute only in CI (launching VS Code locally is forbidden by the intent), and the fix is the deterministic expectation update matching the contributed default the CI logs prove the editor reads
A GraphQL failure that GitHub answers as HTTP 200 with data null and an errors array — a rate limit, a permission error — was unwrapped to null before anyone could see the errors, so a failed linked-issues read came back as a confident 'read' result with no linked issue and an empty default-branch name. requestGraphql now hands back the envelope as it arrived and linkedAnswer reads its errors first, so the query's failure throws and readCriteria reports the unreadable outcome with the message; a recorded rate-limit answer proves it. The ReviewParams.criteriaHeading doc now says what the server enforces: a heading that is present must be a non-empty string.
lbildzinkas
changed the title
3 tasks
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
This change closes issue #41, "Show the linked issues' acceptance criteria", one step of Second Look's v1 plan — a VS Code companion for human pull request review whose engine and extension are TypeScript, the engine a separate local process.
Split out of issue #2, it shows the acceptance criteria of the issues a pull request links. The engine reads the issues a pull request links — closing references, sidebar links, and issues in other repositories — finds the checklist under a configurable heading ("Acceptance criteria" by default), and the panel lists each acceptance criterion with its quoted text, a link to its issue and "not checked". Issue text is untrusted: it is parsed and never followed, and its hidden content is flagged. No model is involved.
The acceptance criteria:
Live validation must not launch the VS Code application on this machine: extension integration tests run only in CI, so live validation happens through the engine protocol and unit tests. To try it by hand, review a pull request that closes an issue with an acceptance-criteria checklist; the criteria appear in the panel.
What Changed
criteriafield of the review result (schema v13); the heading is settable via--criteria-headingon the CLI andcriteriaHeadingon the JSON-RPCreviewparams.second-look.criteriaHeadingsetting and an "Acceptance criteria" section to the overview panel: each criterion's quote (untrusted issue text, hidden content shown and flagged), a button opening its issue on GitHub, and a "not checked" verdict, plus a plain detail line that explains what was read — including that GitHub returns no closing references for a pull request into a non-default branch.Risk Assessment
✅ Low: The prior round's single error is fixed by a correct one-line selector scoping with no remaining sibling sites, and the rest of the change is a well-bounded read-only feature with escaping, defensive validation, and behavioral tests at every boundary.
Testing
The engine half of the ticket's acceptance criteria was exercised fully live and passed. The real serve process answered six reviews over its JSON-RPC protocol against a disposable fake GitHub whose transcript confirms the linked-issues GraphQL query, and the results show criteria quoted and not checked under the default and a custom heading, a cross-repository issue's checklist read, the exact non-default-branch message, a GraphQL rate-limit-style failure reported as unreadable without failing the review, and a blank criteriaHeading refused. The panel half was validated through the product's real webview renderer and DOM execution of the emitted script (quote, issue link, 'not checked', hidden content flagged; issue click posts only openIssue) rather than inside VS Code, because the intent forbids launching the editor here and headless Chromium (Brave) cannot start in this sandboxed session; those two in-editor scenarios are therefore recorded as untested, and the rendered webview HTML is provided as the visual artifact instead of a pixel screenshot.
Evidence: Live engine criteria results (all six scenarios, verbatim JSON-RPC answers)
Evidence: Full live review results from the engine serve process
~/.no-mistakes/evidence/01M468WYTTPNYQ0K95EF5HM5KR/fake-github-transcript.json)~/.no-mistakes/evidence/01M468WYTTPNYQ0K95EF5HM5KR/overview-webview-live-result.html)~/.no-mistakes/evidence/01M468WYTTPNYQ0K95EF5HM5KR/overview-webview-non-default-branch.html)Evidence: DOM transcript: criteria section content and posted messages after real button clicks
Evidence: DOM transcript: part buttons still post openPart after the selector scoping
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 1 issue found → auto-fixed ✅
packages/extension/src/overview.ts:328- The criterion's issue button is rendered as<button type="button" class="pt issue" data-issue="...">(overview.ts:328), and the webview script binds the openPart handler to everybutton.pt(overview.ts:733-735):document.querySelectorAll('button.pt')matches the issue buttons too. An issue button has nodata-partattribute, so its click posts{type:'openPart', part: Number(null)}—Number(null)is 0 — beside the intended{type:'openIssue', ...}(overview.ts:739-741). The host's handle() (overview.ts:130-132) then opensparts[0]in the diff editor. Concrete sequence during the change's intended usage: a reviewer clicks a criterion's issue link on the real overview page → the issue opens on GitHub AND the multi-file diff editor for part 0 spuriously opens, every time. The stub-based tests postopenIssuemessages directly and never execute the webview script, so the double binding is untested. Fix by scoping the openPart binding to buttons that name a part (e.g.button.pt[data-part]) or by dropping theptclass from the issue button (keeping its cursor styling under.issue); update the overview/integration tests that assert the exactclass="pt issue"markup accordingly. This is the only site: story (overview.ts:281) and claim (overview.ts:448) buttons all carrydata-part, so the same invariant holds everywhere else.🔧 Fix applied.
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
node live-check/client.mjs — spawned the real packages/engine/dist/main.js serve process, spoke the initialize/review JSON-RPC protocol, and ran six reviews (default heading, custom heading 'Definition of done', non-default branch with a referencing issue, non-default branch with no links, GraphQL failure envelope, blank criteriaHeading) against the disposable fake GitHub; results captured in engine-live-criteria.json and engine-live-full-results.jsonEVIDENCE_DIR=… npx vitest run packages/extension/test/live-panel-render.test.ts — rendered the live engine results through the real overviewHtml (temporary harness test, removed afterwards)node live-check/panel-dom.mjs — loaded the generated overview webview HTML into jsdom, read the criteria section as the DOM renders it, and clicked the issue button; captured in panel-dom-transcript.jsonnode live-check/panel-dom-parts.mjs — clicked every part button of a fully populated overview page rendered by the real overviewHtml; captured in panel-dom-part-buttons.jsonnpx vitest run packages/engine/test/criteria.test.ts packages/engine/test/github.test.ts packages/engine/test/review.test.ts packages/engine/test/server.test.ts packages/engine/test/cli.test.ts packages/engine/test/send.test.ts packages/extension/test/overview.test.ts packages/extension/test/engine-client.test.ts packages/extension/test/review-result.test.ts — 181 existing tests for the changed surfaces, all passing✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.