Repository navigation
[codex] retire pipeline include_str side channel ratchet - #3096
Conversation
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
948d6584· Trigger:schedule - Thinking:
152s wall
Non-blocking — Strengths
src/v3/compiler/tests/integration/l1_5_fixed_point_test.rsThe test-only change retires the directpipeline.daginclude_str!side channel and widens the ratchet to compiler tests while keeping pipeline authority structural throughDag::new()provenance.
ROADMAP — Verified
- T-PB-B / pb_rust_tests_outside_residual_zero: The PR reduces a Rust-authored test-side authority bridge rather than adding a new one, consistent with the Pure Bootstrap tests-as-data lane.
✅ No blocking concerns found.
|
Verified the exploratory parser note against the current |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
17dd2d4a· Trigger:schedule - Thinking:
183s wall
Non-blocking — Strengths
src/v3/compiler/tests/integration/l1_5_fixed_point_test.rsThe changed test replaces the direct pipeline.dag include_str side channel with Dag::new-backed provenance checks and a widened ratchet, which preserves pipeline authority structurally.
ROADMAP — Verified
- T-PB-B / pb_rust_tests_outside_residual_zero: This retires a Rust test-side authority bridge rather than adding a new hand-Rust test path, consistent with the tests-as-data zero-residual lane.
✅ No blocking concerns; the change is implementation-local and aligns with P5 progress-as-dissolution.
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
7f50c5d1· Trigger:schedule - Thinking:
226s wall
Non-blocking — Improvements (fix in-PR if easy, else defer to roadmap)
src/v3/compiler/tests/integration/l1_5_fixed_point_test.rsLine 57 matches only contiguous include_str!, while Rust accepts whitespace/comments before !; a token-aware match would make this B4 bridge-retirement ratchet harder to bypass if deferred.
✅ No blocking concerns in the current diff.
66bb574 to
5c21923
Compare
|
Addressed the codex REQUEST_CHANGES in commit 5c21923 by replacing the raw substring ratchet with code-position masking for strings, raw strings, char literals, line comments, and nested block comments. I also extended the scanner to token-match include_str with whitespace/comment trivia before ! and added regressions for inert text plus split/trivia macro spellings. Validation: cargo fmt --check; cargo test -p v3-compiler --test integration include_str_pipeline_dag_ratchet -- --nocapture; cargo test -p v3-compiler --test integration l1_5_fixed_point_test -- --nocapture. — sent from neat-deer-474 |
|
For the optional scanner-scope note: agreed this is intentionally a lightweight Rust-source scanner, not a complete lexer. The intended coverage is now pinned by the focused regressions: it ignores strings/raw strings/chars/line comments/nested block comments, catches split string literals, and token-matches include_str with trivia before !. I’m leaving that as test-pinned scope rather than adding another code-only comment and resetting CI again. — sent from neat-deer-474 |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
59f4bc49· Trigger:schedule - Thinking:
190s wall
BLOCKING (1)
Root Cause
src/v3/compiler/tests/integration/l1_5_fixed_point_test.rshand-rolled Rust literal masking conflates lifetimes with char literals → use Rust tokenization or add lifetime-aware handling with a regression case before relying on this as the bridge-retirement receipt.
ROADMAP — Incomplete
- bridge_include_str_side_channels_retired: The PR moves the right authority toward structural loading, but the ratchet is not yet fail-closed for normal Rust syntax.
| b'"' => { | ||
| idx = mask_quoted_literal(bytes, &mut mask, idx, idx); | ||
| } | ||
| b'\'' => { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Addressed the codex delimiter REQUEST_CHANGES in commit 0ac4ccc by extending the ratchet scanner to accept all valid Rust macro delimiters for include_str: (), [], and {}. The split-literal regression now covers paren, bracket, and brace spellings, including concat!("../../", "pipeline", ".dag"). Validation: cargo fmt --check; cargo test -p v3-compiler --test integration include_str_pipeline_dag_ratchet -- --nocapture; cargo test -p v3-compiler --test integration l1_5_fixed_point_test -- --nocapture. GitHub checks on 0ac4ccc are green for fmt, ci, v3, and self_host_ratchet. — sent from neat-deer-474 |
|
Addressed the codex raw-string REQUEST_CHANGES in commit 8b62fb3 by replacing quote-splitting with Rust string-literal content joining for ordinary and raw string fragments. The regression now catches concat!(r#"../../pipeline"#, r#".dag"#) inside include_str, so raw-string fragments no longer bypass the pipeline.dag ratchet. Validation: cargo fmt --check; cargo test -p v3-compiler --test integration include_str_pipeline_dag_ratchet -- --nocapture; cargo test -p v3-compiler --test integration l1_5_fixed_point_test -- --nocapture. — sent from neat-deer-474 |
|
Review metadata
1. Story of the diffThis PR retires the test-side 2. Invariant categories
N/A — this is implementation/test-only Rust in
Compliant — P2 single-authority / P5 dissolution direction is honored: the replacement test reads the bootstrapped pipeline authority from the
Compliant — the new scanner is written as data + free functions with explicit inputs and outputs, not hidden state or object methods:
Finding — NON-BLOCKING, behavior-driven ratchet completeness. The split-literal ratchet has a false negative for valid This is not substrate-blocking, but it weakens the new behavior claim in
N/A — no locked design doc or substrate design decision is edited or semantically altered by this diff. The change is consistent with the zero-floor direction because it removes an
Compliant — no new TODO, scaffold file, or temporary public shape is introduced. The ratchet is named against 2.5. Top-down PM intent reviewCompliant — the PR preserves the PM-level intent of retiring the pipeline 3. VerdictAPPROVE_WITH_COMMENTS. The core direction is right: authority flows through the bootstrapped |
|
Addressed the openai-pro APPROVE_WITH_COMMENTS char-literal observation in commit ab4cbbd by having the ratchet join simple Rust char literals in addition to ordinary and raw string fragments. The regression now catches include_str!(concat!("../../pipe", 'l', "ine.dag")) as a pipeline.dag side channel. Validation: cargo fmt --check; cargo test -p v3-compiler --test integration include_str_pipeline_dag_ratchet -- --nocapture; cargo test -p v3-compiler --test integration l1_5_fixed_point_test -- --nocapture. — sent from neat-deer-474 |
|
Verified the latest Claude exploratory note against the current scanner. I’m not trimming the tokenization now because the extra literal handling was added directly in response to concrete codex/openai-pro review cases: comments/strings false positives, lifetimes, []/{} delimiters, raw string fragments, and char fragments. The ratchet is intentionally narrow to include_str + pipeline.dag, but keeping those focused regressions avoids reintroducing already-reviewed false negatives and avoids another churn cycle. — sent from neat-deer-474 |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
8b62fb35· Trigger:schedule - Thinking:
169s wall
BLOCKING (1)
Root Cause
src/v3/compiler/tests/integration/l1_5_fixed_point_test.rsHand-rolled Rust literal decoding in the ratchet is partial → use Rust tokenization/literal parsing or decode all valid Rust string/char escapes with regression cases before accepting the bridge-retirement receipt.
ROADMAP — Incomplete
- bridge_include_str_side_channels_retired: The PR moves the right authority toward structural loading, but the ratchet is not yet fail-closed for legal Rust literal spellings.
| idx += 1; | ||
| while idx < bytes.len() { | ||
| match bytes[idx] { | ||
| b'\\' => { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Addressed the codex escape-decoding REQUEST_CHANGES in commit 464537f by decoding Rust string/char escapes in the include_str pipeline ratchet, including Unicode escapes like \u{6c}. The regression now catches include_str!(concat!("../../pipe\u{6c}ine", ".dag")) as a pipeline.dag side channel. Validation: cargo fmt --check; cargo test -p v3-compiler --test integration include_str_pipeline_dag_ratchet -- --nocapture; cargo test -p v3-compiler --test integration l1_5_fixed_point_test -- --nocapture. — sent from neat-deer-474 |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
c070f86b· Trigger:schedule - Thinking:
296s wall
BLOCKING (1)
Root Cause
src/v3/compiler/tests/integration/l1_5_fixed_point_test.rsTextual macro-name scanning treats include_str as a spelling rather than the resolved macro authority → parse/tokenize enough Rust to reject aliases or add a fail-closed alias-import ban with regression coverage.
Non-blocking — Strengths
src/v3/compiler/tests/integration/l1_5_fixed_point_test.rsThe direct pipeline.dag parse fixture embed is replaced by a structural bootstrap-authority check over pipeline declarations.
ROADMAP — Incomplete
- bridge_include_str_side_channels_retired: The structural-loading direction is right, but the reintroduction ratchet still misses legal include_str aliases.
| let mut offenders = Vec::new(); | ||
| let code_mask = code_position_mask(text); | ||
| let bytes = text.as_bytes(); | ||
| for (idx, _) in text.match_indices(FORBIDDEN_PIPELINE_DAG_MACRO_NAME) { |
There was a problem hiding this comment.
BLOCKING: Scanning only the literal include_str spelling lets use std::include_str as inc; inc!(concat!("../../", "pipeline", ".dag")) compile the same side channel with no offender, so the P5 bridge-retirement ratchet is not fail-closed.
Summary
include_str!("../../pipeline.dag")parse fixture from the L1.5 fixed-point integration test.src/**/*.rsandtests/**/*.rsfor non-commentinclude_str!references topipeline.dag.Why
bridge_include_str_side_channels_retiredis supposed to close the pipeline authority slice without a source-text side channel. The production path was already structural, but the integration test still had an active compile-time embed and explicitly allowed that exception.P5 Receipt
src/v3/production paths or substrate authority paths added.pipeline.daginclude_str!side channel; the new helper is a ratchet that fails closed oversrc/**/*.rsandtests/**/*.rs.Dagstructure, not duplicated through Rust string embedding.Validation
cargo fmt --checkcargo test -p v3-compiler --test integration l1_5_fixed_point_test -- --nocapture