feat(pipeline-conductor): add pod_required repro admission gate - #9146
Conversation
Design Review (Fable 5) — 🟡 CONCERNSDesign-level review of Design-Verdict: CONCERNS The typo half is machine-enforced; the gate itself — the campaign's central verdict — remains worker-attested prose, the exact rot class this PR's own thesis warns against. Watch
Suggestions
[DESIGN-REVIEWED] bf25414 |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsBoth candidates carry the discovery pass's own "low" confidence, and both depend on deployment conditions I cannot establish as occurring in practice. Candidate 1 ( Candidate 2 (uncaught Nothing else in the diff — the two No findings. [OPUS-REVIEWED] bf25414 Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
First Principles Review (Fable 5) — 🟡 CONCERNSPremise-level review of All evidence gathered — the existing web-verify re-exec twin, the one-entry registry, the env-var consumer counts. Emitting the review. First-Principles-Verdict: CONCERNS The gate is earned; the launcher-hunting re-exec fallback and the one-entry Not justified as shipped
What this change shipsInventory (10 items) — 8 justifiedIntent: make a pod-verification campaign count only live-pod-reproduced fixes as pod-verified, and stop a spec typo from silently disabling that mode. ADDITION (new capability, motivated by a described pilot failure).
Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] bf25414 |
cixuuz
left a comment
There was a problem hiding this comment.
verifier.repro_gate is documented as a two-value enum, but startup never validates the value. An unknown or misspelled value matches neither pod_required nor best_effort, while generic implementation instructions remain reachable. Please validate this field at startup, fail closed on any value outside the two declared options, and add a malformed-value contract test.
iamwhatever
left a comment
There was a problem hiding this comment.
There was one comment from reviewer, please handle it
|
@cixuuz — fixed in
A hyphen in What changed. New Scope is deliberately narrow: only fields with a CLOSED value set are checked, declared once in Contract test. Two of those tests read the accepted set out of the script rather than restating it, so the prose an operator writes the spec from cannot drift from what refuses it — the original defect was two copies of one fact with nothing tying them together. One thing I extended past the finding, flagged here so it is a conscious choice rather than scope creep: Gates: 613 tests green across the four @iamwhatever — that was the only reviewer comment; it is addressed above. Re-requesting review from you both. |
UX Review (Fable 5) — ⏭️ skippedRevision |
8185e95 to
62aed1b
Compare
62aed1b to
bfb9a83
Compare
bfb9a83 to
ace4e0f
Compare
619437f to
4452007
Compare
7630820 to
1b11abb
Compare
The item behind this request is in: verifier.repro_gate is validated at startup and fails closed (exit 2) on any value outside the two declared options, with a malformed-value contract test (details with citations: #9146 (comment)). Current head 1b11abb is green. Dismissing the stale request so re-review can be picked up; re-requesting review.
bolichen97
left a comment
There was a problem hiding this comment.
Requesting changes on two cheap, concrete items. Nothing here is security-blocking, and the enum gate is genuinely good work — but the PR body contradicts the diff, and the gate the title names is not the gate the diff enforces.
1. The body's Scope section is false. It says "No scripts change; the gate is spec + brief + contract tests." The diff's largest artifact is a new 209-line spec_check.py whose exit 2 refuses every run, plus a new env var KIROCREW_SPEC_CHECK_REEXEC. Both Fable lanes flagged this and the body is still unamended, so the record a reviewer reads disagrees with the change they are approving.
2. SKILL.md:91 reads the spec before the gate reads it. The line says "Read the spec, then spec_check.py --spec <path>", so the agent's own unguarded read precedes the guarded one. spec_check.py's docstring names a symlink-at-a-credential-store as its threat, and that threat is realised one line before the gate runs. Swapping the clause order costs nothing and restores the property the script was written for.
Worth being precise about what the two gates are, because they are not equivalent:
The enum gate is real and fails closed. If verifier.repro_gate is present it must be exactly best_effort or pod_required; anything else exits 2 (spec_check.py:85 for _ENUMS, :170-171 for the comparison, :203 for the exit). The value is operator-declared but independently parsed and compared against a table, so a typo cannot pass. The read itself is gated with safe_read_file, resolved-target is_sensitive_path and O_NOFOLLOW, and if that import is unavailable it re-execs under the launcher-sibling interpreter rather than reading plainly (:121-134), terminating at _refuse_unenforceable. The refusal direction is properly pinned — deleting spec_error reddens four named tests, the read-gate refusals are pinned by test_a_symlinked_spec_is_refused_through_the_link and test_an_unenforceable_gate_refuses_rather_than_reading_plainly, and the re-exec terminus by test_the_re_exec_refuses_instead_of_recursing.
The pod-repro admission gate — the one in the title — is prose only. It requires the unmodified worktree to reproduce the failure through a live pod route before any edit (SKILL.md:62-83, brief at :470-486), but nothing parses anything: the decision is the worker LLM's own claim. TestPodReproAdmissionGate pins the presence of phrases in SKILL.md, so it goes red on a rewording and never on a defeated gate. Two consequences follow:
- Nothing in code invokes
spec_check.py. It runs only if the conductor agent obeysSKILL.md:91, andtest_startup_runs_the_check_before_it_dispatches_anythingasserts only that two substrings exist in the markdown. The "startup predicate" is a script guarded by prose about the script. GREENacceptance never looks at the pod trace.SKILL.md:640-651checks check-runs, head-SHA match and reviewer markers only, so a unit-only fix reported asGREENis still markedgreen_verified— the pilot failure this PR exists to prevent, moved from the entrance to the exit. That is GPT's advisory finding and I think it is the substantive one. Either require a recorded trace reference beforeGREEN, or say plainly on the PR that prose enforcement plus campaign audit is the accepted bound.
Two smaller things: fleet_probe.py:1703 still reads its caller-supplied --config with a plain read_text, which is the exact shape spec_check.py's own docstring calls an attack — so the new gate is a point patch on one of two sibling caller-path reads. And _ENUMS has a single entry, making _expected()'s len==1 and len>=3 arms (spec_check.py:139-143) unreachable, with test_the_accepted_set_reads_as_a_sentence_at_any_arity existing only to exercise them.
Also note there are zero ai-review-disposition comments on this PR, so GPT's finding, Design's two Watch items and First Principles' six items are all unanswered for 1b11abb40. CI is green (57 SUCCESS, 8 skipped).
A pod-verification campaign could be reported successful even when workers only produced unit evidence. Add verifier.repro_gate with best_effort as the default and pod_required as a hard admission rule: no source, test, or documentation edit before a live pod reproduces the issue, missing capabilities produce an evidence-bearing standdown, and the same trace must turn green before GREEN. The shared work-order brief changes textually for both modes. Add scripts/spec_check.py as the first startup action. It reads the operator-supplied path through hooks.safe_read_file, validates closed-value fields, and exits 2 instead of silently choosing a mode. Both ACP spawn paths overwrite KIROCREW_RUNTIME_PYTHON with their own sys.executable after environment scrubbing, and the startup procedure uses that absolute interpreter instead of a system python name. Direct and fallback invocations pass -I so user-site, script-directory, current-directory, and inherited Python startup state cannot run before the read gate; -B keeps sealed desktop bundles from writing bytecode. Manual and older invocations still resolve venv, POSIX PBS bin/python3.12, and Windows bundle-root python.exe layouts; KIROCREW_SPEC_CHECK_REEXEC prevents recursive relocation, and an unenforceable gate refuses rather than falling back to a plain read. The conductor may read and use the spec only after the checker exits 0. The live-pod admission verdict remains enforced by the conductor procedure and campaign audit; this change does not add a machine-verified trace artifact. Update the RFC, module spec, agent prompt, and contract tests to pin the mode, startup refusal, sensitive-path read, re-exec terminus, and ordering contract.
1b11abb to
bf25414
Compare
|
Addressed at
Additional points from the review:
|
Addressed every review point at bf25414: accurate body/commit scope, validation-before-read ordering, isolated install-owned interpreter, and substantive rulings on the additional observations. Evidence: #9146 (comment)
|
|
|
|
|
|
|
|
Problem / Motivation
A pipeline marked
pod_requiredcould still fix issues with unit evidence and count the run as pod-verified. A typo such aspod-requiredalso matched neither mode, which silently disabled the admission rule while the spec said it was active.Why it matters
A pod-verification campaign must measure real product-path reproductions. Otherwise a green report proves only that unit tests passed, not that the reported failure was seen and fixed in a live pod.
What changed (motivation → approach → change)
PipelineSpecnow hasverifier.repro_gate.best_effortis the default.pod_requiredforbids source, test, or documentation edits until the unmodified worktree reproduces the issue through a live pod product route. Missing routes, identities, scenarios, host capabilities, or required browser engines produceSTANDDOWN: pod-repro-ineligible — <evidence>; missing=<capability>with no commit or PR. The same live trace must turn green beforeGREEN. Existingbest_effortbehavior stays the same, but every rendered work-order brief gains the repro-gate text because both modes share the template.scripts/spec_check.pyis now the first startup action. It reads the operator-supplied spec throughhooks.safe_read_file, validates closed-value fields, and exits 2 on malformed or unreadable input instead of guessing a mode. Both ACP spawn paths overwriteKIROCREW_RUNTIME_PYTHONwith their ownsys.executableafter environment scrubbing, and startup invokes the checker through that absolute path instead of a systempythonname. Direct and fallback invocations pass-Iso user-site, script-directory, current-directory, and inherited Python startup code cannot run before the read gate.-Bpreserves the sealed desktop bundle's no-bytecode-write rule because isolated mode ignoresPYTHONDONTWRITEBYTECODE. Manual and older invocations still resolve venv, POSIX desktopbin/python3.12, and Windows desktop bundle-rootpython.exelayouts.KIROCREW_SPEC_CHECK_REEXECprevents recursive relocation. Only after the checker exits 0 may the conductor read and use the spec or create the pipeline folder.flowchart LR subgraph Before A1[agent reads spec]:::removed --> B1[checker validates it]:::removed --> C1[pipeline starts]:::ctx end subgraph After A2[checker safely reads and validates spec]:::added --> B2[agent reads validated spec]:::changed --> C2[pipeline starts]:::ctx end classDef added fill:#DCFCE7,stroke:#16A34A,color:#14532D,stroke-width:2px classDef changed fill:#FEF3C7,stroke:#D97706,color:#78350F,stroke-width:2px classDef removed fill:#FEE2E2,stroke:#DC2626,color:#7F1D1D,stroke-dasharray:4 3 classDef ctx fill:#E0F2FE,stroke:#0284C7,color:#0C4A6E linkStyle 0,1 stroke:#DC2626,stroke-dasharray:4 3 linkStyle 2,3 stroke:#16A34A,stroke-width:2px🟩 added · 🟨 changed · 🟥 removed · 🟦 unchanged
The checker now performs the first read; the conductor sees the spec only after validation succeeds.
Scope
This PR changes ten files: the pipeline spec, shared work-order brief, agent prompt, RFC, module spec, three contract/spawn tests, both ACP spawn paths, and the 242-line
scripts/spec_check.pystartup validator. The ACP parents injectKIROCREW_RUNTIME_PYTHON; the validator retainsKIROCREW_SPEC_CHECK_REEXECfor its manual/legacy relocation fallback. The pod admission verdict remains an agent-enforced procedure checked by contract tests and campaign audit. This PR does not add a machine-verified trace artifact beforeGREEN. It does not changefleet_probe.py's conductor-generated config read or replace the one-entry closed-field registry.Tests
python -m pytest -q -n0 test/test_pipeline_conductor_skill_contract.py test/test_acp_client.py::TestSpawnEnvScrub::test_client_spawn_scrubs_sensitive_env_on_default_auto test/test_acp_runtime.py::test_runtime_spawn_scrubs_sensitive_env_on_default_auto test/test_security_posture.py test/test_spawn_audit.py— 185 passed. The tests pin validation before the conductor read, the injected interpreter on both ACP paths, venv/POSIX-desktop/Windows-desktop resolution, and the security/spawn contracts../scripts/docs-lint.sh,python3 scripts/check_builtin_skill_scope.py,python3 scripts/check_subprocess_encoding.py,git diff --check, and the single-commit push guard pass.Manual verification
The original change was exercised by a live ten-issue campaign: four issues entered through real pod reproductions, five stood down with measured missing capabilities, and one recorded an orchestration failure. No unit-only fix counted as pod-loop success.
Related Issues
no linked issue: this is a pipeline-conductor workflow hardening slice from the verification campaign.
Pattern harvest
Rule candidate: review-prompt
Pattern: A guard instruction must place the guard before every unguarded use, and its contract test must assert ordering rather than substring presence.
Checklist
feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)