fix(composer): scope skill discovery to workspace - #8528

💭 I'm reviewing the changed web files—ChatView, ComposerPromptEditor, ChatComposer, terminalContext, and providerSkills—to check if previously flagged issues were resolved and to look for any new problems. Comparing head (41b3e29) against the prior review commit (9aff96c): the plan follow-up skill-invocation drop, the chip-derivation issue, the offset bug, and the readSnapshot approval issue all appear to have fixes in place, though I want to confirm the chip fix is actually correct. I also want to check whether the environment routing for provider skills in ChatView uses the thread's actual environment ID rather than some default.

1. git_tools — 9aff96c..41b3e29

2. github_api_read_only — repos/{owner}/{repo}/pulls/{pull_number}/commits

💭 Since the branch was rebased and squashed, the prior commits no longer exist, so I need to verify whether the earlier finding about the plan follow-up path dropping skillInvocations in ChatView.tsx was actually resolved in the current head commit, since the author claims review follow-ups were folded in.

3. git_tools — skillInvocations|onSubmitPlanFollowUp|composerSkillInvocations|outgoingSkillInvocations

💭 I'm noticing the skillInvocations issue I flagged earlier is still present in the code, but since the branch was rebased, my original comment on that commit may no longer be visible on GitHub. I should check the existing PR review comments to confirm whether that flag is still showing before deciding whether to repost it, since duplicating an already-flagged issue isn't allowed unless the code changed in response.

4. github_api_read_only — repos/{owner}/{repo}/pulls/{pull_number}/comments

💭 The plan follow-up comment is still open at the current head, so I shouldn't repost it. Now I want to check the current versions of the changed web files for any new issues I haven't flagged yet, starting with the terminalContext remapping logic, providerSkills.ts, ChatComposer.tsx, and ComposerPromptEditor.tsx. For providerSkills.ts, I'm wondering whether useProviderSkills's environmentId parameter in ChatView and ChatComposer correctly matches the active environment versus the thread's environment, and whether projectId could get mismatched with an instanceId from a different environment — I need to trace this through ChatView. For terminalContext.ts, I'm checking whether recording output offsets at boundary indices is still correct, and I'm concerned that switching the prompt iteration from a for-of loop (which iterates by code point, preserving surrogate pairs) to index-based access (which iterates by UTF-16 code unit) could break handling of multi-unit characters somewhere downstream. Since the placeholder character is a single code unit, splitting a surrogate pair into two separate appended units still reconstructs the same string, so that particular loop change looks safe. I'm also checking the range validation logic in appendTerminalContextsToPromptWithRanges, noting remapExplicitSkillInvocations is out of scope since packages/shared isn't included, and looking at how ChatComposer's getSendContext falls back between the snapshot value and promptRef when the snapshot is an empty string. Now I want to go back and verify how ChatView sources the environment and instance for useProviderSkills.

5. git_tools — activeProviderSkills|useProviderSkills|activeProviderInstanceId|defaultInstanceIdForDriver|const environmentId

💭 I'm tracing where environmentId comes from in ChatView, checking if it's a thread-scoped prop, and also looking at how skills and projectId flow into the timeline and ChatComposer components nearby.

6. git_tools — environmentId =|environmentId,$

💭 I'm checking that ChatComposer's projectId prop, derived from activeProject, actually matches the environmentId used elsewhere, since ChatComposer might be reused in different contexts where that assumption could break. Since the diff only added the projectId prop at this ChatView call site, I need to search for other usages of ChatComposer to make sure they're updated too, otherwise TypeScript would flag them as missing the now-required prop.

7. git_tools — <ChatComposer

8. git_tools — ChatComposer

💭 Confirmed there's only one call site. Now I want to double check the ComposerPromptEditor changes, particularly whether the offset calculations align with the prompt value returned by the snapshot, since expanded placeholders might shift things.

9. git_tools — function getExpandedAbsoluteOffsetForPoint

💭 I'm wondering whether getExpandedAbsoluteOffsetForPoint correctly accounts for newline separators inserted between paragraphs when computing text content, so I want to trace how the snapshot value is actually generated to verify this.

10. browse_code — apps/web/src/components/ComposerPromptEditor.tsx

💭 I'm wondering whether skill invocation offsets, which rely on expanded text lengths for terminal context nodes, could mismatch the collapsed getTextContent() string offsets used in the snapshot value—since those two representations may differ in length for the same nodes.

11. git_tools — function getComposerNodeExpandedTextLength|function getComposerNodeTextLength

12. git_tools — function getComposerInlineTokenExpandedTextLength|function getComposerInlineTokenTextLength

💭 I'm reasoning that the expanded length of an inline node equals its text content size, so using the expanded offset function should align correctly with the string built from root text content. I need to check what getTextContent returns for special nodes like skill nodes (likely a name reference) and terminal context nodes (possibly a placeholder character). Actually there's a real concern here: if a skill chip's text content is a display name rather than the raw $name form, the downstream validation that slices the prompt string expecting $${name} would fail, causing invalid skill invocations and a validation error. I want to check ComposerSkillNode's getTextContent implementation to confirm whether it preserves the raw name format.

13. git_tools — class ComposerSkillNode

14. browse_code — apps/web/src/components/ComposerPromptEditor.tsx

💭 I'm double-checking how Lexical joins text content across multiple paragraphs, since root/element nodes may insert double newlines between sibling blocks, which affects how ranges align with the raw text. But this composer uses a plain-text plugin, where Enter typically creates line breaks within a single paragraph rather than separate paragraphs — though pasted multi-line text might behave differently since insertRawText could split on double newlines into separate paragraphs, which would throw off my offset calculations. Actually, recalling Lexical's insertRawText more carefully, it just splits on single newlines and tabs, creating line break nodes rather than new paragraphs, so everything stays in one paragraph and the offset math holds. So this isn't a new bug, just consistent pre-existing behavior matching cursor mapping elsewhere. I'm now wondering whether the metadata filtering for enabled skills changes anything about how skill chips render when their skill becomes disabled. Since enabledSkillMetadataByName restricts metadata to enabled skills, a disabled skill token would lack matching metadata, and $setComposerEditorPrompt would fall back to a plain-name label via formatProviderSkillDisplayName. I need to check whether it still renders as a chip at all in that case—if so, the user would see what looks like an invokable skill chip that's actually excluded from collectExplicitSkillInvocations, silently becoming plain text. I should also check whether disabled skills even appear in the picker's search results in the first place. Without metadata, a disabled skill chip falls back to showing the raw name rather than the formatted display name, and loses its description tooltip — this could show up if a draft references a skill that later gets disabled. It's a real behavior change from the diff, though possibly intentional rather than a bug worth flagging. I want to check how skills get marked disabled and whether disabled ones even render as chips, so I'm going to look at the setComposerEditorPrompt logic.

15. git_tools — function $setComposerEditorPrompt

💭 I realize chips still render for any $token pattern regardless of whether it's a known skill, so something like $PATH would visually appear as a chip too — but that's pre-existing rendering behavior. With the fix, this chip displays visually but isn't sent as an actual invocation, which matches the intended design of treating unrecognized tokens as plain text for the model. Now I'm digging into whether disabled skills lose their proper display name — checking if formatProviderSkillDisplayName behaves differently when given a full skill object versus just a name, since disabled skills might fall back to a plainer label. I want to trace how "enabled" gets set during skill discovery, particularly whether Codex or Claude skills marked with disable-model-invocation actually report as disabled in the metadata. This surfaces a real bug: Codex intentionally marks user-invocable-only skills as disabled, and the server's explicit-invocation path is built specifically to still allow those via allowDisabled: true. But now the composer filters disabled skills out before collecting explicit invocations, meaning a $skill chip for one of these Codex skills would render but never actually trigger the skill — it'd just get sent as plain text. I need to check whether the $ menu itself filters by enabled status too, since that determines whether users can even select these skills in the first place.

16. browse_code — In apps/web (and packages/shared), show the implementation of searchProviderSkills and formatProviderSkillDisplayName: does searchProviderSkills filter out skills whose enabled is false? Also show parseCodexSkillsListResponse in apps/server/src/provider/Layers/CodexProvider.ts and how it sets the enabled field of ServerProviderSkill, and how ClaudeSkills discovery sets enabled.

💭 I'm noting that the $ picker filters out disabled skills on web, so a chip for a disabled skill could only appear via typed text or a persisted draft — mobile behaves differently but that's outside this scope. Since the server already supports disabled skills via allowDisabled, and web's picker simply never surfaces them, this seems consistent with existing behavior rather than a real issue. Let me shift to reviewing the terminalContext.ts changes for correctness, checking the current state of that file region.

17. browse_code — apps/web/src/lib/terminalContext.ts

💭 That looks right, though dropped placeholders with no matching context still leave the offset map fine. Now I need to trace the ChatView flow after terminal materialization—if appendElementContextsToPrompt prepends text before the terminal context prompt, the ranges measured against the original prompt could become misaligned. I want to verify whether remapExplicitSkillInvocations then compares against the shifted messageTextForSend, since its guard checking that the sliced range equals "$name" would fail and silently drop the invocation if there's a prepend. Let me check the ChatView code around that area.

18. browse_code — apps/web/src/components/ChatView.tsx

💭 I need to check whether the append helper functions only add text at the end, since prepending would break the offset math and drop invocations. I also need to verify that formatOutgoingPrompt doesn't prefix the text with model directives, since remapExplicitSkillInvocations depends on the outgoing text ending with the trimmed source.

19. browse_code — Show the implementations of appendElementContextsToPrompt, appendPreviewAnnotationPrompt, and appendReviewCommentsToPrompt used by apps/web/src/components/ChatView.tsx. For each, state whether the original prompt text is placed at the beginning of the returned string (suffix-only append) or whether anything is prepended before it. Also show formatOutgoingPrompt in ChatView.tsx.

💭 I'm double-checking that the terminal context prompt is already trimmed before ranges are computed, so there's no offset shift, and now I'm verifying the ranges get correctly remapped back onto the original message text using the same source. Other message-sending paths like queued or retry sends don't seem to introduce new issues here. I'm checking whether ChatComposer's use of provider skills correctly scopes to thread, project, and environment, and it looks fine, along with the staleTime settings on both client and server caches. Now I've noticed the mobile and web versions of providerSkills.ts are byte-identical, newly created files — that's a duplication concern worth flagging, especially since the repo already has a shared client-runtime package for common code. The question is whether their underlying useEnvironmentQuery hooks come from that shared package too, since if so, the duplicated 32-line hook could easily be consolidated there instead of being a borderline nitpick.

20. browse_code — Compare apps/web/src/state/query.ts and apps/mobile/src/state/query.ts: is useEnvironmentQuery defined separately per app or re-exported from packages/client-runtime? Also, are there other hooks that exist as byte-identical copies in both apps/web/src/state and apps/mobile/src/state (e.g. thin wrappers over serverEnvironment atom families)? List a couple of examples if so.

💭 Duplication seems to be the established pattern here rather than a real issue. Checking ComposerPromptEditor, the enabledSkillMetadataByName function is used consistently across ComposerSurroundSelectionPlugin and initialSkillMetadataRef, and skillSignature already accounts for the enabled flag, so chip rewrites happen correctly when that changes. Now I'm tracing how getSendContext handles skillInvocations against different send paths — it falls back appropriately during approval states, and the offsets stay consistent with snapshot.value. I want to check whether web's queued-message flow, where text is sent later while the thread is busy, might bypass skillInvocations entirely, so I'm searching for other startThreadTurn calls in ChatView.

21. git_tools — startThreadTurn(|enqueueThreadMessage|message: {

💭 I'm checking whether other web composer paths besides ChatComposer send text, considering things like PullRequestThreadDialog or draft hooks, but it looks like ChatComposer is the only relevant one on web. Since web doesn't persist skill invocation ranges across reloads, chips get re-derived from text via the skill token regex and metadata, so collectExplicitSkillInvocations recovers them from live editor nodes without needing persistence. One divergence worth noting: because web derives chips from plain text, manually typing a known skill name with surrounding whitespace creates a chip and counts as an explicit invocation, which seems intentional given chips are visible in web, so not a real issue. Now I want to check whether promptForSend gets transformed before terminal context injection — things like mention path expansion or other modifications between the snapshot value and where terminal contexts get appended — so I should trace all the places promptForSend is used.

22. git_tools — promptForSend

💭 Good, the restore path repopulates draft text and chips correctly. I should verify that deriveComposerSendState and trimmed remain unaffected, then check whether sendCtx.prompt is reliably fresh compared to promptRef.current, especially in cases where the composer might not be mounted. Actually I want to trace whether there's a race between programmatic prompt updates (like from the command palette or resetCursorState) and the editor's controlled re-render — if a send happens in the same tick as one of these updates, the snapshot could lag behind promptRef. I should look for a flow where something like setPrompt is called immediately followed by a send trigger to confirm this scenario actually exists.

23. git_tools — getSendContext|submitFromExternal|onSendAnnotation

💭 I'm also checking whether other call sites like the plan follow-up handler use getSendContext similarly, and now I'm considering whether calling readSnapshot() inside getSendContext could have an unintended side effect since it mutates snapshotRef.current, potentially suppressing a subsequent read elsewhere. Actually, I think this scenario is unlikely to matter since handleEditorChange typically keeps snapshotRef current before send is triggered, so this isn't worth flagging as a bug. I'm now looking at whether readSnapshot inside getSendContext could leave promptRef stale after a send failure. Now I'm tracing a subtler staleness path: when skills load async and skillMetadataRef updates via useLayoutEffect, the controlled editor update sets isApplyingControlledUpdateRef so handleEditorChange bails out without refreshing snapshotRef's skillInvocations — meaning snapshotRef could hold stale skill invocation data even though the prompt content itself was reset correctly. But since getSendContext always calls readSnapshot() to recompute live from the editor rather than trusting the cached snapshot, send-time behavior stays correct regardless of this staleness. I'm confirming the equality check in handleEditorChange properly includes skillInvocations length, so any mismatch after the controlled update triggers a real recompute on the next edit rather than a false early return. Let me double check I haven't missed new findings in the latest commits touching web files—since 9aff96c is gone, I should look at the diff between the last two commits, 748a58e and 41b3e29, for anything in the web directory.

24. git_tools — apps/web/** f342eb1..41b3e29

💭 I'm looking at the newest workspace-scoping commit, which is the actual PR branch, and checking the environment-routing logic. I notice ChatView derives activeProviderSkills from the route's environmentId, but activeThread might belong to a different environment—elsewhere in the code, thread scoping explicitly uses activeThread.environmentId instead, so I want to check for a mismatch here. For drafts, though, environmentId still comes from the route/draft selection and activeProject is derived from that same environment, so they should stay consistent, and startThreadTurn uses the same route environmentId too. There's also a subtlety with ChatComposer receiving activeThreadId for drafts, which may be a client-only ID that doesn't exist server-side—but that seems handled gracefully since the server falls back to the project root when it can't find the thread shell. The bigger question is whether ChatView and ChatComposer might diverge on their provider skills queries: ChatView derives its instance id from activeProviderInstanceId with a fallback, while ChatComposer uses its own selectedInstanceId, and if these differ the skill chips shown in the timeline could disagree with what's shown in the composer's picker. I need to trace how ChatComposer computes selectedInstanceId to see if it actually matches ChatView's value.

25. git_tools — const selectedInstanceId|const activeThreadId

💭 That divergence risk seems small since both paths resolve to the same instance normally. Looking at the workspace-scoping commit's UI behavior, I notice useProviderSkills falls back to the environment-wide snapshot skills whenever the query is pending or errors, which is reasonable graceful degradation—though I should also check whether the hook could get invoked twice with different keys, triggering duplicate RPCs. That stale-time overlap seems minor, more a perf note than a real finding, and the hook-rules usage with a null atom looks fine. The bigger concern is that ChatComposer builds its fallback array as a fresh literal each render, unlike ChatView's stable EMPTY_PROVIDER_SKILLS—so when there's no data and no status skills, selectedProviderSkills becomes a new array every render, which would break memoization on the commandItems useMemo and the skills prop passed downstream, causing needless recomputation. The signature-based effect avoids a rewrite loop, but if ComposerPromptEditor is memoized, this identity churn would break that memoization on every parent render — worth checking, though it re-renders on prompt changes regardless. The commandItems recompute is guarded by trigger kind so it's cheap when idle, but the inconsistency with ChatView's module-level EMPTY_PROVIDER_SKILLS constant is still worth flagging as a small fix. Actually the fallback to an empty array only kicks in when the provider status is missing entirely, which is rare, so this churn is low-impact and not worth flagging as a concrete regression. I want to check whether workspace-scoped skills create broader inconsistency in the UI beyond the composer picker and chat timeline — checking if other places still reference the old provider-status-based skills field, like the settings page.

26. git_tools — ?.skills|.skills ??|skills={

💭 Everything checks out consistent, and the only remaining open issue is the previously flagged plan follow-up path. Since the output must conform to the JSON schema, I'll follow that requirement over the literal "All clear" instruction when there are no new findings. Given the outstanding finding still persists at head — confirmed by re-checking that onSubmitPlanFollowUp lacks skillInvocations and onSend still routes through that path when a plan is pending — I'll set the state to failure, summarizing this as a previously identified, still-unresolved issue without posting a duplicate comment or flagging anything new.