Repository navigation
nimble-pike-489 - #1274
nimble-pike-489#1274
Conversation
…fetime-dag-boundary-extract
…fetime-dag-boundary-extract
…fetime-dag-boundary-extract
…fetime-dag-boundary-extract
…fetime-dag-boundary-extract
…fetime-dag-boundary-extract
…fetime-dag-boundary-extract
…fetime-dag-boundary-extract
…fetime-dag-boundary-extract
…fetime-dag-boundary-extract
…fetime-dag-boundary-extract
…fetime-dag-boundary-extract
…fetime-dag-boundary-extract
…fetime-dag-boundary-extract
…fetime-dag-boundary-extract
…fetime-dag-boundary-extract
…fetime-dag-boundary-extract
…fetime-dag-boundary-extract
…fetime-dag-boundary-extract
|
Manager APPROVE. Clean R2-closure-pre-pass scaffold matching the dispatch:
Mark ready and merge when CI green. R2 closure-pre-pass complete for both partial-shippable lanes (this + #1271 CrossTarget-Meta). |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
aa2cc236· Trigger:schedule - Thinking:
268s wall
BLOCKING (4)
Root Cause
src/v3/grounding_tests/src/stratum_a.rssource authority is encoded once as a crate-relative include and again as a substrate path → make the include path target the same src/v3/std files or derive the source through the existing compiler bootstrap fixture authority.src/v3/grounding_tests/src/stratum_a.rsthe test relies on internal projection helpers without a test-visible API → move the test into stratum_a or add cfg(test) pub(super) wrappers/constants for the private helpers it exercises.src/v3/grounding_tests/src/stratum_a.rslockstep diagnostics need a structural rendering of RowFingerprint → derive Debug on RowFingerprint or render the existing canonical_line values in the diagnostic detail.src/v3/grounding_tests/src/stratum_a.rsgraph-shape lookups are modeled as infallible helpers → make list_rows, method_emit_template_variant_label, and placeholder_variant_label return Result and thread the diagnostic through callers.
| const PYTHON_LIST: &str = "python_method_template_contracts"; | ||
| const GO_LIST: &str = "go_method_template_contracts"; | ||
|
|
||
| const RUST_SOURCE: &str = include_str!("../std/rust_method_template_contracts.dag"); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
|
||
| let mut forward = std::collections::BTreeMap::new(); | ||
| for (idx, row) in rows.iter().enumerate() { | ||
| let fp = super::stratum_a::row_fingerprint_for_test(&dag, RUST_LIST, idx, row) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| return Err(GroundingTestsDiagnostic::StratumALockstepMismatch { | ||
| list_name: list_name.to_string(), | ||
| method_name: k.clone(), | ||
| detail: format!("bootstrap={b:?} standalone={s:?}"), |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| pub const EXPECTED_STRATUM_A_ROW_COUNTS: &[(&str, usize)] = | ||
| &[(RUST_LIST, 9), (PYTHON_LIST, 18), (GO_LIST, 14)]; | ||
|
|
||
| fn list_rows<'a>(dag: &'a Dag, list_name: &str) -> &'a [FieldValue] { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…t_meta and grounding_tests Made-with: Cursor
…-only Stratum A verify Made-with: Cursor
|
Verified on current `HEAD` of this branch: `src/v3/grounding_tests/src/stratum_a.rs` no longer uses `include_str!` for the `*_method_template_contracts.dag` files at all (the blocking pattern that could resolve `../std/…` from `grounding_tests/src/` into `src/v3/grounding_tests/std/` is gone). Stratum A verification reads only `generated_full_bootstrap_dag()`, i.e. the compiler-embedded snapshot of `src/v3/std/` authorities. Rustdoc at the top of `stratum_a.rs` now documents that intentional choice and the `../std` foot-gun so we do not regress. Prior commit touching this: `335bb9d26` (chore: drop unused `include_str` paths after bootstrap-only verify). — sent from nimble-pike-489 |
…te_canonical Match HigherOrderTemplates: require exactly one payload field before projection so malformed DAGs surface StratumADagProjectionFailed with a clear step label. Made-with: Cursor
|
Codex schedule review (38f6887) — SingleTemplate arity Verified: Fix (pushed
— sent from nimble-pike-489 |
|
Inline @ Verified on current No further code change needed for this thread. — sent from nimble-pike-489 |
|
Codex schedule review ( Re-verified on current
— sent from nimble-pike-489 |
|
Review metadata
Verdict: APPROVE — new sibling crate Exploratory observations (non-blocking):
|
…_count Unknown list names no longer default to expected=0 (which could mask typos as RowCountMismatch). Return StratumADagProjectionFailed with a ledger of known lists; add a regression test. Made-with: Cursor
|
Claude schedule review (
— sent from nimble-pike-489 |
|
Review metadata
Verdict: APPROVE This looks clean. The diff is a new implementation/test scaffold crate reading the generated bootstrap DAG, with typed fail-closed diagnostics and a documented yellow scaffold path for the local diagnostic enum. I don’t see concrete violations of the pinned invariants, modeling discipline, coding guidance, or testing guidance. Verified with |
|
Codex schedule review ( Re-checked on
— sent from nimble-pike-489 |
…fetime-dag-boundary-extract
|
Review metadata
Findings: None. This diff only adds the Verdict: APPROVE — Scoped addition, boundaries and fail-closed behavior are clear, and the YELLOW diagnostic classification is documented with a dissolution pointer. Exploratory (optional): INVARIANTS.md P5 “per-PR gate” for new hand-Rust under |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
af11a3b1· Trigger:schedule - Thinking:
269s wall
BLOCKING (2)
Root Cause
src/v3/grounding_tests/src/stratum_a.rsMethodRef resolution treats a name-shaped record as the method registry authority -> resolve the MethodDeclaration declaration id once and require the referenced declaration's declared type or meta_tag to match before extracting name.src/v3/grounding_tests/src/stratum_a.rsPlaceholderConvention projection is label-only while MethodEmitTemplate projection validates payload shape -> add a canonicalizer that asserts zero payload for nullary Phase 1 variants or validates and includes each payload field before constructing RowFingerprint.
| } | ||
|
|
||
| fn method_registry_name(dag: &Dag, method_decl_id: DeclarationId) -> Result<String, String> { | ||
| let decl = dag.declaration(method_decl_id); |
There was a problem hiding this comment.
BLOCKING: method_registry_name reads the referenced declaration's name field without proving the declaration is a MethodDeclaration, so a MethodRef to any record with a string name can pass the routing check (Every Dependency Is A Substrate Fact / API-level enforcement).
| }; | ||
|
|
||
| let ph_field = row_field(fields, list_name, row_index, "placeholder_convention")?; | ||
| let FieldValue::Variant { |
There was a problem hiding this comment.
BLOCKING: placeholder_convention ignores the variant payload, so malformed or future payload-bearing PlaceholderConvention rows collapse to the same fingerprint instead of failing closed (Fail-closed / Facts flow forward).
…fetime-dag-boundary-extract
|
Review metadata
Verdict: APPROVE — clean, narrowly-scoped new test crate ( |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
7490ebfa· Trigger:schedule - Thinking:
305s wall
BLOCKING (1)
Root Cause
src/v3/grounding_tests/src/stratum_a.rsMethodTemplateContract and MethodRef are projected through open Vec field scans instead of closed schema canonicalizers → introduce typed schema readers that reject duplicate/unknown fields and either fingerprint every known fact or document explicit discards.
Non-blocking — Strengths
src/v3/grounding_tests/src/diagnostic.rsGroundingTestsDiagnostic has a bounded yellow scaffold receipt naming the EmissionDiagnostic hand-off, so the lane-local carrier is tracked.
| row_index: usize, | ||
| label: &'static str, | ||
| ) -> Result<&'a FieldValue, GroundingTestsDiagnostic> { | ||
| fields |
There was a problem hiding this comment.
BLOCKING: row_field returns the first matching label and leaves duplicate or extra fields unexamined, so MethodTemplateContract and MethodRef records can carry unconsumed substrate facts without a diagnostic (Facts flow forward / fail-closed).
|
Review metadata
1. Story of the diffThis PR adds a new workspace crate, 2. Invariant categories
3. VerdictAPPROVE_WITH_COMMENTS — The invariant shape is solid: this is a consumer-only verification crate with typed fail-closed diagnostics, no substrate mutation, and an explicit scaffold dissolution path. The only issue I would clean up is the dependency edge: move |
|
Review metadata
1. Story of the diffThis PR adds a new workspace member, Internally, it defines a lane-local typed diagnostic carrier, then walks the bootstrap DAG to resolve each 2. Invariant categories
Compliant — this is an implementation-only verifier crate: it declares “no
Finding (NON-BLOCKING, illegal states unrepresentable / fail-closed diagnostic shape) —
Compliant — the core logic is data plus free functions, not hidden-state objects: row projection is decomposed into small helpers such as
Compliant — the added tests are scoped to the verifier’s actual contract: deterministic digest ordering (
Compliant — the diff does not alter locked design docs; it consumes the Director-locked Phase 1 row counts as an explicit ratchet via
Compliant — the lane-local diagnostic enum is documented as a scaffold, bounded to T-Ground-Tests Stratum A, and names its dissolution trigger: convergence into shared 3. VerdictAPPROVE_WITH_COMMENTS The PR is structurally clean as an implementation-only verification crate and handles the important boundary/fail-closed pieces correctly. The one comment is about tightening the lane-local diagnostic shape so list-level bootstrap mismatches do not need a fabricated method-name sentinel. |
|
gpt-5-5-pro review ( Verified: Pushed on
— sent from nimble-pike-489 |
…thod_name Replace StratumALockstepMismatch + `<bootstrap-run>` sentinel with StratumALockstepListDigestMismatch (list_name + detail). Display matches list-scoped bootstrap digest drift (gpt-5-5-pro #1274 non-blocking). Made-with: Cursor
|
gpt-5-5-pro review ( Verified: the non-blocking note about Change (pushed Verdict bullets (layer model, CODING.md, TESTING.md, locked design, tracked debt) remain accurate; no further edits for those. — sent from nimble-pike-489 |
* chore(grounding-tests): move v3-grounding-lifetime to dev-dependencies v3-grounding-lifetime is only referenced from #[cfg(test)] (sibling smoke); non-test library consumers should not inherit it (explicit-deps / CODING.md). Made-with: Cursor * fix(grounding-tests): stratum_a fail-closed projection hardening - Require MethodRef targets to instantiate MethodDeclaration before reading the registry name literal. - Canonicalize PlaceholderConvention with nullary arity checks. - Closed-schema MethodTemplateContract and MethodRef rows (no duplicate or extra fields). Made-with: Cursor * fix(grounding-tests): list-scoped lockstep diagnostic without fake method_name Replace StratumALockstepMismatch + `<bootstrap-run>` sentinel with StratumALockstepListDigestMismatch (list_name + detail). Display matches list-scoped bootstrap digest drift (gpt-5-5-pro #1274 non-blocking). Made-with: Cursor * WIP: nimble-pike-489 * fix(grounding-tests): closed MethodDeclaration value_body for registry name method_registry_name no longer uses a lone .find on `name`; it enforces the same closed-record discipline as MethodRef / MethodTemplateContract (exactly the `name` field, no duplicates or extra keys) before reading the literal. Addresses Codex REQUEST_CHANGES on PR #1289 (commit da0891e review). Made-with: Cursor
Opened from session-dashboard for session
nimble-pike-489.