feat: load a project's own skills, behind per-directory consent - #4736
Conversation
UX Review (Fable 5) — 🟡 CONCERNSUX-level review of 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
Suggestions
[UX-REVIEWED] 1053f7c |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsFINDING -- website/src/components/commandPalette/settingsRegistry.gen.ts:18 -- False positive or not applicable? A repository writer can comment: |
Design Review (Fable 5) — ✅ PASSDesign-level review of 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
[DESIGN-REVIEWED] 1053f7c |
First Principles Review (Fable 5) — 🟡 CONCERNSPremise-level review of 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 shipsIntent: let a chat load skills shipped inside its own project checkout, gated on explicit per-directory consent — an ADDITION (plus a catalog-scoping FIX).
Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] 1053f7c |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsI've traced both injection sites and the assembly flow. Let me confirm the key question: whether They do: Verifying the design contrast: One advisory duplication defect; nothing blocks the merge. FINDING — src/kiro_crew/context.py:2930 — a confined project skill whose [OPUS-REVIEWED] 1053f7c Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
96963e3 to
9abe954
Compare
9abe954 to
ea7e68d
Compare
|
Round 1 dispositions — all findings fixed, nothing deferred. Head
Four mutations were run against the new tests and all four were killed. Two initially Also green: Windows shard 4 ( |
9e0ca77 to
304c77d
Compare
304c77d to
f4be1f0
Compare
f4be1f0 to
06e6042
Compare
06e6042 to
4b3cd05
Compare
|
Round 7 review disposition (
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. |
|
Round 8 review disposition (
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. |
|
Round 9 review disposition (
Verification: 346 affected backend tests passed; mypy/flake8, formatting, docs lint, and diff checks passed. |
|
Round 10 review disposition (
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. |
|
Round 11 review disposition:
Verification: 350 affected backend tests passed; focused review regressions passed; flake8, mypy, black scope, docs-lint, and |
|
Round 12 review disposition:
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 |
|
Round 13 disposition for
Fresh local verification: 360 affected backend tests passed; flake8, mypy, Black baseline gate, docs lint, and |
|
Round 14 disposition
Current head: |
|
Round 15 disposition
Current head: |
|
Round 16 disposition
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 Amended head: |
|
Round 17 disposition
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: |
|
Round 18 disposition Resolved the blocking project-traversal link-swap finding in
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. |
|
Round 19 disposition (GPT 5.6 security-controls blocker): addressed in
Verification: 780 affected backend/config tests passed (1 platform skip); black gate, isort, flake8, touched-file mypy, docs-lint, brand, harness-parity, and |
|
Round 20 disposition: addressed both GPT 5.6 findings in
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. |
|
Round 21 disposition (
Verification: 144 trust tests passed; focused endpoint/write regressions passed; Black, isort, flake8, mypy, docs-lint, and |
|
Round 22 disposition (
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 |
|
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:
Pushed as 1cb9b38. |
|
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:
Pushed as 3908e94. |
|
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:
Pushed as ce30be1. |
Round 26 — Python 3.12 test isolationFixed the failing
Verification:
The prior failure occurred only during pytest teardown on Python 3.12; the product assertion itself passed. |
Round 27 — main drift and frontend warning budgetRebased the single PR commit onto current
Verification:
|
Round 28 — warning-budget headroomRebased onto the latest
Verification:
|
Round 29 — regenerate settings-search registryFixed Frontend Tests shard 3 after reproducing its anti-stale failure locally:
Verification:
|
|
Round 30 (
Local verification after the rebase:
|
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
|
Round 31 (
Post-rebase local verification:
|
Open PR relationship auditThis 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
No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit. |
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
$tokensthe loader could not resolve.Refs #3551. That issue also covers project agents, MCP, and steering, so it stays open.
Consent and safety
A
SKILL.mdenters the agent's context and can instruct it to use tools, so project skills aregated on explicit per-directory consent.
same directory or move a trusted skills root outside the consented project.
trust/project-skills.json, protected by thesecurity keystone and owner-only permissions on POSIX and Windows.
an unreadable or full store rather than overwriting or evicting existing grants.
skills.project_skills_enabledremains an independent operator off switch.the descriptor actually read, preventing time-of-check/time-of-use swaps.
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 includemachine-readable codes, including
skill_trust_store_fullandskill_trust_store_unreadable.Screenshots
Untrusted — listed with its marker
Choosing it asks for consent
After the grant — selectable normally
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
consent, immediate removal after revoke, and isolation between project sessions.
CRLF skill files, unreadable and full stores, strict slot resolution, audit behavior, and the
dashboard trust/cache flows.
TypeScript passed.
1,236 Electron tests, plus the production build.
pass locally for the affected surface.
The module specification is updated in the same commit. It distinguishes project-local
.kiro/skillsdiscovery from$KIROCREW_PROJECT_DIR/skills, which is a synchronization sourcecopied into the installed skills root.
search_skillsremains project-blind because its MCP boundary does not yet have the strictsession-to-project seam required to do this safely.