Skip to content

fix(composer): open PR completion after opening brackets - #1

Merged
amv146 merged 7 commits into
mainfrom
fork/upstream-30296-bracket-completion
Nov 8, 2026
Merged

amv146 merged 7 commits into
mainfrom
fork/upstream-30296-bracket-completion

Conversation

@amv146

@amv146 amv146 commented Nov 8, 2026 •

Copy link
Copy Markdown
Owner

Problem

Typing (# or [# in the composer does not open pull request completion. Token-start-only matching rejects references after opening brackets, and inserting a reference inside square brackets can consume the surrounding opening bracket.

Change

Port pingdotgg#30296 (pingdotgg#30296) by freshly cherry-picking its two feature commits, f9430b73ec1f07e66257e1f5a866d543ab7b867d and b52b804d8ba579174193a0426ecc05b9bdb90214. The original commits are ancestors of fork main but were reverted in ac9a65b0be; this restores only their five-file composer change, not the unrelated Azure DevOps integration fix.

Share the PR matcher across web/desktop and mobile's shared detector. Permit Unicode opening punctuation before #, retain the existing query syntax and word/ordinary URL hash exclusions, and start replacement exactly at #. Exclude opening square brackets from context-link labels to preserve surrounding brackets on insertion.

Retain the newer slash, currency, and side-chat parsing and tests; the only cherry-pick conflict was combining parseSideChatCommand and replaceTextRange imports. Preserve the original authorship and Copilot co-author trailers. No workflow, dependency, contract, provider, or documentation behavior changes in the net PR diff.

The first hosted run exposed a shared checkout blocker before installation or validation. The fork owner corrected it separately on main in 852618b095. Main was merged normally in 63f6a524b7, discarding this branch's duplicate workaround; the PR's workflow matches main exactly, and its net diff remains the five composer files.

Scope and approval

The fork owner explicitly approved this port in the owning T3 Code thread: "Own the port of pingdotgg#30296 ... into amv146/t3code" and "merge into amv146/t3code main ONLY when latest-head readiness is established." This is one of the five separately requested fork PRs, including upstream drafts. The approval applies only to this fork; the upstream PR and its original branch remain untouched.

This is a focused repair of existing PR completion and directly coupled context-link parsing, not a new capability.

Verification

Local Windows validation on the port head:

  • Before the picks, direct execution of the shared detector returned null for (#, [#, and 【#修正, reproducing the defect. After the picks, dependency-free Node assertions passed for exact bracketed trigger ranges/replacement, word and URL exclusions, currency and slash-path exclusions, and retained /btw parsing.
  • git diff origin/main --check - passed.
  • vp test run packages\shared\src\composerTrigger.test.ts packages\shared\src\composerContextReferences.test.ts apps\web\src\composer-logic.test.ts apps\web\src\composer-editor-mentions.test.ts apps\web\src\composer-rich-text-doc.test.ts - blocked before tests ran: the local workspace has no vite-plus installation.
  • vp run check:changed origin/main - blocked before checks ran by the same missing workspace dependencies.
  • vp install --frozen-lockfile - failed: the corporate package feed returned HTTP 404 for @alchemy.run/test during the existing supply-chain policy check. No policy bypass, registry workaround, manifest change, or lockfile change was made.

GitHub-hosted validation on the previous head 63f6a524b71372927372020f1f98fedf76c5bea7:

  • vp run check:changed "$BASE" with BASE=852618b095a70cf480d99831708eb40f18f4c240 - passed. Format, lint, both knip stages, 34 affected-package typechecks, selected tests, and React Compiler coverage all passed. Chromium installation succeeded.
  • The selected test stage passed 152 tests in four files: shared trigger (53), shared context references (37), web composer logic (60), and agent context budget (2). The targeted React Compiler case also passed. This run did not select the unchanged web mentions or rich-text suites; their local attempts remain dependency-blocked, and their upstream results below are historical.

The initial run failed before installation/tests with fatal: No url found for submodule path '.repos/alchemy-effect/submodules/distilled' in .gitmodules. The separate main fix 852618b095 is now integrated, with no duplicate workflow workaround retained. Legacy upstream Blacksmith jobs and all validation policies are retained unchanged.

CodeRabbit's substantive full review covered all five changed files through 63f6a524b71372927372020f1f98fedf76c5bea7 and reported no actionable comments.

Fork main subsequently advanced with the separate glass-opacity port. It was integrated by normal merge 29bb8e6e791294c992dc4f5f2637131314e87b9a; the net composer diff is still the same five files.

  • Latest-head vp run check:changed "$BASE" with BASE=668c823825ec172e158add923c7f09951e97e356 - passed. All stages passed again, including 152 tests across four files and 34 affected-package typechecks.

CodeRabbit's follow-up substantive full review covered all five files on 29bb8e6e791294c992dc4f5f2637131314e87b9a, completed successfully, and again reported no actionable findings.

Main then advanced with the separate Azure DevOps checks port. Normal merge a72be5a9e5 preserves that work while retaining the same five-file composer diff. CodeRabbit's substantive full review of that head completed cleanly. Macroscope is suspended and is not part of the review gate.

The fork owner serialized the remaining merges and provided final target b445f5ae88f863645a32eef79a9172c8dd2ac60d, including the separate location-cache port. Normal merge e69260754f10953224da703b0d1928a3cc7d6542 contains that target, with the same five-file net composer diff and no workflow changes.

  • Final-head vp run check:changed "$BASE" with BASE=b445f5ae88f863645a32eef79a9172c8dd2ac60d - passed, including all stages, 152 tests across four files, and 34 affected-package typechecks.

CodeRabbit's final substantive full review completed successfully on e69260754f10953224da703b0d1928a3cc7d6542, covering all five changed files against b445f5ae88f863645a32eef79a9172c8dd2ac60d, with no actionable findings. The review's explicit coverage metadata names this exact head; this is not a trigger acknowledgment or an incremental skip.

At completion readiness, the branch contains freshly fetched main, no review threads or change-request votes remain, and GitHub reports no configured branch-protection/ruleset requirements or required checks. The latest-head Fork PR validation and CodeRabbit checks both succeeded. The unrelated native-fingerprint Blacksmith job remains queued and is not a configured required gate; no check was disabled or bypassed. CodeRabbit's included-limit warning does not establish whether this review incurred billing; no billing was enabled or further triggers issued after the owner's no-paid-review instruction.

The five existing focused suites listed above are retained without static UI or duplicate-layer tests. The original upstream PR records 427 passing tests across these suites and integrated composer verification; those are historical upstream results, not validation of this fork head.

Reused upstream UI evidence, captured for the original implementation in an isolated Windows web client through T3 Code's Browser panel. These are not new fork captures. The web composer is also used by desktop; native mobile and the Electron shell were not separately exercised. No real browser was opened for this port, and no live userdata or shared pqtest browser was used.

Before: no selector after an opening parenthesis.

Upstream before

After: selector opens and displays the real repository PR.

Upstream after

After selection: both surrounding square brackets are preserved.

Upstream bracketed insertion

Upstream condensed interaction recording:

pr-selector-ui-verification-redacted.mp4

Ported with GPT-6.1 Sol through GitHub Copilot in T3 Code's acpRegistry harness. No subagents.

amv146 and others added 2 commits November 8, 2026 11:45
Recognize Unicode opening punctuation before # and share PR trigger matching across web, desktop, and mobile. Preserve surrounding brackets when inserting a completion.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
(cherry picked from commit f9430b7)
Exclude opening square brackets from context-link labels so inserting a PR inside square brackets does not consume the surrounding opening bracket.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
(cherry picked from commit b52b804)
@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:M labels Nov 8, 2026
Alex Vallone and others added 2 commits November 8, 2026 11:49
Keep checkout default read-only credentials until post-job cleanup, avoiding fatal traversal of undeclared vendored gitlinks before validation.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use origin/main workflow verbatim, removing the duplicate composer-branch checkout workaround.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@amv146

amv146 commented Nov 8, 2026

Copy link
Copy Markdown
Owner Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Nov 8, 2026 •

Copy link
Copy Markdown
Action performed

Full review finished.

@coderabbitai

coderabbitai Bot commented Nov 8, 2026 •

Copy link
Copy Markdown

Explain Change →

No actionable comments were generated in the recent review.

Walkthrough

The shared composer trigger module now detects pull-request queries after supported opening punctuation. The web composer delegates to this detector. Context-reference parsing rejects opening brackets within labels, with added tests for review-comment references after two prefixes.

Changes: Pull-request trigger detection

Layer / File(s) Summary
Pull-request trigger matching and integration
packages/shared/src/composerTrigger.ts, packages/shared/src/composerTrigger.test.ts, apps/web/src/composer-logic.ts
The shared module detects supported # queries at the start of input or after opening punctuation. Tests cover matching, replacement, and non-matching cases. The web composer uses the shared detector.

Context-reference parsing

Layer / File(s) Summary
Context-reference label matching
packages/shared/src/composerContextReferences.ts, packages/shared/src/composerContextReferences.test.ts
The context-link pattern excludes opening brackets from labels. Tests check review-comment reference metadata and source offsets after See [ and See [[.

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: juliusmarminge, t3dotgg

Merge Risk: Minimal · up to e6926

Bracket-prefixed PR completion preserves the surrounding bracket, and no merge-blocking issue is established.

Security architecture risk: Low · up to e6926

The change primarily expands where existing pull-request completion activates and preserves surrounding brackets. Inspected query and reference paths retain their existing identity checks. No introduced security issue was established, but failure recovery and concurrent selection behavior were not fully verified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The demonstrated exposure change is additional composer-text positions activating existing pull-request lookup and insertion. Typed bracket-prefixed queries do not select a different environment, project, or repository in the inspected web request construction. This is not a verification of backend authorization or every client consumer.

Trust Boundaries and Controls

  • observed — In the inspected attachment and provider-projection consumers, a parsed context ID selects an existing record rather than creating attachment ownership or privileged payload data. Unknown IDs do not resolve attachment records, and provider projection represents unmatched references as unavailable. These controls predate this PR.

Resilience and Maintainability Implications

  • observed — The existing web selection path checks current menu membership and expected source text, and registers review context only after replacement succeeds. Prompt replacement and context registration remain separate steps; their complete failure recovery was not verified.
Pre-merge checks: Passed 4

Check name Status Explanation
Title check Passed The title "fix(composer): open PR completion after opening brackets" matches the main change in the summary: the composer now detects a trailing # after opening punctuation, such as ( or [, through a …
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Description check Passed The description follows the repository template and covers Problem, Change, Scope and approval, and Verification. The Problem and Change sections are clear and tie to the code: token-start-only matchi…
Full details: Title check

Explanation

The title "fix(composer): open PR completion after opening brackets" matches the main change in the summary: the composer now detects a trailing # after opening punctuation, such as ( or [, through a shared detectComposerPullRequestTrigger helper. The title is a conventional commit, short, and specific. It does not cover the square-bracket label exclusion in collectComposerContextReferences, but that change supports the same fix, so the title is accurate.

Full details: Description check

Explanation

The description follows the repository template and covers Problem, Change, Scope and approval, and Verification. The Problem and Change sections are clear and tie to the code: token-start-only matching rejects references after ( or [, and square-bracket label handling drops the opening bracket on insertion. The Change section names the shared matcher, the Unicode punctuation rule, the replacement start, and the [ exclusion in context-link labels. The Scope section gives a direct owner approval quote and explains why the change is a focused repair of existing PR completion. The Verification section lists local attempts, including blocked runs and a Node assertion pass, plus hosted CI runs with links, test counts, and before/after screenshots. The template's required agent-model line is present. The description is long and carries a lot of history about fork integrations, but the required sections are all substantively filled. Minor gaps: the Problem section keeps the template's placeholder comment with no added reproduction environment, and the Verification section does not state the Windows browser checks were not re-run on this head beyond the reused upstream evidence, though it does say so.

Finishing Touches

Generate docstrings

  • Commit to this branch
  • Create a new PR
Generate unit tests (beta)

  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Recent review info

Run configuration

Commits

Reviewing files that changed from the base of the PR and between b445f5a and e692607.

Files selected for processing: 5

  • apps/web/src/composer-logic.ts
  • packages/shared/src/composerContextReferences.test.ts
  • packages/shared/src/composerContextReferences.ts
  • packages/shared/src/composerTrigger.test.ts
  • packages/shared/src/composerTrigger.ts

Limit details: You’ve used all 10 included reviews currently available.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@amv146

amv146 commented Nov 8, 2026

Copy link
Copy Markdown
Owner Author

@coderabbitai full review

Follow-up after integrating current fork main: please substantively review latest head 29bb8e6. The automatic incremental run was skipped with "No files to review", while completed full-review coverage still names 63f6a52. The net diff is still the five composer files. This is a latest-head re-review request after the completed initial review, not a duplicate initial trigger.

@coderabbitai

coderabbitai Bot commented Nov 8, 2026 •

Copy link
Copy Markdown
Action performed

Full review finished.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@amv146

amv146 commented Nov 8, 2026

Copy link
Copy Markdown
Owner Author

@coderabbitai full review

Latest-head follow-up after the previous substantive full review completed cleanly: fork main advanced with the separately reviewed Azure DevOps checks PR, now integrated at a72be5a. Please review the five-file composer diff against current main; this ensures substantive coverage of the actual final head, not merely a no-files incremental skip.

@coderabbitai

coderabbitai Bot commented Nov 8, 2026 •

Copy link
Copy Markdown
Action performed

Full review finished.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@amv146

amv146 commented Nov 8, 2026

Copy link
Copy Markdown
Owner Author

@coderabbitai full review

The preceding full review finished cleanly on a72be5a, while the final serialized main integration is e692607. Its automatic incremental check skipped with no new files. Please substantively review this latest head's five-file composer diff against b445f5a. Latest-head Fork PR validation has passed. Main is now held stable, so no further integration is expected before completion.

@coderabbitai

coderabbitai Bot commented Nov 8, 2026 •

Copy link
Copy Markdown
Action performed

Full review finished.


Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 24 minutes.

@amv146
amv146 merged commit 5cb8de1 into main Nov 8, 2026
11 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant