Skip to content

feat(pipeline-conductor): add pod_required repro admission gate - #9146

Merged
iamwhatever merged 1 commit into
mainfrom
fix/pipeline-pod-repro-gate
Sep 10, 2026
Merged

feat(pipeline-conductor): add pod_required repro admission gate#9146
iamwhatever merged 1 commit into
mainfrom
fix/pipeline-pod-repro-gate

Conversation

@pepmach

@pepmach pepmach commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Problem / Motivation

A pipeline marked pod_required could still fix issues with unit evidence and count the run as pod-verified. A typo such as pod-required also 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)

PipelineSpec now has verifier.repro_gate. best_effort is the default. pod_required forbids 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 produce STANDDOWN: pod-repro-ineligible — <evidence>; missing=<capability> with no commit or PR. The same live trace must turn green before GREEN. Existing best_effort behavior stays the same, but every rendered work-order brief gains the repro-gate text because both modes share the template.

scripts/spec_check.py is now the first startup action. It reads the operator-supplied spec through hooks.safe_read_file, validates closed-value fields, and exits 2 on malformed or unreadable input instead of guessing a mode. Both ACP spawn paths overwrite KIROCREW_RUNTIME_PYTHON with their own sys.executable after environment scrubbing, and startup invokes the checker through that absolute path instead of a system python name. Direct and fallback invocations pass -I so user-site, script-directory, current-directory, and inherited Python startup code cannot run before the read gate. -B preserves the sealed desktop bundle's no-bytecode-write rule because isolated mode ignores PYTHONDONTWRITEBYTECODE. Manual and older invocations still resolve venv, POSIX desktop bin/python3.12, and Windows desktop bundle-root python.exe layouts. KIROCREW_SPEC_CHECK_REEXEC prevents 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
Loading

🟩 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.py startup validator. The ACP parents inject KIROCREW_RUNTIME_PYTHON; the validator retains KIROCREW_SPEC_CHECK_REEXEC for 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 before GREEN. It does not change fleet_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.
  • Black, isort, and flake8 pass on all touched Python files; mypy passes on the three touched production modules.
  • ./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

  • 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

@pepmach
pepmach requested a review from a team as a code owner September 6, 2026 22:59
@pepmach
pepmach requested a review from hoang-phan98 September 6, 2026 22:59
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Sep 6, 2026
@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — 🟡 CONCERNS

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

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

  • The skill's stated principle ("a decision this procedure states as prose rots silently; a decision a script computes can be tested") is applied to the enum's spelling but not to the property the enum controls: admission and GREEN rest on a worker LLM asserting a live-pod red/green trace happened ("The pod admission verdict remains an agent-enforced procedure… does not add a machine-verified trace artifact before GREEN"). A worker that fabricates or misjudges a pod trace still counts as pod-verified — the same silent metric corruption the PR exists to prevent, one layer up. Acknowledged as scoped-out, but it leaves pod_required a soft gate advertised as hard.
    Clears when: a follow-up requires a machine-checkable trace artifact (scenario, probe, red/green observables) that a script validates before an item may report GREEN.

Suggestions

  • Have the conductor render only the applicable repro clause per mode instead of shipping the full pod_required block to every best_effort worker, who must self-select against ~15 lines of inapplicable stand-down instructions.

[DESIGN-REVIEWED] bf25414

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

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

Review details

Both candidates carry the discovery pass's own "low" confidence, and both depend on deployment conditions I cannot establish as occurring in practice.

Candidate 1 (-I strips PYTHONPATH, refusing every spec on a PYTHONPATH-only deployment): -I does imply -E, so a checkout whose kiro_crew is reachable only via PYTHONPATH — with no .pth in main site-packages — would fail the import, re-exec under the same interpreter with the same -I, and refuse. But standard installed and editable installs retain main site-packages under -I and are unaffected, and the managed desktop bundles the package in site-packages. Establishing the failure requires assuming a source-checkout-without-editable-install layout that I cannot confirm is a real deployment path — an "if a deployment were to" premise, not a concrete input that occurs in practice. (a) fails.

Candidate 2 (uncaught OSError from subprocess.call in the re-exec exits 1 with a traceback): reachable only when safe_read_file is not importable (the re-exec branch the normal agent path never enters, since sys.executable is injected and imports the package) and bundled_python() returns a path that passes os.path.isfile() but is not an executable interpreter (a manually mis-pointed KIROCREW_RUNTIME_PYTHON). Both conditions require an abnormal, hand-broken setup; neither arises on the injected-runtime path. This is an "if a caller were to mis-point it" premise. (a) fails.

Nothing else in the diff — the two env["KIROCREW_RUNTIME_PYTHON"] = sys.executable injections (placed after the scrub and after extra_env, so agent config cannot redirect the gate) and the safe_read_file-gated, O_NOFOLLOW, fail-closed spec_check.py — surfaces a defect grounded at the required bar.

No findings.

[OPUS-REVIEWED] bf25414

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

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

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] bf25414

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

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — 🟡 CONCERNS

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

All 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 _ENUMS registry serve futures with zero shipped consumers.

Not justified as shipped

  • 8. Launcher-sibling interpreter hunt — zero consumers: with KIROCREW_RUNTIME_PYTHON injected on both spawn paths (client.py:5210, runtime.py:1334), the launcher branch fires only for a human manually running a brand-new script — the description's "older invocations" cannot exist for a file this PR creates. It is also the second spelling of web-verify/scripts/downscale_image.py:54–106 (bundled_python + _REEXEC_ENV; count: 2 implementations, grep bundled_python|_REEXEC), already diverging in layouts covered.
  • 9. _ENUMS registry + arity-general _expected — one consumer, generalized: one entry ever registered (grep _ENUMS: 1 key), and the module spec itself says repro_gate "is the one field with a closed value set". "The seam a future closed field registers in" is so-we-can-later — inherited.

What this change ships

Inventory (10 items) — 8 justified

Intent: 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).

  1. Pipeline specs gain verifier.repro_gate (best_effort default / pod_required) — justified
  2. In pod_required mode, no edit/commit/PR until a live pod shows the failure; same trace must turn green — justified
  3. Every pipeline run now starts with a spec_check.py step; exit 2 refuses the run — justified
  4. An invalid gate value halts startup instead of silently running the generic contract — justified
  5. The spec file is read through the sensitive-path gate; a symlink at a credential store is refused — justified
  6. Every ACP child now carries KIROCREW_RUNTIME_PYTHON — justified (one consumer today, but it is the delivery mechanism itself)
  7. best_effort work-order briefs also gain the repro-admission clause — rides along, declared, harm-free
  8. Under a foreign interpreter with no injected var, the script hunts three install layouts for the launcher's sibling — zero consumers, duplicate of web-verify/scripts/downscale_image.py
  9. One-entry _ENUMS registry with 1/2/3+-arity message rendering — one consumer, generalized
  10. KIROCREW_SPEC_CHECK_REEXEC loop guard — justified (needed by the env-var re-exec that survives item 8's deletion)

Watch

  • The "first pilot silently weakened the gate" defect has no linkable provenance ("no linked issue"), only the description's campaign narrative — acceptable for an addition, but the human merging should know the harm is asserted, not filed.
    Clears when: the campaign report or an issue documenting the pilot's unit-only "pod-verified" runs is linked.
  • Item 8's zero-consumer fallback and item 9's registry both persist as code every future reader of a 242-line startup script must understand.
    Clears when: the subtractions below land, or a second closed field / a real non-injected invocation path is named.

Subtractions

  • Delete the launcher-discovery half of bundled_python() in spec_check.py (keep the KIROCREW_RUNTIME_PYTHON check; refuse when it is absent) — removes ~40 script lines and the five layout tests in TestSpecCheckRunsWhereTheGateIsImportable; the injected path already covers every production spawn.
  • Replace the _ENUMS dict, dotted-path walker, and _expected arity renderer with a direct check of spec["verifier"]["repro_gate"] against the two values — spec_error shrinks to ~10 lines; re-add the table when a second closed field exists.

[FIRST-PRINCIPLES-REVIEWED] bf25414

@github-actions github-actions Bot added readiness: passed Eligible automated validation passed for the current revision and removed readiness: checking Automated validation is still running labels Sep 6, 2026

@cixuuz cixuuz 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.

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 iamwhatever 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.

There was one comment from reviewer, please handle it

@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: passed Eligible automated validation passed for the current revision labels Sep 8, 2026
@pepmach

pepmach commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

@cixuuz — fixed in 8185e95f7. You were right, and the hole was worse than a missing check:

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.

A hyphen in "pod-required" was enough to turn a pod campaign back into unit-fix throughput while the spec file said the gate was on — and because the campaign metric reads the label rather than the evidence, the run would still have counted as pod-verified. That is the exact failure this PR exists to prevent, reintroduced one level down.

What changed. New src/kiro_crew/builtin_skills/pipeline-conductor/scripts/spec_check.py, run once at startup (SKILL.md "Startup (once per run)" step 1, now spec_check.py --spec <path> before the folder is created or any item claimed). Exit 2 is a refusal to start, not a fallback to best_effort — defaulting would be the same silent degradation with an extra step. Error shape, per your ask (field, value, accepted set):

$ spec_check.py --spec typo-spec.json
malformed spec: verifier.repro_gate 'pod-required': expected 'best_effort' or 'pod_required'
$ echo $?
2

Scope is deliberately narrow: only fields with a CLOSED value set are checked, declared once in spec_check.py's _ENUMS table (spec_check.py:53), which is the seam a future closed field registers in. A misspelled repo, branch pattern or threshold already fails loudly at first use; an enum is the one shape that fails silently. An absent field is not an error — omission is how a pipeline asks for its documented default, so existing specs keep running — while an explicit null is a value and is refused.

Contract test. TestSpecCheckRefusesAnUndeclaredGate in test/test_pipeline_conductor_skill_contract.py:299. The malformed-value case you asked for is test_a_misspelled_gate_is_refused_by_name_value_and_option_set, asserting the message carries all three facts. Alongside it: both declared values still accepted, an omitted gate takes the default, non-string values (None, 0, True, [], "", "BEST_EFFORT") refused rather than coerced, a verifier block spelled as a scalar refused (it would otherwise hide the field and read as absent), the CLI's exit codes, and the message rendering at 1/2/3+ values. Red-first: 8 of the 9 fail on the previous head with FileNotFoundError on the absent script.

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: docs/system-specs/modules/pipeline-conductor.md was missing the verifier block entirely (the Design lane called this out too), so its spec table documented a field set the skill no longer had. It now carries the block, the new script, and why the check fails closed.

Gates: 613 tests green across the four test_pipeline_conductor_* suites plus test_security_posture.py and test_spawn_audit.py; mypy src/kiro_crew/ clean; black/isort/flake8 clean; scripts/docs-lint.sh and check_builtin_skill_scope.py pass.

@iamwhatever — that was the only reviewer comment; it is addressed above. Re-requesting review from you both.

@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 8, 2026
@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5) — ⏭️ skipped

Revision bf254142345cf0153484f7403b901dc95e3f12fd touches no user-facing surface (no changes under website/ or committed screenshots), so the UX review was skipped. Advisory — does not block merge.

@pepmach
pepmach force-pushed the fix/pipeline-pod-repro-gate branch from 8185e95 to 62aed1b Compare September 8, 2026 02:41
@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 8, 2026
@pepmach
pepmach force-pushed the fix/pipeline-pod-repro-gate branch from 62aed1b to bfb9a83 Compare September 8, 2026 03:15
@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 8, 2026
@pepmach
pepmach force-pushed the fix/pipeline-pod-repro-gate branch from bfb9a83 to ace4e0f Compare September 8, 2026 04:43
@pepmach
pepmach force-pushed the fix/pipeline-pod-repro-gate branch from 619437f to 4452007 Compare September 9, 2026 01:01
@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 9, 2026
@pepmach
pepmach force-pushed the fix/pipeline-pod-repro-gate branch 2 times, most recently from 7630820 to 1b11abb Compare September 9, 2026 01:36
@github-actions github-actions Bot added readiness: passed Eligible automated validation passed for the current revision and removed readiness: checking Automated validation is still running labels Sep 9, 2026
@pepmach
pepmach dismissed iamwhatever’s stale review September 9, 2026 09:23

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 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.

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 obeys SKILL.md:91, and test_startup_runs_the_check_before_it_dispatches_anything asserts only that two substrings exist in the markdown. The "startup predicate" is a script guarded by prose about the script.
  • GREEN acceptance never looks at the pod trace. SKILL.md:640-651 checks check-runs, head-SHA match and reviewer markers only, so a unit-only fix reported as GREEN is still marked green_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 before GREEN, 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.
@pepmach
pepmach force-pushed the fix/pipeline-pod-repro-gate branch from 1b11abb to bf25414 Compare September 9, 2026 22:25
@pepmach

pepmach commented Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

Addressed at bf254142345cf0153484f7403b901dc95e3f12fd.

  1. The review record now matches the diff. The commit body and What changed name spec_check.py, KIROCREW_SPEC_CHECK_REEXEC, and the ACP-injected KIROCREW_RUNTIME_PYTHON. The new Scope text is:

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.py startup validator. The ACP parents inject KIROCREW_RUNTIME_PYTHON; the validator retains KIROCREW_SPEC_CHECK_REEXEC for 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 before GREEN. It does not change fleet_probe.py's conductor-generated config read or replace the one-entry closed-field registry.

  1. The guarded read now happens first. SKILL.md:91-107 runs the checker through the install-owned interpreter with -I -B (:93-95) before the conductor reads the spec; only exit 0 permits that read (:107). The contract test asserts checker commands < spec read < folder creation, not mere substring presence. Both ACP paths overwrite the runtime path after env scrubbing (acp/client.py:5210, acp/runtime.py:1334), and the fallback uses the same isolated argv (spec_check.py:165).

Additional points from the review:

  • The body now explicitly accepts the present bound: pod admission is procedure-enforced and campaign-audited; this PR does not claim a machine-verified trace artifact before GREEN.
  • fleet_probe.py --config remains unchanged because the procedure creates <spec-dir>/probe-config.json itself (SKILL.md:55-56), unlike the operator-supplied pre-start --spec path. That boundary is now declared in Scope rather than hidden.
  • _ENUMS remains the one closed-field registry. Its prose/value coupling and message arities stay contract-tested; Scope now declares that choice.
  • Fresh local GPT 5.6 Sol and Opus 4.8 reviews both returned no findings on this exact head. The server lanes are re-running; current-head bot concerns will be dispositioned individually before the PR is reported green.

@pepmach
pepmach dismissed bolichen97’s stale review September 9, 2026 22:26

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)

@pepmach
pepmach requested a review from bolichen97 September 9, 2026 22:26
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: passed Eligible automated validation passed for the current revision and removed readiness: passed Eligible automated validation passed for the current revision readiness: checking Automated validation is still running labels Sep 9, 2026
@pepmach

pepmach commented Sep 9, 2026

Copy link
Copy Markdown
Contributor Author
  • Rebutted — span=d622c9b39b64

This PR machine-checks the closed enum and makes the pod-evidence contract explicit; it does not claim that a script proves the red/green trace. The human review required either a trace artifact or an explicit acceptance of procedural enforcement plus campaign audit, and the PR body now states that accepted boundary verbatim. Adding a trace schema, storage contract, and validator is a separate capability, not completion work for this enum/startup gate. Findings that require that separate capability in this PR are covered by this ruling.

@pepmach

pepmach commented Sep 9, 2026

Copy link
Copy Markdown
Contributor Author
  • Rebutted — span=898cdb0f32f5

The shared brief deliberately carries both named mode branches, and each branch is guarded by the exact validated enum value. Rendering only one branch would add a spec-to-template generation path without fixing an incorrect or ambiguous behavior; best_effort remains explicitly the default and unchanged. That renderer is disproportionate to this admission-gate PR.

@pepmach

pepmach commented Sep 9, 2026

Copy link
Copy Markdown
Contributor Author
  • Rebutted — span=1a0c72ee5813

The fallback has a declared present consumer: direct operator invocation documented in spec_check.py's Usage section. It preserves the same fail-closed sensitive-read gate when the ACP-injected variable is absent and covers the shipped venv, POSIX PBS, and Windows bundle layouts. It is therefore neither unreachable nor future-only; findings that treat standalone invocation as nonexistent are covered by this ruling.

@pepmach

pepmach commented Sep 9, 2026

Copy link
Copy Markdown
Contributor Author
  • Rebutted — span=7b835b80890c

_ENUMS is the single implementation source for the accepted closed values, and the prose-coupling test reads that source instead of restating it. Replacing it with an inline one-field branch does not remove a behavior or defect; it removes the registration seam while leaving the same tuple and traversal obligations elsewhere. The small message renderer is fully tested and prevents error-shape drift, so this is an intentional bounded table, not speculative capability.

@pepmach

pepmach commented Sep 9, 2026

Copy link
Copy Markdown
Contributor Author
  • Rebutted — span=b1cf47a7869e

The PR records the live campaign evidence in Manual verification with exact outcomes: four admitted issues, five measured stand-downs, and one orchestration failure. It explicitly says no issue is linked or closed. A separate issue URL would not change the mechanism, evidence, or merge semantics of this addition, so provenance is declared rather than falsely implied through a closing reference.

@pepmach

pepmach commented Sep 9, 2026

Copy link
Copy Markdown
Contributor Author
  • Rebutted — span=5dbaf04cdd69

Both mechanisms have current roles: the fallback keeps direct invocation fail-closed across shipped layouts, and the registry keeps validation plus prose-coupling on one accepted-value source. Their branches are bounded by focused contract tests and documented in the module. A second enum field or production caller is not required before code may enforce one declared contract safely.

@pepmach

pepmach commented Sep 9, 2026

Copy link
Copy Markdown
Contributor Author
  • Rebutted — span=267cb7259eca

Deleting launcher discovery would make the script fail closed for every documented direct invocation even when the install-owned interpreter is present and verifiable. The fallback does not weaken the normal ACP path: injected sys.executable wins first, and every relocation uses -I -B before importing the gate. Removing a tested supported path is not a correctness fix for this PR.

@pepmach

pepmach commented Sep 9, 2026

Copy link
Copy Markdown
Contributor Author
  • Rebutted — span=3874fe2cfcb7

A direct verifier.repro_gate branch would retain the accepted tuple and nested-shape checks while coupling them to one bespoke path. The table keeps one source for values and one walker for closed fields; tests read that source and pin all error arities. Rewriting equivalent tested behavior into a special case would not reduce a reachable defect, so this subtraction is not warranted here.

@iamwhatever
iamwhatever merged commit c64ec83 into main Sep 10, 2026
67 of 73 checks passed
@iamwhatever
iamwhatever deleted the fix/pipeline-pod-repro-gate branch September 10, 2026 00:15
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Sep 10, 2026
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