feat(skills): let a qualifier address one of several colliding package skill keys

[rnoack1]

Problem / Motivation

The skill catalog keys a package-contributed skill package/<relative-path>. For a
root shaped <...>/packages/<Pkg>/<event>/skills, the name that distinguishes the
bundle lives in the root, so it is absent from the relative path — and two
bundles vendoring a skill at the same relative path collide on one key.

_resolve_package_skill_path handles that collision fail-closed on purpose: it
returns None and logs rather than serving an arbitrary one of the two, because a
reader who opens one skill and silently gets another has no way to notice. Correct,
but the consequence is that both copies become unaddressable — the key names two
files, so it names none.

Enumeration had the mirror-image gap: enumerate_skill_catalog kept whichever root
was walked first (setdefault) and dropped the other, so the surviving key was one
the resolver then refused — a phantom row in the agent-template editor, with the
other copy unreachable entirely.

Why it matters

That contradicts the invariant the two functions document about each other: a key
the catalog offers must be one the resolver accepts, and vice versa. Today it is
violated silently in both directions — a listed key that 404s, and a skill that is
installed and loadable by agents but absent from the catalog with no user-visible
signal.

The omission scales with how many bundles share a skill set, so it is not a corner
case for an install that vendors several. It was REPORTED on one such install as
well over a hundred skills missing from the catalog — that figure comes from the
report, not from this repo: the only in-repo extra_skills() returns []
(platform/defaults.py), so no layout here produces a collision and no in-repo
test can reproduce the count. What the repo CAN show is the mechanism, and the
tests pin exactly that: a synthesised two-root collision loses a row to the
enumerator's setdefault and the resolver's fail-closed refusal.

What changed (motivation → approach → change)

An optional qualifier in the key: package/<qualifier>:<rel>.

A qualifier names the wanted copy's root by its own identity. It narrows an
already-globbed candidate set and never widens the search — the same roots are
globbed with the same patterns, so a qualifier can only turn a refusal into a hit;
it can never reach a root the unqualified request would not have searched. The
exclusion of core-owned roots (a package/ key must never answer with the user's own
editable skill) is therefore untouched, and there is a test that names that root's
segment as a qualifier and still gets None.

A qualifier is a DERIVED, identity-bound token: a blake2b digest of the wanted
copy's root canonical path TOGETHER WITH that root's st_dev, st_ino,
st_ctime_ns and st_mtime_ns, so a key reads
package/05c564ec5e9e4b7a8c1d2e3f4a5b6c7d:<rel>. Both sides RE-DERIVE it with the same function, so
resolution is the exact inverse of enumeration; membership of a segment in a root is
explicitly NOT the contract, because a replaced root that still carries a segment would
then re-bind a held key to a different file. Deriving from the root ALONE also makes the
key stable: installing or removing an unrelated bundle cannot re-spell it. It stays mapping-free even so: the core has no root-to-package
mapping and needs none, since the identity half is taken from the root's own path rather
than from any package name, and that knowledge belongs to whichever edition installs the
roots. A qualifier carrying /, .. or a leading ~ is refused up front, so a
malformed one can only fail closed.

Four properties worth calling out:

• Additive for every key that does not carry the separator. Such a key takes the
  pre-existing code path unchanged, and split_package_skill_key treats a half-empty
  qualifier (:x, x:) as unqualified rather than as an empty glob pattern. A key
  that DOES carry : is the one deliberate exception — see the reservation below.
• One helper computes the collision set for both sides. _package_collision is
  the only place the set is built, and enumeration and resolution both call it, so the
  two cannot drift into disagreeing about which roots collide — the drift that produces
  a phantom row. It folds the raw glob hits to one entry per DISTINCT resolved file via
  _dedupe_entries, and builds from the EXACT relative-path tier only, never the union
  with the nested-leaf tier. The layout that makes this matter is one
  _collect_skills_under documents as supported: a root that installs a skill by
  symlinking to another root's copy. Both spellings reach the tier and collapse to the
  one file, so the aliasing root does not inflate the set.
• The qualifier is checked even against a lone candidate. Skipping it when a tier
  holds one match would make package/PkgB-<identity>:shared-skill serve PkgA's copy under a
  200 when only PkgA bundles that path — the same silent wrong-content failure the
  unqualified path fails closed to avoid.
• Enumeration emits the same grammar. A relative path backed by two distinct
  files becomes one qualified key per copy, qualified by each holding root's identity
  digest. A path found in one root only — or reached through a
  symlink alias — keeps its plain key.

Why : and not -

The route grammar already reserves a lone - segment ahead of the verbs
(/api/skills/{name:.+}/-/tree), so a - segment inside a key would make
package/Pkg/-/tree ambiguous between a detail GET and a tree GET. : is a legal
path character (RFC 3986 pchar) needing no escaping.

: is reserved, and that reservation is this PR's one behaviour change to an
existing key shape. The character is legal in a POSIX directory name, so a skill
directory literally named foo:bar already enumerated and resolved before this
grammar existed. A key carrying the separator now has exactly one reading — the
qualified one — so such a skill is omitted from the catalog and 404s on open;
_resolve_skill_root has no verbatim fallback.

Reading the key both ways and disambiguating by the installed root set is the
obvious alternative, and it is rejected here on purpose: it makes a key's MEANING a
function of which roots happen to be installed, so uninstalling a root — precisely
when a key goes stale — silently re-points an existing key at a DIFFERENT package's
skill. A single reading cannot do that; it can only stop resolving. The cost is
therefore paid openly: a colon-named skill loses the key it had, the omission is
unconditional rather than root-set dependent, and
test_a_colon_named_skill_is_omitted_and_unresolvable pins both halves. A key without
the separator still takes exactly the pre-existing code path.

Why the resolver had to change, not just the detail path

api_skill_detail does not go through _resolve_skill_root; it reads the row's
own path. Teaching only detail would make a colliding skill openable while its
/tree 404s. That asymmetry is worse than the clean omission it replaces, so the
resolver change is the mandatory half.

Why a root that cannot be identified is omitted rather than kept

A path SEGMENT cannot serve as the qualifier at all, which is why an identity was
chosen instead. Any segment rule is evaluated against whichever roots collide at
derivation time, so the value it picks MOVES when that set does: a segment can differ at
its own index while still occurring deeper in a sibling root (/x/PkgA/skills vs
/x/nested/PkgA/skills), matching both candidates and making the resolver refuse a key
enumeration had just offered. A digest over the root's own canonical path and its own stat identity is a
function of that root ALONE, so widening the candidate set cannot re-spell an
already-minted key. The stat terms are why an in-place bundle replacement re-spells its
keys rather than silently rebinding one already held.

A digest exists for every root that canonicalises, so a collision is addressable
whatever the two roots' paths look like — including one being a path prefix of the
other. The omission branch survives only as a fail-closed backstop: when a root does
not canonicalise it yields no identity, so the path is omitted and logged with its
absolute path rather than listed under a key that resolves to nothing. That keeps the
enumeration invariant intact in the ONE direction it claims — every key the catalog
offers is one the resolver accepts — and is diagnosable rather than silent, and it
affects only a key that already resolved to nothing. The converse is deliberately not
claimed: resolution is WIDER than enumeration, because it also accepts a leaf-name key
through its nested tier, so a key the catalog never listed can still resolve. Read as
two-way the claim would be false; one-way it is what the test below asserts.

It is not, however, the only behaviour change to an existing key. The reserved
package/ prefix is the other one, and it is deliberate and tested rather than
incidental:

• api_skill_detail now discards a load_skill hit for any package/-prefixed
  name, where before that hit won and was served.
• enumerate_skill_catalog prunes every core row whose own relative path keys
  into the reserved prefix, with a warning.
• A file read under a package/ key is refused when its inode carries
  more than one link, and the endpoints answer 404 (file_not_found) rather
  than 403: the descriptor gate reports nothing for an absent or unopenable file
  too, so the response cannot claim a denial it is unable to substantiate, and the
  withheld read is carried on the audit trail instead. A hardlink canonicalises to its own in-root path, so
  containment and the sensitive-name check both pass while the bytes belong to the
  shared inode; st_nlink on the opened descriptor is the only signal that a second
  name exists. This reaches keys that predate the qualifier, and the scope is uneven
  by endpoint: api_skill_detail already read through this same gate before this
  change, so for the detail route the refusal is INHERITED rather than new, while the
  /file route read through read_skill_file's plain read_text and carried no link
  check, so there it IS new. The cost lands on hardlink-DEDUPLICATING installs —
  content-addressed package stores, store optimisation, cp -al deploys — where a
  legitimate, non-colliding package skill routinely has st_nlink > 1, and a /file
  read that used to succeed answers 404 with zero colliding roots. It is intentional and unconditional for this read-only
  territory; the sibling territories keep the reader they already had.

So a core-owned skill physically installed at skills/package/<rel> was
detail-openable before this change and is now invisible — absent from the catalog
and 404 on open. That is the price of making a package/ key mean the package
territory and nothing else: leaving the core hit in place is what let one key name
two files, serving the core copy in the detail modal while /tree resolved the
packaged copy, so a spec written from the modal would load a file the tree never
showed. Both halves are covered:
test_a_core_row_under_the_reserved_prefix_is_pruned for the enumeration prune,
and test_detail_serves_the_package_copy_not_a_core_skill_of_that_literal_name
for the detail discard.

Such a file is remediable only by hand. The create endpoint used to accept a
package/ name and write it to the core root, so an install can genuinely carry one,
and the refusal above stays UNCONDITIONAL for it: every mutating verb on a package/
key answers 405 with Allow: GET, stranded or not. The remediation surface is the
prune warning, which names the file's ABSOLUTE path — once the row is dropped that log
line is the only surface saying where the file is, and the key alone is relative to a
core root the reader cannot infer — so an operator removes it directly. Letting
DELETE fall through to the core loader instead would buy an API verb for a population
only a pre-reservation create could have produced, at the cost of a resolver-plus-row
answerability probe and a conditional Allow contract in permanent API semantics.

Declared rider: audit helper rename and scope

_audit_unread is now _audit_tool, and it is called on refused MUTATIONS as well as on
refused reads. The rename follows the widened scope rather than preceding it: the old name
asserted the call site was a read, and once package/ became a read-only prefix the same
helper had to record a refused PUT/DELETE, which the old name would have mis-described at
every one of those sites. Nine call sites read the new name and none the old.

Declared rider: the unresolved-mapping warning is generic

The warning chip, its note and the count fire for ANY mapped key the catalogue does not
resolve — not only a qualified one. That is deliberate: an unresolved mapping is the same
user-facing condition whatever spelled it, and gating the surface on the qualified grammar
would have left a plain stale key silently unexplained, which is the failure this PR exists
to remove. It does mean the surface appears on installs that never collide.

Declared rider: PACKAGE_KEY_PREFIX

One change here is not the qualifier and is called out rather than left to be found:
the "package/" prefix is now a named constant. Three sites read it — key
enumeration and path resolution in _shared.py, and the detail endpoint in
prompts.py — and the string decides which grammar a key is read under, so a
respelling at any one of them is a silent divergence. A constant applied to only
some of those sites buys nothing, so all three use it and a test asserts the literal
is spelled exactly once in the two modules that parse the key. That test, not a
comment, is what keeps a fourth site from reintroducing the drift.

Not changed, deliberately

• No route change. {name:.+} already accepts embedded slashes, and : adds no
  new segment.
• No JSON field change. Only the key value varies. Rows carry no bundle-name
  field, and the picker derives no qualifier at all: that surface never receives one.
• No cached ids. The catalog is computed per call.
• No new re-exports. handlers/__init__.py is byte-identical to main; every
  caller of the new helpers imports them from _shared directly.
• _match_package_row needed one gate. Its exact-key leg returned the FIRST row
  whose key matched, so two rows listed under one key served an arbitrary one of them
  under a 200 — the collision this grammar exists to disambiguate, reached through the
  one leg that was not ambiguity-checked. The leg now refuses a duplicate key exactly as
  the leaf leg already refused an ambiguous leaf. A producer-emitted qualified key still
  hits it directly. Its leaf fallback
  compares the key remainder to a row's name, and for a qualified key that
  remainder is <qualifier>:<rel>, which matches no leaf — so the fallback goes
  quiet for qualified keys. That is the wanted behaviour: a leaf match would serve
  whichever package happened to be the only row with that leaf, ignoring the
  qualifier that was the whole point of the key. A test pins it.
• Persisted skill:// URIs. skill_key_for_uri inverts through
  enumerate_skill_catalog, so a URI written against a colliding skill now inverts
  to a key that resolves, where before it inverted to one that did not.

Read/write territory parity is only partly fixed here — two siblings are left

This PR reserves the package/ prefix on the write paths as well as the read
paths, because that is the prefix it introduces a grammar for. The same divergence
still exists for the other two prefixed territories, and this PR deliberately does
not close it:

• kiro-user/ and kiro-workspace/ still reach the core loaders on the write
  paths, and still pass the create sanitiser: both new guards test
  safe_name.startswith(PACKAGE_KEY_PREFIX) only, so a PUT, DELETE or create
  naming either sibling behaves exactly as it did before this PR.
• Read and tree route those same prefixed keys to their own territories, so a write can
  still land in the core root under a key the reader is served from elsewhere.
• detail likewise still prefers a core file over the copy the tree view shows, for
  those two prefixes.

The point patch is deliberate: package/ needed enforcing on both sides here because
this PR is what makes package/ keys answerable at all, and leaving create open would
let the API manufacture an orphan it has no verb to remove. Extending the same
territory check to the two siblings is a behaviour change to already-shipped,
already-consumed key spaces, with its own migration question for each, so it is left as
follow-up rather than folded in behind a grammar change.

Producer side lands separately

Catalog enumeration in this repo DOES mint qualified keys — _merge_package_walks
writes one per distinct copy whenever two installed roots bundle the same relative path,
and a test asserts the catalogue's own key set. What no edition in this repo ships is a
LAYOUT that collides, so on a default install nothing qualifies and every existing key
behaves exactly as before; that is what makes this additive rather than inert. The
producer that will create such layouts is a downstream edition vendoring this core as a
git subtree, and its change depends on this landing here first and reaching it on
the next subtree pull. It must emit the identity-digest token above, never a
path segment: keys are derived, so the reliable path is to re-enumerate and
use the key the catalogue lists.

The kiro-user/ and kiro-workspace/ prefixes disclosed below carry the same
one-key-two-files hazard and are NOT fixed here; that follow-up is tracked as #8244.

A qualifier is derived, not a name any producer owns. It is a function of the
root's own canonical path, so it does NOT move when an unrelated bundle is installed or
removed, and no other root can produce it. What can still change is whether a given rel
collides at all: if a second bundle starts or stops vendoring the same relative path, a
copy moves between its plain key and a qualified one. A key persisted across that change
resolves to nothing rather than to the wrong file. Failing closed is the intended
direction (a stale key 404s; there is no verbatim fallback), so a consumer should
re-enumerate rather than treat a key as a long-lived identifier. Whether a given
producer's install paths are stable enough to persist such keys is a property of
that producer, not of this core.

Tests

test/test_resolve_skill_root_package.py gains 45 tests:

• The grammar — an unqualified key is untouched, and a half-empty qualifier degrades
  to unqualified rather than to an empty glob.
• Each of two colliding copies resolves to its own file, both through
  _resolve_package_skill_path and through _resolve_skill_root (the routed entry
  point /tree and /file use).
• Filter-before-dedupe: with three roots where one installs the skill as a symlink to
  another's copy, each root's own qualifier resolves to its own spelling. This fails
  on the filter-after-dedupe ordering because the aliasing spelling is discarded
  before the qualifier is ever applied.
• Fail-closed cases: a qualifier naming a root that lacks the path returns None
  instead of substituting the other copy; a qualifier that matches several colliding
  roots still refuses and logs; a qualifier holding a / or .. cannot match; ..
  anywhere in a qualified key is rejected before the split.
• The narrows-never-widens property: naming a core-owned root's segment still returns
  None.
• Enumeration: a colliding path is listed once per copy; a symlink alias keeps its
  plain key; an unqualifiable collision is omitted with a warning.
• The reserved package/ prefix: a core row keying into it is pruned from the
  catalog, and detail serves the package copy rather than a core skill of that
  literal name. The fold writes each qualified key unconditionally, so what is
  asserted is the CALLER's precondition — that the prune has already run when the
  fold is entered — rather than a re-check inside the fold that no caller can
  reach. Moving the prune after the fold fails that test.
• The prefix-literal drift guard described above.
• The reservation itself: a colon-named skill is omitted AND unresolvable, a stale
  qualified key never falls back onto one, and an uninstalled root's stale key never
  binds to another package's copy.
• Write-side enforcement: create refuses a reserved package/ name, and EVERY
  package/ key refuses PUT/DELETE with Allow: GET — including a STRANDED one,
  whose only remediation surface is the absolute path named in the prune warning.

Each behavioural test was checked in both directions — it fails on the tree without
the change it covers, for that change's own reason, and passes with it.

The enumeration invariant is asserted by resolving every enumerated key and
comparing it to the file it was enumerated for. Asserting the package/ prefix
instead would pass vacuously under any key change — a false all-clear on exactly the
property being changed.

Manual verification

N/A — unit coverage is sufficient here. The hardlink refusal above IS user-visible (a
previously-readable skill answers 404 on a hardlink-deduplicating install), and it is
covered by unit tests rather than by clicking through. The KEY change has no UI surface
(only the key
value varies, and the frontend keys editability, grouping and the read-only badge off
the source field, not the key prefix), and no edition in this repo ships a colliding
layout, so no qualified key appears on a default install and there is nothing to click
through. The behaviour that matters is
key → path resolution, which is exactly what the tests exercise, including the
routed _resolve_skill_root path the tree and file endpoints call.

Gates run locally: pytest (full backend, 75465 passed), isort, flake8, mypy —
all green. scripts/local-gate.py classifies this diff as touching BOTH surfaces plus meta
(frontend=True meta=True backend=True, its "full gate" path), which is what the
~1,850 changed lines under website/ mean: the frontend gate applies to this change
and is not waived. Frontend result at this head: eslint src clean, the i18n runner
19/19, the skills-editor suite 38/38, and the SkillsTab cross-surface guard 20/20.
The full website suite additionally fails three tests in
src/test/ChatInput.lexical.test.tsx; that failure reproduces with every
website/src change on this branch reverted to its parent commit, so it is base-side
rather than introduced here. This host also carries a set of
pre-existing BACKEND suite failures unrelated to skills (git-config and real-symlink
environment issues); the failing node-id set is identical with and without this
change, and no skills, catalog, or prompts test is among them.

Screenshots

Captured by website/scripts/capture-package-skill-key-qualifier.mjs, which drives the
REAL built SPA (website/dist) with every /api/** call stubbed, and which ASSERTS as
well as photographs: it exits non-zero unless both colliding copies list as separate
selectable options and unless a refused write actually re-enumerates the catalog.

A real collision is two bundles vendoring the SAME skill, so the two rows share a name
AND a description and nothing in the row itself tells them apart. GET /api/skills now
re-keys such a row onto its qualified spelling, so each colliding copy DOES reach the
picker under its own key, and the row renders a third line naming the copy's distinguishing
path so a keyboard user can tell which bundle they are picking. Where a qualified key is
already mapped into an agent's skill:// resources, the chip renders its readable half
plus the same distinguishing path the picker row showed — with no digest fallback at all, because a 32-hex qualifier names nothing a reader could act on — which is the surface that has
to say WHICH copy is mapped.

Related Issues

#8244 — filed for the one gap this PR discloses rather than closes.

That gap is the kiro-user/ and kiro-workspace/ one-key-two-files hazard described
under What changed: those two prefixed territories still reach the core loaders on
their write paths, because both new guards test PACKAGE_KEY_PREFIX only, so a write can
still land in a core root under a key the reader is served from elsewhere. It is left open
here deliberately — closing it means changing the core loaders, which is a different
change from this grammar fix — and #8244 carries the reproduction so it is not lost.

[github-actions]

GPT 5.6 Review (fork) — ✅ no blocking findings

Reviewed c6f996809cae6b6c4a5fb4bda116adc84589c3d1 via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] c6f9968

[github-actions]

First Principles Review (Fable 5, fork) — 🟡 CONCERNS

Premise-level review of c6f996809cae6b6c4a5fb4bda116adc84589c3d1 via the fork AI-review pipeline — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

I have everything I need. Assembling the review.

First-Principles-Verdict: CONCERNS

The stat-bound qualifier and its pinned-descriptor apparatus defend a persist-time window that the path-based skill:// URI reopens the moment the write lands.

Not justified as shipped

• Item 7 — oversized: the description justifies the st_dev/st_ino/st_ctime_ns/st_mtime_ns terms with "that file's skill:// URI would be persisted into the agent's config: a skill the user never selected, under a 200, durably" — but apply_skill_mapping persists a ~/-relative PATH URI (_skill_uri_for_path, _shared.py:1312, "portable across machines and home dirs"), which rebinds to any replacement at that path forever after the write. The durable half of the claim doesn't exist; only a seconds-wide pre-write race is closed.
• Item 4 — rides along: severable per the PR's own doc ("(1) and (3) are [severable]"), though entailed by the prune.
• Item 6 — rides along: declared; extends the documented descriptor gate, at a named cost to hardlink-deduplicating installs.
• Item 10 — undeclared: comment-history-baseline.json entries lowered for shell_normalizer.py and test_config_loader.py, two files this diff never touches.

What this change ships

Inventory (10 items) — 6 justified

Intent: make each of several package bundles vendoring a skill at the same relative path individually listable and openable, instead of both silently vanishing — a FIX of the catalog↔resolver invariant (added tests fail on base), shipped as an additive key grammar.

1. Two bundles vendoring the same skill path each list and open under their own qualified key — justified
2. The skills list re-keys colliding rows onto the spelling that resolves — justified
3. A skill directory named with :, a glob character, .., or an out-of-root symlink is omitted and 404s (declared) — justified
4. Edit/create/delete on package/ keys now refuse 405/400 instead of writing a core-root copy — rides along
5. A core skill folder literally named package/… disappears from the catalog, file kept on disk — justified
6. Package skill file/detail reads refuse hardlinked inodes — rides along
7. Qualifier binds root stat identity, adding pinned-fd reads, row-identity capture, write-time rebind checks, and wholesale omission of colliding skills where the OS can't pin — oversized
8. Rejected mapping writes return code: skills_unknown; responses add unmanaged_skills (consumers counted: AgentsPage.tsx, AgentSkillsEditor.tsx) — justified
9. Editor marks unresolved mapped keys with warning chips, labels colliding twins "Located in …", refreshes its cache after a refusal — justified
10. Screenshots, capture scripts, 14 locale files (all convention-mandated) plus baseline lowering for two untouched files — undeclared

Watch

• The stat terms are symptom-level for the harm named: post-write, the persisted path URI rebinds on replacement regardless. Their real cost is counted: chmod or a sibling install re-spells every qualified key ("cursor, not a name" contract, forever), and where supports_pinned_walk() is false (Windows — no O_NOFOLLOW) colliding skills are omitted entirely, so the reported defect stays unfixed exactly there. The pinned-read machinery guards only the qualified branch; the diff's own comment leaves 4 sibling read territories (unqualified package/, kiro-user/, kiro-workspace/, core) on the by-name walk. Clears when: the author names a concrete harm the stat terms remove that the persisted path URI does not reintroduce immediately after the write.

Subtractions

• Derive the qualifier from the canonical root path alone; delete the stat terms and everything that exists only to honor them: identity_out plumbing, _open_pinned_root/_descend_pinned/descend_nofollow/_pinned_skill_tree, capture_package_row_identities/_strip_package_row_identities/_audit_replaced_row, _qualified_entry_still_binds, and the three supports_pinned_walk() gates — which also un-breaks the fix on Windows.
• Delete the if err == "missing" branch in api_skill_file (prompts.py): zero producers — grepped "missing" across the patch and base; read_skill_file returns "not found", and every new path returns "unsupported"/"read failed"/"invalid path".

[FIRST-PRINCIPLES-REVIEWED] c6f9968

[github-actions]

Design Review (Fable 5, fork) — 🟡 CONCERNS

Design-level review of c6f996809cae6b6c4a5fb4bda116adc84589c3d1 via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

The design gate is complete: I read the full PR description, the backend hunks (_shared.py, prompts.py, agents.py, hooks.py, pinned_fs.py, interfaces.py), the frontend editor changes, and confirmed no existing test pins were deleted or weakened, temp-screenshots/ is an established repo convention, and the pinned-fs helpers build on existing seam functions.

Design-Verdict: CONCERNS

Sound, exhaustively reasoned grammar fix; the one trade a human should ratify is the unconditional hardlink 404 on dedup installs.

Watch

The /file hardlink gate regresses working reads on legitimate installs with no remedy but restructuring. The description admits it: "a legitimate, non-colliding package skill routinely has st_nlink > 1, and a /file read that used to succeed answers 404 with zero colliding roots" — so on content-addressed or cp -al edition installs the dashboard skill browser silently loses the whole package/ territory, signalled only by an audit line. The security cause is real (a hardlink alias defeats canonicalize-then-open) and detail already gates this way, but this hunk extends the cost to keys that predate the qualifier, inside a PR whose stated purpose is key addressability, and is verified only by unit tests ("Manual verification: N/A").
Clears when: a human ratifies the "roots must hold real files, not links into a shared store" constraint in interfaces.py as the edition contract, or the refusal gains a user-visible signal/remedy beyond the SEL line.

Suggestions

• Surface the hardlink/unreadable refusal count on the same warning-chip surface the unresolved-mapping rider adds — the operator-facing gap is identical (a 404 indistinguishable from "not installed"), and the chip machinery is already in this PR.

[DESIGN-REVIEWED] c6f9968

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed c6f996809cae6b6c4a5fb4bda116adc84589c3d1 via the fork AI-review pipeline; updated in place on each push.

Review details

I've reviewed the full diff: the core security-critical Python (_shared.py qualifier derivation/resolution/enumeration, prompts.py descriptor-gated skill reads, hooks.py dir_fd read path, pinned_fs.py descend_nofollow, platform/interfaces.py), the AgentSkillsEditor.tsx frontend, and the i18n catalogs.

I actively tried to break the changed code:

• The dir_fd/dir_fd_rel path in safe_read_file_bytes_nolink still runs the same fstat/st_nlink>1/within_root real-path/is_sensitive_path checks against the opened descriptor — the pin is preserved, not bypassed.
• Every new descriptor (_open_pinned_root, _descend_pinned, os.dup, fwalk top fd) is closed in a finally; no leak found.
• Qualifier re-derivation uses a consistent canonical-path basis on both the enumeration and resolution sides via the shared _package_collision/_dedupe_entries, and containment is enforced on canonical forms; every failure mode I could construct (stale qualifier, replaced bundle, symlinked parent, glob/colon names, unpinnable platform) resolves to a 404/omission — fail-closed — not to wrong-content disclosure, credential exposure, or a crash.

No candidate needed falsifying (the discovery pass produced none), and my own extension pass surfaced nothing meeting the (a)/(b)/(c) bar at 80+.

No findings.

[OPUS-REVIEWED] c6f9968

[rnoack1]

Verified fixed at head 6094392071b3e6ea9d2d89268a3b7161455cc50c. This supersedes my earlier reply, whose line numbers have since moved — please use the coordinates below instead.

Your mechanism was real, and I reproduced it before believing the fix, so this is not a rebuttal.

Fail-first check. I built both layouts you named on a real filesystem and ran them through the actual enumerate/resolve seam. Both pass at this head. Because "it passes" proves nothing on its own, I then reintroduced the defect — widening the resolver's set to the nested-leaf tier and deriving the qualifier from that set again — and got your exact result back:

AssertionError: layout A (nested-leaf): 1 enumerated key(s) do not resolve:
  [('package/PkgA:shared-skill', None, ...)]

So the probe does detect the drift; the tree no longer has it.

What closes it. I took the stronger of the two options you offered — one shared helper rather than re-synchronising two derivations:

• _package_collision at _shared.py:1634 is the only place the collision set is computed. Both sides call it: the resolver at :700, enumeration at :1761. Two implementations cannot disagree when there is one.
• It builds from the exact relative-path tier only — :701 globs f"{name}/SKILL.md", never the */{name}/SKILL.md union that admitted your root3.
• It dedupes by resolved file identity internally at :1667, so a symlink alias collapses onto the file it aliases instead of inflating the set.
• holders, foreign and _dedupe_resolved are all gone — 0 occurrences in the file.

Why the class is now unreachable rather than merely re-synchronised. A qualifier is _root_identity_token(root) at :1520: a digest of that root's own canonical path. It is a function of the root alone, so widening the collision set cannot shift any root's qualifier. Your sentence narrowing the mechanism to "an extra holder sharing the qualifier segment" is what pointed at this — with an identity digest there is no shared segment to collide on. A probe deriving one root's qualifier alone and again beside five added siblings gets the same token every time.

Tests. test_every_enumerated_package_key_resolves at test_resolve_skill_root_package.py:2347 now carries both fixtures you asked for: a nested-leaf root at :2375 and a symlink-alias root at :2380, plus a guard at :2392 asserting the nested root actually contributed a key — without that the assertion could pass while exercising neither shape. test_the_collision_set_is_computed_in_exactly_one_place at :2258 pins the single call-site count so the two derivations cannot silently split again. 82 pass on 3.12 and 3.10.

Nothing here is owed back to you — flagging it only so the changes-request is not left blocking work that is done.

[rnoack1]

Thanks — this was correct at the sha you reviewed (1118db3e5), and it is the finding that drove the design change. Settled at head 34e14fcbd3647b2943abdd122fded86704cca780, with the coordinates below re-read at that head.

Your required change, taken on the arm you offered second: one shared helper. The collision set is now computed in exactly one place, _package_collision (src/kiro_crew/dashboard/handlers/_shared.py:1667), with two call sites and no third: the resolver at :700 and the enumerator at :1794. So the two cannot drift, because there is no second implementation to drift from.

The other half of the asymmetry is gone too. The resolver's derivation input is the exact tier only — :701 globs f"{name}/SKILL.md"; the nested */{name}/SKILL.md tier is applied only after the qualifier has already selected roots, so it no longer participates in the derivation. holders, foreign and _dedupe_resolved are all 0 occurrences in that file now; deduplication happens inside _package_collision itself.

The mechanism you identified is now unreachable rather than merely fixed. A qualifier is no longer a distinguishing path segment; it is _root_identity_token (:1553), a blake2b digest of the root's own canonical path. Because it is a function of one root, widening the candidate set cannot shift it — which is precisely the property your finding relied on being absent.

Measured on your two layouts at this head, both reproduced verbatim from your review:

• Layout 1 (nested-leaf tier: root3 = packages/PkgA/eventId-3/skills holding PkgA/shared-skill/SKILL.md) enumerates 3 keys and all 3 resolve to their own file. root3 contributes the rel PkgA/shared-skill, a different rel, so it no longer joins shared-skill's derivation set at all.
• Layout 2 (root3's shared-skill a symlink to root1's) enumerates 2 keys, both resolving — the alias correctly folds into one entry, since it is the same file.
• Set-invariance, measured directly: root1's minted qualifier is byte-identical with 2 roots and with 3 (package/0a4758aae56c in that run). That is the PkgA → eventId-1 shift you saw, tested for and absent.

On the test you challenged — test_every_enumerated_package_key_resolves (test/test_resolve_skill_root_package.py:2416) now carries both fixtures you named as missing: a nested-leaf root (:2444) and a symlink-alias root (:2449), plus a vacuity guard asserting the nested-leaf root actually contributed a key (:2461) so the resolve assertion at :2466 cannot pass on an empty set. There is now a sibling test covering the three non-package/ territories the same way.

And your last point was still live, in the direction you did not expect — so thank you for it. The over-broad holders/foreign sentence had already gone, but two surfaces still described the segment machinery in the present tense after it had been replaced: _merge_package_walks's docstring ("When no segment tells the colliding roots apart…") and one description paragraph asserting that segment membership picks the qualifier — which contradicted the same description's own statement that segment membership is explicitly not the contract. Both are corrected at this head: omission is now documented as what it actually is, a fail-closed backstop for a root that does not canonicalise.

One thing I have deliberately not resolved on my own, in case you have a view: the invariant is now absolute for every key the catalog offers, but some paths are deliberately not offered — a rel containing the reserved separator, a glob metacharacter, .., or a leading ~ in the unprefixed territory, and a skill symlinked outside its root. Those are omitted and logged with their absolute path rather than listed under a key that 404s. The description names that set explicitly.

---

Additional control run since posting, at the same head. Rather than rest on your two
layouts, I swept combinations of deliberately hostile root shapes — exact, nested-leaf,
symlink-alias, a root that is a path PREFIX of another, and a root whose own path segment
equals the rel name — enumerating each layout and resolving every key it minted:

combination size	layouts checked	produced qualified keys	invariant violations
2	22	10	0
3	41	34	0
4	50	49	0

113 layouts, 0 violations. The middle column is the vacuity guard and is the reason the
zero means anything: without asserting that layouts actually produced QUALIFIED keys, zero
violations would be consistent with collisions never having been exercised at all. I also
ran an inverted control that corrupts a minted qualifier, and it is caught as unresolvable,
so the check can fail for the intended reason.

[rnoack1]

Re-verified at head 45247a849670134aebd280636e2350a719cef12a, after this branch was rebased onto current main and its conflict with the read-only-territory change resolved. Your finding was correct at the sha you reviewed; the coordinates below are re-read at this head, and _shared.py is blob-identical to the head my earlier note cited, so those line numbers still hold.

The two collision sets are now one. _package_collision (src/kiro_crew/dashboard/handlers/_shared.py:1667) is the only collision computation, with exactly two call sites and no third: the resolver at :700 and the enumerator at :1794. There is no second implementation left to drift from — which was the second remedy arm you offered.

The other half of the asymmetry is gone as well. The resolver's derivation input at :701 globs the exact tier only (f"{name}/SKILL.md"); the nested */{name}/SKILL.md tier is applied afterwards, once the qualifier has already selected roots, so it no longer participates in the derivation. holders, foreign and _dedupe_resolved are each 0 occurrences in that file now — deduplication happens inside _package_collision itself. Those zeros are positive-controlled: the same grep form returns 5, 6 and 7 hits for _package_collision, _root_identity_token and enumerate_skill_catalog in the same file, so they are facts about the file rather than about my query.

The mechanism you identified is unreachable rather than patched. A qualifier is no longer a distinguishing path segment; it is _root_identity_token (:1553), a blake2b digest of the root's own canonical path. Being a function of one root, it cannot shift when the candidate set widens — which is the property your finding required to be absent.

Measured on your two layouts at this head:

• Layout 1 (nested-leaf: root3 = packages/PkgA/eventId-3/skills holding PkgA/shared-skill/SKILL.md) enumerates 3 keys and all 3 resolve to their own file. root3 contributes the rel PkgA/shared-skill — a different rel — so it no longer joins shared-skill's derivation set at all.
• Layout 2 (root3's shared-skill a symlink to root1's) enumerates 2 keys, both resolving; the alias correctly folds into one entry, being the same file.
• Set-invariance, measured directly: root1's minted qualifier is byte-identical with 2 roots and with 3. That is the PkgA → eventId-1 shift you saw, tested for and absent.
• Beyond your two layouts: a sweep over combinations of hostile root shapes — exact, nested-leaf, symlink-alias, a root that is a path prefix of another, and a root whose own segment equals the rel name — covering 113 layouts, of which 93 minted qualified keys, with 0 invariant violations. The middle figure is the vacuity guard: without asserting that layouts actually produced qualified keys, zero violations would be consistent with collisions never being exercised. An inverted control that corrupts a minted qualifier is caught as unresolvable, so the check can fail for the intended reason.

On the test you challenged — test_every_enumerated_package_key_resolves (test/test_resolve_skill_root_package.py:2416) now carries both fixtures you named as missing: a nested-leaf root (:2444) and a symlink-alias root (:2449), plus a guard asserting the nested-leaf root actually contributed a key, so the resolve assertion cannot pass on an empty set. A sibling test covers the three non-package/ territories the same way.

One note from the rebase, since it touches your area: resolving against the read-only-territory change kept both write guards rather than either replacing the other — kiro-user//kiro-workspace/ and package/ each answer 405 with Allow: GET, and each is refused on create. I also carried that change's descriptor gate into the package row read, which this branch relocates to run after the resolver; without that the hardlink defence would have been silently dropped from that path.

[rnoack1]

You were right at the commit you reviewed, and all three parts are now fixed in code rather
than answered in prose. Re-verified at head 85c711db69c9c6472abdd95689471849cfc7f27a.
Your line numbers have drifted, so each site is re-located by content below.

Why the mechanism you described was real. At 1118db3e5, the sha your review names,
_root_segment_qualifier was live (5 occurrences) and the resolver did carry its own separate
_dedupe_resolved (4 occurrences). So the qualifier really was a path SEGMENT re-derived per
resolve against a set the enumerator had not counted — exactly your mechanism, and exactly the
shape that lists a key detail then 404s. Both symbols are gone at head (0 occurrences each).

1. The two sides now derive from one set, restricted to the exact tier. You offered two
arms; both are in. _package_collision is defined once at _shared.py:1667 and has exactly two
call sites — the resolver at _shared.py:700 and the enumerator at _shared.py:1794, so the two
cannot drift. And the derivation set is no longer the union you flagged: _shared.py:701 passes
only root.glob(f"{name}/SKILL.md"), the exact relative-path tier. The nested-leaf tier is
collected separately at _shared.py:710, after the qualifier filter at _shared.py:705, so it
never widens the derivation set.

The qualifier also no longer depends on that set at all. _root_identity_token
(_shared.py:1553) is a blake2b digest of the root's own canonical path, applied per root at
_shared.py:1704: qualifiers = [_root_identity_token(root) for root in roots]. The set decides
only WHETHER a rel is qualified (len(copies) < 2, _shared.py:1701), never WHICH qualifier a
root gets — so a wider or narrower set cannot shift a derivation deeper or to None.

Your cited _shared.py:704 now reads return None; the derivation itself sits at
_shared.py:700-701.

2. The test now carries both fixtures you named.
test_every_enumerated_package_key_resolves (test_resolve_skill_root_package.py:2416) builds a
nested-leaf root at :2445-2447 and a symlink-alias root at :2452
(.symlink_to(root_a / "shared-skill", target_is_directory=True)), passes both to the catalog at
:2454, and asserts the invariant at :2466. There is also a vacuity guard at :2459-2461
asserting the nested-leaf root actually contributed a key, so the test cannot pass by building a
fixture that never exercises the tier.

3. The docstring sentence you quoted is still there, and is now true.
_shared.py:1742-1743 reads "Omitting keeps the documented invariant absolute — every key this
walk offers is one _resolve_skill_root accepts". Omission is the mechanism that makes it
absolute: a rel that would not resolve is omitted and logged with its absolute path rather than
offered. The description is scoped the same way — it claims the invariant over keys the catalog
OFFERS, and names the omitted populations rather than glossing them.

I did not take your third arm (weaken the description to name layouts where an enumerated key
does not resolve), because the invariant now holds and naming such layouts would document
behaviour that is no longer there.

Measured at this head, not just read. Both layouts from your review behave correctly: the
first enumerates three keys, all of which resolve; the second collapses the alias to two. The
qualifier for a given root is byte-identical at two and at three colliding roots. A sweep over
113 hostile root layouts — exact, nested-leaf, symlink-alias, path-prefix and segment-shadowing
shapes — minted 93 qualified keys with zero that failed to resolve, and a sensitivity control
that perturbs the derivation does fire, so the sweep is not vacuous.

[rnoack1]

You were right at the commit you reviewed, and this is fixed in code rather than answered in
prose. Re-verified at head 23ae3b32abf4826b2efc84bd6c627131e73e1aa5. Your line numbers have
drifted, so each site below is re-located by content.

Why the mechanism you described was real. At 1118db3e5, the sha your review names, the
qualifier was a path SEGMENT chosen against the colliding set (_root_segment_qualifier, 5
occurrences) and the resolver computed its own separate collision set (_dedupe_resolved, 4
occurrences). That is exactly the set-dependent derivation your objection describes, and the
shape that mints a key enumeration offers and resolution refuses. Both symbols are gone at head
(0 occurrences each).

1. One shared derivation, restricted to the exact tier. You offered two arms and both are in.
_package_collision is defined once at _shared.py:1667 with exactly two call sites — the
resolver at _shared.py:700 and the enumerator at _shared.py:1794 — so there is no second copy
of the rule left to drift. The derivation set is no longer the union you flagged: _shared.py:701
passes only root.glob(f"{name}/SKILL.md"), the exact relative-path tier. The nested-leaf tier is
collected separately at _shared.py:710, after the qualifier filter at _shared.py:705, so it
never widens the set.

The qualifier also no longer depends on that set. _root_identity_token (_shared.py:1553) is a
blake2b digest of the root's own canonical path, applied per root at _shared.py:1704:
qualifiers = [_root_identity_token(root) for root in roots]. The set decides only WHETHER a rel
is qualified (len(copies) < 2, _shared.py:1701), never WHICH qualifier a root gets — so
widening or narrowing it cannot shift a derivation deeper or to None.

2. The test carries both fixtures you named.
test_every_enumerated_package_key_resolves (test_resolve_skill_root_package.py:2416, body to
:2468) builds a nested-leaf root at :2445 and a symlink-alias root at :2452
(.symlink_to(root_a / "shared-skill", target_is_directory=True)), and asserts the invariant at
:2466. A vacuity guard at :2459-2461 asserts the nested-leaf root actually contributed a key,
so the test cannot pass on a fixture that never exercises the tier.

3. The docstring sentence you quoted is still there and is now true. _shared.py:1742 reads
"Omitting keeps the documented invariant absolute — every key this walk offers is one
_resolve_skill_root accepts". Omission is the mechanism that makes it absolute: a rel that would
not resolve is omitted and logged with its absolute path rather than offered.

I did not take your third arm (weaken the description to name layouts where an enumerated key does
not resolve), because the invariant now holds and naming such layouts would document behaviour
that is no longer there.

Measured at this head, not just read. Both layouts from your review behave correctly: the
first enumerates three keys, all resolving; the second collapses the alias to two. A given root's
qualifier is byte-identical at two and at three colliding roots. A sweep over 113 hostile root
layouts — exact, nested-leaf, symlink-alias, path-prefix and segment-shadowing shapes — minted 93
qualified keys with zero that failed to resolve, and a sensitivity control that perturbs the
derivation does fire, so the sweep is not vacuous.

[rnoack1]

You were right at the commit you reviewed, and the remedy you asked for is the one that
shipped. Re-verified at head 9351866a90c8629ba8a7f003704d708a76b4a152.

Why the mechanism was real. At 1118db3e5, the sha your review names, the resolver did
carry its own holders/_dedupe_resolved set and the qualifier was a path SEGMENT chosen
against whichever roots collided at derivation time — so widening the set shifted the value,
which is exactly the disagreement you describe. All three of those symbols are gone at head:
holders 0, _dedupe_resolved 0, _root_segment_qualifier 0 (positive control:
_package_collision 5, ^def 67 in the same file).

One collision set, not two. You offered "derive the collision set in one shared helper
that both the enumerator and the resolver call" — that is what is there.
_package_collision is defined once at _shared.py:1667 and has exactly two call sites:

• resolver — _shared.py:700, passing [(root, hit) for root in roots for hit in root.glob(f"{name}/SKILL.md")] at _shared.py:701
• enumerator — _shared.py:1794, passing that rel's walked entries

Both admissions you name are addressed at the call site rather than inside the helper. The
derivation set is the exact relative-path tier only; the nested-leaf */{name}/SKILL.md
tier is collected separately at _shared.py:710, after the qualifier filter at
_shared.py:705-707, so it never widens the set the qualifier is derived from. And the
qualifier no longer depends on the set at all: _root_identity_token (_shared.py:1553) is a
blake2b digest of the root's own canonical path, applied per root at _shared.py:1704. The
set decides only WHETHER a rel is qualified (len(copies) < 2, _shared.py:1701), never
WHICH qualifier a root gets.

I tried to reproduce the mint-then-fail on both shapes you named, and it does not. A
fixture per shape, auditing every minted key by resolving it and comparing to the file it was
enumerated for:

• nested-leaf tier root — 2 keys minted, the nested-leaf root contributed 1, all resolve.
• symlink-alias root — the alias is traversed (glob positive-controlled to see through it, so
  the zero is not an artefact of traversal defaults); the alias dedupes to one copy, so the
  rel is keyed unqualified, and it resolves.
• all four together (two exact + nested-leaf + alias) — 3 keys minted, all resolve.

That clean result only means something because the audit is proven able to fail: re-deriving
the qualifier differently on the two sides makes it report 2 MINT-THEN-FAIL keys, with
your signature — enumeration offers a qualified key and resolution returns nothing. Two
earlier versions of that control did NOT fire and I discarded them rather than report them;
both failed for the same instructive reason, that merely widening the resolver's set is a
no-op once the qualifier is a per-root digest. The defect needs a set-dependent derivation,
which is the property the replaced segment machinery had and this one does not.

On the description. The sentence you quoted — "When no segment tells the colliding roots
apart" — is no longer in it (0 occurrences, against 11 for "segment"), because the segment
machinery it described is gone. Omission is now grounded in a root that does not canonicalise
and yields no identity, which is what the code does. The test-side claim you quoted is
accurate: the invariant is asserted by resolving every enumerated key, and its fixtures now
include both a nested-leaf root and a symlink-alias root plus a guard that the nested-leaf
root actually contributed a key.

[rnoack1]

Item 1 — "enumeration still emits qualified keys the resolver refuses" — is fixed at e152e3921645d97db65594fbe046c548bb30832d, and the fix is that both sides now derive the collision set from one function rather than two spellings.

_package_collision is defined once at src/kiro_crew/dashboard/handlers/_shared.py:1676 and has exactly two call sites:

• _shared.py:700 — the resolver, which globs f"{name}/SKILL.md" across the package roots and passes the result in
• _shared.py:1805 — the enumerator, on the same shape

Neither side computes membership itself, so there is no second collision rule that could disagree with the first. Both None returns are fail-closed and are documented at _shared.py:1751: when the function cannot mint a qualifier for every colliding copy it mints none, so a key is never offered on the strength of a partial derivation.

The qualifier itself is _root_identity_token (_shared.py:1553), a digest of the holding root's canonical path alone. Because it is a function of the root and nothing else, widening or narrowing the candidate set cannot re-spell an already-minted key — that was the other half of how the two sides could drift apart.

The invariant is enforced rather than asserted: test/test_resolve_skill_root_package.py:2416 resolves every key the catalog emits and compares it to the file it was enumerated for, including the nested-leaf and symlink-alias cases, with a vacuity guard so it cannot pass by enumerating nothing.

One correction I owe you in the other direction, found by a review lane on a later sha and now stated in the code: resolution is deliberately wider than enumeration, not its exact inverse. The resolver also accepts a bare leaf name through its nested tier, so a rel the catalog lists at its full relative path stays reachable under the leaf too. The direction your item names — a key offered that resolves to nothing — is the one that is closed and tested; I have narrowed the claim in _shared.py so it no longer says more than that.

[rnoack1]

Both required changes are in place at head eedfbe5a42c6ca2599dc9f03653d6fc87aca0ad5.

The collision set is derived in one shared helper that both sides call. _package_collision is defined once at src/kiro_crew/dashboard/handlers/_shared.py:1678 and called at :702 by the resolver and :1807 by the enumerator — the single-helper remedy your review asked for, so the two cannot drift. The holders / foreign derivation the report describes is gone: a grep for holders, foreign and _dedupe_resolved across that file returns 0 (positive control on the same command: _package_collision 5 hits, _root_identity_token 6). Your cited :704 is now a fail-closed return None guard immediately after that shared call.

The qualifier is no longer a path segment. It is a per-root identity digest (_root_identity_token), a function of the root's canonical path alone. That removes the mechanism in your report rather than patching it: because the digest does not depend on what else is in the candidate set, a wider set at resolve time cannot re-spell a genuine holder's qualifier deeper or to None.

The invariant test now carries both fixtures you named. test_every_enumerated_package_key_resolves at test/test_resolve_skill_root_package.py:2416 builds the layout your report documents — a nested-leaf root packages/PkgA/eventId-3/skills holding PkgA/shared-skill/SKILL.md at :2445-2447, and a symlink-alias root at :2452 whose copy is another root's — plus a vacuity guard at :2459-2461 so the assertions cannot pass on a catalogue that never saw those shapes. It then resolves every enumerated key and compares it to the file it was enumerated for.

I checked that the test can actually fail for your defect, rather than assuming its absence. Reintroducing the mechanism — making the derivation set-dependent while feeding the resolver and the enumerator different sets — makes that test fail with package/143790fc...:shared-skill did not resolve to its own file. Two narrower mutations did not reproduce it and were discarded; the two-part one does, so the guard is live rather than vacuous.

One clarification on the invariant's wording, which your review was right to press. The guarantee is one-directional, and the text now says so: every key enumeration emits resolves. The converse does not hold, because resolution deliberately also accepts a leaf-name key through its nested tier, so a rel listed at its full relative path stays reachable under the bare leaf. The "inverse" phrasing is removed from _shared.py:646 and the description no longer claims a two-way absolute.

[rnoack1]

Both required changes are in place at head f435b2aa793990fdab846394c3a6520b91735e6a, and I re-verified each at source rather than assuming the earlier rounds settled them.

The collision set is derived once, by a helper both sides call. _package_collision is defined at src/kiro_crew/dashboard/handlers/_shared.py:1684 and called at :706 by the resolver and :1813 by the enumerator — the single-helper remedy your review asked for, so the two cannot drift. The holders / foreign derivation the report describes is gone: a grep for holders, foreign and _dedupe_resolved across that file returns 0, with a positive control on the same command showing _package_collision at 5 hits. Your cited :704 is now inside that shared call's fail-closed guard.

The re-derivation hazard is removed rather than patched. The qualifier is a per-root identity digest, a function of the root's own canonical path alone. Because it does not depend on what else is in the candidate set, a wider set at resolve time cannot shift a genuine holder's qualifier deeper or to nothing — which is the mechanism the report rests on. Relatedly, the resolver's derivation set is the EXACT tier only; the nested-leaf */{name}/SKILL.md glob runs after the qualifier filter, so a nested-leaf root cannot join that set at all.

Both layouts from your report are now permanent tests, not incidental fixtures. test/test_resolve_skill_root_package.py:3366 builds the three-root nested-leaf layout exactly as reported (packages/PkgA/eventId-1/skills, packages/PkgB/eventId-2/skills, and packages/PkgA/eventId-3/skills holding PkgA/shared-skill/SKILL.md), and :3405 builds the symlink-alias variant. Each enumerates and then resolves every emitted key against the file it was emitted for.

I checked those tests can actually fail for your defect rather than passing vacuously. Reintroducing the mechanism — making the derivation set-dependent while feeding the resolver and the enumerator different sets — makes the nested-leaf test fail with resolved to nothing; removing the enumerator's dedupe makes the alias test fail with the alias was not collapsed to one pair. Two narrower mutations did not reproduce it and were discarded as vacuous.

One clarification your review was right to press: the invariant is one-directional and the text now says so. Every key enumeration EMITS resolves; the converse does not hold, because resolution deliberately also accepts a leaf-name key through its nested tier. The "inverse" phrasing is removed from _shared.py (0 occurrences) and the description no longer claims a two-way absolute.

[rnoack1]

Re-verified at head 96fa20507d825bbfa5332bb414def1b25a2aee90 — two shas on from my previous reply, so here is the evidence against the current tree rather than an older one.

Your Required change offered three routes. The code takes the one you named as preferable, and the two specific mechanics you asked for are in place as well:

• "derive the collision set in one shared helper that both the enumerator and the resolver call, so the two cannot drift." _package_collision is defined once at src/kiro_crew/dashboard/handlers/_shared.py:1684 and is the only derivation on either side: the resolver calls it at :706, the enumerator at :1813. There is no second spelling to drift from.
• "dedupe holders by resolved file identity." That fold is inside the shared helper (_dedupe_entries), so both sides get it from the same place rather than each applying it.
• "restrict the derivation set to exact relative-path holders rather than the union with the nested-leaf tier." The set passed at :707 is the exact tier alone. The nested-leaf */{name}/SKILL.md glob is collected separately at :716, which runs after the qualifier filter at :711-713 — so a nested-leaf root cannot enter the set the qualifier is derived against.

The holders / foreign derivation your review describes is gone: a grep for holders, foreign and _dedupe_resolved across that file returns 0, with a positive control on the same command showing _package_collision at 5 hits and _root_identity_token at 6. Your cited :704 is now inside that shared call's fail-closed guard.

Separately, the re-derivation hazard is removed rather than worked around: the qualifier is a per-root identity digest, a function of the root's own canonical path. Because it does not depend on what else is in the candidate set, a wider set at resolve time cannot shift a genuine holder's qualifier deeper or to nothing — which is the mechanism the report rests on.

"Then extend test_every_enumerated_package_key_resolves with a symlink-alias root and a nested-leaf root so the invariant is actually pinned." Done, in test/test_resolve_skill_root_package.py: the nested-leaf root at :2445-2447 (packages/PkgA/eventId-3/skills holding PkgA/shared-skill/SKILL.md) and the symlink-alias root at :2452, with a vacuity guard at :2461 so the assertions cannot pass on a catalogue that never saw those shapes. Both of your reproductions are also standalone tests now, at :3366 and :3405.

I checked those tests can actually fail for your defect rather than passing vacuously. Reinstating the mechanism — making the derivation set-dependent while feeding the resolver and the enumerator different sets — fails them with resolved to nothing; removing the enumerator's dedupe fails the alias case with the alias was not collapsed to one pair. Two narrower mutations did not reproduce it and were discarded.

On the wording your title flagged: the invariant is one-directional and the text now says so. Every key enumeration EMITS resolves; the converse does not hold, because resolution deliberately also accepts a leaf-name key through its nested tier. The "inverse" phrasing is gone from the source (0 occurrences) and the description no longer claims a two-way absolute.

[github-actions]

UX Review (Fable 5, fork) — 🟡 CONCERNS

UX-level review of c6f996809cae6b6c4a5fb4bda116adc84589c3d1 via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

The screenshots the PR adds under temp-screenshots/package-skill-key-qualifier/ are not on disk in this lane (fork — binary markers only), so no first-time reader has seen any of the new surfaces. I've read the full frontend diff (AgentSkillsEditor.tsx, en.manual.json, AgentsPage.tsx) and reconciled every added control against that. Final review:

UX-Verdict: CONCERNS

Removing a warned chip silently also adds the held pick — the compound action lives only in a tooltip and a bare "+ name" fragment.

Watch

• Compound remove: after a blocked add, the chip's X performs remove-AND-add (remove() carries pendingAdd), but the visible affordance is the habituated remove-X plus "+ {{pick}}" — a fragment that fails a cold read; the full sentence ("Remove {{name}} and add {{pick}}") is title/aria-label only, invisible to touch and to anyone who dismissed the earlier notice. Low frequency (stale-key refusal) × moderate impact (unexpected mapping added, reversible) — make the visible carry text explicit, e.g. "removing adds {{pick}}".
• No visible way to discard the held pick: pendingAdd clears only on a successful save, so a user who changed their mind must remove the blocker (pick gets added) then remove the pick. Fold into the same fix: an explicit carry affordance with a dismiss.

Evidence gaps

• Picker rows' "Located in {{where}}" disambiguator line — screenshots 1/5 exist in the patch but are unreadable here; no blind read ran (push the branch to this repo to get one).
• Chip-side disambiguator and qualified-key chip label — screenshot 2, same.
• Unresolved warn chip (AlertTriangle, bg-warn-subtle) and the "{{count}} mapped skill no longer matches…" count line — screenshot 7, same.
• Rejected-write notices (key_changed_repick / _remove_blocked / _carry) and the "+ {{pick}}" carry state — screenshots 3/6, same.

Suggestions

• also_adds_pick: replace "+ {{pick}}" with a verb-carrying string such as "then adds {{pick}}" so the chip itself states the side effect.

[UX-REVIEWED] c6f9968

[rnoack1]

Both claims from your review verified at the current head b29ed28c706998ba29abff4cf14587d4a2f63699. Your review was written against 1118db3e5, and the mechanism you named has since been closed at cause level — here is the evidence rather than an argument.

"The path is omitted and logged" — src/kiro_crew/dashboard/handlers/_shared.py:1870-1882. When the collision helper returns no qualifiers and two or more distinct copies remain, the enumerator emits a logger.warning naming the rel, the number of roots that share every path segment, and the on-disk paths of the affected SKILL.md files, then continues. So the rel is omitted from the catalogue and the omission is diagnosable rather than silent.

"The invariant is asserted by resolving every enumerated key" — test/test_resolve_skill_root_package.py:2464-2466. For every key the enumerator produced, the test calls _resolve_skill_root(key) and compares the result with the file that key was enumerated for. The two fixture shapes your review said were missing are now both present: a nested-leaf root at :2445 and a symlink-alias root at :2452, with a vacuity guard at :2459-2461 asserting the nested-leaf root actually contributed a key — so the fixture cannot satisfy the assertions without exercising both shapes.

On the derivation asymmetry itself, which was the substance of your finding: the collision set is now computed by one shared helper, _package_collision (_shared.py:1735), called from exactly two sites — the resolver at :719 and the enumerator at :1867. The resolver derives over the exact-relative-path glob alone, so the union with the nested-leaf tier that your reproduction relied on no longer exists. Your suggested remedy of "derive the collision set in one shared helper that both the enumerator and the resolver call" is what shipped.

One related hardening landed on this head after a separate review pass, and it is worth naming because it removes the last route into the same class of rebind: a qualified key is now answered with the candidate whose identity was already checked (_shared.py:733) rather than by re-globbing the root that check selected. The re-glob left two windows open — a bundle replaced at that path between the two reads, and the nested-leaf tier being reachable when the second read found nothing — and both are closed by reusing the checked candidate. Fewer or more than one match refuses rather than guessing.

[rnoack1]

Verified at head 0353f691cc2a3ca8bffa5d604008fc938128f4c6. Your review was written against 1118db3e5, and the derivation has changed since, so I re-checked both of your repro layouts rather than assume.

The mechanism you describe — an extra holder that shares a qualifier segment shifting a genuine holder's derived qualifier deeper or to None — required the qualifier to be a distinguishing path segment. It no longer is:

• src/kiro_crew/dashboard/handlers/_shared.py:1854 — qualifiers = [_root_identity_token(root) for root in roots]. The qualifier is now a per-root digest (blake2b over the root's canonical path plus st_dev:st_ino:st_ctime_ns), so it depends only on that root. Adding a third holder cannot move another holder's qualifier, whatever segments it carries.

Your two structural asks are also both in place:

• One shared collision helper. _shared.py:723 (resolver) and _shared.py:1947 (enumerator) are the only two call sites, and both call the same _package_collision. There is no second derivation to drift.
• holders deduped by resolved identity. _shared.py:1850 — that helper opens with copies = _dedupe_entries(entries), so the symlink-alias root of your second repro is collapsed on the resolver side exactly as it is on the enumerator side.
• Derivation restricted to exact relative-path holders. _shared.py:723-724 builds the qualified set from root.glob(f"{name}/SKILL.md") only — the exact tier. The nested-leaf */{name}/SKILL.md tier is built separately at _shared.py:738-740 and is not unioned into the qualified derivation, so root3 of your first repro does not join the set that derives root1's qualifier.

And the test you asked for is extended: test/test_resolve_skill_root_package.py:2416 (test_every_enumerated_package_key_resolves) now builds a nested-leaf root (<root>/<Pkg>/<rel>/SKILL.md) and a symlink-alias root in the same fixture, asserts the fixture actually exercises both shapes before asserting anything else, then resolves every enumerated key back to its own file. It passes at this head.

Separately, and not something you raised: the qualifier is now re-derived against a pinned root descriptor (_pinned_qualified_hit, _shared.py:1701) rather than against the root's name, and the skill directory is opened relative to that descriptor so the traversal stays inside the verified root. A qualified key fails closed where the platform cannot pin.

[rnoack1]

Fixed at head 610535af63af2f6e77ba61b06a7d483231f89737. Your item 1 — "enumeration still emits qualified keys the resolver refuses, the invariant does not hold" — was correct, and it was still correct at the head before this one, for a reason that was not in your original mechanism. Recording what I read.

What was actually breaking it at the previous head. Resolution had grown a platform precondition: the qualified path opened a pinned directory descriptor and returned None when pinned_fs.supports_pinned_walk() was false. Enumeration had no matching gate. So on any platform without openat-style directory descriptors — every native Windows install — the catalogue minted qualified keys that the resolver categorically refused. That is your finding exactly: enumerated keys the resolver will not accept.

The fix is a subtraction, not a patch. The whole pin path is gone at this head, and qualified keys resolve by the same by-name digest re-derivation everywhere:

• src/kiro_crew/dashboard/handlers/_shared.py:735 — the qualified branch now ends return matched[0][1], the candidate whose re-derived digest equalled the key's qualifier. No descriptor, no platform precondition. supports_pinned_walk no longer appears in that file at all, so there is no platform on which resolution refuses what enumeration offered.
• src/kiro_crew/hooks.py — back to byte-identical with the base branch; the dir_fd read parameter that path needed is gone.

On your original mechanism — two different collision sets. That specific shape does not reproduce here, and the reason is worth stating because it is not what your review assumed:

• _shared.py:1790 — qualifiers = [_root_identity_token(root) for root in roots]. The qualifier is a per-root digest (blake2b over the root's canonical path plus st_dev:st_ino:st_ctime_ns), not a distinguishing path segment. It depends only on that root, so an extra holder cannot shift a genuine holder's qualifier deeper or to None, whatever segments it carries.
• _shared.py:721 (resolver) and _shared.py:1883 (enumerator) are the only two call sites, and both call the same _package_collision. There is no second derivation to drift.
• _shared.py:1786 — that helper opens copies = _dedupe_entries(entries), so the symlink-alias root of your second repro is collapsed on the resolver side exactly as on the enumerator side.
• The resolver's qualified set is built from root.glob(f"{name}/SKILL.md") — the exact tier only. The nested-leaf */{name}/SKILL.md tier is built separately and is not unioned into the qualified derivation, so root3 of your first repro does not join the set that derives root1's qualifier.

The test you asked for. test/test_resolve_skill_root_package.py:2416 (test_every_enumerated_package_key_resolves) builds a nested-leaf root (<root>/<Pkg>/<rel>/SKILL.md) and a symlink-alias root in one fixture, asserts the fixture actually exercises both shapes before asserting anything else, then resolves every enumerated key back to its own file. It passes at this head.

Also dropped in the same commit, since it existed only to observe the pin path: the success-read audit line on the package detail read. The refusal audits are unchanged.

[rnoack1]

Re-verified at head 87e945392e4a7f7e1b5992ed8dd0312f9586d586.

Your item — description/code mismatch on the enumeration invariant: satisfied at this head.

The mismatch you found was real: the description claimed the invariant absolutely while the
resolver and the enumerator each built their own collision set, so enumeration could mint a
qualified key that resolved to nothing. Both sides now derive it from one function:

• src/kiro_crew/dashboard/handlers/_shared.py:1762 — _package_collision, the single
  computation.
• Its only two callers are the resolver at :721 and the enumerator at :1894. (A third
  match on that name is the def line itself.)
• test/test_resolve_skill_root_package.py:2416 —
  test_every_enumerated_package_key_resolves enumerates, then resolves every key back,
  carrying both a nested-leaf root and a symlink-alias root plus a vacuity guard so the
  assertion cannot pass on an empty catalog.

One thing your review did not name, corrected in the same commit. The description also
claimed the picker rendered a per-row qualifier. It cannot: GET /api/skills passes each
source catalog through unchanged and never calls enumerate_skill_catalog, so no qualified
key reaches that surface. The picker's qualifier span, its origin derivation and the test
that asserted the behaviour are removed, and the screenshot showing two colliding picker
rows is gone from the description — it depicted a state the endpoint cannot produce. A
qualified key is still addressable where it is genuinely reachable: a key already mapped
into an agent's skill:// resources, whose chip renders the readable half plus the digest.

Also in this commit, from a CI failure rather than from review: the read-time identity check
compared a private (st_dev, st_ino) tuple, and a shard hit a filesystem that handed a
replacement directory the inode its predecessor had just released, so a swapped bundle read
as unchanged. The check now re-derives the qualifier through _root_identity_token, the same
function that mints it. The residual is recorded in the docstring at
src/kiro_crew/dashboard/handlers/prompts.py:60: a same-path replacement inside one
st_ctime_ns granule is still indistinguishable to any stat-based witness, measured at 39
of 40 delete-and-recreate cycles on one host.

[rnoack1]

Verified at head 42967088b37d9c8767d8b0a7d7099204922c62c9. Your finding was correct, and all three of the changes you named are in. Details below by search string rather than line number, since the numbers in your review have drifted.

The two sides now derive the collision set in one function. _package_collision is defined once in _shared.py, and it has exactly two call sites: the resolver and the enumerator. Nothing else computes a collision set, so the two cannot drift apart again — which was the shape of the defect rather than any single symptom of it.

The nested-leaf tier no longer joins the derivation set. The resolver passes [(root, hit) for root in roots for hit in root.glob(f"{name}/SKILL.md")] — the exact relative path only. There is no */{name}/SKILL.md term in that input, so a tier-2 leaf root can no longer enter holders and shift a genuine holder's qualifier deeper or to None. The resolver is still deliberately wider than the enumerator in the accept direction, because it also honours a leaf-name key through its nested tier; what it no longer does is let that tier change how a qualifier is derived.

Symlink aliases are deduplicated inside that shared function. _package_collision folds its entries through _dedupe_entries, which keys on resolve() on both sides rather than on a prefix test of the unresolved path — so an aliased copy is one distinct file, not a second holder. That is the same dedupe the enumerator was already applying; it now applies to both sides because both go through the one function.

Your reproduction is a permanent test rather than a fixed case. test_every_enumerated_package_key_resolves now builds the layout from your report: packages/PkgA/eventId-3/skills holding PkgA/shared-skill/SKILL.md (the documented tier-2 leaf, carrying the colliding PkgA segment), plus packages/PkgA/eventId-4/skills whose shared-skill is a symlink_to root A's copy. It asserts the fixture actually exercises both shapes before it asserts anything else, so the test cannot go vacuous if a future change stops producing them, then resolves every enumerated key back to its own file. Green at this head, with the surrounding drift-shape tests.

The description no longer claims the invariant is absolute. It now states the invariant holds "in the ONE direction it claims", which is the narrower statement your review asked for.

Your closing note about the mechanism sentence being over-broad was also right, and worth recording: an extra holder only poisons the derivation when its own path carries the segment the enumerator picked, which is why the three-root test with disjoint segments passed while the guarantee was false. Both fixture roots above are built to carry the colliding segment specifically so that the test exercises the real mechanism rather than the general shape.

[rnoack1]

Re-verified at head dfef3369641ba9ffea593850dcc59acdc02b7523 (the branch has since been rebased onto current main, so this confirms the item still holds there rather than only at the head I cited before). Evidence by search string, since the line numbers in your review have drifted.

The two derivations are one function. _package_collision is defined once in _shared.py and has exactly two call sites: the resolver and the enumerator. Nothing else computes a collision set, so they cannot drift apart again — which was the shape of the defect rather than any single symptom of it.

The resolver's derivation input is the exact relative path only. It passes [(root, hit) for root in roots for hit in root.glob(f"{name}/SKILL.md")]. There is no */{name}/SKILL.md term in that input, so the tier-2 leaf layout can no longer join the set. That tier still exists a few lines above, but only to accept a leaf-name key — it no longer feeds the derivation.

Symlink aliases are deduplicated inside that shared function, via _dedupe_entries, which keys on resolve() on both sides rather than on a prefix test of the unresolved path. So an aliased copy is one distinct file, not a second holder.

On the mechanism specifically — worth stating because it is what makes the whole class unreachable rather than merely fixed at these two sites. A qualifier is _root_identity_token(root), a digest of that root's own canonical path. It is not a distinguishing path segment. So the membership of the collision set can change only whether a qualifier is emitted (fewer than two distinct copies means the rel is keyed unqualified), never what it is. Your finding turns on a segment that is unique at enumeration time and no longer unique at resolve time; a per-root digest has no such dependency. The function's own docstring now says this outright: "it must be an identity rather than a distinguishing path segment."

I checked that empirically rather than asserting it: re-adding the nested-leaf tier to the resolver's derivation input leaves test_every_enumerated_package_key_resolves passing, because the extra holder cannot shift another root's digest.

And the test is not passing vacuously. Both layouts from your report are now permanent fixtures in it — packages/PkgA/eventId-3/skills holding PkgA/shared-skill/SKILL.md, and a packages/PkgA/eventId-4/skills symlink alias, both carrying the colliding PkgA segment — behind an assertion that fails if the fixture stops exercising either shape. To confirm it still bites, I injected a genuine enumerate/resolve asymmetry (enumeration minting a qualifier for a lone copy the resolver keys unqualified) and it failed as it should: assert 'package/only-a' in {'package/915ea9c6…:PkgA/shared-skill': …}. Reverted immediately; 13 related tests pass at this head.

The description no longer claims the invariant is absolute — it states that it holds "in the ONE direction it claims", which is the narrower wording your review asked for.

Your closing note was also right and is worth recording: the mechanism sentence was over-broad, since an extra holder only mattered when its own path carried the segment the enumerator had picked — which is why the original three-root test passed while the guarantee was false. Both fixtures above are built to carry the colliding segment for exactly that reason.

[rnoack1]

@bolichen97 — this is addressed at head 3e38820fe. Both halves of the objection, with the lines I read:

The two collision sets are now one derivation. _package_collision is defined once at src/kiro_crew/dashboard/handlers/_shared.py:1843, and both sides call it rather than each building their own set:

• enumerator — _shared.py:1975: entries, qualifiers = _package_collision(entries)
• resolver — _shared.py:739-740: copies, qualifiers = _package_collision([(root, hit) for root in roots for hit in root.glob(f"{name}/SKILL.md")])

The resolver's input is the exact relative-path glob only. There is no union with the nested-leaf tier, which was the specific widening your required-change note named.

The qualifier no longer depends on which roots are in the set. _shared.py:1882 reads qualifiers = [_root_identity_token(root) for root in roots] — a digest of each root's own canonical path. It takes no other-roots argument, so adding or removing a root cannot shift another root's qualifier. That is the mechanism your reproduction relied on: at the sha you reviewed the derivation was _root_segment_qualifier(root, [o for o in holders if o != root], name), which took the other holders as an input, so a wider set genuinely re-derived a different segment. That function and the holders set no longer exist.

Reproduced both of your layouts at this head, asserted the way your review specifies — resolving every enumerated key and comparing it to the copy it was enumerated for:

• root1 packages/PkgA/eventId-1/skills + root2 packages/PkgB/eventId-2/skills + root3 packages/PkgA/eventId-3/skills holding PkgA/shared-skill/SKILL.md — 3 keys enumerated, all 3 resolve to the copy they were enumerated for, including the tier-2 package/PkgA/shared-skill.
• The no-nested-tier variant, root3 packages/PkgA/eventId-2/skills whose shared-skill is a symlink to root1's copy — 2 keys enumerated, both resolve correctly.

No key resolves to None and none resolves to a different copy. Note the qualifiers now render as a 32-hex digest rather than the PkgA / PkgB segments in your reproduction — that difference is the fix, not a difference in fixture.

The description text you quoted is gone. The sentences about "no segment tells the colliding roots apart" and a "qualifier drawn from a shared segment" have 0 occurrences in the current body, and it now states the opposite explicitly: membership of a segment in a root is not the contract, because a replaced root that still carries a segment would otherwise keep resolving.

Pinned so it cannot regress. The invariant test carries your deeper packages/<Pkg>/<event>/skills layout with both a nested-leaf root and a symlink-alias root, plus a vacuity guard that fails if the fixture mints no qualified key. It is mutation-controlled: reintroducing both properties your bug needed — a wider resolver set and a set-dependent derivation — makes it fail with both qualified keys unresolved, matching what you observed.

[rnoack1]

Verified at head 1676cfbe01. Both of your reproductions were run as tests against this head, and your second point was right in a way the first one no longer is — so there is a code change here, not just a measurement.

Your reproductions resolve at head. I built both layouts exactly as written — packages/PkgA/eventId-1/skills, packages/PkgB/eventId-2/skills, plus the tier-2 leaf root packages/PkgA/eventId-3/skills holding PkgA/shared-skill/SKILL.md; then the symlink variant with packages/PkgA/eventId-2/skills aliasing root1's copy. Layout A enumerates 3 package keys, layout B enumerates 2, and every one resolves to the copy it was enumerated for (guarded so an empty catalogue fails rather than passes).

The mechanism has moved since the revision you read. The qualifier is no longer a path segment: _root_identity_token(root) digests that root's own canonical path (src/kiro_crew/dashboard/handlers/_shared.py:727-728), so no other root can produce it and it cannot "shift deeper" when the holder set widens. The two symbols your description turns on, holders and the segment-qualifier helper, are both 0 occurrences in that file now. The collision set is also already a single shared derivation, which was your required change: _package_collision is defined once at _shared.py:1851 and called from exactly two places — the resolver at :739-740 and the enumerator at :1983. The resolver's qualified branch builds its input from the exact relative-path glob only (:740) and returns at :743/:752/:756/:757; the */{name}/SKILL.md tier at :760 is reached only for an unqualified key.

But your point about the test was correct, and it was still correct at head. You wrote that the invariant test does not actually pin the guarantee. The fixtures had since gained both shapes — a nested-leaf root and a symlink-alias root — yet I measured whether the test bites, by applying the two drifts you name:

• union the */{name} tier into the qualified collision set → forward test still passed
• drop the alias dedupe from the shared fold → forward test still passed

Both invisible. The reason is the digest: widening the holder set cannot re-spell a key the enumerator did mint, so "every enumerated key resolves" stays true. What widening actually breaks is the other direction — the resolver starts accepting qualified keys the catalogue never minted, and resolution stops being enumeration's inverse, which :730-732 claims it is.

So the fix is to assert that direction, which nothing did: test_no_qualified_key_the_catalog_withheld_resolves in test/test_resolve_skill_root_package.py. It constructs the qualified spellings for the nested-leaf and symlink-alias roots, asserts the catalogue genuinely withheld each one (non-vacuity, or the test would be satisfied by keys that were never candidates), then asserts the resolver refuses both. Under the two mutations above it now fails — the first on the main assertion, the second on the non-vacuity guard, since without dedupe the catalogue really does start offering the alias key.

Suites at this head: 127 on this file plus 122 under a Windows-behaviour simulation, and prompts 261 / skills 150 / skill-browser 83 / hooks 167 unchanged.

Thanks for the layouts — the forward invariant looked airtight and the mutation run is what showed it was load-bearing in only one direction.

[rnoack1]

You were right, and about the sha you reviewed. Both reproductions are fixed at head df3fb821e982a68552b1e79f4c24d74af6e3abfe, by the shared-helper route you named rather than the narrower one.

Your mechanism was the code at 1118db3e5. Measured on that tree: _package_collision did not exist (0 call sites), the qualified branch did glob */{name}/SKILL.md into its derivation set, and _root_identity_token did not exist — so the qualifier was a path segment that could shift deeper or to None exactly as you described. Nothing about that was a false positive.

What is at head instead:

• src/kiro_crew/dashboard/handlers/_shared.py:739-741 — the qualified branch calls _package_collision over root.glob(f"{name}/SKILL.md") only. The */{name}/SKILL.md tier is no longer unioned into the derivation set, which is the restriction you asked for.
• _package_collision (def at _shared.py:1889) is the single shared helper: the resolver calls it at :739 and the enumerator at :2021. There is no second spelling of the collision set to drift from, which is the "one shared helper both call" remedy.
• The helper dedupes by resolved file identity, so a symlink-alias copy collapses onto the copy it aliases rather than surviving as an extra holder.
• _root_identity_token (_shared.py:1719, derivation documented at :727-728) makes the qualifier a digest of the root's own canonical path. It is therefore set-independent: widening the holder set cannot re-spell a key the enumerator minted.

Both of your layouts, run at this head. Layout (a) — root1 PkgA/eventId-1/skills and root2 PkgB/eventId-2/skills holding shared-skill, root3 PkgA/eventId-3/skills holding PkgA/shared-skill — enumerates 3 keys and all 3 resolve to their own file. Layout (b), root3's shared-skill a symlink to root1's copy, enumerates 2 keys (the alias deduped) and both resolve; the ordering variant where you saw both qualified keys return None also resolves both. The keys are now digest-qualified (package/23d5ce57…:shared-skill), not segment-qualified, which is why the derivation cannot shift.

On the test you flagged as not pinning the invariant — that is fixed too. test_every_enumerated_package_key_resolves (test/test_resolve_skill_root_package.py:2442) now carries both shapes you said were absent: a nested-leaf root at :2471-2473 and a symlink-alias root at :2476-2478, both deliberately sharing the PkgA segment with a genuine holder, plus a non-vacuity guard at :2485-2487 asserting the nested root actually contributed a key. A separate test pins the inverse direction — that no qualified key the catalogue withheld resolves — since the forward invariant alone is blind to a resolver whose collision set is wider than the enumerator's.

Your narrowing was the useful part: because the extra holder only poisons the derivation when it shares the picked segment, a three-root fixture with disjoint segments passes while the guarantee is false. That is why the two new roots share PkgA rather than being merely additional.

This head also fixes an unrelated issue in the same area you may want to glance at: an unqualified package/ read on a platform without dir_fd answered 501 for an absent file while serving its present sibling 200, because the capability short-circuit ran ahead of the missing/withheld classification. It is now gated on the read having actually needed pinning (prompts.py, both the file and detail paths).

[rnoack1]

@bolichen97 — settled at head 1548bd70c5, by the shared-helper route you asked for.

One derivation, both callers. _package_collision is defined once, at
src/kiro_crew/dashboard/handlers/_shared.py:1947, and its only two call sites are the resolver
(_resolve_package_skill_path, :739) and the enumeration (_merge_package_walks, :2079). The
holders / foreign derivation your report named has zero occurrences in that file. The
resolver's entry set at :740 is the exact-relative-path tier only, not the union with the
nested-leaf tier.

The mechanism is no longer expressible. At :1985-1986 the qualifier for each holder is
_root_identity_token(root) — a digest of that root's own canonical path. It does not depend on
the holder set, so an extra holder cannot shift another holder's qualifier deeper or to None.
Installing or removing an unrelated bundle leaves every other key unchanged.

Your reproductions are tests now. The three-root nested-leaf layout and the symlink-alias
variant both resolve, alongside the invariant that resolves every enumerated key. I checked they
discriminate rather than merely pass: re-widening the resolver's collision set with the nested-leaf
tier no longer breaks anything (that is the point of the root-alone digest), while reverting the
qualifier to a set-relative segment — the shape your report describes — fails both layouts,
0 passed / 2 failed.

One thing your review prompted that is worth flagging separately: reserving : used to swallow a
skill directory legitimately named with a colon, since the key partitioned on the first : with no
fallback. A qualifier is now recognised only in the shape the catalogue mints (fixed-width lowercase
hex), so such a directory keys to its own path again.

[rnoack1]

@bolichen97 — I measured this rather than reasoning about it, and your headline turned out
right while the location moved. Head is now 6d48693519.

The invariant holds at this head, including on your layout. I built the three roots you
described — PkgA/eventId-1/skills and PkgB/eventId-2/skills each holding
shared-skill/SKILL.md, plus PkgA/eventId-3/skills holding the tier-2 leaf
PkgA/shared-skill/SKILL.md — enumerated the catalogue and then resolved every key it
emitted. Three keys, zero phantom rows:

RESOLVES  package/3300f9c3b6e343316cc3e61bc5c03d3a:shared-skill
RESOLVES  package/5db7d2b29d40aa090eb9596d5e642b15:shared-skill
RESOLVES  package/PkgA/shared-skill

The third root's nested leaf is keyed at its own full relative path, not folded into the
shared-skill collision set, so that set is {eventId-1, eventId-2} on both sides. The
package/PkgA:shared-skill your report predicted is never minted.

Two further layouts came out the same: two roots differing only above the rel, and both
roots nested. And the measurement is not vacuous — narrowing the resolver's own set at
src/kiro_crew/dashboard/handlers/_shared.py:746 makes exactly your predicted failure
appear on your layout, which is what tells me the zero means something.

Why the mechanism no longer applies. Your quoted qualifiers are PkgA and PkgB —
path segments, which is what the grammar used at the sha you reviewed. It now derives the
qualifier at :1992 as _root_identity_token(root), a digest of that root's own canonical
path and stat identity. Being a function of one root, it cannot shift when the candidate
set widens, so an extra holder cannot re-spell another holder's key. The set itself comes
from a single computation, _package_collision at :1953, called from exactly two places:
the resolver at :745 and the enumeration fold at :2085.

Your headline was still correct, one layer down. "Description / code mismatch" held at
this head, and the mismatch resolved in favour of the description. The description says
the omission branch survives only as a fail-closed backstop for a root that fails to
canonicalise — but the code's own warning still blamed "roots that share every path
segment", the segment-era phrasing. Under a digest that cause is unreachable: two distinct
roots always canonicalise to distinct paths and so to distinct digests, leaving a failed
resolve()/stat() as the only trigger. So the log sent an operator to inspect paths that
were never the problem. Fixed at :2093 to name the real cause, with a test that pins the
new wording and rejects the old one; I checked it fails if the old text comes back.

The invariant text itself I left alone: it already claims one direction only and states that
the converse is deliberately not claimed, because resolution is wider than enumeration
through the nested tier.

[rnoack1]

Verified at head f0ba3d200332dea97b871c332dec7374fe8716af. Your required change is implemented, and the coordinates below are what those lines read at that head. Your review was written against 1118db3e5, before the qualifier changed from a path segment to a per-root digest.

One shared collision-set helper, called by both sides. _package_collision is defined once, at src/kiro_crew/dashboard/handlers/_shared.py:1953, and has exactly two call sites: the resolver at :745 and the enumeration fold at :2085. Both build the set through that single helper rather than each spelling its own, so they cannot drift apart.

Dedupe by resolved file identity, inside the helper. :1988 reads copies = _dedupe_entries(entries). Because it sits in the shared helper, both callers receive the same deduplicated set instead of each applying its own rule — a symlink alias reaching another root's copy collapses to one entry for the resolver exactly as it does for enumeration.

The derivation no longer keys on a path segment. :1992 reads qualifiers = [_root_identity_token(root) for root in roots] — a digest of each root's own canonical path. Your reproduction turns on an extra holder that shares the segment enumeration picked, shifting a genuine holder's derived qualifier deeper or to None. A per-root digest does not change when the holder set widens, so that shift cannot occur. I also grepped that file for a segment-style derivation and found none, with the same pattern matching a synthetic sample, so the zero is a measured absence rather than a pattern that failed to match.

Both layouts you reproduced on are pinned as fixtures. In test_every_enumerated_package_key_resolves (test/test_resolve_skill_root_package.py), the nested-leaf tier at :2496 and the symlink-alias root at :2501, both carrying the colliding segment. The test passes against them, so the invariant is asserted by resolving each enumerated key rather than by its prefix.

Two items on this change are still open pending a maintainer decision, so this is not a claim that the whole change is clear: the scope of the hardlink read-refusal — whether it should extend to the by-name read path the non-package skill territories use — and the unresolved-key filtering in the skills editor.

[rnoack1]

Re-verified at head 903ffb8eb028e0c6252b979a12f82e8f4969d8f5. Taking your items separately, with the line I read for each.

1. The enumerator and the resolver no longer derive from two collision sets. There is one computation: _package_collision at src/kiro_crew/dashboard/handlers/_shared.py:1957, with exactly two call sites — the resolver at :743 and the enumerator at :2089. Both the alias dedupe (:1992) and the qualifier derivation (:1996) live inside it, so there is no second set to keep in step by hand. The holders set you named is gone: zero occurrences in that file, against five for _package_collision as a control that the search works.

The other half of the asymmetry is closed too. The resolver's derivation input at :744 globs the exact tier only (f"{name}/SKILL.md"); the nested */{name}/SKILL.md tier is applied afterwards at :763-764, once the qualified branch has already returned, so it cannot widen the set the qualifier is derived against.

2. You were right that enumeration could still offer a key the resolver refuses — and the mechanism you identified was not the only one. A second, independent instance was live until this head: _row_dir_key decided file-versus-directory from the name suffix, so a bundle directory legitimately named notes.md was keyed by its parent, the collision fold grouped it under the wrong bundle, and its colliding copies stayed unqualified. It now discriminates on which field carries the location (_shared.py:1283-1299): dir already names the bundle and resolves as-is, path names the SKILL.md and steps up to its parent, neither returns None. Both call sites (:1192, :1210) pass the row so the helper can tell. test_a_bundle_directory_named_like_a_file_keys_by_itself (test/test_resolve_skill_root_package.py:2467) covers it, and reinstating the suffix test makes that one fail.

3. The invariant is asserted by resolving, not by prefix. test_every_enumerated_package_key_resolves (test/test_resolve_skill_root_package.py:2486) hands every enumerated key back to the resolver and compares it against the file it was enumerated for (:2556), across the colliding layouts — including a root that shares every path segment with a sibling, where no segment could distinguish it; :2547 asserts that property of the fixture so it cannot quietly stop being the case under test. The opposite direction is asserted separately at :2559, because a resolver whose set is wider than the enumerator's is invisible to the forward test.

4. On the description mismatch your title named. One claim was still wrong at the head you reviewed and for some time after: the body said the mapped chip fell back "to the first 8 hex", while the code refuses any digest fallback at website/src/components/AgentSkillsEditor.tsx:326-327 and again at :448-449. The prose was the defect there, not the code, and it is corrected in the description now.

Two things on this change are still open rather than settled, so this is not a claim that the whole diff is clear: the scope of the hardlink read refusal on package/ keys — whether it belongs in this change at all, given it regresses reads on hardlink-deduplicating installs that never collide — and the stat terms in the qualifier digest, which make a key re-spell when the root's metadata changes. Both are trade-offs for a maintainer to weigh rather than defects I can settle.

[rnoack1]

Thanks for the two reproductions — I ran both as tests against the head rather than reasoning about them, and they are now permanent regressions. New head ba2e5ea159.

Fixed in code. Your third point was right, and it was the part with a real gap: both root shapes existed only inside a six-root fixture, and I measured that fixture blind to the drift. test/test_resolve_skill_root_package.py:2559 and :2604 now build your three-root layouts exactly as reported — packages/PkgA/eventId-1|3/skills with the tier-2 leaf, and the alias variant — and assert them in the direction that can detect it. Controls: unioning the */{name}/SKILL.md tier into the qualified derivation set fails the nested-layout test; dropping the resolver's dedupe fails the alias-layout test. The pre-existing six-root forward test passes under both mutations, which is why the new ones assert the withheld-key direction instead of only that minted keys resolve.

On the mechanism. At this head the qualifier is not a path segment. src/kiro_crew/dashboard/handlers/_shared.py:1994 derives it as _root_identity_token(root), a digest of each root's own canonical path, so a sibling carrying the same segment cannot shift a holder's qualifier deeper or to None — the step both reproductions turn on. Widening the resolver's set therefore cannot re-spell a key the enumerator minted; what it changes is which keys are accepted, which is the direction the new tests assert.

Two supporting coordinates. The qualified branch's derivation input at src/kiro_crew/dashboard/handlers/_shared.py:744 globs {name}/SKILL.md only — the */{name} tier is built at :764, reached only when there is no qualifier — and _dedupe_entries runs inside the shared helper at :1990, so both callers collapse aliases identically rather than one of them keeping a distinct holder.

That is also the shared-helper option you preferred, and it is what the code does: one definition at src/kiro_crew/dashboard/handlers/_shared.py:1955 with two call sites, the resolver at :743 and the enumeration fold at :2087 (2 references, 1 definition), so the two sets cannot be spelled differently.

Where the drift is pinned. test/test_resolve_skill_root_package.py:2642 detects both of the changes you name — tier union and dropped dedupe — and now each of your layouts detects its own. If your run derived path segments rather than digests, it was against the earlier revision of this branch; if you have a layout that still returns None at ba2e5ea159, name it and I will add it the same way.

[rnoack1]

Verified at head a6ff6cfd626089536be90887bd8a71522ce190e1 — this is fixed, and your analysis of why the existing test missed it was correct.

The derivation no longer depends on a path segment. A qualifier is now a digest of each root's canonical path (_root_identity_token, src/kiro_crew/dashboard/handlers/_shared.py:1821), not a distinguishing segment. That removes the variable both of your reproductions turn on: an extra holder that happens to carry the same segment as a genuine holder can no longer shift that holder's qualifier deeper or to None, because no segment is consulted.

The enumerator and the resolver derive the collision set from one code path. _package_collision has exactly one definition (_shared.py:1991) and both sides call it, so there is no second construction to disagree with.

Both layouts you reported are now fixtures. Your nested-leaf case — root3 at packages/PkgA/eventId-3/skills holding PkgA/shared-skill/SKILL.md — is test_the_reviewed_nested_leaf_layout_keeps_every_key_resolvable (test/test_resolve_skill_root_package.py:3215), and your symlink-alias case is test_the_reviewed_symlink_alias_layout_keeps_every_key_resolvable (:3254), with narrower siblings at :2512 and :2557.

You were right that the old test could not catch it. test_every_enumerated_package_key_resolves (:2439) contains neither a symlink-alias nor a nested-leaf root, so it passed while the guarantee was false. I checked that directly rather than assuming: with the nested tier unioned into the qualified set, and again with the alias dedupe dropped, that test still passes — it is blind to both. The new fixtures each fail under their own mutation, so they detect the mechanisms rather than asserting the property by construction.

The tests also assert the withheld direction — that the resolver accepts no qualified key the catalogue never minted — because the forward direction alone passed under both mutations.