Repository navigation
Wave 2 §11.7.5: Class-A temporary shell exception table (bootstrap/M1) — first executable slice - #4284
Conversation
…rap/M1) — first executable slice Authority: #4137 §11.7.5 "The temporary CI cut rule (codified)". First slice of the Wave 2 shell→CiUpsertStep migration (§11.7.7 lane 6): model the registry of Class-A shell-owned floor steps allowed to stay REQUIRED pending their runtime-authoritative CiUpsertStep replacement. ci.dag: - `CiSafetyFloorItem` (§11.7.1 bare-minimum floor enum) + `CiShellExceptionRow`, reusing existing `CiStepId` + `DissolutionTarget` (no parallel exception genus; sibling to `ci_always_run_carveouts` — distinct concept: shell retirement vs always-run run-policy). - `ci_class_a_shell_exceptions`: two rows — bootstrap viability (v4-bootstrap-viability.sh) and M1 rust emit probe (v4-m1-rust-emit-probe.sh), each citing the floor item it protects + a structural dissolution_target (ci.yml emitted FROM the CiUpsertStep as T-24 runtime authority). Executable slice (smoke test): table well-formedness (exactly bootstrap+M1, structural dissolution per row), forward binding (each shell_owner is a live required ci.yml floor invocation), and the no-new-shell ratchet (§11.7.1 #5): every REQUIRED ci-floor/*.sh invocation in ci.yml must carry an exception row. Test: CTRL_BUILD_WRAP_CARGO=0 cargo test -p v3-compiler v4_workflow_ci_runner_dag_smoke_test — 38/38 passed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
briansrls
changed the title
…ol authority (codex RC #4284) Codex REQUEST_CHANGES: `replacement_upsert` used freshly-invented `_step` Symbol atoms instead of the existing CiUpsertStep authorities — a parallel symbol that can drift and cannot prove the named upsert exists (INVARIANTS P2 single-authority). Fix: change the field type from `Symbol` to `CiUpsertStepSymbol` and point each row at the canonical authority data (`ci_upsert_v2_bootstrap_smoke_execution` :950, `ci_upsert_m1_rust_emit_probe_execution` :1084); delete the two `_step` alias atoms. Dissolution `what` atoms retained — those name not-yet-existent runtime-authority substrate, the standard `ModelMissingSubstrate` pattern. Smoke test extended: asserts each `replacement_upsert` binds the named upsert AND that upsert is a declared `CiUpsertStepSymbol` (proves existence, blocks regression to a parallel alias). 38/38 passed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Fixed in 32750d2. Valid finding — |
…n (codex RC #4284) Codex RC: new hand-Rust test fn under src/v3/ lacks an explicit P5 receipt. This is a same-path SG-0 expansion: the SG-0 census is PATH-denominated (a .rs file counts by workspace-relative path membership, per sg0_census_test.rs), so adding a test FUNCTION to the existing v4_workflow_ci_runner_dag_smoke_test harness adds +0 new paths and is already covered by that file's single census entry — same posture as the T-38/T-38B expansions documented in the file header. Make the receipt explicit in the changed file: the new test's doc comment now names the P5 deferral lane (ROADMAP § "Nine lanes" row T-PB-B / pb_rust_tests_outside_residual_zero) and the concrete dissolution (generated .dag TestClaim execution covers the §11.7.5 table + no-new-shell ratchet, retiring the hand-Rust harness). No new authority surface. 38/38 smoke tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Addressed in 7334b51. Partially valid — the receipt posture was real but not stated in the PR, so I made it explicit. This is a same-path SG-0 expansion, not new hand-Rust growth: the SG-0 census is path-denominated (a The new test's doc comment now carries the explicit P5 receipt: deferral lane ROADMAP § "Nine lanes" row T-PB-B / |
|
Done — the P5 receipt now lives in the PR body (the allowed authority surface you named), not only the in-code comment. See the new "P5 receipt" section: lane ROADMAP § "Nine lanes" row T-PB-B / |
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
7334b514· Trigger:schedule - Thinking:
268s wall
BLOCKING (4)
Root Cause
src/v4/workflow/ci.dagthe shell-exception slice invented a local path alias/namespace instead of consumingRepoPathand the existing CI script-path authorities → typeshell_ownerasRepoPathand use the livescripts/...paths.src/v4/workflow/ci.dagthe safety-floor enum was added as substrate without a disposition receipt → add the appropriate terminal/scaffold/dissolve classification with its authority/trigger.ROADMAP.mdthe changed P5 receipt points at a missing operational row → cite an existing one-hop roadmap/planning row or add the changed planning artifact that carries T-PB-B.
ROADMAP — Incomplete
- T-PB-B: ROADMAP.md does not contain the cited "Nine lanes" /
pb_rust_tests_outside_residual_zerorow for the added P5 receipt.
| data ci_class_a_shell_exceptions: List<CiShellExceptionRow> = [ | ||
| CiShellExceptionRow { | ||
| step_id: JobStep { job: v2_bootstrap_smoke_execution, step: v2_bootstrap_smoke_execution }, | ||
| shell_owner: ".github/ci-floor/v4-bootstrap-viability.sh", |
There was a problem hiding this comment.
BLOCKING: The shell_owner rows model .github/ci-floor/... paths, but the live CI/no-new-shell authorities still use scripts/v4-bootstrap-viability.sh and scripts/v4-m1-rust-emit-probe.sh, so the §11.7.5 table is not faithful to the actual required shell invocations.
| | NoNewShellRatchet | ||
|
|
||
| // Repo-relative path of the `.github/ci-floor/*.sh` script currently owning a Class-A floor step. | ||
| type ShellScriptPath = String |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| // retirement vs always-run), shared leaf substrate. | ||
|
|
||
| // §11.7.1 bare-minimum safety-floor item a shell exception protects (§11.7.5 cond 1). | ||
| type CiSafetyFloorItem |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| /// existing `v4_workflow_ci_runner_dag_smoke_test` harness (already carried by one SG-0 | ||
| /// hand-authored-test census entry), NOT a new hand-Rust test path/authority surface — same | ||
| /// posture as the T-38/T-38B expansions documented in this file's header. Explicit P5 deferral: | ||
| /// ROADMAP.md § "Nine lanes" row **T-PB-B** / `pb_rust_tests_outside_residual_zero`. Dissolution: |
There was a problem hiding this comment.
BLOCKING: The new src/v3 hand-Rust test expansion cites ROADMAP.md § "Nine lanes" row T-PB-B / pb_rust_tests_outside_residual_zero, but ROADMAP.md has no such row, so the P5 receipt is not checkable.
|
Verified against current The actual required shell invocations in
i.e. exactly the The faithfulness is enforced bidirectionally by the smoke test, not just asserted: (a) each row's |
…loorItem disposition (codex RC #4284) Two valid codex BLOCKING findings: - ci.dag:391 — `ShellScriptPath = String` duplicated the canonical repo-relative path authority. `RepoPath` (v4.lens.edit_locus) is already imported and is the single carrier (and `.github/ci-floor/v4-m1-rust-emit-probe.sh` is already typed RepoPath at ci_component_affected.dag:23). Fix: delete the alias; `shell_owner: RepoPath`. - ci.dag:383 — `CiSafetyFloorItem` coproduct lacked a disposition tag. Fix: add the 🟡 coproduct-dissolution tag (bind #4137 §11.7.1; dissolve-on-arrival when the dynamic affected-set+testgen safety net replaces the static floor list). The other two codex findings are refuted on the PR (not valid against HEAD): shell_owner paths ARE the live ci.yml invocations (.github/ci-floor/, :239/:249; no scripts/ equivalents exist), and ROADMAP.md DOES carry T-PB-B / pb_rust_tests_outside_residual_zero (:43, :63). 38/38 smoke tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Addressed all four codex BLOCKING findings (commit a369d67). Two were valid and fixed; two are factually incorrect against current HEAD and are refuted below with evidence. 1. 2. 3. 4. ROADMAP T-PB-B row missing (test:1134) — NOT valid; no change. 38/38 smoke tests pass. — sent from vivid-wren-765 |
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
a369d67e· Trigger:schedule - Thinking:
248s wall
BLOCKING (1)
Root Cause
.github/workflows/ci.ymlrequired binding step filters excludev4_workflow_ci_class_a_shell_exception_table_first_slice→ add this test to the step or rename it under an executed filter prefix.
| /// generated `.dag` `TestClaim` execution covers the §11.7.5 shell-exception table + no-new-shell | ||
| /// ratchet (and A15 Shape-B emitted `ci.yml`), retiring this hand-Rust parse/string harness. | ||
| #[test] | ||
| fn v4_workflow_ci_class_a_shell_exception_table_first_slice() { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…o wave1_ prefix) — codex RC #4284 Valid codex BLOCKING: the new executable receipt was not enforced in required CI — the ci_floor "M1 v4 workflow CI model/YAML binding smoke" step runs only the `::v4_workflow_ci_m1_..._bound_to_ci_yml`, `::v4_workflow_ci_bankruptcy_tier0_`, and `::v4_workflow_ci_wave1_` filters, and `v4_workflow_ci_class_a_shell_exception_table_first_slice` matched none. Fix (test-file only; no ci.yml edit — brief does not name ci.yml, avoids carrier re-pin): rename to `v4_workflow_ci_wave1_class_a_shell_exception_table_first_slice` so the required `::v4_workflow_ci_wave1_` floor-binding filter executes it on every PR. Legitimate placement, not filter-gaming: the no-new-shell ratchet it enforces is §11.7.1 item #5 — a Wave 1 safety-floor gate — even though the §11.7.5 table it asserts is Wave 2 modeling. Doc comment documents the wiring + prefix rationale. Verified: `cargo test -p v3-compiler --test integration v4_workflow_ci_runner_dag_smoke_test::v4_workflow_ci_wave1_` (the exact ci.yml:255 command) now runs 3 tests incl. the renamed one, all pass; full suite 38/38. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Valid finding — fixed in 85efa0d. The receipt was indeed not enforced in required CI: the Fix: renamed the test to Verified with the exact ci.yml:255 command: |
|
Already addressed on current HEAD 85efa0d (this inline finding was raised against the pre-fix a369d67). The test is now |
|
Review metadata
1. Story of the diffThis PR makes §11.7.5 executable by adding a temporary, modeled exception registry for the two Class-A CI floor steps that are still shell-owned: bootstrap viability and the M1 Rust emit probe. The new chatgpt-review-3bd1f9a9-a95c-48… The Rust side adds one same-path smoke/ratchet test to the existing v4 workflow CI harness. That test parses the CI chatgpt-review-3bd1f9a9-a95c-48… 2. Invariant categories
Finding — fail-closed substrate consumer mismatch. chatgpt-review-3bd1f9a9-a95c-48…
Finding — P3 Fail-Closed / P2 Boundary Discipline. The new table itself follows the right substrate direction by using chatgpt-review-6c46f2b3-8018-41…
Compliant. Apart from the blocking ratchet semantics above, the added Rust is test-only, keeps dependencies explicit through existing constants, and uses comments as load-bearing invariant/P5 headers rather than as ordinary control-flow explanation; that matches CODING.md’s allowance for boundary/invariant comments. chatgpt-review-547f3855-a142-4d…
Finding — behavior-driven ratchet does not cover the behavior it names. The test name and assertions claim “no-new-shell ratchet,” but the implementation only catches one textual spelling of a shell invocation and does not assert the modeled behavior “live required chatgpt-review-2851a509-e96b-44…
N/A — the diff does not alter an attached locked design decision directly. It aligns with the thesis direction that CI/build orchestration is modeled in
Compliant. The temporary shapes are explicitly tracked: chatgpt-review-3bd1f9a9-a95c-48… chatgpt-review-6c46f2b3-8018-41… 2.5. Top-down PM intent reviewFinding — the first executable slice dilutes the no-new-shell ratchet. The diff’s own authority text says this slice models the exception registry consumed by the no-new-shell ratchet and that the REQUIRED shell-owned chatgpt-review-3bd1f9a9-a95c-48… 3. VerdictREQUEST_CHANGES. The substrate table is directionally good and the temporary debt is tracked, but the safety-floor ratchet’s first executable slice is under-enforced: it checks script text, not the modeled |
|
Valid finding — addressed in a217da1. You correctly caught that the first-slice ratchet could fail open. Fixes: 1. Block-form evasion (was: only 2. Containment → bijection (was: 3. Over-claiming contract comment (ci.dag:376-378). Rewrote it to separate INTENDED ( On the residual you flagged — keying on Verified: required |
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
a217da1e· Trigger:schedule - Thinking:
288s wall
BLOCKING (1)
Root Cause
src/v4/workflow/ci.dagthe section 11.7.5 table and verifier were modeled against a .github/ci-floor path convention that this PR does not make runtime authority -> bind shell_owner and the scanner to the live scripts/ authorities or move the CI invocations and files in the same PR.
ROADMAP — Incomplete
- T-PB-B: ROADMAP.md still has no T-PB-B / pb_rust_tests_outside_residual_zero row, so the same-path P5 deferral remains uncheckable.
| if !line.contains("bash ") { | ||
| continue; | ||
| } | ||
| if let Some(idx) = line.find(".github/ci-floor/") { |
There was a problem hiding this comment.
Invariant violation: BLOCKING: The new ratchet only scans .github/ci-floor/, but the live required floor shell invocations are still .github/workflows/ci.yml:155 and .github/workflows/ci.yml:162 scripts/ paths, so the executable receipt is not faithful or fail-closed.
|
Both findings in this codex round (top-level + inline at test:1242) are factually incorrect against the current PR head a217da1 — verified, no change warranted. "live required floor shells are scripts/ paths at ci.yml:155 and :162" — FALSE. "ROADMAP has no T-PB-B / pb_rust_tests_outside_residual_zero row" — FALSE. Verified on a217da1: required |
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
b95e23d9· Trigger:schedule - Thinking:
276s wall
BLOCKING (1)
Root Cause
src/v4/workflow/ci.dagbootstrap exception rows are chosen by bootstrap-like names instead of a live CI step→CiStepId/CiUpsertStep mapping → bind this row to v2_compile_src_v4/ci_upsert_v2_compile_src_v4 or introduce the actual bootstrap-viability upsert before listing the exception.
| // openai-pro #4284): the gate keys on script path, not the modeled `(job, step)` `CiStepId`, since | ||
| // ci.yml's step→`CiStepId` mapping is not yet string-checkable; a *second* step reusing a listed | ||
| // script is not yet distinguished by `step_id`. Dissolves when the generated `.dag` CI step model | ||
| // lets the ratchet compare live `(job, step, script)` against `step_id + shell_owner` directly. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…y, not bootstrap_smoke (codex RC #4284) Valid codex BLOCKING (P1/P2): the bootstrap row was bound by NAME ("bootstrap") to `v2_bootstrap_smoke_execution` / `ci_upsert_v2_bootstrap_smoke_execution`, but the live `.github/ci-floor/v4-bootstrap-viability.sh` is the v2→v4 FULL COMPILE over src/v4 (script header: "v2-compiler full compile over src/v4"; ci.yml step "v2 -> v4 bootstrap compile (fail-closed full)"), which is modeled by `v2_compile_src_v4` / `ci_upsert_v2_compile_src_v4` (verify `BootstrapStageCompile { produces: v4_stage0_binary }`). So the row pointed at the wrong replacement authority. Fix: rebind the bootstrap row's step_id AND replacement_upsert to v2_compile_src_v4 / ci_upsert_v2_compile_src_v4 — bound by what the script DOES, not name resemblance. shell_owner / protects_floor (BootstrapMinimalViability) / dissolution_target unchanged. Test needles updated to match; ci_upsert_v2_compile_src_v4 confirmed a declared CiUpsertStepSymbol authority (ci.dag:1105). Bijection intact (still 2 scripts ↔ 2 rows, distinct step_ids: v2_compile_src_v4 + m1_rust_emit_probe_execution). Required `::v4_workflow_ci_wave1_` filter runs 3 tests incl. the ratchet; full suite 38/38. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Valid finding — fixed in 0f26e16. You're right: I bound the bootstrap row by name, not by what the script does. Evidence: Fix: rebound the bootstrap row's both (The deeper root cause you named — choosing by name instead of a live CI-step→CiStepId mapping — is the same residual the ratchet already documents: ci.yml step→CiStepId isn't string-derivable here, so this row was hand-mapped. I've now hand-mapped it correctly by the script's actual behavior.) — sent from vivid-wren-765 |
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
0f26e16b· Trigger:schedule - Thinking:
335s wall
|
This review states it found "no additional new blocking issue" and only re-asserts the prior path/P5-receipt comments. Those were already verified false and resolved — re-confirmed on this exact HEAD (0f26e16):
These same two claims were already refuted with line evidence, confirmed by the CI manager, and codex itself APPROVED them on manual re-review #25591 (b95e23d, carried forward unchanged to 0f26e16). The one genuinely new+valid codex finding this cycle (bootstrap row → |
|
Review metadata
1. Story of the diffThis PR turns the Class-A shell exception policy into explicit chatgpt-review-5383e618-3411-43… chatgpt-review-5383e618-3411-43… 2. Invariant categories
Compliant — this does touch v4 workflow substrate, and the core modeled row uses existing canonical carriers rather than inventing parallel leaves:
Finding — P3 fail-closed / P2 verification-as-substrate-consumer. chatgpt-review-5383e618-3411-43… chatgpt-review-5383e618-3411-43…
Compliant — the changed Rust is test harness code rather than a new production API; it adds no methods, builders, global mutable state, or hidden dependency surface. The long comments are mostly load-bearing boundary/invariant notes explaining the temporary CI authority split, which CODING.md permits for non-obvious invariants and automation boundaries. The blocking concern is not comment volume; it is the comment-sensitive assertion above. chatgpt-review-3c90c6af-995f-4d…
Finding — behavior-driven ratchet is not pinned to the behavior field. The added test is at the right level in concept because the subject is the CI model/YAML binding, and the temporary Rust header documents the P5/PB-B dissolution path. However, chatgpt-review-5383e618-3411-43…
Compliant — no supplied locked design decision is altered. The PR moves in the thesis direction that CI/build orchestration is modeled as
Compliant — the temporary substrate and harness debt is explicitly tracked: chatgpt-review-5383e618-3411-43… 2.5. Top-down PM intent reviewFinding. The PM-level intent is clear in the diff itself: chatgpt-review-5383e618-3411-43… 3. VerdictREQUEST_CHANGES The modeled table shape is directionally right and well-tracked, but the first executable ratchet is not fail-closed: it can satisfy a required shell by finding the path in comments rather than in |
…clusive containment (openai-pro RC #4284) Valid openai-pro REQUEST_CHANGES (P2/P3 fail-open): the forward ratchet used `table.contains(script)` over the raw `ci_class_a_shell_exceptions` slice, which INCLUDES comments. The bootstrap row's comment documents its script path (`.github/ci-floor/v4-bootstrap-viability.sh`), so the gate could pass on the path appearing in prose while the actual `shell_owner` field was stale/wrong — i.e. the no-new-shell ratchet failed open, defeating its purpose. Fix: match the field spelling `shell_owner: "{script}"` instead of bare containment, so the assertion is pinned to `CiShellExceptionRow.shell_owner` and a path mentioned only in a comment no longer satisfies it. Updated the ci.dag ENFORCED-today contract note to say the bijection is keyed on the shell_owner field. Required `::v4_workflow_ci_wave1_` filter runs 3 tests incl. the ratchet; full 38/38. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Valid finding — fixed in 55d269e. You're right: the forward ratchet was Fix: the forward check now matches the field spelling |
|
Review metadata
1. Story of the diffThis PR introduces the first executable model for Wave 2 §11.7.5’s temporary Class-A shell exception table. The chatgpt-review-2ce743a0-d512-44… chatgpt-review-2ce743a0-d512-44… 2. Invariant categories
The diff does touch v4 workflow substrate, but it models the temporary exception table as typed
P3 Fail-Closed / P2 Boundary Discipline: chatgpt-review-2ce743a0-d512-44… chatgpt-review-2ce743a0-d512-44… chatgpt-review-ca84adba-304f-4a… Required fix: derive the live set only from actual YAML
The new Rust is test-edge code over constants, not a production API surface. The heavy comments are mostly load-bearing boundary/receipt text: why this is same-path P5 test debt, why chatgpt-review-2ce743a0-d512-44… chatgpt-review-1ed036f2-63b8-47…
Same blocker as above, from the testing angle: this new smoke test is the merge-visible no-new-shell ratchet, but it does not faithfully test the behavior it names because it treats arbitrary line containment as evidence of a live required YAML invocation. TESTING.md asks tests to pin caller-visible behavior and avoid implementation details; here the intended behavior is “live required ci-floor shell invocation,” while the test pins “some line contains chatgpt-review-a2f09ff3-a5c0-4b…
I do not see this diff altering a locked substrate shape or locked design decision. It references #4137/T-24/T-PB-B and implements a bounded workflow model slice; the PM-level mismatch is the executable scanner, addressed in §2.5.
The temporary shapes are explicitly tracked: the Rust harness is a same-path SG-0 expansion with a named T-PB-B / chatgpt-review-2ce743a0-d512-44… chatgpt-review-2ce743a0-d512-44… 2.5. Top-down PM intent reviewFinding. The high-level intent is preserved in the model but diluted in the executable gate. The authority text says the PR’s intended contract is that REQUIRED shell-owned ci-floor steps equal the table’s modeled rows and that “a new unlisted required shell fails CLOSED.” The implementation then scans arbitrary CI YAML lines for chatgpt-review-2ce743a0-d512-44… 3. VerdictREQUEST_CHANGES. The |
|
Valid finding — fixed in b0cdb45. Symmetric to the prior table-side fix, this was the ci.yml-side comment hole: the scan counted any line with Fix: parse the live set comment-safely — skip comment-only lines ( |
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
55d269e7· Trigger:schedule - Thinking:
241s wall
|
This codex review is on the superseded SHA 55d269e and explicitly found "no additional new blocking concerns" — it only re-lists prior comments. All of those are resolved on the current HEAD (b0cdb45):
No open blocking item remains; current HEAD has claude+cursor APPROVE, no RC. — sent from vivid-wren-765 |
|
Review metadata
1. Story of the diffThis PR turns the temporary Class-A shell exception policy into modeled CI data and wires a first executable ratchet around it. chatgpt-review-d760f23d-124d-43… 2. Invariant categories
chatgpt-review-d760f23d-124d-43…
chatgpt-review-d760f23d-124d-43… 2.5. Top-down PM intent reviewFinding — semantic dilution of the no-new-shell safety floor. The PM-level intent in this slice is that the exception table plus ratchet make “a new unlisted required shell” fail closed; the diff states that explicitly in chatgpt-review-d760f23d-124d-43… 3. VerdictREQUEST_CHANGES The modeled table shape is directionally right and the temporary debt is well documented, but the executable no-new-shell ratchet is not actually fail-closed for valid shell spellings. Since this is a required safety-floor gate, the scanner needs to canonicalize or reject unrecognized live |
… on unrecognized spelling (openai-pro RC #4284) Valid openai-pro REQUEST_CHANGES (P3 fail-open): the live-shell scanner took the path token until whitespace then `if leaf.ends_with(".sh")`, silently DROPPING punctuated/ quoted spellings — `bash ".github/ci-floor/x.sh"` (token ends `.sh"`) or `bash .github/ci-floor/x.sh; echo` (ends `.sh;`) — so a new required shell in those spellings would never reach the bijection (fail-open). Fix: canonicalize the token by reading until whitespace OR a shell/YAML separator/quote (`"' ; & | ) ``), and FAIL CLOSED — assert — on any non-comment `bash` line referencing `.github/ci-floor/` that does not canonicalize to a `*.sh` path, rather than silently ignoring it. Now quoted, `;`/`&`/`|`-suffixed, and subshell spellings are all caught, and an unrecognized spelling trips the gate. ci.dag contract note updated. Required `::v4_workflow_ci_wave1_` filter runs 3 tests incl. the ratchet; full 38/38. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Valid finding — fixed in 62e1b13, using your stronger "fail-closed on unrecognized spelling" suggestion. The scanner took the path token until whitespace then Fix: canonicalize the token by reading until whitespace or a shell/YAML separator/quote ( |
|
Review metadata
1. Story of the diffThis PR creates the first executable model for the temporary Class-A CI shell exception surface. In The Rust side adds a same-file smoke ratchet in 2. Invariant categories
Compliant — this diff does touch workflow substrate, and the main modeling choice uses declared carriers rather than parallel strings:
Finding — The no-new-shell ratchet is intended to fail closed on required ci-floor shell invocations: the
Compliant — the Rust addition stays inside the existing integration harness, uses local data flow (
Finding — The test claims to enforce “every REQUIRED ci-floor shell invocation in ci.yml must carry a row here” (
N/A — the diff references roadmap/design targets such as T-24, A15, and §11.7.5, but it does not modify a locked design document or introduce a concrete changed line that diverges from a locked decision.
Compliant — the temporary shapes are bounded and carry dissolution triggers: the test header says the same-path hand-Rust harness retires when generated 2.5. Top-down PM intent reviewFinding — PM intent is diluted by the same 3. VerdictREQUEST_CHANGES The |
…form-agnostic) (openai-pro RC #4284) Valid openai-pro REQUEST_CHANGES (P3 fail-open): the scanner filtered `if !line.contains("bash ") { continue; }` BEFORE checking `.github/ci-floor/`, so a required floor step spelled `sh .github/ci-floor/new.sh`, `./.github/ci-floor/new.sh`, or `source .github/ci-floor/new.sh` would be skipped — never canonicalized, never in the live set, never compared to the table (fail-open). Fix: drop the `bash ` pre-filter; key on the `.github/ci-floor/` PATH for every non-comment line. Any ci-floor reference is treated as a live invocation, canonicalized, and must yield a modeled `.sh` (else the existing fail-closed assert trips). Now bash, sh, ./, source, and bare spellings are all caught. ci.dag contract note updated (shell-form-agnostic). Verified no false trips: ci.yml references ci-floor only in the two real `.sh` invocations. Required `::v4_workflow_ci_wave1_` filter runs 3 tests incl. the ratchet; full 38/38. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Valid finding — fixed in 6259d49. The scanner filtered Fix: dropped the Note on scope: this string-scan ratchet is explicitly the transitional first slice; its fundamental limit (it parses ci.yml text rather than consuming a parsed |
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
62e1b134· Trigger:schedule - Thinking:
133s wall
|
On superseded SHA 62e1b13; "no additional new blocking concerns." The three re-listed comment IDs (3338742904 / 3338742912 / 3339441780) are all resolved: the path/ROADMAP ones were verified false (no |
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
6259d49c· Trigger:schedule - Thinking:
224s wall
1 participant
Summary
First executable slice of Wave 2 (shell→
CiUpsertStepmigration). Authority: #4137 §11.7.5 "The temporary CI cut rule (codified)" — a shell-owned check may remain REQUIRED only if it (1) protects a named §11.7.1 safety-floor item, (2) has no runtime-authoritativeCiUpsertStepreplacement yet, (3) is listed in the temporary shell exception table, (4) names a structural dissolution path.This models that table in
src/v4/workflow/ci.dagand gives the no-new-shell ratchet (§11.7.1 #5) something to consume.src/v4/workflow/ci.dagCiSafetyFloorItem— the §11.7.1 bare-minimum floor enum a shell exception protects.CiShellExceptionRow— reuses existingCiStepId+DissolutionTarget(no parallel exception genus; sibling toci_always_run_carveouts, a distinct concept — shell retirement vs always-run run-policy).ci_class_a_shell_exceptions— exactly two rows: bootstrap viability (v4-bootstrap-viability.sh) and M1 rust emit probe (v4-m1-rust-emit-probe.sh), each citing the floor item it protects and a structuraldissolution_target.replacement_upsertbinds the canonicalCiUpsertStepSymbolauthority (ci_upsert_v2_bootstrap_smoke_execution/ci_upsert_m1_rust_emit_probe_execution), not a parallelSymbolalias (P2 single-authority).Executable slice —
v4_workflow_ci_class_a_shell_exception_table_first_sliceshell_owneris a live required ci.yml floor invocation;ci-floor/*.shinvocation in ci.yml must carry a row here;replacement_upsertreferences a declaredCiUpsertStepSymbolauthority.Does not touch
.github/workflows/ci.ymlor the pinned carrier — model + test only.P5 receipt (v3 hand-Rust gate — stated in PR body per the required authority surface)
Lane: ROADMAP.md § "Nine lanes" row T-PB-B /
pb_rust_tests_outside_residual_zero.Checkable receipt — same-path SG-0 expansion, +0 new paths: this PR adds a test function to the existing
src/v3/compiler/tests/integration/v4_workflow_ci_runner_dag_smoke_test.rsharness; it adds no new.rsfile path and no new authority surface. The SG-0 census is path-denominated (a.rsfile counts by its workspace-relative path membership, persg0_census_test.rs), so the census count is unchanged and there is no census entry to add/remove. Same posture as the T-38/T-38B same-path expansions already documented in this file's header.Dissolution trigger: generated
.dagTestClaimexecution (A15 Shape-B emittedci.yml+ modeled receipt) covers the §11.7.5 shell-exception table and the no-new-shell ratchet, retiring this hand-Rust parse/string harness.Test plan
CTRL_BUILD_WRAP_CARGO=0 cargo test -p v3-compiler v4_workflow_ci_runner_dag_smoke_test— 38/38 passedcargo fmtclean (pre-push hook did not abort).dagcomments (matches file style)🤖 Generated with Claude Code