Skip to content

feat: load a project's own skills, behind per-directory consent - #4736

Merged
bolichen97 merged 1 commit into
mainfrom
feat/project-scoped-skills
Aug 24, 2026
Merged

feat: load a project's own skills, behind per-directory consent#4736
bolichen97 merged 1 commit into
mainfrom
feat/project-scoped-skills

Conversation

@kyleseaman

@kyleseaman kyleseaman commented Aug 20, 2026

Copy link
Copy Markdown
Collaborator

Problem

A project can ship skills in <project>/.kiro/skills, but Kiro Crew did not load them.
The dashboard could also list skills without identifying which chat was asking, so multiple
open projects could produce an empty or incorrect catalog.

This PR fixes both halves together: the loader discovers the requesting chat's project skills,
and the dashboard sends the session key used to resolve that same project. Fixing only the
catalog would offer $tokens the loader could not resolve.

Refs #3551. That issue also covers project agents, MCP, and steering, so it stays open.

Consent and safety

A SKILL.md enters the agent's context and can instruct it to use tools, so project skills are
gated on explicit per-directory consent.

  • Trust is keyed by the canonical real path. A symlink cannot create a second identity for the
    same directory or move a trusted skills root outside the consented project.
  • Grants live under the Kiro Crew data home in trust/project-skills.json, protected by the
    security keystone and owner-only permissions on POSIX and Windows.
  • Unknown, unreadable, malformed, or newer-version trust data fails closed. Mutations refuse
    an unreadable or full store rather than overwriting or evicting existing grants.
  • skills.project_skills_enabled remains an independent operator off switch.
  • Project files are opened without following links and checked against the confined root using
    the descriptor actually read, preventing time-of-check/time-of-use swaps.
  • Grant and revoke actions are audited. Admission outcomes are recorded once per project and
    outcome per process so the audit remains useful without adding a write to every message.

The grant endpoint derives its directory from the requesting chat slot, never from a
client-supplied path. Revocation accepts a stored path so a grant remains removable after its
directory disappears.

Behavior

The loader accepts the project at its entry points rather than its constructor. Callers without
a project behave exactly as before, and the cache is keyed per project so chats cannot share
project-skill results.

An untrusted project skill is visible with a Needs trust marker. Choosing it opens the
consent dialog instead of inserting an unloadable token. Once granted, the skill behaves like
an installed skill; revoking the grant removes it immediately. Escaped skill paths and project
skills shadowed by a global skill are omitted because no grant could make those rows loadable.

Trust verbs are exposed at GET/POST/DELETE /api/skills/-/trust. New non-2xx responses include
machine-readable codes, including skill_trust_store_full and
skill_trust_store_unreadable.

Screenshots

Untrusted — listed with its marker

picker showing a project skill needing trust

Choosing it asks for consent

the project skills trust dialog

After the grant — selectable normally

picker showing the trusted project skill

The capture script uses the built SPA with fixture-backed API responses so the untrusted and
trusted states are reproducible for one exact directory.

Verification

  • End-to-end loader tests cover invisible/unresolvable before consent, visible/resolvable after
    consent, immediate removal after revoke, and isolation between project sessions.
  • Regression tests cover symlink and junction escapes, descriptor-pinned reads, oversized and
    CRLF skill files, unreadable and full stores, strict slot resolution, audit behavior, and the
    dashboard trust/cache flows.
  • Current-base focused verification: 310 backend tests passed; 53 frontend tests passed;
    TypeScript passed.
  • Full frontend verification passed earlier in this repair cycle: 22,968 Vitest tests and
    1,236 Electron tests, plus the production build.
  • Formatting, changed-file lint/type checks, docs lint, brand, scrub, and harness-parity gates
    pass locally for the affected surface.

The module specification is updated in the same commit. It distinguishes project-local
.kiro/skills discovery from $KIROCREW_PROJECT_DIR/skills, which is a synchronization source
copied into the installed skills root.

search_skills remains project-blind because its MCP boundary does not yet have the strict
session-to-project seam required to do this safely.

@kyleseaman
kyleseaman requested a review from a team August 20, 2026 18:13
@kyleseaman
kyleseaman requested a review from a team as a code owner August 20, 2026 18:13
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Aug 20, 2026
Comment thread src/kiro_crew/skills.py Fixed
Comment thread src/kiro_crew/skills.py Fixed
@github-actions

github-actions Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5) — 🟡 CONCERNS

UX-level review of 1053f7c695b3a9d86a72c2fa4143f0253a94bf5a — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

UX-Verdict: CONCERNS

The consent flow reads well, but its failure notice shows the wrong sentence and its undo promise points at the wrong page.

Watch

  • Failed trust shows the decline message. In ProjectSkillsTrustDialog.tsx, when the grant lands but snapshot?.trusted !== true, setError(...decline_consequence) renders "If you decline, nothing is loaded and your message is left as you typed it." — the user clicked Trust this folder, not decline, and the same sentence already sits in the body two lines above, so the alert reads as a duplicate non-sequitur that names neither what happened (project skills are switched off) nor what to do. Low frequency (only with the hard off switch set) × high confusion × every retry. Fix: a dedicated string, e.g. "Trust was saved, but project skills are turned off in Settings."
  • "You can withdraw this later from the Skills settings page" points at the wrong surface. The withdraw list (ProjectSkillsTrustList) renders in Agent Capabilities → Skills (SkillsTab.tsx); Settings → Skills (SkillsPanel.tsx) has no trust list. A user honoring the promise opens Settings → Skills, finds nothing, and concludes the grant is permanent. Moderate frequency × trust-eroding × every lookup. Fix the hint to name "Agent Capabilities → Skills" (or move the list).

Suggestions

  • decline button "Not now" could restate the outcome ("Don't load") so the safe exit names its action, matching the confirm's verb+object pattern.

[UX-REVIEWED] 1053f7c

@github-actions

github-actions Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 1053f7c695b3a9d86a72c2fa4143f0253a94bf5a and found no blocking issues.

This comment is updated in place on each push.

Review details

FINDING -- website/src/components/commandPalette/settingsRegistry.gen.ts:18 -- "channels.app-client-id-teams" and the other new Teams entries change command-palette behavior outside the stated project-skills scope -> Fix: remove these unrelated entries from this PR.
[GPT-REVIEWED] 1053f7c

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 1053f7c695b3a9d86a72c2fa4143f0253a94bf5a: <one-sentence reason>

@github-actions

github-actions Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

Design-level review of 1053f7c695b3a9d86a72c2fa4143f0253a94bf5a — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

A real prompt-injection boundary (cloned-repo SKILL.md entering agent context) closed with fail-closed, keystone-backed consent; the shape matches the repo's existing trust patterns.

Suggestions

  • The trust snapshot (GET /api/skills/-/trust) has no field distinguishing "platform unsupported" (Windows fail-closed in project_skill_traversal_supported) from "no project / not trusted" — a supported: false flag would let the dashboard explain the feature's absence instead of silently showing nothing.

[DESIGN-REVIEWED] 1053f7c

@github-actions

github-actions Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 1053f7c695b3a9d86a72c2fa4143f0253a94bf5a — 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.

Reading the remaining test/UI hunks confirmed the picture; I have enough to issue the review.

First-Principles-Verdict: CONCERNS

The feature earns its place, but a Teams settings-registry regen, a11y relabels, and a one-flag config hardening with counted unfixed siblings ride along undeclared.

What this change ships

Intent: let a chat load skills shipped inside its own project checkout, gated on explicit per-directory consent — an ADDITION (plus a catalog-scoping FIX).

  1. A project's .kiro/skills load into its own chat after consent — justified (Support project-level .kiro/ configuration (agents, MCP, steering) in Crew dashboard #3551)
  2. Untrusted project skills listed with "Needs trust"; choosing one opens a consent dialog — justified (external-content boundary)
  3. Trust grant/revoke API + withdraw list in settings — justified
  4. New config key skills.project_skills_enabled, default on — justified operator off-switch
  5. Skills catalog now resolves the requesting chat's project, not a shared fallback — justified (Dashboard skill resolution ignores the requesting chat slot, so kiro-workspace/ skills fail closed with two projects open #2457)
  6. Project skills fail closed on Windows — justified (platform limit), declared
  7. Malformed/unreadable config disables project skills only — symptom-level point patch
  8. App callers now 404 on skills list/browse without an owned, project-bound slot — undeclared
  9. $-token expansion moved off the event loop — rides along, undeclared
  10. Teams palette entries + composer/dropdown a11y relabels — undeclared riders

Watch

  • _fail_closed_project_skills_config hardens one flag against schema-default replacement and truthy-string coercion, while ≥3 security-relevant siblings keep the exact hazard the PR's own comment names (bool(...) at config/loader.py:6533, 6536, 6933, where the string "false" reads permissive). Cause is nameable; the general fix is larger — accepted-and-deferred, but say so.
  • _deny_foreign_app_skill_slot denies the whole catalog to an app whose owned slot has no project; the named harm was only "another slot's project as a read oracle". The smallest version gates project resolution, not global rows.
  • Riders 9–10 are fine changes in the wrong PR; each is invisible to a reader of this description.

Subtractions

  • Drop list_kiro_skills's project_dir param and _kiro_skill_roots's kiro-workspace branch (dashboard/handlers/_shared.py:607–621) — this PR removed its last production consumer (grep list_kiro_skills(: one call, bare, _shared.py:897; the rest are tests).
  • Drop the four *-teams entries from settingsRegistry.gen.ts; regenerate in the PR that owns Teams settings.
  • Defer the ChatPane/ChatInput aria/eslint tweaks (role="dialog", ResizeBadge span→button) to their own change.

[FIRST-PRINCIPLES-REVIEWED] 1053f7c

@github-actions

github-actions Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed 1053f7c695b3a9d86a72c2fa4143f0253a94bf5a — this comment is updated in place on each push.

Review details

I've traced both injection sites and the assembly flow. Let me confirm the key question: whether build_session_context (which injects confined bodies) and the triggered-skills block in build_message both reach the model in a single turn.

They do: build_message calls build_session_context on a new session (context.py:2582), then continues — with no is_new_session gate — into the triggered-skills block at context.py:2909 that re-injects via load_skill. The candidate's stated uncertainty (mutual exclusivity) is resolved against it.

Verifying the design contrast: get_triggered_skills skips always:true (skills.py:3948), and unconfined on-demand skills contribute only a summary line in session context (body only on trigger) — so unconfined skills never double-inject. Confined skills get their full body in session context via _append_project_skill_bodies (skills.py:4230, unconditional over all confine_root rows) and their full body again through the triggered path, with no dedup. A confined SKILL.md carrying triggers (not always:true) that word-overlaps the first message is injected twice.

One advisory duplication defect; nothing blocks the merge.

FINDING — src/kiro_crew/context.py:2930 — a confined project skill whose triggers match the first message of a session (or a post-compaction reinjection turn) has its full body injected twice — once by build_session_context_append_project_skill_bodies and again here via content = self.skills.load_skill(name, project) (the second copy passing no max_bytes, so unbounded) — because confined skills, unlike unconfined ones, are both always in session context and eligible to trigger-match, wasting the largest single context block and risking truncation of other content → Fix: in this triggered-skills loop skip names whose row has confine_root set (they are already delivered by _append_project_skill_bodies), or conversely inject confined bodies only via the trigger path — pick one delivery site.

[OPUS-REVIEWED] 1053f7c

Verdict parsed from the review's SHA-scoped output markers for commit 1053f7c695b3a9d86a72c2fa4143f0253a94bf5a.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 1053f7c695b3a9d86a72c2fa4143f0253a94bf5a: <one-sentence reason>

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Aug 20, 2026
@kyleseaman
kyleseaman force-pushed the feat/project-scoped-skills branch from 96963e3 to 9abe954 Compare August 20, 2026 21:12
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 20, 2026
Comment thread src/kiro_crew/skills.py Fixed
@kyleseaman
kyleseaman force-pushed the feat/project-scoped-skills branch from 9abe954 to ea7e68d Compare August 20, 2026 21:35
@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Round 1 dispositions — all findings fixed, nothing deferred. Head ea7e68db4.
Full reasoning is in the PR body under "Review round 1"; the short form:

  • Symlinked project skills root escapes consent (GPT, blocking) — correct, and it ran
    deeper than filed. The root is now realpath-resolved and refused when it points out of
    the consented directory, at both the enumeration and the body-load site. Two escapes the
    finding did not name are closed too: a symlinked SKILL.md inside a properly contained
    root (the directory check cannot see it, and its frontmatter reaches the catalog and the
    injected skills index, which tells the agent to read that path), and the project walk
    inheriting the skill-provider roots that exist for an app's own symlink.
    validate_file_path() does not cover this — it is a sensitivity denylist, not containment.

  • Discovery blocks the gateway event loop (GPT, blocking) — accepted; the whole $skill
    expansion now runs via asyncio.to_thread at its one async caller. One correction to the
    framing: the branch did not introduce this. main already calls _expand_dollar_skills
    directly inside async def _run_chat and already walked the skills dir plus every extra
    path synchronously; the branch's change at that site is a single argument, so it added one
    more root to a pre-existing on-loop walk. That is why the fix moves the call off-loop
    rather than narrowing what project skills may be discovered. The context-assembly paths
    are not on-loop — build_message is offloaded via run_in_embed_pool at all ~14 call
    sites, and /api/skills already ran in discovery_executor().

  • Windows junctions bypass the trust-store redirect guard (GPT, blocking) — correct;
    Path.is_symlink() is False for a junction. Now uses the repo's own
    platform_compat.is_link_or_junction / unlink_link_or_junction.

  • Catalog and loader disagree on the project (Opus) — true, and the consequential half
    was not in the finding: the grant endpoint shared the mismatch, so consent issued from
    a chat with no project bound was recorded against another chat's project, with the
    explanatory refusal suppressed. Catalog, trust read and grant now resolve through a strict
    requesting_slot_project(); revoke keeps the permissive helper because revoking only
    narrows what loads. The loader stays the strict side deliberately — giving it the fallback
    would inject one project's skills into a chat not bound to it, which is fail-open.

Four mutations were run against the new tests and all four were killed. Two initially
survived and the tests were strengthened rather than the mutations weakened: a
symlinked-subdirectory case turned out to be refused by a pre-existing check (so it pinned
nothing new, and was replaced by a provider-root case that does), and the call-site root
check is now defence-in-depth with the walker's own confinement, so it is pinned on the
property it uniquely provides — the logged refusal reason, without which an operator who
granted trust would see an empty list and no explanation.

Also green: Windows shard 4 (test_store_is_owner_only asserted POSIX mode bits unguarded
and read 0o666; guarded to match the sibling suite) and De-Amazon Scrub Lint (a real home
path in a fixture). Coverage Gate was a cascade — backend-test=cancelled -- failing closed, downstream of a cancelled shard.

@kyleseaman
kyleseaman force-pushed the feat/project-scoped-skills branch 2 times, most recently from 9e0ca77 to 304c77d Compare August 20, 2026 21:49
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Aug 20, 2026
@kyleseaman
kyleseaman force-pushed the feat/project-scoped-skills branch from 304c77d to f4be1f0 Compare August 20, 2026 21:59
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Aug 20, 2026
@kyleseaman
kyleseaman force-pushed the feat/project-scoped-skills branch from f4be1f0 to 06e6042 Compare August 20, 2026 22:13
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 20, 2026
@kyleseaman
kyleseaman force-pushed the feat/project-scoped-skills branch from 06e6042 to 4b3cd05 Compare August 20, 2026 22:17
@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Round 7 review disposition (0eccd8983):

  • Fixed the blocking Windows network-probe path: grant treats expected_path as an exact opaque echo and rejects mismatches before filesystem resolution. Revoke matches stored keys first, so vanished network grants remain removable, and rejects unmatched UNC/device paths before canonicalization. Added regressions that fail if either client/stored network text reaches canonical_key.
  • Fixed malformed project-skill metadata crashing chat assembly: confined read-only metadata now uses replacement UTF-8 decoding, while unconfined/writable skill reads remain strict to prevent silent metadata loss. Added a trusted-project invalid-UTF-8 listing regression and retained the strict writer regression.

Verification on the rebased tree: 344 affected backend tests passed; 53 focused frontend tests passed; TypeScript, mypy/flake8, black scope, docs lint, and all 18 i18n checks passed.

@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Round 8 review disposition (81bde9946):

  • Added the shared dashboard-owner authorization boundary to GET, POST, and DELETE /api/skills/-/trust. Non-owners and app tokens now receive coded HTTP 403 (dashboard_owner_required) before any consent state or project path is read or mutated.
  • Added one regression covering all three handlers and retained the existing owner-path 400/409 tests.

Verification after rebasing onto current main: 345 affected backend tests passed; 53 focused frontend tests passed; TypeScript, mypy/flake8, docs lint, and all 18 i18n checks passed.

@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Round 9 review disposition (281861c6e):

  • Removed the second open from confined project/provider skill reads. A hardened-reader refusal—including replacement or removal after enumeration—now degrades directly to no metadata/body and cannot abort a chat turn. Unconfined writer reads still propagate genuine read/decode failures.
  • Moved KiroCrewConfig to a module-level import and verified the config/skill modules import without a cycle.
  • Added a regression that forces the hardened reader to refuse a vanished path and proves no reopen occurs.

Verification: 346 affected backend tests passed; mypy/flake8, formatting, docs lint, and diff checks passed.

@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Round 10 review disposition (0f73db437):

  • Replaced confined project os.walk traversal with a no-follow scandir walk that rejects symlinks/junctions before classifying entries. Confined directory links and linked SKILL.md files are pruned even for in-project targets; global provider trees retain link traversal. Tests prove confined walking never delegates to os.walk and never resolves a linked target.
  • Mutable trust-store reads now reject any non-object grant row. Grant and revoke both preserve a partially unknown store byte-for-byte instead of filtering rows during rewrite.
  • The consent prompt now snapshots its initiating slot, grants against that slot, and compares it to a live active-slot ref before token insertion. A deferred-grant regression switches chats mid-request and proves the new draft is untouched.

Verification: 350 affected backend tests and 58 focused frontend tests passed; TypeScript, all 18 i18n checks, mypy/flake8, ESLint (0 errors), formatting, docs lint, and diff checks passed.

@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Round 11 review disposition:

  • Fixed project catalog containment: collect_skills_blocking now calls the legacy Kiro scanner only for global skills. Project rows are constructed by SkillsLoader.catalog_project_skills() through the confined no-follow enumeration and descriptor-pinned reader, so linked project targets are not read before filtering.
  • Fixed SEL coverage for authorization denials: all three trust endpoints now emit a denied log_api_access event with a verb-specific operation before returning the coded 403; an unwritable SEL preserves the denial response.
  • Added regressions proving the unconfined scanner never receives a project path, escaped skills stay absent while genuine pre-consent rows remain visible, and read/grant/revoke non-owner denials are audited.
  • Updated the memory/skills/hooks system spec.

Verification: 350 affected backend tests passed; focused review regressions passed; flake8, mypy, black scope, docs-lint, and git diff --check passed.

@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Round 12 review disposition:

  • Fixed mutable-alias consent: the trust snapshot now returns canonical project_key; the dialog displays the readable path but must echo that key as expected_key; grant canonicalizes the current slot once, requires an exact match, and persists the same key. Missing keys fail closed.
  • Fixed linked-root probing: .kiro/skills is rejected as a link/junction before realpath or isdir, preventing a Windows UNC junction from initiating SMB authentication.
  • Fixed untrusted catalog metadata: project descriptions pass through both exfiltration-URL and credential redactors before the dashboard response.
  • Fixed malformed off-switch coercion: only a missing value or literal boolean true enables project skills; strings, numbers, arrays, and objects fail closed.
  • Added backend and frontend regressions for all four boundaries and updated the system spec.
  • Rebased onto current main (fd4bd31f3).

Verification after rebase: 358 affected backend tests passed; 61 config/schema tests passed; 59 focused frontend tests passed; TypeScript, ESLint, flake8, mypy, black scope, docs-lint, and git diff --check passed.

@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Round 13 disposition for 429e070c1:

  • Fixed app slot isolation for project-scoped skill metadata. App callers must now positively own the slot selected by X-Session-Key; foreign, unscoped, missing, and absent selectors return the same audited 404. The same rule also covers project skill tree, file, and detail reads.
  • Moved reviewed-project confirmation into grant_project_trust. The grant primitive canonicalizes once, compares the operator-reviewed key, and persists that exact resolution, closing the canonical-name replacement race.
  • Added regressions for foreign/unscoped/missing app selectors and for exactly-one canonicalization under a directory-to-symlink replacement.

Fresh local verification: 360 affected backend tests passed; flake8, mypy, Black baseline gate, docs lint, and git diff --check passed.

@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Round 14 disposition

  • Fixed the Python lint matrix failure by applying the repository-required isort ordering to the new ReviewedProjectChanged import.
  • Verified the full src/kiro_crew + test import-order gate, flake8 and mypy on the touched backend modules, and 94 focused trust/browser tests.
  • Amended the existing single commit and force-pushed with an exact lease.

Current head: 14c1a37893b4085a15b87c247f03bcc48704ad46.

@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Round 15 disposition

  • Fixed the projectless-slot fallback identified by GPT 5.6. App skill reads now require the exact X-Session-Key slot to be both owned by the caller and bound to its own project.
  • Projectless owned slots receive the same audited 404 as foreign, unscoped, missing, and absent slots, so owner-only shared-project fallback cannot lend an app another slot’s project.
  • Added an HTTP regression covering catalog, tree, file, and detail reads; it reproduced the catalog leak before the fix and now passes all four paths.
  • Updated the memory/skills/hooks contract.
  • Verified 361 affected tests, repository-wide import ordering, touched-file flake8/mypy, Black baseline, docs lint, and git diff --check.

Current head: 75ccfb1a742967d47d717fc79e690a613abf616b.

@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Round 16 disposition

  • Fixed the Python 3.10 backend-shard failure caused by optional JSON Schema validation erasing malformed skills.project_skills_enabled values and turning them into the enabled missing-field default.
  • The loader now normalizes this security off-switch before advisory schema validation: only an absent flag or the boolean true enables project skills; malformed flag values and a malformed skills section fail closed.
  • Added regression coverage for the malformed-section case and reran the existing malformed-value matrix with jsonschema installed locally to reproduce CI.

Verification: 816 focused skill/config tests passed (1 skipped), including the optional-validator path; Black baseline, repository-wide isort, touched flake8/mypy, docs-lint, and git diff --check all passed.

Amended head: a855fd2b32246067842e7d451cbafa30cbd8dba1.

@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Round 17 disposition

  • Addressed the GPT 5.6 blocker: confined project metadata no longer calls path-following stat() before the no-link reader. Trigger scans, listings, and all other frontmatter consumers now admit the bytes first through the descriptor-pinned confined reader.
  • Confined metadata caching uses a digest of those admitted bytes, and size_bytes comes from the same byte payload; unconfined/operator-installed skills retain the existing one-stat mtime cache.
  • Added a TOCTOU regression that warms both enumeration and metadata caches, swaps SKILL.md for an escaping link, forbids any stat of the swapped path, and verifies trigger/list metadata fail closed.
  • Rebased the sole PR commit onto the latest main (9bc2d7c60).

Verification after rebase: 817 focused skill/config tests passed (1 skipped), including JSON Schema-enabled config validation; Black baseline, repository-wide isort, touched flake8/mypy, docs-lint, and the targeted TOCTOU regression all passed.

Amended head: 9d1f670fa4a3f1a7e97dde2b3d527a29b6f18a49.

@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Round 18 disposition

Resolved the blocking project-traversal link-swap finding in eb688feba.

  • Project roots and descendants are now opened component-by-component through pinned directory descriptors with O_DIRECTORY | O_NOFOLLOW; traversal uses scandir(fd) and child opens relative to the parent descriptor.
  • Confined enumeration verifies the supplied lexical root is exactly the granted project's .kiro/skills root before opening it.
  • Platforms without the required no-follow dirfd primitives fail project skills closed before canonicalizing a project path; global and installed skills remain available. The Windows support table and project-skills spec document this boundary.
  • Added deterministic regressions for path-based scans, mismatched confined roots, a directory swapped to a symlink between scan and descent, and unsupported-platform behavior with path resolution forbidden.
  • Regenerated config-baseline.json for skills.project_skills_enabled.

Verification: 768 passed, 1 skipped across the changed backend/config suites; simulated unsupported-platform run: 102 passed, 137 skipped; black, isort, flake8, touched-source mypy, docs-lint, brand, harness-parity, and diff checks pass.

@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Round 19 disposition (GPT 5.6 security-controls blocker): addressed in 95d81aabe.

  • Successful dashboard-owner authorization for project-skill trust endpoints now emits an allowed SEL API-access event.
  • Successful app slot ownership/project-binding authorization for project-skill browsing now emits an allowed app_isolation event naming the selected slot.
  • Added regression coverage for both allow decisions and updated the memory/skills and SEL specs.

Verification: 780 affected backend/config tests passed (1 platform skip); black gate, isort, flake8, touched-file mypy, docs-lint, brand, harness-parity, and git diff --check all passed.

@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Round 20 disposition: addressed both GPT 5.6 findings in 555b12bad.

  • A present config.json or config.local.json that cannot be read, parsed, or interpreted as an object now forces skills.project_skills_enabled off before the defaults return or schema normalization. This covers a truncated base config and a truncated overlay alongside a valid base config.
  • The skill picker and its focus prefetch now key the React Query cache by both slot key and current project, so changing projects within one slot cannot serve the previous catalog for the five-minute TTL.
  • Added regressions for both config sources and same-slot project switching; updated the project-skills spec.
  • Rebased the sole PR commit onto current main (380f87945).

Verification: 782 affected backend/config tests passed (1 platform skip); 29 directly affected frontend tests passed; production frontend build passed; Python/TypeScript typing, lint, black/isort, docs-lint, brand, harness-parity, and working-tree diff checks passed.

@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Round 21 disposition (d68f85ba2):

  • Resolved GPT 5.6 blocking finding: trust-store atomic write and owner-lockdown OSErrors are now normalized to TrustStoreUnreadable, so grant and revoke return the existing coded HTTP 409 instead of leaking an unstructured 500.
  • Added primitive regressions for grant/revoke disk failures and endpoint regressions for skill_trust_store_unreadable.
  • Updated the memory/skills/hooks system spec.

Verification: 144 trust tests passed; focused endpoint/write regressions passed; Black, isort, flake8, mypy, docs-lint, and git diff --check passed.

@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Round 22 disposition (7a4d85ee9):

  • Resolved GPT 5.6 cache blocker: the trust-store signature now includes metadata-change time and mode, so permission/ACL changes invalidate cached grants and fail closed on reread.
  • Resolved traversal blocker: descriptor-confined project-skill discovery stops at 64 directories below .kiro/skills, retaining files at the cap without risking Python recursion failure.
  • Resolved picker advisory: both production composers now provide project cache identity; callers without it use zero staleness and refetch on reopen. Focus prefetch and picker use the same policy.
  • Updated the memory/skills/hooks system spec and added adversarial regressions for all three cases.

Verification: 324 affected backend tests and 74 focused frontend tests passed; production frontend build passed; Black, isort, flake8, mypy, ESLint (no errors), docs, brand, harness-parity, and working-tree diff checks passed. Rebased cleanly onto 6762fedad.

@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Round 23 review disposition

Resolved the blocking stale trust-completion race in ChatInput. Each consent request now snapshots its chat slot, project, and monotonic request identity. Completion is accepted only while all three still match; an older completion cannot insert into a switched project or close/overwrite a newer consent prompt. The inserted token also comes from the validated request snapshot.

Added deterministic regressions for (1) switching projects in the same chat while a grant is pending and (2) closing a pending prompt, starting a newer request in the same slot/project, and then resolving the older request.

Verification after rebasing onto current main:

  • focused frontend tests: 5 files, 72 tests passed
  • production frontend build passed
  • touched-file ESLint: 0 errors (4 pre-existing ChatInput warnings)
  • docs lint passed
  • clean working tree; one PR commit over main

Pushed as 1cb9b38.

@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Round 24 review disposition

Resolved the GPT 5.6 confined-pointer blocker. A project skill is now always placed in the full-body partition, even when its frontmatter requests pointer-only delivery, so its content is read only through the descriptor-confined reader. The pointer renderer independently refuses confined project paths as defense in depth, and both catalog surfaces report the effective forced-body behavior.

Added a regression covering split behavior, direct pointer rendering, and catalog metadata for an opted-out project skill. Updated the system spec and shipped skills documentation.

Verification after rebasing onto current main:

  • 347 focused skill/trust/trigger tests passed
  • Black formatting, flake8, and mypy passed
  • docs lint and diff checks passed
  • clean working tree; one PR commit over main

Pushed as 3908e94.

@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Round 25 review disposition

Resolved the GPT 5.6 initial-index confinement blocker. Both legacy and budgeted skills context now inject every admitted project skill body through the confined loader; only unconfined skills can appear as filesystem path summaries. This closes the remaining path-following route from the initial Available Skills block while preserving the existing unconfined lazy-load behavior.

Added a parameterized regression proving both context modes include the confined body and omit the project SKILL.md path. Updated the confinement specification.

Verification:

  • 389 focused context/trust/trigger/listing/versioning/agent-template tests passed
  • Black formatting, flake8, and mypy passed
  • docs lint and diff checks passed
  • clean working tree; one PR commit over current main

Pushed as ce30be1.

@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Round 26 — Python 3.12 test isolation

Fixed the failing Backend Tests (3.12, 4) teardown:

  • confined the regression test's os.open / os.close / os.scandir monkeypatches to the walker invocation
  • restored the real process-wide os functions before pytest cleans temporary directories
  • kept the recursion-bound assertions unchanged

Verification:

  • focused regression: 1 passed
  • black gate: passed
  • isort check: passed
  • flake8: passed
  • git diff --check: passed

The prior failure occurred only during pytest teardown on Python 3.12; the product assertion itself passed.

@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Round 27 — main drift and frontend warning budget

Rebased the single PR commit onto current main and fixed the merged-tree frontend gate:

  • added accessible names to the agent and model filter inputs
  • reduced the merged ESLint total from 682 to the allowed 680 without raising the budget
  • retained the Python 3.12 monkeypatch-lifetime fix from Round 26

Verification:

  • npx tsc -b: passed
  • npx eslint src/ --max-warnings 680: passed
  • focused UI tests: 11 passed
  • focused Python regression: 1 passed
  • git diff --check: passed
  • branch shape: 0 behind / 1 ahead

@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Round 28 — warning-budget headroom

Rebased onto the latest main and removed all six remaining lint warnings in the chat components already touched by this PR:

  • used a native button for the focusable resize-details control
  • kept image-load measurement and conditional file-label behavior with narrowly documented lint exemptions
  • labeled both picker dialogs and documented their delegated keyboard handling
  • preserved the model-switch diagnostic log with an explicit exemption

Verification:

  • TypeScript: passed
  • exact ESLint gate: passed at 674 / 680 warnings
  • production frontend build: passed
  • focused UI tests: 11 passed
  • focused Python 3.12 teardown regression: 1 passed locally
  • git diff --check: passed
  • branch shape: 0 behind / 1 ahead

@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Round 29 — regenerate settings-search registry

Fixed Frontend Tests shard 3 after reproducing its anti-stale failure locally:

  • ran the repository-owned npm run gen:settings generator
  • added the four Teams settings that landed on main without a regenerated registry
  • kept the change confined to the generated registry file (54 generated lines)

Verification:

  • settings registry tests: 5 passed
  • TypeScript: passed
  • generated-file ESLint: passed
  • git diff --check: passed
  • branch shape: 0 behind / 1 ahead

@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Round 30 (e44514f54) addresses both blocking GPT 5.6 findings from the prior head and rebases onto current main:

  • enforcement-audit de-duplication now records its marker only after a successful SEL write, so transient audit failures retry on the next enforcement without breaking chat loading;
  • confined project catalog repo_scope metadata now receives the same credential and exfiltration-URL redaction as description before reaching the dashboard;
  • added regressions for both behaviors and updated the project-skills system specification.

Local verification after the rebase:

  • 422 affected backend tests passed;
  • Black scope gate, isort, flake8, mypy, and docs lint passed;
  • 52 focused frontend tests passed;
  • production frontend build passed;
  • ESLint: 0 errors, 674 warnings (budget 680);
  • settings registry regeneration produced no diff;
  • branch is 0 behind / 1 ahead of main.

A project can ship skills in `<project>/.kiro/skills`, but nothing loaded them.
Two independent failures stacked: the ENGINE never scanned that directory at all
(the string `.kiro/skills` appeared nowhere in skills.py), and the dashboard's
listing endpoint, which does resolve it, was called without a session key -- so
the server fell back to "the one project every open slot shares" and answered
with nothing once two chats sat on different projects.

Fixing only the listing would have been worse than leaving it: the picker would
offer a project skill whose `$token` the loader still could not resolve, so the
turn would proceed with no skill and no warning. Both layers move together.

A SKILL.md is prose, but it enters the agent's context and can instruct it to run
anything, so loading one out of whatever repository happens to be open is an
execution-adjacent decision. It is therefore gated on explicit per-directory
consent. Trust is keyed on the canonical realpath, because the directory IS the
resource: a symlink cannot alias its way into another directory's grant, and a
rename cannot orphan one. The store lives at `<data home>/trust/`, already a
whole-directory keystone entry, so the agent's own file tools can neither read
nor forge it. Every unknown resolves toward "untrusted": an unreadable store, a
malformed store, a schema version newer than this build, a relative path, a
missing directory, and a path naming a file all yield no grant.

`skills.project_skills_enabled` is the operator's hard off switch, independent of
any recorded grant.

The loader takes the project at its public entry points rather than in its
constructor, so the twelve existing construction sites are untouched and a caller
that wants project skills opts in by passing one. Trust verbs live at
`/api/skills/-/trust`; the grant derives its directory from the requesting slot,
never from a client-supplied path, so no caller can consent on behalf of a
directory the operator never opened.

An untrusted project skill is listed and marked rather than hidden, because a
silently absent skill is indistinguishable from one that does not exist --
choosing it opens the consent dialog instead of inserting a dead token.

`search_skills` (the MCP tool) stays project-blind: only a session key reaches
that boundary, and resolving a project there needs a seam that does not exist
yet. Named here rather than left to be discovered.

Refs #3551
@kyleseaman

Copy link
Copy Markdown
Collaborator Author

Round 31 (1053f7c69) addresses the GPT 5.6 project-body memory finding and rebases onto current main:

  • session-start and post-compaction context assembly now pass the skills section cap as an independent confined-body budget, including when lazy loading is off;
  • project bodies are admitted whole only while they fit; unsafe checkout paths are never substituted for omitted bodies;
  • the loader checks the enumerated size before opening and gives the descriptor-pinned read the exact remaining byte budget, closing the replacement race without materializing an oversized file;
  • added a regression proving the final section is bounded, every confined read is capped, and later files are not opened once they cannot fit;
  • updated the project-skills specification.

Post-rebase local verification:

  • 494 affected backend/context tests passed;
  • Black scope gate, isort, flake8, mypy, and docs lint passed;
  • production frontend build passed;
  • settings registry regeneration produced no diff;
  • branch is 0 behind / 1 ahead of main.

@bolichen97

Copy link
Copy Markdown
Collaborator

Open PR relationship audit

This is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion.

Relationship findings

  • PR #7181 is OVERLAPPING relative to this PR. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7181: KEEP. The merged per-directory consent path covers only capabilities that live in the working directory of the session that is using them. 7181 covers capabilities that travel with a portable bundle, which the merged code cannot express. Files: src/kiro_crew/skills.py.

No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants