Skip to content

feat(cron): human-approved vault secret grants for script crons - #7787

Merged
bolichen97 merged 2 commits into
mainfrom
feat/cron-vault-secrets
Sep 5, 2026
Merged

feat(cron): human-approved vault secret grants for script crons#7787
bolichen97 merged 2 commits into
mainfrom
feat/cron-vault-secrets

Conversation

@CrysisDeu

@CrysisDeu CrysisDeu commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

Problem / Motivation

Script crons cannot use secrets. .env was deliberately scrubbed from every cron subprocess (_CRON_ENV_DENY) for sound security reasons, but that removed the only way a deterministic cron could hold a credential — a user's escalation bot (post events to a Slack sandbox workspace, poll reactions to answer common questions) simply cannot run anymore, because there is nowhere to store a token that reaches the job.

Why it matters

Zero-token script crons are the product's answer for deterministic polling, and the credential gap forces those workflows back onto LLM crons or off the platform entirely. Meanwhile the encrypted SecretVault (agent-fenced, AES-256-GCM) already ships — it just has no path to a cron.

What changed (motivation → approach → change)

Goal: get vault secrets into cron subprocess envs without weakening the vault's agent fence, in an agent-first flow. Approach chosen over alternatives: agents may request, only the owner may grant — unconditional agent self-grant was rejected because the agent authors the scripts, so self-grant would collapse the vault into agent-readable storage, with prompt injection as the realistic driver.

  • Store: per-job secret_env (env-var name → vault secret name) + secret_env_pin, and a separate secret_env_pending* request record. Persistence-layer validation: env-name grammar, protected-name deny set (_CRON_ENV_DENY, PATH, LD_/DYLD_/PYTHON/KIROCREW* prefixes), 16-entry cap, SCRIPT jobs only (an agent job would expose plaintext to the model; a command job's pin could cover only the command text, never the helper files the command invokes).
  • Unforgeable code pin: the grant pins the pending/active domain, the job id, the grant mapping, and the script spec + message + body bytes with HMAC-SHA256 keyed by a vault-fenced secret (derived from the vault key), so a forged cron-store entry cannot mint a pin the runner accepts — even on hosts whose OS sandbox backend degrades to "none". At fire time run_script_sandboxed reads the body once, verifies the pin (constant-time), and executes those verified bytes from a private temp dir that is also the script's sys.path entry — a granted script can neither be swapped after approval nor import an unpinned sibling from the live agent-writeable crons/ dir (the import fails instead of running with the secret). Resolution happens in-memory via SecretVault.get_many; values travel to the child over stdin (never the execve environment) behind a protected-key filter, so a grant can never override product-internal keys.
  • Agent-first request flow: new cron_secret_request MCP tool (ownership-checked) records a pending request and best-effort raises the standard approval card inline in the requesting chat session; the card resolves through the same pin-re-verified promotion path as the dashboard.
  • Machine/human/owner boundary enforced in-handler: /api/crons is a prefix entry in the mixed internal-auth paths, so PUT /api/crons/{id}/secrets refuses proven X-Internal-Secret callers (403 operator_only) and requires the dashboard owner (is_owner_dashboard_request, 403 owner_only — a non-owner !dashboard token cannot grant), while POST .../secret-request-card requires the machine credential (its only caller is the MCP tool). Grant metadata in GET /api/crons is serialized only into owner-view responses.
  • Schedule-page UI: pending-request banner with Approve/Deny (approval unlocks only once the script it covers has rendered), read-only active-grant viewer with an arm-then-confirm "Revoke all", a pending-request badge on the job row, and an empty state that names the mint path (grants are minted only through an agent request — there is no hand editor); 16 new i18n keys in all 12 languages.

Tests

test/test_cron_secret_env.py (51 tests): grant validation (grammar, protected names, caps), keyed-pin computation, forged plain-hash pin rejected, body-swap fail-closed, granted script cannot import a live crons/ sibling (proven end-to-end; ungranted scripts keep sibling imports), vault resolution fail-closed with no-secret-name-echo, env injection into real child processes for both runners, store round-trip, persistence gates (agent jobs refused, pin required, revoke clears), MCP tool writes pending-only (and cron_update cannot smuggle the active fields), approve/deny endpoint flow incl. 409 code_changed, machine-credential refusal, non-owner dashboard token refusal, and inline-card semantics. website/src/pages/SchedulePage.secrets.test.tsx (4 tests): approve/deny wiring, draft semantics, picker flow. Existing contract suites extended: cron string-field anti-drift table, cron-list response shape, error-code ratchet.

Full backend suite: the branch's failure set is byte-identical to clean origin/main's on the same host (pre-existing environmental failures only); the diff adds zero failures.

Manual verification

Deployed the branch to an isolated pod, seeded a vault entry + script cron + pending agent request, and exercised the Schedule-page flow in a real browser — the screenshots below are captured from the shipped panel. End-to-end child-process tests cover the injection path.

Screenshots / video

Captured from the shipped build (website/capture/cron-secrets-panel.tsx, real component + stubbed script endpoint):

The pending request as the operator sees it — direction caption, the script the approval covers, Approve enabled only after it rendered:

Pending agent request with Approve/Deny

The active grant, read-only, with "Revoke all" armed (first click relabels, second revokes):

Active grant with armed revoke

Empty state — names where a grant comes from

No grant yet

Related Issues

no linked issue: feature request raised in internal Slack discussion (cron secrets gap after .env scrubbing), no tracked GitHub issue exists.

Checklist

  • At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|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)
  • No secrets, credentials, or internal references in the diff

@CrysisDeu
CrysisDeu requested a review from a team September 2, 2026 02:27
@CrysisDeu
CrysisDeu requested a review from a team as a code owner September 2, 2026 02:27
@CrysisDeu
CrysisDeu requested a review from dwu96 September 2, 2026 02:27
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Sep 2, 2026
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — 🟡 CONCERNS

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

Design-Verdict: CONCERNS

Sound fail-closed design, but the PR description documents a chat-approval surface the code deliberately does not ship, and the agent is the only mint path.

Watch

  • Phantom description (security-relevant). The description claims the MCP tool "best-effort raises the standard approval card inline in the requesting chat session," a "POST .../secret-request-card" machine-credential endpoint, and "inline-card semantics" tests. None exist in the diff — mcp_cron.py states "No in-chat approval surface, deliberately," server.py adds only PUT /api/crons/{id}/secrets, and the test file has no card tests. The shipped design is stricter (good), but a reviewer or auditor reading the description would map a trust boundary that isn't there. Rewrite the description to match the dashboard-only approval flow.
  • The owner cannot mint or re-mint a grant without an agent round-trip. direct_grant_removed says "nothing binds what the owner saw to what gets pinned" — but the approval path's expected_source_sha256 + source_not_reviewable attestation solves exactly that. Since the pin binds message and the delivery fingerprint, any benign job edit kills the grant and stalls the workflow until the operator prompts an agent to call cron_secret_request again. Deliberate, but the stated rationale is inconsistent with the mechanism already built; expect this friction to surface as a follow-up ask.

Suggestions

  • The HMAC pin + epoch file + CAS commit + ~150-line compensating-restore state machine all exist because the active grant record lives in the agent-writable cron store; persisting the active grant (mapping + pinned body digest) under the agent-fenced .vault/ — where the epoch file already lives — would remove forgery, replay, and most of the two-store compensation logic in one move. Worth evaluating as a follow-up simplification, not a rework of this PR.

[DESIGN-REVIEWED] ea93970

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5) — 🟡 CONCERNS

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

UX-Verdict: CONCERNS

The approval flow itself is well-built, but its banner is buried at the bottom of the edit dialog and one error points at a control that doesn't exist.

Watch

  • The row badge ("Secret request awaiting approval") opens a dialog where the pending banner renders after the full vertical JobForm and Script source toggle (SchedulePage.tsx:1426-1427, inside a max-h-[86vh] scroll body) — on ordinary viewports the user who came to approve sees only the edit form and must guess to scroll. Hits every approval (the PR's headline flow), friction not failure, every time. Fix: render the pending banner first in DialogBody, or scroll it into view on open.
  • The code_changed 409 body — "ask the agent to re-request (or grant directly)" — surfaces verbatim via ErrorNotice, but the dashboard ships no direct-grant control (secretsList is added to the client and used only by tests; the panel is banner + active list + revoke). The user hunts for a nonexistent affordance. Rare (race path) but misleading each time. Fix: drop "(or grant directly)".

Suggestions

  • Gate secrets_active_help on active.length > 0: the empty state (secrets-panel-empty.png) shows "Injected into this job's environment at run time…" directly under "No secrets granted," describing grants that don't exist.

[UX-REVIEWED] ea93970

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — 🟡 CONCERNS

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

I have everything needed. The change inventory is complete, consumer counts are run, and conventions verified. Final review:

First-Principles-Verdict: CONCERNS

Ships an earlier revision's description: the inline approval card and secret-request-card endpoint it documents do not exist in this diff.

What this change ships

Intent: let a deterministic script cron hold a credential again (agent requests, owner reviews the exact code and approves) — an ADDITION.

  1. Agent requests secrets via new cron_secret_request MCP tool (pending-only) — justified (self-grant would collapse the vault's agent fence)
  2. Owner approves/denies/revokes on a new Secrets panel in the Schedule job dialog — justified
  3. Approve unlocks only after the exact script rendered; approval echoes its digest — justified (agent-writable script boundary)
  4. Granted runs execute pin-verified bytes over stdin, strict sandbox, siblings unreachable; any code/mapping/delivery drift fails closed — justified, cause-level
  5. Pending-request badge on the job row — declared, justified
  6. GET /api/crons/{id}/script gains reviewable + sha256 — justified (consumed by the approval flow)
  7. Grant names serialized into owner-view cron list only — justified (multi-human boundary)
  8. api.secretsList client method — zero consumers, undeclared leftover
  9. Capture harness + temp-screenshots/ evidence — justified (documented repo convention)
  10. Four cron modules black-formatted, baseline pruned, own commit — justified (documented convention)

Watch

The description documents a design the diff removed: "best-effort raises the standard approval card inline in the requesting chat session" and "POST .../secret-request-card requires the machine credential" — grep secret-request-card over the patch: 0 matches, and the shipped mcp_cron.py comment says "No in-chat approval surface, deliberately." The claimed "inline-card semantics" and "picker flow" tests also don't exist. The shipped design is narrower and safer; correct the description so nobody audits against an endpoint that isn't there.

Subtractions

  • Delete api.secretsList (website/src/api/client.ts:2622) and its mocks in SchedulePage.secrets.test.tsx — 0 shipped consumers (grepped website/src; SecretsPanel.tsx:63 reads /api/secrets via bare get); it's the removed picker's remnant.
  • Delete the stale comments describing the removed design: SchedulePage.tsx "(the chat card is best-effort)", the JobSecretsPanel docstring's "and a small direct-grant editor", and cronSecretsGrant's jsdoc "Body: {secret_env: {...}} grants" — the server refuses non-empty maps (direct_grant_removed).

[FIRST-PRINCIPLES-REVIEWED] ea93970

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

FINDING -- src/kiro_crew/dashboard/handlers/cron.py:1152 -- omitted snapshots become None, disabling the CAS so a stale denial can discard a replacement request -> Fix: require and validate both snapshot fields before denial.

[GPT-REVIEWED] ea93970

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

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

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

Review details

I've verified the mechanism thoroughly. Let me assess the single candidate against the falsification bar.

Findings from my verification:

  1. _bump_grant_epochs_for (cron.py:2708-2734) does call grant_epoch_ids() unconditionally whenever removed_ids is non-empty, before the per-job grant filter. Confirmed.

  2. _remove_job_locked (cron.py:2741) calls it with no try/except, and remove_job_async (cron.py:2520) propagates. api_cron_delete (cron.py handler:440-443) catches only CronStoreBusy/CronStoreUnreadable. A plain ValueError from _read_grant_epochs is not CronStoreUnreadable (which is a ValueError subclass but a distinct type), so it would escape → 500. Confirmed for single-delete.

  3. The candidate's claim that api_cron_batch_delete also crashes is false: it wraps the whole batch in except Exception (handler:493) and reports failures cleanly. So that limb is wrong.

The decisive test — bar (a), a condition that occurs in practice:

The trigger is a corrupt/unreadable .grant_epochs.json. But this file is written exclusively through _write_grant_epochs (tempfile → fsync → atomic os.replace → dir fsync), always as json.dumps(dict[str,int]), always 0o600 owned by the gateway user, always inside the cross-process _grant_epochs_guard flock. There is no code path that can produce a partial write, a non-dict payload, a non-integer value, or a permission that blocks the same user's read. Corruption can only arise from external tampering, manual edits, disk bit-rot, or a truncated backup restore — not from normal operation.

Moreover, the fail-closed propagation is documented as intentional (the _bump_grant_epochs_for and _read_grant_epochs docstrings state corrupt epoch state must refuse grant operations until the owner repairs it), and when the file is corrupt the system is already in an operator-must-intervene state. The residual harm is that a grant-free single delete returns a 500 instead of a clean coded error while in that already-broken state — an availability/UX degradation, over-fencing (not a boundary breach), no data loss, no crash of the service, no key exposure.

(a) resolves to "if the epoch file were to become corrupt," a conditional that does not occur in this code's own atomic-write regime. The candidate's own confidence is "low" and concedes the behavior may be an accepted trade-off. It does not clear the 80+ bar. I find no other grounded defect in the changed lines.

No findings.

[OPUS-REVIEWED] ea93970

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

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

Comment thread src/kiro_crew/cron_script.py Fixed
@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 Sep 2, 2026
@CrysisDeu
CrysisDeu force-pushed the feat/cron-vault-secrets branch from 3488526 to 728bab2 Compare September 2, 2026 03:27
@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 Sep 2, 2026
@CrysisDeu

Copy link
Copy Markdown
Collaborator Author
  • Active grants trust the agent-writable cron store (origin: validation) span=d9648bc6ddff

disposition: fixed in 728bab2. The pin is now HMAC-SHA256 keyed by <config_dir>/.vault/.grant_pin_key — a vault-fenced secret (agent tools cannot read it; bind-hidden from sandboxed cron children in every mode) — so a forged store entry cannot carry a pin _secret_env_precheck accepts (constant-time compare). Deserializing the fields stays; injection now requires a pin only the product's own grant paths can mint. Regression test: test_forged_plain_hash_pin_is_rejected.

Agent script edits crons.json -> loader trusts forged grant and pin -> runner injects and exposes a vault secret.

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author
  • Secret grants and metadata are not owner-gated span=5437f6d2510d

disposition: fixed in 728bab2. PUT /api/crons/{id}/secrets now requires is_owner_dashboard_request (403 owner_only) in addition to refusing machine credentials, reusing the dashboard's single owner definition (same gate as ask_question card resolution). GET /api/crons serializes the secret_env* metadata fields only into owner-view responses, so a non-owner !dashboard token neither reads pending vault names nor approves. Regression test: test_non_owner_dashboard_token_cannot_grant.

Authenticated non-owner -> reads a pending vault name and approves it -> agent-authored code receives the owner's secret.

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author
  • Code pins exclude mutable executable dependencies span=31cfe0f000f2

disposition: fixed in 728bab2. When a grant is present, the launcher's sys.path entry is now the pinned private temp dir — the live agent-writeable crons/ dir is never on the path — so an unpinned sibling module cannot be imported at fire time: import helper raises instead of executing agent-rewritten code with the secret in env. Ungranted scripts keep sibling imports (their dir stays on sys.path). A granted script that needs helpers must inline them into the one approved body. Regression test: test_granted_script_cannot_import_live_crons_sibling.

Approved script imports an agent-writable helper -> helper changes while the main pin remains valid -> modified code executes with secrets.

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author
  • Secret editor overflows narrow viewports span=8eacc61ffa7a

disposition: fixed in 728bab2. The add-grant row is narrow-first: flex-col items-stretch stacking with the env-name input at w-full, switching to a row at sm:. The native <select> is also replaced with the themed SimpleSelect (which independently cleared the Frontend Lint red on the same line).

320px viewport -> fixed-width input, select, and button remain one row -> controls overflow or become inaccessible.

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author
  • Operator-granted cron secrets reach agent-rewritten sibling modules the code pin never covers span=1066b06e1864

disposition: fixed in 728bab2, exactly along the review's first suggested line: when a grant is present the launcher no longer adds the live crons/ dir to sys.path — the pinned temp copy's own private dir is the path entry — so import helper from the agent-writeable dir fails at fire time instead of loading unpinned code with the secret in env. The pin's guarantee ("a body swapped in after the check cannot run with the secrets") now covers the import surface too. Ungranted scripts keep their sibling imports. Regression test: test_granted_script_cannot_import_live_crons_sibling.

a granted job.py that does import helper loads helper.py from disk at fire time ... the rewritten helper.py runs with the vault secret in env.

@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 Sep 2, 2026
@CrysisDeu
CrysisDeu force-pushed the feat/cron-vault-secrets branch from 728bab2 to 649b868 Compare September 2, 2026 04:03
@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 Sep 2, 2026
@CrysisDeu
CrysisDeu force-pushed the feat/cron-vault-secrets branch from 649b868 to da0fd5f Compare September 2, 2026 04:48
@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 Sep 2, 2026
@github-actions github-actions Bot added the readiness: action required A blocking check or review needs attention label Sep 2, 2026
@CrysisDeu
CrysisDeu force-pushed the feat/cron-vault-secrets branch from 935f990 to 26ad062 Compare September 2, 2026 19:24
@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 Sep 2, 2026
@CrysisDeu
CrysisDeu force-pushed the feat/cron-vault-secrets branch from 26ad062 to 6a8a428 Compare September 2, 2026 21:26
@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 readiness: checking Automated validation is still running labels Sep 2, 2026
@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

/ai-review override gpt f860fe2: Statically enumerating every executable dependency of arbitrary Python is undecidable and hiding every writable path is a read-only filesystem that breaks legitimate scripts; the operator approves the exact script body (fully previewable, digest-bound at approval) including any dynamic-load behaviour it chose, and the layered controls close every route the operator did NOT approve.

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

AI-review override not recorded: f860fe218fe5622276ecf180a31d4cafb0619928 is not the current PR head. Re-run the command with 4efeae1fb7f42b2e86cc180c13896e55a6fd85c6.

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

/ai-review override gpt 4efeae1: A same-UID peer reading the child's stdin pipe needs /proc//fd or ptrace access, which equally exposes the gateway's own memory, the vault master key, and any alternative channel (env is worse: /proc//environ); no userland IPC defends against a same-UID peer, that boundary is the OS user, and the short-lived stdin pipe is strictly narrower than the env/argv delivery it replaces.

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Human judgment recorded

@CrysisDeu marked the gpt AI finding as false positive, not applicable, or explicitly accepted for 4efeae1fb7f42b2e86cc180c13896e55a6fd85c6.

A same-UID peer reading the child's stdin pipe needs /proc//fd or ptrace access, which equally exposes the gateway's own memory, the vault master key, and any alternative channel (env is worse: /proc//environ); no userland IPC defends against a same-UID peer, that boundary is the OS user, and the short-lived stdin pipe is strictly narrower than the env/argv delivery it replaces.

This decision applies only to this commit. A new push requires a new judgment.

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

CI status note — Dependency Audit / Audit Production Dependencies red is external, not this PR.

  • Failure text on all four attempts (initial + 3 reruns): npm audit timed out after 120s for website/package-lock.json.
  • This PR does not touch website/package-lock.json, website/package.json, scripts/check_npm_audit.py, or .github/workflows/dependency-vulnerability.yml (git diff --name-only origin/main...HEAD on those paths is empty), so the audited input is byte-identical to main.
  • From an independent host, https://registry.npmjs.org/-/ping answers in ~50 ms, but both POST /-/npm/v1/security/advisories/bulk and POST /-/npm/v1/security/audits/quick do not respond within 60 s — the same endpoints npm audit waits on. Other branches' runs of this job flipped between success and failure in the same window (00:53–01:20 UTC).

Will re-run the job once the advisory endpoints respond again. Both AI review lanes (GPT, Opus) are already clean on edbe1a31e; Backend Tests (3.12, 3) is a separate single-test wall-clock flake (test_security_regex_linearity::test_long_nonshell_line_does_not_blow_up, 6.24 s vs 6.0 s ceiling; file untouched by this PR) queued for rerun when the CI run completes.

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

CI status note — Backend Tests (3.12, 3) red is inherited from main, not this PR.

Coverage Gate red is downstream of the missing shard. Everything else on edbe1a31e is green, both AI review lanes are clean, and Dependency Audit is the separate npm advisory-endpoint outage noted above. Will rebase once either fix lands on 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 #7414 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 #7414: KEEP. Mechanical conflict only; nothing to coordinate beyond an ordinary rebase by whoever lands second. Files: src/kiro_crew/cron_script.py.
  • PR #7670 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 #7670: KEEP. Complementary changes contending for the same lines. Only sequencing is needed, not a decision between them. Files: src/kiro_crew/cron_script.py, src/kiro_crew/slack/gateway.py.

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

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

/ai-review override gpt 1da529c: Statically enumerating every executable dependency of arbitrary Python is undecidable and hiding every writable path is a read-only filesystem that breaks legitimate scripts; the operator approves the exact script body (fully previewable, verbatim-rendered, digest-bound at approval) including any dynamic-load behaviour it chose, and the layered controls close every route the operator did NOT approve.

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Human judgment recorded

@CrysisDeu marked the gpt AI finding as false positive, not applicable, or explicitly accepted for 1da529c4627b0b5937b6f8e5e7aa7d6b4d68294b.

Statically enumerating every executable dependency of arbitrary Python is undecidable and hiding every writable path is a read-only filesystem that breaks legitimate scripts; the operator approves the exact script body (fully previewable, verbatim-rendered, digest-bound at approval) including any dynamic-load behaviour it chose, and the layered controls close every route the operator did NOT approve.

This decision applies only to this commit. A new push requires a new judgment.

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

Status: review-ready on 1da529c46.

  • All CI checks green (62 success / 4 skipped, 0 failing). The two external reds noted above are resolved upstream: the regex-linearity timing ceiling by test: stop the sensitive-regex linearity ceiling flaking on loaded CI #8384 and the npm-audit gate moving to releases-only by ci: run the npm audit gate on releases only; retry transient faults #8362; this head is rebased on top of both.
  • Opus 4.8: [OPUS-REVIEWED] on this head, no blocking findings.
  • GPT 5.6: one finding on this head — the standing "mutable external helpers" class — carries a recorded human override (see the linked judgment); the GPT check reports success on that basis. Its previous blocker (a completed one-shot re-firing when the grant-epoch bump fails) is fixed in this head with a regression test.

Ready for human review. Nothing here merges automatically.

bolichen97
bolichen97 previously approved these changes Sep 4, 2026

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Deep security review — feat + touches src/kiro_crew/secrets/vault.py.

Approving. The trust expansion (agent-authored cron code holding an owner vault credential) is fenced at every boundary it crosses.

Approval gate — not bypassable on any path traced: (1) Handler refuses proven X-Internal-Secret callers (403 operator_only, SEL-audited), since /api/crons is a PREFIX internal path. (2) require_owner_dashboard_request excludes non-owner dashboard tokens. (3) Promotion is CAS-bound to what was displayed: mapping, request timestamp, and REQUIRED expected_source_sha256 of the rendered body — unreviewable source is refused, closing the masked-span hiding route. (4) Audit-or-deny: unwritable SEL refuses approval with nothing mutated. Direct grants removed.

Scoping: store holds vault NAMES only; plaintext never reaches the store or execve env. Active pin is HMAC-SHA256 under a vault-derived subkey binding domain + job id + delivery fingerprint + mapping + script spec + body bytes. Monotonic epoch with thread lock + cross-process flock; revoke bumps before clearing; every removal path bumps first and aborts the delete on failure.

Injection surface: key grammar ^[A-Z][A-Z0-9_]*$, deny-list for PATH/HOME/SHELL/IFS/BASH_ENV and KIROCREW/LD_/DYLD_/PYTHON prefixes, re-checked on delivery. Values ride stdin JSON post-execve, python -I blocks planted sitecustomize, verified bytes execute from payload never from a swappable pathname. _GRANTED_ENV_KEYS strips names from descendant envs.

75 targeted tests cover forged pins, replay, CAS races, machine credential, non-owner token, unreviewable source, and short-secret scrubbing.

Follow-ups (non-blocking): (1) temp-screenshots/cron-vault-secrets/*.png committed — remove. (2) Key deny-list covers Python/loader hijacks but not NODE_OPTIONS/GIT_SSH_COMMAND/PERL5LIB — only affects child processes, verbatim in approval banner. (3) Pending pin is unkeyed by design; an agent writing the store directly could plant a pending request without a SEL event but still cannot obtain a secret without owner approval.

Verdict: MERGE_WITH_FOLLOWUP.

@bolichen97

Copy link
Copy Markdown
Collaborator

Approved (see review above) but merge conflicts are blocking the squash. Could you rebase onto current main and push? The approval will hold once the head is updated.

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

UX Review dispositions

Every finding from the UX Review lane (CONCERNS on d191de144) is implemented in this PR; the lane passed on the re-run. One line per finding:

  • Both screenshots are stale (editor never shipped; Approve shown enabled without the source block) — Implemented. Removed secrets-panel-editor.png; recaptured three shots from the shipped panel via a new capture harness (website/capture/cron-secrets-panel.tsx + shoot-cron-secrets-panel.mjs, real component + stubbed script endpoint): pending request with the "Script this approval covers" block rendered and Approve enabled only after it, active grant with Revoke armed, and the empty state. The PR body's "grant editor" claim is corrected and every media URL is pinned to the current head.
  • No mint path is discoverable from the empty state — Implemented. secrets_none is followed by secrets_none_hint ("Ask the agent to request secrets for this job."), translated in all 12 catalogs.
  • "Revoke all secrets" is one un-armed click with an expensive recovery — Implemented. The button now uses useArmedDelete like the page's Delete: first click relabels to secrets_revoke_all_confirm ("Revoke all?"), second click revokes, and the label decays back on the hook's timer. Test shows the active grant read-only and revokes it whole after arming asserts the first click does not call the API.
  • Pending requests are only visible inside the job's detail dialog — Implemented. The job row's name cell carries a KeyRound warn badge (secrets_pending_badge, "Secret request awaiting approval", also read by assistive tech) whenever secret_env_pending is non-empty.
  • The ENV ← name arrow is the only carrier of direction — Implemented. A one-line caption (secrets_direction_caption, "environment variable ← vault secret") sits above the pending list and above the active list.

No finding was deferred.

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

/ai-review override gpt a4c62b0: Bumping the grant epoch before the store save is the deliberate fail-closed order: the deleted record survives as agent-readable history in an agent-writable store, so saving first would leave a replayable pin whenever the bump fails. A save failure after the bump leaves remaining jobs with refusing (dead) pins, healed by one re-approve, never a live grant on a record the operator deleted. Folding both writes into one transaction adds a persistence path for a failure that cannot leak.

@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Human judgment recorded

@CrysisDeu marked the gpt AI finding as false positive, not applicable, or explicitly accepted for a4c62b0a021045f6e676df266eb7f729f4d9da63.

Bumping the grant epoch before the store save is the deliberate fail-closed order: the deleted record survives as agent-readable history in an agent-writable store, so saving first would leave a replayable pin whenever the bump fails. A save failure after the bump leaves remaining jobs with refusing (dead) pins, healed by one re-approve, never a live grant on a record the operator deleted. Folding both writes into one transaction adds a persistence path for a failure that cannot leak.

This decision applies only to this commit. A new push requires a new judgment.

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.

4 participants