fix(skills): refuse writes to kiro-user/ and kiro-workspace/ keys

[bolichen97]

Closes #8244.

Problem

kiro-workspace/<rel> skill keys resolve against the requesting session's project directory on the READ path (via _resolve_skill_root -> <project>/.kiro/skills), and kiro-user/<rel> resolves per-machine (~/.kiro/skills). But the write handlers joined the key straight onto a core skills root. The same key therefore named a different file on write than the reader was shown, so an edit or delete could hit a file the reader never saw. This is the same one-key-two-files shape closed for package/, left open for the two open-standard skill-key territories.

Contract decision

The issue asked for a design decision among three options (session-scope the key, refuse on the write path, or document the ambiguity). This PR chooses refuse-on-write:

• kiro-user/ and kiro-workspace/ are already documented read-only in api_skills.
• Their read path is session/machine scoped while the write path joined the key onto a core root.
• Refusing the write keeps read and write naming the same file with a small, focused change. Session-scoping the write would require threading project state into skills.create/update/delete_skill (a much larger change and behavior expansion).

Changes

• src/kiro_crew/dashboard/handlers/prompts.py: new READONLY_SKILL_KEY_PREFIXES = ("kiro-user/", "kiro-workspace/").
  • api_skill_detail: PUT/DELETE on these prefixes -> 405 with Allow: GET and code readonly_skill_prefix, placed before any session-key work so skills.* is never reached.
  • api_skills_create: create into these prefixes -> 400 with code reserved_skill_prefix, checked against the sanitised name so Kiro-Workspace/Foo is also caught.
  • Reads and non-prefixed writes are untouched.
• test/test_skill_write_territory_guard.py (new): 10 async direct-handler tests.
• docs/system-specs/modules/memory-skills-hooks.md and docs/system-specs/modules/learn-cron-dashboard.md: both spec sites that document GET/PUT/DELETE /api/skills/{name} now record the refusal and its two error codes, so the spec no longer implies PUT/DELETE succeed for a kiro-workspace/ key.

Independence from PR #7105

The package/ write guard (PACKAGE_KEY_PREFIX) referenced in the issue is not on main — it arrives with open PR #7105, which explicitly names kiro-user/ and kiro-workspace/ as the untracked follow-up. This change was designed fresh against main and touches only these two prefixes with a separate READONLY_SKILL_KEY_PREFIXES tuple, introducing no PACKAGE_KEY_PREFIX reference. The two changes can land in either order without a symbol or logic collision.

Testing

• Targeted subset: python -m pytest test/test_skill_write_territory_guard.py test/test_dashboard_pinned_write_migration.py test/test_skill_browser.py -n0 -q => 114 passed.
• Test quality independently falsified: with the guard removed, the 6 refusal tests fail (200 instead of 405/400, i.e. the mutating handler reached skills.*), while the 4 scoping/GET regression tests stay green; restoring the guard returns all 10 to green.
• Lint/type: flake8 clean, black --check unchanged, isort clean, mypy no issues on prompts.py.
• Error-code ratchet: unchanged (both new responses carry a code).
• Semantic review: APPROVED, no blocking concerns.

Non-blocking notes

• The create-guard runs after the JSON/content-required checks, so a reserved-prefix create with a malformed body reports content_required first (cosmetic; the write is still refused).
• A bare kiro-workspace/ with no leaf sanitises to kiro-workspace and resolves to a harmless non-territory file (not a shadowing hazard, since the reader only treats kiro-workspace/<leaf> as a territory).

Pattern harvest

Rule candidate: AUTOSDE recurring-defect-patterns
Pattern: one key space where the READ verb resolves the name through a
root-selecting indirection (per-session, per-machine, or per-provider) while the
WRITE verbs hand the same raw name to a writer that joins it onto one fixed
root. The two verbs then name different files for a single key, so an edit or
delete lands somewhere the reader was never shown and reports success. Flag a
changed line that adds or extends a mutating handler over a key space whose read
path is root-selecting, unless the write path resolves the root the same way or
refuses the prefix outright.

Why it generalises rather than being a one-off: this key space has three sibling
territories resolved by the same root-selecting reader (kiro-user/,
kiro-workspace/, package/) and one fixed-root writer behind all of them, so
each prefix is an independent instance of the same defect — which is exactly how
this one survived the first pass. The shape is not skill-specific: any endpoint
family that resolves reads through a session-scoped root and writes through a
global one has it.

[github-actions]

Design Review (Fable 5) — ✅ PASS

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

All claims verified: the read path resolves these prefixes via session/machine-scoped roots (_shared.py:1048) while the writers join onto the core root (skills.py:2498), the guard fires before skills.* is reached, the only other writer path (discover.py) keys by provider-controlled <provider>/<slug> names, and docs were updated in the same commit.

Design-Verdict: PASS

Refuse-on-write is the proportionate contract enforcement for already-documented-read-only territories; the alternatives were weighed and the diff matches the description exactly.

Suggestions

• Hoist the territory prefixes into one shared constant in handlers/_shared.py next to _resolve_skill_root/_skill_key_roots, so a future territory added to the reader cannot ship without a write stance — the comment "the literals must match" is the drift the defect class already exploited once (package/ vs these two landing in separate PRs).

[DESIGN-REVIEWED] 6ef3788

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 6ef37882759705cb0684db856c3a7e842a753880 — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 6ef3788

Verdict parsed from the review's SHA-scoped output markers for commit 6ef37882759705cb0684db856c3a7e842a753880.

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

[github-actions]

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of 6ef37882759705cb0684db856c3a7e842a753880 — 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.

All claims verified. I have everything needed for the review: the defect is real (create_skill joins onto a fixed core root at skills.py:2502 while GET resolves via _resolve_skill_root), the territories were already documented "read-only here" in the api_skills docstring on main, no PACKAGE_KEY_PREFIX guard exists on main, and the only other user-facing writer (discover.py:545/549) builds its key from a code-registered provider name so it can't spell these prefixes.

First-Principles-Verdict: PASS

Closes a reported one-key-two-files defect (#8244) at the contract the docs already promised; every item is the fix or mandated by AGENTS.md.

What this change ships

Intent: stop the skills endpoint from editing/deleting a different file than the reader was shown for kiro-user//kiro-workspace/ keys — a FIX.

1. PUT/DELETE on a kiro-user//kiro-workspace/ key now answers 405 instead of touching a core-root file — justified
2. Create whose sanitized name lands in either territory now answers 400 — justified
3. Two new machine-readable error codes — justified (AGENTS.md mandates code on non-2xx bodies)
4. Both spec sites updated same-commit — justified (spec-management invariant)
5. New direct-handler test file — justified

Verification: api_skills's docstring on main already declares both territories "read-only here", so refuse-on-write closes the gap between documented contract and behavior — cause level, not symptom. Grepped create_skill|update_skill|delete_skill callers under src/kiro_crew/: 3 files; the only other user-facing writer (discover.py:545,549) keys writes as <registered-provider>/<slug>, so it cannot reach these prefixes — no unguarded sibling call site. READONLY_SKILL_KEY_PREFIXES has 2 real consumers (both guards in prompts.py). No existing mechanism does this job: skills._safe_name only blocks traversal.

Watch

The third territory with the identical read/write divergence, package/, stays unfixed here — declared, deferred to open PR #7105 (grep confirms no PACKAGE_KEY_PREFIX on main). If #7105 dies, this defect survives there; accepted-and-deferred, not a demand.

[FIRST-PRINCIPLES-REVIEWED] 6ef3788

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 6ef37882759705cb0684db856c3a7e842a753880 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 6ef3788

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

[bolichen97]

CI status on 6ef37882759705cb0684db856c3a7e842a753880

62 of 65 checks are green, including every blocking review lane (GPT 5.6, Opus 4.8, Design, First Principles), PR Hygiene, and both Windows and macOS test jobs. Three reds remain and none of them is this diff:

Backend Tests (3.12, 3) fails one test, test/test_security_regex_linearity.py::test_long_nonshell_line_does_not_blow_up, on its 6.0 s absolute wall-clock ceiling — 6.12 s on the first attempt and 6.41 s on a re-run. main's own CI run for 5df44de2f fails the identical assertion at 6.08 s with this branch absent, and src/kiro_crew/security.py is byte-identical between this branch and origin/main (this PR touches one dashboard handler, one new test module, and two module specs). The ceiling was calibrated before #7441 added pass 4 (_check_alt_traversal_reaches_fence) to is_sensitive_bash_command, which is the function the test measures; #8282 and #8349 are the open PRs addressing that cost. Uninstrumented on a dev box the same call takes 1.9 s, so this is a raised constant factor rather than the quadratic regime the test was written to catch. The bound was deliberately not touched.

Coverage Gate is a pure dependent of that shard, and PR Readiness aggregates the two.

Dependency Audit also failed twice with npm audit timed out after 120s, on two different lockfiles across the two attempts; it passed on the third. This PR changes no JavaScript and no lockfile.

A rebase onto a main tip that carries the pass-4 cost fix, plus a re-run of shard 3, is what clears the remaining reds.

[bolichen97]

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 #7105 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 #7105: MERGE_DISCUSSION. Complementary halves of one contract, not duplicates: neither covers the other's prefixes. They collide textually at both insertion points, so the landing order needs deciding and the second PR must rebase; folding the two prefix tuples into one territory table is worth a maintainer decision at that point. Files: src/kiro_crew/dashboard/handlers/prompts.py.
• PR #7393 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 #7393: REBASE. Complementary guards on the same handlers; both are needed and neither is redundant. The second to land needs a mechanical rebase of the shared insertion points and the shared spec paragraphs. Files: src/kiro_crew/dashboard/handlers/prompts.py, docs/system-specs/modules/learn-cron-dashboard.md.
• PR #8249 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 #8249: MERGE_DISCUSSION. Write-side key policy on the same handler, materially different goal and code. Files: src/kiro_crew/dashboard/handlers/prompts.py. The two independent directions used different labels; the matrix conservatively retains OVERLAPPING for coordination.

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

[bolichen97]

CI now fully green on 6ef37882759705cb0684db856c3a7e842a753880

Supersedes the earlier CI-status note on this PR, which reported three reds still outstanding. All checks are now green and PR Readiness is success (readiness: passed). No code change was needed and nothing was pushed — the head SHA is unchanged, so every review lane's verdict on this revision still stands.

What the reds actually were. Only one root cause, and it was not this diff:

• Backend Tests (3.12, 3) failed on test/test_security_regex_linearity.py::test_long_nonshell_line_does_not_blow_up — an absolute wall-clock assertion (elapsed < 6.0) that measured 6.41s. That file is not touched by this PR.
• Coverage Gate was purely downstream of it (backend-test=failure -- failing closed), not an independent coverage finding.

Why it is a load flake and not a complexity regression. The test guards against a quadratic anchor whose signature its own docstring records as ~27s, versus ~1.5s for the fixed path. Measured locally on this revision:

condition	elapsed
no coverage	~2.0s (3 runs)
under coverage tracing, idle box	2.5-3.2s (3 runs)
CI shard 3 observed	6.41s

6.41s is ~4x below the quadratic signature, so the regex is linear as intended. The relevant detail is that shard 3 runs pytest -n auto --cov=kiro_crew --cov=sage_lib, so coverage tracing plus full core saturation eats most of the headroom the 6.0s ceiling assumes — the budget is ~2x clear under CI conditions, not the ~4x its docstring implies. Re-running the failed jobs on the identical snapshot passed both.

The full file passes locally (57 passed), as do this PR's own 10 new tests.

One hypothesis checked and disproved, recorded so nobody re-derives it: since no .test_durations is committed, pytest-split falls back to an even split by test count, so this PR's 10 added tests do shift shard indices. But collecting --splits 4 --group 3 both with and without test/test_skill_write_territory_guard.py places the linearity test in group 3 either way — this PR did not move it into the failing shard. The under-budgeted ceiling is pre-existing and owned by main, not by this diff; it is left untouched here rather than widened from an unrelated PR.

Remaining gate is human. mergeable_state is blocked solely on the required approving review — require_last_push_approval is set and the last pusher is bolichen97, so this needs a maintainer who is not the last pusher. All four blocking review lanes (GPT 5.6, Opus 4.8, Design, First Principles) report PASS with zero findings on this revision, there are no unresolved threads, and no finding is awaiting a disposition.

[operator: bolichen97]

[chenmingwei23]

Approving: PR Readiness green (the repo's only required check), no failing lanes, MERGEABLE.