Repository navigation
T-38: reject negated corpus tally predicates - #4180
Conversation
|
Verified against the current PR body on #4180: this is already addressed without a code/planning-file change. The body has exactly one P5 Disposition section stating that this extends the existing |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
898dd16a· Trigger:schedule - Thinking:
280s wall
BLOCKING (1)
Root Cause
scripts/v4-testclaim-corpus-eval.shstring-regex receipt treats a positive zero-comparison substring as sufficient evidence instead of validating the enclosing Bool expression → parse the generated manual_corpus_all_pass body or reject symmetric false-equality wrappers before accepting the tally conjunction
| "generated tally source for zero Fail/Deferred predicate", | ||
| ) | ||
| normalized_all_pass = "".join(all_pass_body.split()) | ||
| inverted_zero_comparison = re.compile( |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Investigated the CI failure on head |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
93f55d42· Trigger:schedule - Thinking:
111s wall
BLOCKING (1)
Root Cause
scripts/v4-testclaim-corpus-eval.shstring-level blacklist of selected inverted forms → validate the enclosing Bool expression or require an exact top-level tally.fail == Zero && tally.deferred == Zero shape before accepting the receipt
| ) | ||
| normalized_all_pass = "".join(all_pass_body.split()) | ||
| inverted_zero_comparison = re.compile( | ||
| r"(?<![A-Za-z0-9_:])(?:!\(*|\(*false\)*={2}\(*)" |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Verified against current #4180 head 503cfbc and the current PR body: this P5 receipt already exists without needing a code/planning-file change. The body has exactly one P5 Disposition section stating that this extends the existing hand-written Rust integration test src/v3/compiler/tests/integration/v4_workflow_ci_runner_dag_smoke_test.rs without adding a new SG-0 census path, and it names the concrete dissolution row: src/v4/TASKS.md T-38 plus ROADMAP.md Milestone shape item 3 for direct .dag / TestClaimRun execution replacing this Rust ratchet. — sent from neat-hawk-413 |
|
Investigated the CI failure on head 503cfbc. fmt and affected pass; ci fails only because ci_floor fails closed. The failing ci_floor step is M1 v4 full-tree rust emit probe: v2 compile succeeds with compiled: 351 files emitted, 0 diagnostics, then emitted Rust cargo check exits 101 on the known full-tree generated-Rust debt class (TestClaimRun missing generics, FreeMonoid dropck, nat_cata move, etc.). I compared current PR run 26740397675 against recent main ci run 26734801058. Both show the same 26 rustc error classes; the only count drift is in existing E0432/E0433 import-resolution lines. This PR diff only changes scripts/v4-testclaim-corpus-eval.sh and src/v3/compiler/tests/integration/v4_workflow_ci_runner_dag_smoke_test.rs, with no src/v4 or emitter changes, so there is no scoped fix to push here. — sent from neat-hawk-413 |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
503cfbcb· Trigger:schedule - Thinking:
207s wall
BLOCKING (1)
Root Cause
scripts/v4-testclaim-corpus-eval.shstring-level receipt accepts an occurrence of the predicate instead of the returned Bool expression → parse/validate the function body AST or require the exact final returned expression to be the zero Fail && zero Deferred conjunction
| r"tally\.fail={2}[^&|;=!A-Za-z0-9_:]*(?:Nat::)?[Zz]ero\b[^&|;=]*&&" | ||
| r"[^&|;]*tally\.deferred={2}[^&|;=!A-Za-z0-9_:]*(?:Nat::)?[Zz]ero\b[^&|;=]*(?:;|\})" | ||
| r"(?:^|;)\(*tally\.fail={2}[^&|;=!A-Za-z0-9_:]*(?:Nat::)?[Zz]ero\b\)*&&" | ||
| r"\(*tally\.deferred={2}[^&|;=!A-Za-z0-9_:]*(?:Nat::)?[Zz]ero\b\)*(?:;|\})" |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Investigated the CI failure on head 0d5a3b8. fmt and affected pass; ci fails only because ci_floor fails closed. The failing ci_floor step is still M1 v4 full-tree rust emit probe: v2 compile succeeds with compiled: 351 files emitted, 0 diagnostics, then emitted Rust cargo check exits 101 on the existing full-tree generated-Rust debt class (TestClaimRun missing generics, FreeMonoid dropck, nat_cata move, etc.). I compared current PR run 26744865142 against recent main ci run 26734801058. Both show the same 26 rustc error classes; this PR run has fewer E0432/E0433 import-resolution lines than main and no new error classes. This PR diff only changes scripts/v4-testclaim-corpus-eval.sh and src/v3/compiler/tests/integration/v4_workflow_ci_runner_dag_smoke_test.rs, with no src/v4 or emitter changes, so there is no scoped fix to push here. — sent from neat-hawk-413 |
|
Review metadata
1. Story of the diffThis PR tightens the T-38 generated corpus-eval receipt so 2. Invariant categories
N/A — the diff is implementation/test receipt logic in
Finding — Because that
Compliant — aside from the blocking regex boundary above, the added script code keeps the existing free-function style, uses a named local regex plus a typed
Finding —
N/A — the diff does not alter a locked design document or semantic design decision; it stays within the existing T-38 receipt-checking surface.
N/A — no new TODO, scaffold, bridge, or temporary lane is introduced. The issue above is not tracked debt; it is a fail-open receipt bug in the changed code. 2.5. Top-down PM intent reviewFinding — 3. VerdictREQUEST_CHANGES. The PR is pointed in the right direction and the explicit inversion guard is useful, but the tightened conjunction regex still fails open on an inner-block/non-returned conjunction. Please anchor the receipt to the actual returned tail and add the nested-block negative regression before merge. |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
61e83ebf· Trigger:schedule - Thinking:
278s wall
✅ The changed receipt closes the previously flagged bypasses and I found no new blocking concerns.
|
Review metadata
1. Story of the diffThis PR tightens the T-38 generated corpus-eval receipt around 2. Invariant categories
N/A — diff is implementation/test receipt logic in a shell/Python script and Rust integration test; it does not alter
Finding — fail-closed / early detection:
Compliant — the changed implementation stays file-scoped and explicit: local compiled regexes,
Finding — regression misses the bypass shape:
N/A — no locked design document or locked substrate/design decision is changed; the diff is a receipt-gate implementation change.
N/A — no new TODO, scaffold, temporary bridge, or deferred marker is introduced. 2.5. Top-down PM intent reviewCompliant with the intended direction, but blocked by the implementation gap above. The PR is semantically aimed at preserving the T-38 gate by making generated corpus evaluation fail closed when 3. VerdictREQUEST_CHANGES The PR is focused and mostly shaped correctly, but the receipt can still accept a function that returns an inverted tally predicate before an unreachable positive tail. That is exactly the class of failure this PR is supposed to close, so it needs a fail-closed fix plus a regression for the early-return bypass before merge. |
|
Investigated the CI failure on #4180 head 4605e93. The completed failed jobs are ci_floor plus the aggregate ci job that fails closed because ci_floor failed. fmt and affected pass. The failing ci_floor step is still M1 v4 full-tree rust emit probe: v2 compile succeeds with |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
4605e939· Trigger:schedule - Thinking:
188s wall
Non-blocking — Strengths
scripts/v4-testclaim-corpus-eval.shThe PR is implementation-only and keeps the receipt fail-closed against the previously demonstrated negation and tail-expression bypasses.
✅ No blocking concerns found.
|
Review metadata
1. Story of the diffThis PR tightens the T-38 generated corpus-eval receipt so chatgpt-review-be5d4169-01d2-43… 2. Invariant categories
No predicate/walker dissolution trigger applies because this is not a substrate/
chatgpt-review-be5d4169-01d2-43… Although the long-term direction is
2.5. Top-down PM intent reviewCompliant — the PM-level intent is preserved. The highest-level direction says tests and CI/build orchestration should be structurally modeled, with Rust surfaces dissolving over the zero-floor trajectory, while current migration guidance still permits Rust integration tests until the hand-authored surface reaches zero. chatgpt-review-a70b79fe-9a33-42… chatgpt-review-eef43eb7-cfac-43… This PR does not dilute that plan into permanent Rust test authority; it tightens a T-38 receipt so generated corpus evaluation cannot pass through an earlier positive expression, explicit return bypass, or negated tally predicate. chatgpt-review-be5d4169-01d2-43… 3. VerdictAPPROVE No blocking findings. The PR narrows the receipt in the fail-closed direction, adds focused regression cases for the bypasses it closes, and does not introduce substrate changes, design drift, or untracked debt. |
Summary
Follow-up to #4159: the generated corpus-eval receipt now rejects unary-negated zero comparisons such as
!(tally.fail==Zero)and!(tally.deferred==Zero). The existing conjunction probe still requires zero Fail and zero Deferred, but an explicitinverted_zero_comparisonguard now fails closed before accepting the receipt.The T-38 smoke regression now exercises direct, parenthesized, and double-parenthesized negation on both tally fields.
P5 Disposition
Explicit deferral receipt: this PR extends the existing hand-written Rust integration test
src/v3/compiler/tests/integration/v4_workflow_ci_runner_dag_smoke_test.rswithout adding a new SG-0 census path. It is a bounded T-38/T-22 receipt hardening patch forscripts/v4-testclaim-corpus-eval.sh.Concrete dissolution row:
src/v4/TASKS.md§ T-38 — TestClaim execution harness (T-38 closes whenitems 1-3: CI runs T-22 eval oversrc/v4/test/claim/manual/*.dag, results surface asTestClaimRunverdicts/workflow data, and the shell corpus gate is deleted). This also aligns withROADMAP.mdMilestone shape item 3, where structuralTestClaimpredicates are evaluated by generated or substrate runners. When that T-38 row lands, this Rust parse/string ratchet should be deleted in favor of direct.dagTestClaimRunverdict execution.Test plan
bash -n scripts/v4-testclaim-corpus-eval.shrustfmt --check src/v3/compiler/tests/integration/v4_workflow_ci_runner_dag_smoke_test.rstally.fail == Zeroandtally.deferred == Zeroctrl-build -- /opt/cargo/bin/cargo test -p v3-compiler --test integration v4_workflow_ci_runner_dag_smoke_test::v4_workflow_ci_t38_script -- --quiet