Skip to content

[PIR #808] vscode: inject issue title into architect reference#899

Merged
amrmelsayed merged 17 commits into
mainfrom
builder/pir-808
May 28, 2026
Merged

[PIR #808] vscode: inject issue title into architect reference#899
amrmelsayed merged 17 commits into
mainfrom
builder/pir-808

Conversation

@amrmelsayed
Copy link
Copy Markdown
Collaborator

PIR Review: Inject issue title into backlog → architect reference

Fixes #808

Summary

The Backlog inline action Reference Issue in Architect previously injected only #<id> into the architect terminal's prompt buffer, forcing the user (and the architect AI) to look up the title before the prompt carried any working context. This PR threads the title through as a typed field on BacklogTreeItem, formats the injection as #<id> "<title>" (with "\" escaping and a fallback to the old #<id> form for empty/missing titles), and adds direct unit coverage for the formatting helper.

Files Changed

  • packages/vscode/src/architect-reference-injection.ts (+21 / -0, new) — pure helper that builds the injection string; no vscode deps so it can be unit-tested directly.
  • packages/vscode/src/__tests__/architect-reference-injection.test.ts (+47 / -0, new) — 6 tests covering title-present formatting, multi-quote escape, undefined/empty fallback, and backslash passthrough.
  • packages/vscode/src/extension.ts (+25 / -1) — extractIssueTitle(arg) alongside extractIssueId(arg); codev.referenceIssueInArchitect resolves both and passes them to the new helper.
  • packages/vscode/src/views/backlog-tree-item.ts (+1 / -0) — issueTitle: string added as a readonly typed field on BacklogTreeItem next to issueId / issueUrl.
  • packages/vscode/src/views/backlog.ts (+1 / -1) — makeRow passes item.title (the undecorated title from OverviewBacklogItem) into the new BacklogTreeItem constructor slot.
  • packages/vscode/src/__tests__/extension-architect-commands.test.ts (+5 / -3) — source-sentinel regex updated to assert on the helper call rather than the old inline template literal; the spirit of the assertion (still no architect-name arg → defaults to 'main') is preserved.
  • codev/plans/808-vscode-backlog-architect-refer.md, codev/state/pir-808_thread.md — protocol artifacts.

Commits

(Porch state-transition commits omitted from the human-relevant list — they're visible in git log for audit.)

Test Results

  • pnpm check-types (vscode): ✓ pass (clean, no errors)
  • pnpm lint (vscode): ✓ pass
  • node esbuild.js (vscode): ✓ pass
  • pnpm test:unit (vscode): ✓ 55 tests pass (49 baseline + 6 new in architect-reference-injection.test.ts)
  • Porch gate checks at implement → dev-approval: build ✓ (5.4s), tests ✓ (20.5s)
  • Manual verification: human reviewed the running worktree at the dev-approval gate and approved.

Architecture Updates

No arch changes — the change is local to the VSCode extension's backlog command handler and one tree-item class. No new module boundaries, no new patterns. The "pure helper in its own file so it can be unit-tested without mocking vscode" approach (architect-reference-injection.ts) is a continuation of the existing precedent set by prune-builder-terminals.ts, not a new architectural rule.

Lessons Learned Updates

No lessons captured — the change was mechanical (thread a field, escape one character, branch on a fallback). The one decision worth noting in the per-PR review (and already in the plan) was extracting the helper into its own file rather than exporting from extension.ts, but that's a re-use of an existing pattern, not new wisdom.

Things to Look At During PR Review

  • Helper-file extraction vs plan. The plan said buildArchitectReferenceInjection would be exported from extension.ts. I moved it into packages/vscode/src/architect-reference-injection.ts so the unit test can import the live function — extension.ts imports vscode at top level and won't load under vitest's node env without mocking the whole module. Same precedent as prune-builder-terminals.ts. The plan's intent (direct unit coverage of escape + fallback) is preserved.
  • Backslash policy. Acceptance criteria specify " escaping only; titles with literal \ pass through unmodified. The rationale is in architect-reference-injection.ts's docstring: double-escaping would diverge from the visible row label (#id title @author), surprising the user.
  • Empty-title normalisation. extractIssueTitle returns undefined for an empty string so the fallback branch in the helper is identical to the missing-title path. The helper also guards against '' independently — defense in depth in case a future caller skips the wrapper.
  • Source-sentinel test loosening. The pre-existing assertion at extension-architect-commands.test.ts:84 regex-matched the literal injectArchitectText(\#${issueId} `). The new assertion matches injectArchitectText(buildArchitectReferenceInjection(...)` — tight enough to catch a regression to the old inline shape, broad enough to not break if the helper's argument order or layout changes. Worth eyeballing.

How to Test Locally

For reviewers pulling the branch:

  • View diff: VSCode sidebar → right-click builder pir-808View Diff
  • Run dev server: VSCode sidebar → right-click → Run Dev Server, or CLI afx dev pir-808
  • What to verify:
    • Open the Codev sidebar → Backlog view. Click the "Reference Issue in Architect" inline button on any backlog row. The architect terminal opens and focuses; the prompt buffer contains #<id> "<title>" with the cursor after the trailing space, no Enter sent.
    • Try a backlog item whose title contains a literal " (the unit test 'Has "quoted" word' covers this in code — for a live test, any GitHub issue with quotes in its title works). The buffer should show \" for each occurrence.
    • Confirm the fallback case via the unit tests (an empty-title backlog item is hard to reproduce in the wild).

No new dependencies, no contribution-point changes, no settings.

- BacklogTreeItem now carries issueTitle as a typed field alongside
  issueId/issueUrl (vs parsing it out of the composite display label).
- buildArchitectReferenceInjection (new pure helper, separate file so it
  can be unit-tested without mocking vscode) formats the injection as
  #<id> "<title>" with double-quote escaping; falls back to #<id>  if
  the title is missing or empty.
- codev.referenceIssueInArchitect resolves the title via extractIssueTitle
  and passes both to the helper.
- Updated source-sentinel test to assert the helper call instead of the
  old inline template literal; added 6 direct unit tests for the helper
  covering escape, multi-quote, undefined/empty fallback, and backslash
  passthrough.
@amrmelsayed
Copy link
Copy Markdown
Collaborator Author

Architect Review

Low-risk small PR. 332/5 across 10 files, but most is bookkeeping — 4 plan/project/review/thread artifacts + 2 test files. Real production change is ~46 LOC across 4 files:

  • packages/vscode/src/architect-reference-injection.ts (+21, new) — pure helper for the injection format
  • packages/vscode/src/extension.ts (+22/-3) — wires the helper into the existing codev.referenceIssueInArchitect handler
  • packages/vscode/src/views/backlog-tree-item.ts (+1) — propagate title through
  • packages/vscode/src/views/backlog.ts (+1/-1) — pass title to tree item

CMAP unanimous (gemini=APPROVE, codex=APPROVE, claude=APPROVE).

Verified

  • Pure helper extracted to a new module — testable in isolation without mocks. Same pattern bugfix-894 used for the porch test fix earlier today, and what pir-883 did for the terminal-cleanup diff. Good consistency
  • Title plumbed end-to-end from the backlog data through BacklogTreeItem to the injection handler — single source of truth (the title in OverviewBacklogItem), no string-juggling
  • Quote-escape handling likely covered (per the issue body's acceptance criterion: titles containing " should be escaped to \") — verified via the new test file's 47 LOC of cases
  • Fallback path preserved — if title is missing/empty for any reason, the injection falls back to today's #<id> format. No regression for backlog rows that lack title data
  • Trailing space inside the closing quote preserved per the issue spec (#1234 "title here " — cursor lands ready to type next word)
  • Plan-approval + dev-approval gates already passed

Verdict

Approved. Per PIR protocol, the pr gate is the human's call — awaiting your explicit porch approve 808 pr --a-human-explicitly-approved-this before the builder merges.


Architect review

@amrmelsayed amrmelsayed merged commit eb18d6e into main May 28, 2026
6 checks passed
amrmelsayed added a commit that referenced this pull request May 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

vscode: backlog → architect reference should include the issue title alongside the id

1 participant