vscode: review-comment polish (placeholders, reviews coverage, author identity)#900
Merged
Conversation
- plan-review.ts: set controller.options for Codev-specific input copy (prompt + placeHolder) - plan-review.ts: extend ELIGIBLE_PATH_REGEX to include codev/reviews/ so review files get the gutter '+' - plan-review.ts + commands/review.ts: take OverviewCache and read currentUser at write-time for the REVIEW marker author, falling back to 'architect' when Tower hasn't fetched yet or gh is unconfigured - extension.ts: pass overviewCache through to both call sites
Collaborator
Author
Architect ReviewLow-risk polish bundle. 299/15 across 7 files, but only 3 production files:
Net production change: ~34 LOC. Each of the four issue-body items fits cleanly into this budget. CMAP unanimous (gemini=APPROVE, codex=APPROVE, claude=APPROVE), with one cosmetic claude COMMENT on a stale extension.ts comment addressed in Verified
Composes with the recently-merged spec/plan/review workAfter this lands, the integrated review workflow becomes:
VerdictApproved. Per PIR protocol, the Architect review |
amrmelsayed
added a commit
that referenced
this pull request
May 28, 2026
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.
PIR Review: VSCode review-comment polish pass
Fixes #857
Summary
Four-part polish on the native VS Code Comments wiring for
codev/{plans,specs,reviews}/*.md. The inline+input now carries Codev-specific copy,codev/reviews/*.mdfiles joined the eligible-paths set, the REVIEW marker author now flows fromOverviewData.currentUser(Tower's existing GitHub-login identity, same source used by the Backlog view's "assigned to you" sort) with anarchitectfallback, and the Comments-panel aggregation was verified working with the existing controller wiring.Files Changed
packages/vscode/src/comments/plan-review.ts(+22 / -7)packages/vscode/src/commands/review.ts(+7 / -2)packages/vscode/src/extension.ts(+2 / -2)codev/plans/857-vscode-spec-plan-review-commen.md(+141 / -0) — plan artifactcodev/state/pir-857_thread.md(+26 / -0) — builder thread logcodev/projects/857-vscode-spec-plan-review-commen/status.yaml(+22 / -0) — porch stateCommits
fd31172a[PIR vscode: spec/plan review comments — polish pass (placeholders, reviews/ coverage, author identity, panel discoverability) #857] Plan draft52088f5f[PIR vscode: spec/plan review comments — polish pass (placeholders, reviews/ coverage, author identity, panel discoverability) #857] Plan revised — use OverviewData.currentUser for authorac8cdfe3[PIR vscode: spec/plan review comments — polish pass (placeholders, reviews/ coverage, author identity, panel discoverability) #857] Implement vscode review-comment polishb0547869[PIR vscode: spec/plan review comments — polish pass (placeholders, reviews/ coverage, author identity, panel discoverability) #857] Implement: thread log update(Plus porch-managed bookkeeping commits for gate / phase transitions.)
Test Results
pnpm --filter codev-vscode check-types: ✓ passpnpm --filter codev-vscode lint: ✓ passpnpm --filter codev-vscode test: ✓ pass (83 tests, 0 new — see "Architecture Updates")buildcheck: ✓ (6.4s)testscheck: ✓ (20.6s)dev-approvalgate: human approved after exercising the inline+flow on plans, specs, and reviews files, plus the paletteCodev: Add Review Commentcommand and the Comments panel aggregation.Architecture Updates
No
codev/resources/arch.mdchanges — this PR is a polish bundle on existing VS Code wiring; no module boundaries or patterns moved. The one mild design decision worth noting (read author at write-time rather than caching at activation) is local to the comments module and doesn't merit arch-doc real estate.No new unit tests were added. The four changes are: (a) one assignment to a VS Code API surface (
controller.options), (b) one regex literal edit, (c) a one-line read fromOverviewCache.getData()?.currentUserat write-time in two call sites, and (d) a no-op verification. Each is mechanically obvious from the diff; behavior testing happens against the running VS Code Extension Host (covered by the human's exercise of the worktree at thedev-approvalgate). Adding unit harness aroundvscode.comments.createCommentControlleror stubbingOverviewCachefor these one-liners would inflate the test suite without catching anything the dev-approval review wouldn't.Lessons Learned Updates
No
codev/resources/lessons-learned.mdchanges from this PR specifically — the design course-correction (git-config →OverviewData.currentUser) is captured in the builder thread (codev/state/pir-857_thread.md) and was a single-issue redirect, not durable engineering wisdom.That said, the redirect itself is the type of "ask before reaching for a subprocess" pattern that PIR's plan-approval gate exists to surface — the original plan would have shipped a working but parallel identity mechanism, and the reviewer's question redirected to the canonical one with one round-trip. That's the gate doing its job, not a lesson worth documenting.
Things to Look At During PR Review
overviewCache.getData()?.currentUser ?? 'architect'atplan-review.ts:148andcommands/review.ts:25— confirm read-at-write (not at activation) is the right shape. Rationale in the plan's Risks section: Tower can refreshcurrentUsermid-session via SSE, so freezing it at activation would stale-pin the author. Lookup is a synchronous in-memory cache hit; no perf concern.'architect'— chosen so existing committed<!-- REVIEW(@architect): ... -->markers and any new ones written before Tower's first fetch use a consistent placeholder. TheREVIEW_COMMENT_PATTERNregex already matches any@([^)]+), so back-compat is preserved.controller.optionsatplan-review.ts:46-49— VS Code documentsoptionsas a controller-level property; the two strings shape every "+ new" and reply input belonging to this controller. No way to target only plan/spec/review vs. other use surfaces because this controller has exactly one surface.label('Codev Plan Review') andcommentingRangeProvider, which are the only prerequisites for panel inclusion per the VS Code Comments API contract.How to Test Locally
For reviewers pulling the branch:
afx dev pir-857What to verify (mirrors the plan's Test Plan):
+input oncodev/plans/*.mdshows "Type your review comment, then Submit" (Fix 1)+appears oncodev/reviews/*.mdand submit lands a REVIEW marker inline (Fix 2)@architect(Fix 3 — both via the inline+and via theCodev: Add Review Commentpalette command)<!-- REVIEW(@architect): ... -->markers in committed files still render as collapsed comment threads (back-compat)