feat(coding-agent): keep shadowed same-name skills independently selectable - #2409
feat(coding-agent): keep shadowed same-name skills independently selectable#2409yangjj-iso wants to merge 2 commits into
Conversation
…ctable When multiple skill files declare the same frontmatter name, Atomic currently keeps only the precedence winner and discards the loser. This makes the shadowed skill completely inaccessible without renaming or moving files. This change retains collision losers in a shadowedSkills array and introduces a skill catalog (skill-catalog.ts) that exposes source-qualified selectors like /skill:tdd@user and /skill:tdd@builtin. The bare /skill:name continues to select the precedence winner. Autocomplete, pi.getCommands(), RPC get_commands, extension bindings, and collision diagnostics all share the same qualified identities. A qualified selector is exact: an unknown or ambiguous alias reports an error and never falls back to the bare winner. The skill block parser gains an optional candidate attribute for internal identity. The ExtensionContext surface gains an optional getSkillCatalog() method so extensions can resolve exact skill selectors programmatically. Closes bastani-inc#2328
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const loserSource = this.findSourceInfoForPath(d.collision.loserPath, sourceInfos); | ||
| const label = loserSource?.scope ?? loserSource?.source ?? "path"; | ||
| lines.push( | ||
| theme.fg( | ||
| "dim", | ||
| ` ${theme.fg("warning", "✗")} ${this.formatPathWithSource(d.collision.loserPath, this.findSourceInfoForPath(d.collision.loserPath, sourceInfos))} (skipped)`, | ||
| ` ${theme.fg("warning", "✗")} ${this.formatPathWithSource(d.collision.loserPath, loserSource)} (/skill:${name}@${label})`, |
There was a problem hiding this comment.
Collision disclosure derives each losing skill's qualifier from scope or source, instead of using the catalog's collision-specific sourceLabel. For colliding temporary/local skills, this displays /skill:duplicate@temporary for both losers, while the catalog only resolves unique selectors such as duplicate@beta and duplicate@gamma. Copying the displayed command therefore cannot select the identified skill. Format the command from the catalog's canonical selector, or share its unique-label calculation.
Artifacts
Executable colliding-skill selector harness
- The exact non-production Bun harness creates the three colliding skills, invokes catalog construction and diagnostic formatting, and asserts selector resolution, showing the mismatch.
Expected catalog selectors resolve to each displayed loser
- Executed the harness with its catalog-derived expectations; `duplicate@beta` and `duplicate@gamma` each resolve to their respective loser path, establishing the correct comparison baseline.
Printed diagnostic selectors fail to resolve
- Executed the harness assertion against the selectors actually printed by diagnostics; both `duplicate@temporary` selectors fail and the command exits 1, confirming the defect.
Production diagnostic file had no local changes
- Executed a focused git diff/status check for the reported production file and the harness, showing no production-code modification was made.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/coding-agent/src/modes/interactive/interactive-resource-disclosure.ts
Line: 39-44
Comment:
**Collision diagnostics print unresolvable selectors**
Collision disclosure derives each losing skill's qualifier from `scope` or `source`, instead of using the catalog's collision-specific `sourceLabel`. For colliding temporary/local skills, this displays `/skill:duplicate@temporary` for both losers, while the catalog only resolves unique selectors such as `duplicate@beta` and `duplicate@gamma`. Copying the displayed command therefore cannot select the identified skill. Format the command from the catalog's canonical selector, or share its unique-label calculation.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| // Use a path-derived suffix for duplicates | ||
| const used = new Set<string>(); | ||
| for (const skill of candidates) { | ||
| const shortLabel = labelMap.get(skill.filePath)!; | ||
| if ((labelCounts.get(shortLabel) ?? 0) <= 1) { | ||
| labelMap.set(skill.filePath, shortLabel); | ||
| used.add(shortLabel); | ||
| continue; | ||
| } | ||
| // Derive a unique label from the base directory name | ||
| const baseName = skill.baseDir.split(/[\\/]/).filter(Boolean).pop() ?? shortLabel; | ||
| let candidate = baseName; | ||
| let suffix = 2; | ||
| while (used.has(candidate)) { | ||
| candidate = `${baseName}-${suffix++}`; | ||
| } | ||
| used.add(candidate); | ||
| labelMap.set(skill.filePath, candidate); | ||
| } |
There was a problem hiding this comment.
A duplicate generic-path group can generate a fallback label equal to a later candidate's unique short label. For example, a path candidate under a builtin directory is assigned builtin before a later uniquely builtin-sourced candidate is processed, so both receive sourceLabel and ID tdd@builtin. Qualified resolution selects only one of the two while transcript and catalog identity metadata cannot distinguish them. Reserve every unique short label before assigning path-derived fallbacks.
Artifacts
Narrow skill catalog label collision harness source
- The authored Bun harness constructs control and collision skill groups, invokes `getSkillCatalog`, and records labels, IDs, selectors, and resolution; it directly exercises the reported ordering case.
Control run with distinct fallback and builtin labels
- Executed `bun trex-artifacts/skill_catalog_collision_harness.ts control` in `/home/user/repo`; labels, candidate IDs, and selectors are unique and `tdd@builtin` resolves to the builtin candidate.
Collision run with duplicate builtin labels and candidate IDs
- Executed `bun trex-artifacts/skill_catalog_collision_harness.ts collision` in `/home/user/repo`; two candidates receive label and ID `tdd@builtin`, while selectors remain unique and resolution selects the qualified later candidate.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/coding-agent/src/core/skill-catalog.ts
Line: 86-104
Comment:
**Fallback labels collide with reserved source labels**
A duplicate generic-path group can generate a fallback label equal to a later candidate's unique short label. For example, a path candidate under a `builtin` directory is assigned `builtin` before a later uniquely builtin-sourced candidate is processed, so both receive `sourceLabel` and ID `tdd@builtin`. Qualified resolution selects only one of the two while transcript and catalog identity metadata cannot distinguish them. Reserve every unique short label before assigning path-derived fallbacks.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| ): VerbatimCompactionStats { | ||
| const beforeTail = region.tokenEstimate; | ||
| const afterTail = keptTail ? tailEstimate : 0; | ||
| const tokensBefore = beforeTail + (keptTail ? tailEstimate : 0); |
There was a problem hiding this comment.
When fresh compaction drops a protected tail, tokensBefore excludes tailEstimate even though that tail was present in the original context. A 1,000-token region with a dropped 300-token tail and 250-token result is reported as 1,000 tokens and 75% reduction instead of 1,300 tokens and 80.8%. Include the tail in tokensBefore unconditionally while retaining the conditional inclusion in tokensAfter.
Artifacts
Focused harness source for a dropped protected tail
- Authored TypeScript harness invokes computeWholeContextStats with a 1,000-token region and a dropped 300-token tail, asserting whole-context pre-compaction accounting; it defines the executed check.
Captured focused harness source
- Captured output of reading the exact harness source before runtime execution; it shows the inputs and expected whole-context assertions.
Runtime output showing dropped tail excluded from stats
- Captured Bun execution output shows observed tokensBefore 1000 and percentReduction 75 versus expected 1300 and 80.8, followed by the assertion failure; the reported behavior is confirmed.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/coding-agent/src/core/compaction/compaction-runner.ts
Line: 117
Comment:
**Dropped tails are omitted from pre-compaction stats**
When fresh compaction drops a protected tail, `tokensBefore` excludes `tailEstimate` even though that tail was present in the original context. A 1,000-token region with a dropped 300-token tail and 250-token result is reported as 1,000 tokens and 75% reduction instead of 1,300 tokens and 80.8%. Include the tail in `tokensBefore` unconditionally while retaining the conditional inclusion in `tokensAfter`.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
|
Superseded by #2449. This branch's headline change — source-qualified selectors for shadowed same-name skills (#2328) — has since landed on This PR also bundled the #2052 compaction-stats fix, which is still needed. It has been rebased onto the latest Closing in favor of #2449. |
1 participant
| Related unit tests | 18/18 pass |
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Greptile Summary
This change exposes source-qualified access to shadowed skills and updates compaction statistics. Reproduced failures show that interactive collision diagnostics can advertise selectors that do not resolve, skill catalog fallback labels can duplicate another skill’s identity, and dropped protected tails are excluded from the reported pre-compaction context size and reduction percentage. These issues should be corrected before merge.
Confidence Score: 2/5
Not safe to merge until the three reproduced correctness issues are fixed.
Executable harnesses exercised the affected production paths and reproduced three independent non-security failures: invalid displayed selectors, duplicate skill identities, and inaccurate whole-context compaction statistics.
Files Needing Attention: packages/coding-agent/src/modes/interactive/interactive-resource-disclosure.ts, packages/coding-agent/src/core/skill-catalog.ts, and packages/coding-agent/src/core/compaction/compaction-runner.ts.
What T-Rex did
Comments Outside Diff (3)
General comment
SourceInfovaluesscope: "temporary"andsource: "local", the diagnostic displays/skill:duplicate@temporaryfor both losers. The catalog disambiguates the same candidates asduplicate@betaandduplicate@gamma; resolving either displayed selector fails rather than selecting its displayed loser.interactive-resource-disclosure.tsderives the qualifier directly asloserSource?.scope ?? loserSource?.source ?? "path", whereasskill-catalog.tsapplies group-level uniqueness logic and replaces colliding short labels with base-directory labels.getSkillCatalog(or share the catalog's unique-label derivation) and format the loser command from that canonical selector.General comment
pathlabels, a first candidate whosebaseDirends inbuiltinreceives fallback labelbuiltin. A later candidate with unique short source labelbuiltinis processed afterward and receives the same label. The resulting catalog contains duplicatesourceLabeland duplicate candidateidvalues (tdd@builtin).uniqueLabelInGrouprecords labels inusedonly while processing the second pass. A unique short label is not reserved before duplicate fallback labels are generated, so an earlier path-derived fallback can consume a later unique candidate's short label.usedwith all labels whose count is one before generating any duplicate fallbacks, or otherwise ensure each fallback is checked against every short label in the group as well as generated labels.General comment
computeWholeContextStatsreportstokensBeforeas 1,000 and reduction as 75% for a 1,000-token region plus a dropped 300-token protected tail and 250 post-compaction tokens. A pre-compaction whole-context metric should instead report 1,300 tokens and 80.8% reduction.packages/coding-agent/src/core/compaction/compaction-runner.ts:117,tokensBeforeconditionally addstailEstimateonly whenkeptTailis true, even though the tail was present before compaction.tailEstimateintokensBefore; continue to conditionally include it only intokensAfterbased onkeptTail, then calculate the percentage from those whole-context values.Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "feat(coding-agent): keep shadowed same-n..." | Re-trigger Greptile