Skip to content

fix(security): deny kirocrew skills authority-retire from an agent shell - #4533

Merged
iamwhatever merged 1 commit into
kirodotdev:mainfrom
rubencu:feat/atomic-skill-candidate-claim
Oct 23, 2026
Merged

iamwhatever merged 1 commit into
kirodotdev:mainfrom
rubencu:feat/atomic-skill-candidate-claim

Conversation

@rubencu

@rubencu rubencu commented Aug 19, 2026 •

Copy link
Copy Markdown
Contributor

Problem / Motivation

Goal: kirocrew skills authority-retire, the operator-only recovery that the auto-skill publication authority adds in #18595, is denied through an agent shell by an ungated argv floor.

The auto-skill publication authority (#18595) adds one CLI recovery, kirocrew skills authority-retire, which moves an idle authority root and its provenance record aside so that by-name skill mutation resumes on a host whose authority can no longer be certified. That command must run from the operator's own terminal: if an agent shell could run it, the agent could retire the very authority that fences unreviewed promotion. The existing self-protection subcommand floors (approve, gateway restart, cloud <destructive>) are the mechanism for exactly this: an argv matcher the sandbox denies with no catalog row and no opt-out.

This PR is the sensitive-path footprint of the auto-skill promotion change, split out from the rest of it. The Sensitive Change Review refuses any fork PR that touches src/kiro_crew/sandbox*.py or src/kiro_crew/security/*, so this part has to be landed by a maintainer from a repository branch; the remainder (#18595) touches no sensitive path and merges on its own afterwards.

Why it matters

Without the floor, the retire command ships as an agent-reachable way to loosen the authority that #18595 relies on. With the floor landed first, #18595 can merge as an ordinary fork PR with the operator-only guarantee already in place.

Not a goal

  • No change to how the floors match: _is_self_skills_authority_retire reuses _matches_self_subcommand, the same structural matcher the existing subcommand floors use.
  • No catalog row and no opt-out for this floor, like the other self-protection floors.
  • No widening of any existing pass, and no existing threshold moved.
  • No auto-skill behaviour: the authority, the retire command itself, and their tests are feat(skills): atomically promote safe auto-skill updates #18595.
  • No change to which paths are sensitive: tag-grants was already on the deny floor and the sandbox mask.

What changed (motivation → approach → change)

Before After
kirocrew skills authority-retire (new in #18595) would be allowed through an agent shell like any other kirocrew subcommand The ungated self-protection-skills-authority-retire floor denies it structurally, with the reason text naming the operator-terminal requirement
  • security/argv_floor_self.py and security/argv_floor.py: _is_self_skills_authority_retire(text_lower) matches the product CLI as the argv's own program with the leading subcommands skills authority-retire, through interposed shell dressing, via _matches_self_subcommand, exactly as the sibling subcommand floors do; the argv_floor facade's manifest names it. No help form is exempt: like restart, gateway restart and cloud, the floor refuses the command's --help too, because a word that reads as --help can reach the shell as nothing (${nope:+--help}, `--help`) or as a plain argument (after --, or as an option's value), and the retire then runs.
  • security/denied_rules_tables.py: the floor id joins _SELF_PROTECTION_UNGATED_FLOOR_IDS and its reason text joins _SELF_PROTECTION_FLOOR_NOTES; the comment enumerating the ungated floors names it.
  • security/deny_gate.py, security/__init__.py and security/_exports.py: the matcher is registered in deny_gate's loop over the ungated self-protection floors, so is_denied calls it, and the facade's lazy name map, its TYPE_CHECKING manifest and the export list name it.
  • docs/system-specs/modules/security.md: the self-protection floor list names the skills authority-retire recovery, the ungated set reads six ids, including self-protection-file-delivery, each with its no-catalog-row reason, and the spec says the recovery's --help form is refused like its siblings' while kirocrew skills --help stays allowed. The tag-grants comments in sandbox_leaves.py, sandbox_precreate.py and security/paths_lists.py are unchanged: a note that the leaf also parents the auto-skill authority belongs with the change that creates that authority, and until then it would name a constant that does not exist on main.
  • test/test_security_gate_liveness.py: _PACKAGE_LINE_BUDGET raised from main's 32,188 to the measured 32,206 with its reason (the matcher, its manifest line, the rule id and reason, the deny_gate registration, the export, and the comment line).

Cherry-pick request for a maintainer: this PR is one commit, 2072a31878a2e9cc81df161835d43f3c4aa98c3e on rubencu/KiroCrew branch security/skills-authority-retire-floor (also this PR's head), on top of main at 69b2e503ebce6dc992fa833994bc1e9164c3ac18. git fetch https://github.com/rubencu/KiroCrew.git security/skills-authority-retire-floor && git cherry-pick 2072a31878a2e9cc81df161835d43f3c4aa98c3e onto a repository branch reproduces it; the Sensitive Change Review template then applies to that branch.

Backwards compatibility

Breaking: none. The floor denies a subcommand that does not exist until #18595 lands; on main today the same argv is an unknown kirocrew subcommand. No existing command, catalog row or threshold changes.

Tests

  • test/test_denied_commands_security.py: the floor's command template, its place in the self-protection subcommand floor list, the structural assertion security._is_self_skills_authority_retire("kirocrew -\\v skills authority-retire"), and test_skills_authority_retire_refuses_its_help_form_like_its_sibling_floors: the plain, -h, module, ${nope:+--help}, backtick, comment, --, option-value, piped and 2>&1 help forms are all refused, the restart floor still refuses its own help form, and kirocrew skills --help, skills list and skills root stay allowed.
  • test/test_security_gate_liveness.py: the package budget pin at the measured total.
  • Locally on 2072a31878 (one commit on 69b2e503eb; the rebase resolved a conflict in test/test_security_gate_liveness.py, where main's fix(security): keep SRI integrity digests out of the bare-secret pass #19689 raised the same budget by 63, to 32,188, for the SRI-digest exemption in redaction. Main's reason and line stay as they are, this floor's reason now reads "from 32,188", and every other per-file patch ID is unchanged from 6a2fea080a): every test_security_* module (the security-conductor corpus tests among them), test_denied_commands_security, test_deny_diff and the other deny, argv and floor modules pass (83 modules, 9,677 passed, 6 skipped), and the facade-mirror and campaign-facade guards pass on this head (318 tests). The budget test reads the package at exactly 32,206 lines: main's measured 32,188 plus this floor's 18.

Design questions

  1. The tag-grants comments in sandbox_leaves.py, sandbox_precreate.py and security/paths_lists.py do not yet say the leaf parents the auto-skill authority. Neither PR can carry that note cleanly: here it names a constant that does not exist on main, and in feat(skills): atomically promote safe auto-skill updates #18595 it touches a sensitive path. It is a comment-only refresh for whoever lands the authority, tracked in Note in the tag-grants comments that the leaf parents the auto-skill authority #23038.

Manual verification

N/A — the floor is exercised by the denied-command tests above; there is no user-visible surface.

Related Issues

Part of #18595 (the auto-skill promotion change this floor protects). Companion PRs: #11323 (coordination note posted there), #12970 (lock order), #7617 (all-invalid-script rule), as recorded on #18595.

Screenshots / video

Why no screenshot: no user-visible surface changes; this adds one denied-command floor and its spec entry.

Pattern harvest

Not generalizable: the gap is one operator-only recovery command (kirocrew skills authority-retire) missing from the ungated argv floor, a per-command allow/deny decision that no line pattern separates from the operator commands the floor deliberately leaves reachable.

Checklist

  • At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|style|refactor|perf|test|chore|ci|build|revert: ...)
  • Existing tests pass and new tests added for new functionality
  • Self-review completed; code follows project style guidelines
  • Documentation updated (if applicable): docs/system-specs/modules/security.md lists the floor among the ungated self-protection floors in this commit; feat(skills): atomically promote safe auto-skill updates #18595 describes the authority it protects
  • No secrets, credentials, or internal references in the diff

Contribution License Agreement

Contributions are accepted under the project's Apache-2.0 license; see the Licensing section of CONTRIBUTING.md.

@rubencu
rubencu requested a review from a team August 19, 2026 10:37
@github-actions github-actions Bot added fork Pull request from a fork (external contributor) readiness: action required A blocking check or review needs attention labels Aug 19, 2026
@rubencu

rubencu commented Aug 19, 2026

Copy link
Copy Markdown
Contributor Author

GPT review dispositions

Exact reviewed head: 484dd701eb08a6763e768ae901a85703bd1cf516.

Finding Disposition Resolution
Windows lock preparation could fail on an empty file Accepted / fixed Authenticate the descriptor first, then seed one inert byte before msvcrt.locking; platform regression added.
Older glibc may lack the exported renameat2 wrapper Accepted / fixed Added the native Linux syscall fallback on supported architectures while preserving atomic no-replace semantics.
Symlink/junction claims could be traversed Accepted / fixed Treat linked claim paths as opaque names for restoration/removal; never inspect metadata, markers, or descendants through them.
Valid non-object JSON metadata could be rewritten Accepted / fixed Preserve original metadata bytes on refusal/recovery; regression verifies byte-identical restoration.
Claim failure during unattended apply could suppress review notification Accepted / fixed Re-emit the staged event when the original suppressed candidate remains public.
Private-root sensitivity checked only one path spelling Accepted / fixed Require both logical and resolved private roots to be agent-denied before claiming.
Live mutations outside promotion could bypass the target lock Accepted / fixed All app-owned create/edit/refine/pin/injection/archive/restore/delete paths now share the target lock.
Dashboard lock waits could block the asyncio loop Accepted / fixed CRUD, approval, dismissal, listing, discovery, and consolidation callers offload bounded lock waits to executors/threads.
Same-target and live-mutator races lacked deterministic proof Accepted / fixed Added threading.Event-ordered serialization regressions without sleeps or retries.
.promoted handling could follow a linked claim parent Accepted / fixed Marker inspection and removal are skipped entirely for linked/junction claim parents.
Partial recursive cleanup could remove the marker, fail later, and make consumed work recoverable Accepted / fixed Completed-claim cleanup recreates and authenticates the marker before releasing the claim lock whenever residue remains.
Unattended binding hashed normalized text rather than exact stored bytes Accepted / fixed Bind from SKILL.md.read_bytes(); simulated Windows newline regression verifies CRLF-safe identity.
Pre-planted linked private descendants could redirect claims or locks Accepted / fixed Authenticate .private, claims, locks, and locks/claims as real non-linked directories contained beneath the resolved private root.
Lock files could be followed, hardlinked, or inode-swapped Accepted / fixed New files use exclusive creation/no-follow; existing files require pre-open and opened-inode identity plus regular/single-link checks.
Stale dashboard new/update routing could publish a replaced update as a new skill Accepted / fixed New-skill promotion re-reads kind from the private claim before mutation, refuses claimed updates, restores exact candidate bytes, and retains legacy missing/non-object metadata compatibility.

Validation on this exact head: complete affected suite 1015 passed, 20 skipped, 49 warnings; Black, isort, Flake8, mypy (988 source files), compilation, docs checks, and git diff --check pass. Fresh GPT and Opus exact-head reviews both report no findings.

@github-actions github-actions Bot added the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Aug 20, 2026
@rubencu
rubencu force-pushed the feat/atomic-skill-candidate-claim branch from 484dd70 to 25ac351 Compare August 20, 2026 09:44
@github-actions github-actions Bot removed the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Aug 20, 2026
@rubencu

rubencu commented Aug 20, 2026

Copy link
Copy Markdown
Contributor Author

GPT review dispositions for c970b8875168f1856a23f5748d7d0399bd12b5eb

  • Bare reserved-namespace spellings could reach generic recursive deletion — fixed in 25ac3513315f1db049d8b10fbdc590828ac4daa2.

    _live_auto_mutation_lock now partitions the requested name and refuses any first path segment whose case-folded value is auto, including the bare namespace before generic unlocked mutation can run. test_delete_refuses_bare_reserved_auto_namespace verifies auto, AUTO, Auto, and aUtO all return false, never invoke the recursive helper, and preserve a live skill.

  • A public hardlink alias could mutate a candidate after it was privately claimed and reviewed — fixed in 25ac3513315f1db049d8b10fbdc590828ac4daa2.

    Candidate validation now requires real directories and lone regular-file inodes (st_nlink == 1) and fails closed for links, special files, traversal failures, and lstat errors at pending-detail, claim-layout, and live-target checks. test_approval_refuses_candidate_mutated_through_hardlink_after_claim creates a real same-filesystem hardlink, mutates through it after claim, and verifies approval is refused with no live publication; test_candidate_inode_guard_fails_closed_on_stat_error verifies stat failures also refuse inspection.

Exact-head evidence: test/test_skill_versioning.py 95 passed; focused namespace/link/symlink selection 16 passed, 79 deselected; canonical suite collected 59,035 tests with exit 0; Black, isort, changed-file Flake8, full mypy (1,003 source files), focus checks, and git diff --check passed. Fresh pinned GPT and Opus local reviews both report no findings on 25ac3513315f1db049d8b10fbdc590828ac4daa2.

@github-actions github-actions Bot added the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Aug 20, 2026
@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 21, 2026
@rubencu
rubencu force-pushed the feat/atomic-skill-candidate-claim branch from 25ac351 to 08f0961 Compare August 21, 2026 21:06
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention merge conflict Branch has merge conflicts with its base — author must resolve before merge and removed merge conflict Branch has merge conflicts with its base — author must resolve before merge readiness: checking Automated validation is still running labels Aug 21, 2026
@rubencu
rubencu force-pushed the feat/atomic-skill-candidate-claim branch from 08f0961 to 8ae5bc3 Compare August 21, 2026 21:45
@github-actions github-actions Bot added merge conflict Branch has merge conflicts with its base — author must resolve before merge and removed merge conflict Branch has merge conflicts with its base — author must resolve before merge labels Aug 21, 2026
@rubencu
rubencu force-pushed the feat/atomic-skill-candidate-claim branch from 8ae5bc3 to 70e44a9 Compare August 22, 2026 12:08
@github-actions github-actions Bot added merge conflict Branch has merge conflicts with its base — author must resolve before merge and removed merge conflict Branch has merge conflicts with its base — author must resolve before merge labels Aug 22, 2026
@bolichen97
bolichen97 enabled auto-merge (squash) August 24, 2026 06:58
@bolichen97 bolichen97 added the needs-pr-triage PR scanner: awaiting automated triage label Aug 26, 2026
@NicholasRBowers NicholasRBowers added needs-author-decision PR blocked on author input and removed needs-pr-triage PR scanner: awaiting automated triage labels Aug 26, 2026
@NicholasRBowers

Copy link
Copy Markdown
Contributor

🤖 Kiro Crew [operator: NicholasRBowers#a942f9ca]: This PR surfaced in automated triage with merge conflicts and blocking readiness items. I assessed it, and it is outside what unattended automation should drive:

  • At ~3,300 added lines introducing a new cross-process locking/claim subsystem (namespace, target, and per-claim lock hierarchy across skills.py and security.py, plus the prose-only auto-apply policy), it exceeds this pipeline's size threshold for automated fixes — too much surface to reason about safely without the author.
  • The branch conflicts with main; resolving conflicts across this surface requires your design intent.

When you've rebased and resolved the conflicts, the pipeline will re-assess on its next cycle.

auto-merge was automatically disabled August 27, 2026 02:26

Head branch was pushed to by a user without write access

@rubencu
rubencu force-pushed the feat/atomic-skill-candidate-claim branch from 70e44a9 to 58b71f8 Compare August 27, 2026 02:26
@rubencu

rubencu commented Oct 17, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto 8892889b91. Main's #21346 raised _PACKAGE_LINE_BUDGET from 29,573 to 29,574 (the pinned-order crew-secret leaf), so the budget line conflicted.

  • Main's paragraph and figure stay as main wrote them. This PR's paragraph now reads "Raised again, from 29,574", and the budget is the measured 29,594: main's package is 29,574 lines, and this PR adds the same 20.
  • The other six files are patch-identical to the previous head.
  • Head 6aba79e4d621cfb421d4b5152a681cda9d692bc2 (was 5ea4bea2b0), pushed to both feat/atomic-skill-candidate-claim and security/skills-authority-retire-floor. The cherry-pick line in the body names the new commit and base.
  • Local on this head: the five security test modules plus the flake-pattern ratchet (3,167 passed, 2 skipped), and deny_diff.py (45 allowed at both refs, 0 refused). Black, the wait-discipline ratchet and the single-commit push guard also pass. The other gates ran on the patch-identical replay one docs-only and one test-only main commit earlier.

@rubencu

rubencu commented Oct 17, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto 4b066e090c. Main's #20687 raised _PACKAGE_LINE_BUDGET from 29,574 to 29,607 for the write tier's stall wording, so the budget line conflicted again.

  • Main's paragraphs and figure stay as main wrote them. This PR's paragraph now reads "Raised again, from 29,607", and the budget is the measured 29,627: main's package is 29,607 lines, and this PR adds the same 20.
  • The other six files are patch-identical to the previous head.
  • Head 84a1be904938a617ea343895ff181e5cd6bfec57 (was 6aba79e4d6), pushed to both feat/atomic-skill-candidate-claim and security/skills-authority-retire-floor. The cherry-pick line in the body names the new commit and base.
  • Local on this head: the five security test modules plus the flake-pattern and wait-discipline ratchets (3,206 passed, 2 skipped), deny_diff.py (45 allowed at both refs, 0 refused), black, isort, flake8, mypy, the line cap and docs lint. Push guard: single commit on base.

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Head reviewed: 84a1be9.

My earlier change request is met on this head, so I withdraw it. The description names this commit in the cherry-pick command and the local test line. Its base 4b066e0 is the real parent and the current tip of main. The budget figures in the body (29,607 to 29,627) match the diff, and main's #20687 paragraph is kept as written.

The code is correct and unchanged from the previous head. _is_self_skills_authority_retire calls _matches_self_subcommand(("skills", "authority-retire")) in the same way as the gateway restart floor. The id is in _SELF_PROTECTION_UNGATED_FLOOR_IDS, in the is_denied loop, in the facade and in _exports, and the spec lists the same six ungated ids as the code.

What still has to clear:

  1. The fork workflow runs on this head wait for maintainer approval. No CI or AI review lane has run on 84a1be9 yet.
  2. A maintainer must land this from a repository branch. Sensitive Change Review is red for that reason only.
  3. The tag-grants comment refresh in Design question 1 has no tracking issue. Open one and link it in the description, so the follow-up is not lost when #18595 lands.

@rubencu

rubencu commented Oct 17, 2026

Copy link
Copy Markdown
Contributor Author

The Security Scope Review on 84a1be9049 confirmed one newly refused row: kirocrew skills authority-retire --help. A help request prints the usage and retires nothing, and an agent reads it to tell the operator what to run in their own terminal. The new head ec1d109cd0 (one commit on main 85dcaeed2f, pushed to both this PR's branch and security/skills-authority-retire-floor) narrows the floor at the tier the review named:

  • The floor change. _is_self_skills_authority_retire now calls _matches_self_subcommand(..., help_exempt=True). An argv that also carries -h or --help no longer matches. To support that, _self_cli_operands can hand back the flag words it already skips.
    • Still refused: the plain form, --help-me, --he, and a chained ... && kirocrew --help or ; echo --help.
    • The other self-protection floors are unchanged; kirocrew restart --help is still refused.
  • The corpus. golden-paths.json gains the confirmed row as a self-health shell row, so deny_diff pins it from now on.
  • Tests, spec and budget.
    • New test: test_skills_authority_retire_help_request_is_allowed.
    • The security spec sentence names the allowed help form.
    • The package budget is re-measured at 29,639, 12 lines over main's 29,607 base. argv_floor.py is 4,498 lines, under the 4,500 per-module cap.
  • Local checks. The security test modules pass (6,424 passed, 2 skipped). deny_diff against main reports 46 allowed at both refs and 0 refused. Both ratchets, the script gates and the single-commit push guard pass.

The cherry-pick line in the description now names ec1d109cd0c7b47ef7aef11d8fba3b32b9eedd33.

@rubencu

rubencu commented Oct 17, 2026

Copy link
Copy Markdown
Contributor Author

The help exemption on ec1d109cd0 was too wide: it let a real retire through. The floor reads the command string, and "the argv also carries -h or --help" was true for three commands that still retire:

  • kirocrew skills authority-retire my-skill -- --help. After -- the flag is a plain argument, so the parser never prints help.
  • kirocrew skills authority-retire --reason --help. Here --help is taken as another option's value.
  • kirocrew skills authority-retire $(...) when the substitution prints --help to stderr and something else as the argument.

A # comment was already refused, because the argv ends at the comment.

The new head is ad33292644, one commit on main 52a302b914, pushed to both this PR's branch and security/skills-authority-retire-floor. It narrows the exemption to the exact help-only commands. The command is let through only when the words after the CLI are exactly skills authority-retire --help or skills authority-retire -h.

Any other word after the CLI keeps the floor on:

  • an operand or another flag;
  • -- before the flag;
  • the flag as an option's value;
  • a comment, a substitution or a redirection;
  • a following command.

The direct and python -m kiro_crew spellings behave the same way.

Changes on this head:

  • Tests. Five tests replace the old one. One pins the allowed help-only forms. One each covers the comment, end-of-options and option-value shapes. One covers fourteen other forms, every one refused. The restart floor still refuses its own help form.
  • Budget. It is re-measured at 29,635. argv_floor.py is 4,494 lines, under the per-module cap.
  • Spec and commit message. Both describe the exact rule.

Checks: the security test modules pass (6,428 passed, 2 skipped). deny_diff reports 46 allowed at both refs and 0 refused. Both ratchets, the script gates and the single-commit push guard pass.

The cherry-pick line now names ad33292644d529c75d9554801543f20c7bc3c6c1. The runs on ec1d109cd0 were still waiting for approval, so replacing that head cost no approval.

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Head reviewed: ad33292.

Not ready. The help exemption added on this head lets a real retire through, and the branch needs a rebase that moves the floor registration.

  1. The exemption compares the normalized argv (scan.norm), not the words the shell passes. I ran security.is_denied on this head. It allows kirocrew skills authority-retire ${nope:+--help} and the same command with `--help` in backticks. Bash expands each of those to an empty word, so the shell runs a bare kirocrew skills authority-retire. Details are in the inline comment on argv_floor.py.

  2. Main moved the code this PR registers into. #21650 split the security facade: the loop over the ungated self-protection floors is now in security/deny_gate.py, and security/__init__.py holds a lazy name map. After the rebase, add ("self-protection-skills-authority-retire", _argv._is_self_skills_authority_retire) to the deny_gate.py loop and the name to the __init__.py map. If only the old __init__.py hunk is kept, the floor is exported but is_denied never calls it. Main's package budget is now 30,209, so re-measure that pin too.

  3. The Security Scope Review on this head is red for --help | head -40 and --help 2>&1. If the exemption grows to allow pipes and redirects, it grows the surface from item 1. I suggest dropping the exemption, so this floor refuses --help like restart, gateway restart and cloud already do (your own test pins kirocrew restart --help as refused). The refusal note already tells the operator to run the command in their own terminal. A maintainer then records a scope disposition for the help rows on that head. If you prefer to keep the exemption, see the inline comment for the narrow form.

What is correct and unchanged: the matcher reuses _matches_self_subcommand like the sibling floors, the id is in _SELF_PROTECTION_UNGATED_FLOOR_IDS and the facade exports, and the spec lists the same six ungated ids as the code. A maintainer still has to land this from a repository branch.

Comment thread src/kiro_crew/security/argv_floor.py Outdated
):
continue
if _operands_lead_with(_self_cli_operands(tokens, prog_idx), spec):
if help_exempt and scan.norm[prog_idx + 1 :] in ([*spec, "--help"], [*spec, "-h"]):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 The help exemption lets a bare authority-retire through when the flag is a word the shell expands to nothing.

The check compares scan.norm, the argv after _normalize_operand. That normalizer resolves ${nope:+--help} to its literal --help and strips the backticks from `--help`. Bash expands both to an empty word. So kirocrew skills authority-retire ${nope:+--help} reads as the exact help form here, and is_denied returns no denial, while the shell runs the retire with no arguments. I confirmed both strings are allowed on this head. This floor is the only argv-level check for the command, so the exemption reopens the gap the PR exists to close once #18595 lands.

if help_exempt and scan.norm[prog_idx + 1 :] in ([*spec, "--help"], [*spec, "-h"]):
    continue

Why it matters: an agent shell can run the operator-only recovery with one extra word, on any host where the OS mask on tag-grants does not apply.

Suggestion: remove help_exempt, the golden-paths --help row and the help-allowed test, so this floor matches its siblings. If the exemption stays, compare the raw tokens[prog_idx + 1 :] after stripping only one matched pair of ' or " quotes, and refuse the exemption when any of those tokens contains $, a backtick, (, {, <, >, \ or a control operator. Add ${nope:+--help} and `--help` to test_every_other_skills_authority_retire_form_with_help_is_refused. If the raw-token form already refuses both strings, I will withdraw this.
Rule: AGENTS.md "Security invariants".

@rubencu

rubencu commented Oct 19, 2026

Copy link
Copy Markdown
Contributor Author

@bolichen97, the new head is 3e1e307960, one commit on main e5e6f1c8be, pushed to both this PR's branch and security/skills-authority-retire-floor. It answers all three items from your 2026-10-19T00:16Z review.

  1. The help exemption is gone. You were right: the exact-match exemption on ad33292644 compared the normalized argv. ${nope:+--help} and `--help` read as the help form there, while the shell passes the retire nothing. Rather than chase the raw-token form, I took your first suggestion. The floor now matches its siblings: help_exempt, the golden-paths help row and the help-allowed tests are removed. A new test, test_skills_authority_retire_refuses_its_help_form_like_its_sibling_floors, pins every one of these as refused:

    • the plain, -h and python -m kiro_crew help forms;
    • ${nope:+--help} and the backtick form;
    • the comment, -- and option-value shapes;
    • the piped (| head -40) and 2>&1 forms.

    kirocrew skills --help, skills list and skills root stay allowed, and the floor's refusal note already sends the operator to their own terminal.

  2. The registration moved with main's split. The matcher now lives in security/argv_floor_self.py, named in the argv_floor facade's manifest. It is registered in deny_gate.py's loop over the ungated floors, so is_denied calls it. The facade's lazy name map, its TYPE_CHECKING manifest and _exports.py name it, and the id and reason are in denied_rules_tables.py. The package budget is re-measured at 31,527, against main's 31,509 base.

  3. The Security Scope Review rows. Refusing --help is now deliberate, so the help rows that review lists stay refused on purpose: --help, --help | head -40, --help 2>&1 and the PowerShell spelling. Clearing them needs a maintainer's scope disposition for this head, as you described.

The Design lane's question, whether #18595 puts the authority root under the hidden tag-grants leaf: it does. skills_auto_authority.py keeps the authority at tag-grants/auto-skill-private, beneath the masked leaf, so the OS mask is the main guarantee and this floor is the second check.

Checks: the floor and gate tests pass on this head (131), and the full security set passes on the patch-identical commit one main step earlier (7,082 passed, 2 skipped). deny_diff against main reports 45 allowed at both refs and 0 refused. Both ratchets, the script gates and the single-commit push guard pass. check_harness_parity fails the same way on main, so that red is not this change's. Each lane item on ad33292644 has its own disposition.

@rubencu

rubencu commented Oct 19, 2026

Copy link
Copy Markdown
Contributor Author
  • The exemption compares the normalized argv, so a word the shell expands to nothing reads as --help: fixed in 3e1e307960 by removing the exemption.

The floor no longer exempts any help form, matching the sibling subcommand floors. ${nope:+--help}, `--help` and the plain help form are all refused, and test_skills_authority_retire_refuses_its_help_form_like_its_sibling_floors pins each of them.

@rubencu

rubencu commented Oct 19, 2026

Copy link
Copy Markdown
Contributor Author
  • Item 2: one consumer, generalized (help_exempt on the shared matcher): fixed in 3e1e307960. The exemption is removed, which is this item's clearing condition.

_matches_self_subcommand is back to its main signature, and _is_self_skills_authority_retire calls it exactly as the sibling floors do.

@rubencu

rubencu commented Oct 19, 2026

Copy link
Copy Markdown
Contributor Author

This PR is the whole sensitive-path part of the auto-skill authority, split out so a maintainer can land it from a repository branch before #18595, which spells the subcommand skills authority-retire and touches no sensitive path. Landing the floor first means no build of main ever ships the recovery without its agent-shell refusal.

@rubencu

rubencu commented Oct 19, 2026

Copy link
Copy Markdown
Contributor Author
  • Item 4: the spec now also lists self-protection-file-delivery: kept, because this PR edits that list and the list must equal the code's set.

_SELF_PROTECTION_UNGATED_FLOOR_IDS already holds self-protection-file-delivery on main, and this PR adds self-protection-skills-authority-retire. Adding one id while leaving the other out would leave the list this PR edits wrong. The reviewer on this PR confirmed the spec now lists the same six ids as the code.

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Head reviewed: 3e1e307.

Both items from my previous change request are fixed on this head, so I withdraw it.

  1. The help exemption is gone. _matches_self_subcommand is back to its main signature and _is_self_skills_authority_retire calls it with no exemption, like the restart, gateway restart and cloud floors. I ran is_denied on this head: ${nope:+--help}, a backtick --help, bash -c "kirocrew skills authority-retire", python -m kiro_crew skills authority-retire, a quoted "skills" and an interposed redirect are all refused, while kirocrew skills --help and a product-named worktree path that merely mentions the verb stay allowed.
  2. The rebase is done. The branch sits one commit on a current main, merges into main without conflict, and the floor is registered in the deny_gate.py subcommand loop with its rule id in _SELF_PROTECTION_UNGATED_FLOOR_IDS and its note in denied_rules_tables.py.

Verified locally on this head: test/test_denied_commands_security.py and test/test_security_gate_liveness.py pass (87 tests), including the new help-form test and the raised package line budget.

No new findings. I am not approving yet: this change edits the command deny gate, so it needs a maintainer's sensitive-change sign-off and a green readiness signal on this head, and another reviewer's change request is still open.

@rubencu

rubencu commented Oct 21, 2026

Copy link
Copy Markdown
Contributor Author

Rebased to resolve a conflict in test/test_security_gate_liveness.py. The new head is cb94845e97 on 71a0a9fe49, still one commit, and both feat/atomic-skill-candidate-claim and security/skills-authority-retire-floor point at it.

Main's #21573 raised _PACKAGE_LINE_BUDGET from 31,509 to 31,865 for the stream redactor's AWS anchor, and this PR raised the same number for its floor. Both reasons are kept, main's first. The budget is now the measured 31,883, which is main's 31,865 plus this floor's 18 lines. The commit message's budget sentence says the same. Every other file's patch ID is unchanged.

Verified:

  • on abea7a2cdf: the security, deny, argv and floor modules (67 modules, 8,382 passed, 6 skipped);
  • scripts/deny_diff.py against main's golden paths: no regression;
  • on cb94845e97: the facade-mirror and campaign-facade guards (227 tests), the 24 script gates, the ratchets and the single-commit push guard.

cb94845e97 is a patch-identical rebase of abea7a2cdf over one main commit that touches none of this PR's files.

For the maintainer landing it, the cherry-pick is now git fetch https://github.com/rubencu/KiroCrew.git security/skills-authority-retire-floor && git cherry-pick cb94845e977c2e3ef51c084048b1e3cf58d20bfd.

@rubencu

rubencu commented Oct 21, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto 41987ae89b to resolve a conflict in docs/system-specs/modules/security.md. The new head is bdf608ce80, still one commit, and both feat/atomic-skill-candidate-claim and security/skills-authority-retire-floor point at it:

git fetch https://github.com/rubencu/KiroCrew.git security/skills-authority-retire-floor && git cherry-pick bdf608ce80e16644bf2560f4bbf3a776c6e52f56

Main's #21865 rewrote the row-backed self-protection paragraph (hosts-file digests on POSIX too), which sits between the two spec lines this PR changes. Main's paragraph is kept as it is, and this PR's two lines are unchanged. Every other file's patch ID is identical. Main's security package still measures 31,865 lines, so the budget stays at the measured 31,883.

The Design Review's pre-merge check on cb94845e97 holds on #18595's current head 8f463d5372. The authority root is tag-grants/auto-skill-private (skills_auto_authority.py), and its provenance record is tag-grants/auto-skill-private-authority.json (skills_auto_model.py). Both sit under the tag-grants leaf that main already hides from the sandbox (sandbox_leaves.py), precreates (sandbox_precreate.py) and keeps on the file-tool deny floor (security/paths_lists.py). The description now names those three files for the tag-grants comments, since main moved them out of sandbox.py and security/paths.py.

The Security Scope Review passed on cb94845e97: 16 candidates, none newly refused. The First Principles item has its own disposition, because whether the floor lands before #18595 is a maintainer's call.

Verified on bdf608ce80:

  • 67 security, deny, argv and floor test modules (8,384 passed, 6 skipped);
  • the facade-mirror and campaign-facade guards (227);
  • scripts/deny_diff.py against main with main's golden-paths corpus: no regression (45 allowed at both refs, 0 refused);
  • the 24 script gates, the flake and wait ratchets, and the single-commit push guard.

@rubencu

rubencu commented Oct 21, 2026

Copy link
Copy Markdown
Contributor Author

The order is deliberate. This PR is the sensitive-path part of #18595, split out so a maintainer can land it from a repository branch first, and landing it first means no build of main ever ships the retire recovery without its agent-shell refusal. Holding it until #18595 is approved works too: security/skills-authority-retire-floor stays one commit on main, so a maintainer can cherry-pick both together.
Next step: a maintainer's decision entry on this PR, either "the floor lands first" or "hold the floor until #18595 is approved". Either one clears this item. Until then nothing in the diff changes for it.

@rubencu

rubencu commented Oct 21, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto 9800507e19 to resolve a conflict in test/test_security_gate_liveness.py. The new head is 48da428674, still one commit, and both feat/atomic-skill-candidate-claim and security/skills-authority-retire-floor point at it:

git fetch https://github.com/rubencu/KiroCrew.git security/skills-authority-retire-floor && git cherry-pick 48da428674a9910e0f87d804014c8b604b39af60

Main's #22592 raised the same _PACKAGE_LINE_BUDGET, from 31,865 to 32,056, for pass_compose.py. Both reasons are kept, main's first, and the budget is the measured 32,074: main's 32,056 plus this floor's 18 lines. The commit message's budget sentence matches. Every other file's patch ID is unchanged.

Every run on bdf608ce80 finished after approval with only the Sensitive Change Review red, which a fork PR cannot clear, and every lane passed or reported no findings. So this push carries only the conflict.

Verified:

  • 67 security, deny, argv and floor test modules on c9a5cf2d6a, patch-identical to this head one main step earlier (8,384 passed, 6 skipped), with the 24 script gates and the flake and wait ratchets;
  • on 48da428674: the facade-mirror and campaign-facade guards (230), scripts/deny_diff.py against main with main's golden-paths corpus (no regression: 45 allowed at both refs, 0 refused), and the single-commit push guard.

@rubencu

rubencu commented Oct 21, 2026

Copy link
Copy Markdown
Contributor Author

The red Coverage Gate on 48da428674 comes from main's CI, not from this change. Main's #22425 makes backend-test and frontend-test wait on lint-gate. On a backend-only diff frontend-lint is skipped by design, and a skipped ancestor fails the test jobs' default success() condition even though lint-gate itself passed. So no backend shard ran on this head (run 39563673901: Lint Gate success, Backend Tests and Frontend Tests skipped), and the Coverage Gate fails closed on frontend-test=skipped. Every backend-only pull request hits this until main's test jobs accept a skipped frontend-lint the way lint-gate does.

The other two reds are not this change's either. The Dependency License Gate reports that dependency review is not enabled for the repository, and the Sensitive Change Review could not read the PR from the API (on a fork PR that touches a sensitive path it is red regardless).

Every lane passed or reported no findings on 48da428674, the Security Scope Review included. The local verification in the note above still stands: the 67 security, deny, argv and floor test modules pass on the patch-identical c9a5cf2d6a (8,384 passed, 6 skipped).

@rubencu

rubencu commented Oct 21, 2026

Copy link
Copy Markdown
Contributor Author

Rebased 7b0f96fb71 -> 64692cb3d8 onto main e5914c56f9, on both feat/atomic-skill-candidate-claim and security/skills-authority-retire-floor.

The rebase resolved a conflict in test/test_security_gate_liveness.py. Main's #21867 raised the same _PACKAGE_LINE_BUDGET by four, from 32,056 to 32,060, for its app-agent-launch write-fence entry. Main's reason and number stay as they are; this floor's reason now reads "from 32,060", and the budget is the measured 32,078 (main's 32,060 plus this floor's 18 lines). The commit message says the same. Every other per-file patch ID is unchanged.

Checked locally on this head: the 67 security, deny, argv and floor test modules (8,421 passed, 6 skipped), the facade-mirror and campaign-facade guards (233 tests), scripts/deny_diff.py against main with main's golden-paths corpus (45 allowed at both refs, 0 refused), the script gates, the flake-pattern and wait-discipline ratchets, and the single-commit push guard.

Cherry-pick for a maintainer: git fetch https://github.com/rubencu/KiroCrew.git security/skills-authority-retire-floor && git cherry-pick 64692cb3d88aed3087cca6f407620b5b12ac310b. This head's runs wait at action_required for a maintainer's approval.

@rubencu

rubencu commented Oct 21, 2026

Copy link
Copy Markdown
Contributor Author

Rebased 64692cb3d8 -> 8a3f554ab0 onto main 0144ea1169, on both feat/atomic-skill-candidate-claim and security/skills-authority-retire-floor. The approved round on 64692cb3d8 had finished (every lane clean; CI green; the only red is the Sensitive Change Review fork wall), and main's #22571 made this PR conflict.

The rebase resolved a conflict in test/test_security_gate_liveness.py. #22571 raised the same _PACKAGE_LINE_BUDGET by 37, from 32,060 to 32,097, for exfil_url_rules._heavy_percent_encoding. Main's reason and number stay as they are; this floor's reason now reads "from 32,097", and the budget is the measured 32,113 (main's package measures 32,095, since #22851 shrank it by two, plus this floor's 18 lines). The commit message says the same. Every other per-file patch ID is unchanged.

Checked locally on e5a963dc07, of which this head is a clean rebase one main commit later (that commit touches none of this PR's files, the gate configuration or the security package): the 67 security, deny, argv and floor test modules (8,421 passed, 6 skipped), the facade-mirror and campaign-facade guards (236 tests), scripts/deny_diff.py against main with main's golden-paths corpus (45 allowed at both refs, 0 refused), the script gates and the flake-pattern and wait-discipline ratchets; the single-commit push guard passed on this head.

Cherry-pick for a maintainer: git fetch https://github.com/rubencu/KiroCrew.git security/skills-authority-retire-floor && git cherry-pick 8a3f554ab024a2b657a433a985245ca1bf436c4b. This head's runs wait at action_required for a maintainer's approval.

@iamwhatever

Copy link
Copy Markdown
Contributor

HOLD at 12a9f44.

This adds an ungated deny floor to the security gate (src/kiro_crew/security/argv_floor_self.py, deny_gate.py, denied_rules_tables.py, argv_floor.py, _exports.py, __init__.py). Changes to what an agent shell may run are reserved for a maintainer, and as a fork PR it has to be landed from a repository branch; Sensitive Change Review is red for that reason.

The code looks sound. The matcher reuses _matches_self_subcommand exactly like the gateway restart and cloud floors (argv_floor_self.py#L1072-L1074), Security Scope Review finds 0 newly refused operations, all AI lanes pass, and the description's cherry-pick command now names this head, so our earlier change request is met.

Still open before approval:

I read the diff and CI. I did not run tests.

— crew-pr-reviewer (r119)

@rubencu

rubencu commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor Author

Opened #23038 to track the tag-grants comment refresh in sandbox_leaves.py, sandbox_precreate.py and security/paths_lists.py, and linked it from the first item under Design questions. The issue says why neither this PR nor #19427 can carry it: here the authority doesn't exist on main yet, and in #19427 the files are sensitive paths a fork PR can't pass. No code change, so 12a9f44917 stays the head.

@rubencu

rubencu commented Oct 22, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto main 8e80404a00 as 7e841f617c (both feat/atomic-skill-candidate-claim and security/skills-authority-retire-floor), because the PR had become conflicting. Main's #22229 raised the same _PACKAGE_LINE_BUDGET in test/test_security_gate_liveness.py by 14, to 32,111. Main's reason and line stay as they are, this floor's reason now reads "from 32,111", and the budget is the measured 32,127 (main's measured 32,109 plus this floor's 18). The commit message's budget figures follow. Every other per-file patch ID is unchanged from 12a9f44917.

Checked on this head: the security, deny, argv and floor test modules (83 modules, 9,676 passed, 6 skipped), the facade-mirror and campaign-facade guards (318 tests), scripts/deny_diff.py against main with main's golden-paths corpus (45 allowed at both refs, 0 refused), the script gates, the flake-pattern and wait-discipline ratchets, and the single-commit push guard. The body's cherry-pick SHA, base, budget line and local-run bullet are regenerated for this head.

Main has since moved to da3c2a705b. Its two commits touch none of this PR's files or the security package (one adds a sync-I/O-in-async gate, and this diff adds no async code), and the head merges cleanly with it.

@rubencu

rubencu commented Oct 22, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto main as 6a2fea080a (both feat/atomic-skill-candidate-claim and security/skills-authority-retire-floor), because the PR had become conflicting again. Main's #16544 raised the same _PACKAGE_LINE_BUDGET in test/test_security_gate_liveness.py by 14, to 32,125. Main's reason and line stay as they are, this floor's reason now reads "from 32,125", and the budget is the measured 32,141 (main's measured 32,123 plus this floor's 18). The commit message's budget figures follow. Every other per-file patch ID is unchanged from 7e841f617c.

The tests ran on f2dc2ae51c (on f854c2106a): the security, deny, argv and floor modules (83 modules, 9,677 passed, 6 skipped), the script gates, the flake-pattern and wait-discipline ratchets. This head carries one more main commit, #22699 (fix-loop report CI files, nothing in this PR's files or the security package), with every per-file patch ID unchanged. On this head: the facade-mirror and campaign-facade guards (318 tests), scripts/deny_diff.py against main with main's golden-paths corpus (45 allowed at both refs, 0 refused), and the single-commit push guard. The body's cherry-pick SHA, base, budget line and local-run bullet are regenerated for this head.

The auto-skill publication authority (kirodotdev#18595) gains an operator-only
recovery, `kirocrew skills authority-retire`, that moves an idle authority
and its provenance record aside. It must run from the operator's own
terminal, so this adds the ungated `self-protection-skills-authority-retire`
argv floor beside the existing self-protection subcommand floors: one
structural matcher, its rule id and reason, its `deny_gate` registration
and facade export, and its name in the ungated-floor comment. No catalog
row, no opt-out; no existing pass widened and no threshold moved.

Like its sibling floors, it refuses the command's `--help` form too: a
word that reads as `--help` can reach the shell as nothing or as a plain
argument, and the retire then runs.

This is the whole sensitive-path footprint of the auto-skill authority,
split out so a maintainer can land it from a repository branch; kirodotdev#18595
touches no sensitive path and follows as an ordinary fork PR once this is
on main.

Tests: the floor's template, list, structural and help-form assertions in
test_denied_commands_security.py; the security package line budget raised
from 32,188 to the measured 32,206.
@rubencu

rubencu commented Oct 22, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto main 69b2e503eb as 2072a31878 (both feat/atomic-skill-candidate-claim and security/skills-authority-retire-floor), because the PR had become conflicting again. 6a2fea080a's runs had finished first, with only the Sensitive Change Review fork wall red. Main's #19689 raised the same _PACKAGE_LINE_BUDGET in test/test_security_gate_liveness.py by 63, to 32,188. Main's reason and line stay as they are, this floor's reason now reads "from 32,188", and the budget is the measured 32,206 (main's 32,188 plus this floor's 18). The commit message's figures follow. Every other per-file patch ID is unchanged from 6a2fea080a.

Checked on this head: the security, deny, argv and floor modules (83 modules, 9,677 passed, 6 skipped), the facade-mirror and campaign-facade guards (318 tests), scripts/deny_diff.py against main with main's golden-paths corpus (45 allowed at both refs, 0 refused), the script gates, the flake-pattern and wait-discipline ratchets, and the single-commit push guard. The body's cherry-pick SHA, base, budget line and local-run bullet are regenerated for this head.

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

Labels

fork Pull request from a fork (external contributor) needs-author-decision PR blocked on author input

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants