Repository navigation
feat: CX reads callback_element_position from algebra template (S7) - #414
Conversation
ChatGPT ReviewVerdict: approve, with one non-blocking follow-up. Grounded in the thesis, this is the right direction. The project’s bar is “is what you said sound?” and not “can I make it execute,” with termination/cost facts meant to fall out of the model rather than from downstream guesswork. The diff moves From an invariants perspective, I do not see a new violation in this PR. The repo’s own guidance says the fix belongs at the deepest missing fact, not as a downstream compensation, and that heuristics are evidence of lost structure rather than something to tune locally. This change is a root-cause fix in that sense: the fact already exists upstream in the algebra template, and complexity now consumes it instead of reconstructing it. The Roadmap alignment looks good. My one concern is that the heuristic is reduced, not fully eliminated. In the diff, The other thing I would have liked is an explicit regression test tied to this contract. There are already CX tests around fold-over-children lambdas, including an ignored So my review is: good change, thesis-aligned, no blocker found. The only nit is that |
briansrls
left a comment
There was a problem hiding this comment.
codex · gpt-5.4 · a5e1ff21
Review (INVARIANTS: 1, MODELING: 1+/1-, ROADMAP: 0✓/1!)
INVARIANTS — Violations (1)
ROOT CAUSE ANALYSIS
src/v2/complexity.dagsrc/v2/00_core.dagandsrc/v2/04_lookup.dagstill model the callback contract as optionalalgebra_template, so this new consumer compensates by guessing the lambda element slot; that guess then propagates to vacuous success at2289/2682/3223(and toparams.get(pos as usize)in stage0) when the fact is missing or malformed, which weakens CX instead of surfacing the broken contract. The upstream fix is to make callback-bearing algebra methods carry a validated callback witness from lookup, ideally reusingdsl/std/types.dagNonNegativeIntfor the index or a dedicatedMethodSemanticsvariant, and reject iteration methods that lack that witness so CX reads a total structural fact fromdsl/std/algebra.dag(M5/M9).
MODELING — Strengths
src/v2/complexity.dagReading the callback element slot fromAlgebraFieldTemplate.callback_element_positionis the right compositional direction because it reusesdsl/std/algebra.dagas the single authority instead of re-encoding fold/map conventions in CX.
MODELING — Improvements
src/v2/complexity.dagMake the callback contract a total structural part ofMethodSemanticswith a validated non-negative index, so the compiler layer consumes a proof-carrying fact from std rather than an optionalIntplus fallback.
ROADMAP — Incomplete
- Step 2: Lambda param provenance from callee contracts: The PR switches three complexity consumers to
callback_element_position, but the consumer still falls back tolastor vacuousNone, so S7 consumption is not yet fail-closed.
This PR moves CX closer to the std/algebra.dag single authority, but it still misses the thesis bar because missing or invalid callback contracts are guessed away instead of rejected.
| let params = lambda_param_names_at(texpr: lambda, source_indices: si) | ||
| match method_callback_element_position(method_semantics: method_semantics) { | ||
| Some { value: pos } => params |> skip(pos) |> first | ||
| None => params |> last |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Review (INVARIANTS: 1, MODELING: 1+/1-, ROADMAP: 0✓/1!) INVARIANTS — Violations (1) ROOT CAUSE ANALYSIS
MODELING — Strengths
MODELING — Improvements
ROADMAP — Incomplete
This PR moves CX closer to the |
ChatGPT ReviewVerdict: approve, with one non-blocking follow-up. This is thesis-aligned. gunbc’s bar is that soundness facts should be declared structurally so correctness falls out of the model, rather than being reconstructed by downstream convention. In the patch, chatgpt-review-7bb2a661-5504-4f… I do not see a new invariant violation in the diff. This looks like a root-cause fix, not a downstream compensation: the authority already exists upstream in the algebra template, and the PR makes CX consume that authority instead of re-deriving it locally. That matches the modeling rule that data tables stay single-authority and dispatch stays structural, and it matches the roadmap’s M1 Step 2 description almost exactly: S7 declared chatgpt-review-11fbedc7-592f-4d… chatgpt-review-74956c3d-91a0-46… My only concern is that the new behavior is fail-safe, but not fully fail-loud. In the diff, I would also want one focused regression test. The existing CX tests already encode the semantic target — fold-over-children lambdas should treat the element parameter as an So my review is: no blocker found, good direction, approve. The one follow-up is to add contract/arity validation plus a regression test that locks the new authority in place. |
Replace positional heuristic (last lambda param) in iteration_element_name with structural dispatch via callback_element_position declared on each algebra template. Closes REVIEW #11. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Remove heuristic fallback (params |> last) when callback_element_position is absent. All current algebra iteration methods declare their position, so this changes no behavior — it only closes the silent guess path. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
CI runners regularly exceed 60s under shared load. Bump to 120s to stop false failures while profiling is pending. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
6224391 to
d985b0c
Compare
briansrls
left a comment
There was a problem hiding this comment.
codex · gpt-5.4 · cc65bf8f
Review (INVARIANTS: 2, MODELING: 0+/0-, ROADMAP: 0✓/0!)
INVARIANTS — Violations (2)
ROOT CAUSE ANALYSIS
src/v2/complexity.dagUpstream, callback provenance is still only a raw Int? on AlgebraFieldTemplate, not a shared validated callback-contract boundary on MethodSemantics. That gap forces the .dag consumer to index ad hoc and the generated Rust to cast ad hoc; if the template fact is absent, negative, or out of range, the new path can only collapse to none, which then propagates into skipped descent and evidence work instead of a structural error. The upstream fix is to model callback binding once when algebra templates instantiate method semantics, using a non-negative validated param-reference type tied to the CallableOf arity so invalid contracts become unrepresentable or diagnostic.
ROADMAP — Unclaimed
- M1 Step 2: Lambda param provenance from callee contracts: This PR advances the step by replacing the last-param convention in complexity.dag with callback_element_position, but ROADMAP.md still describes callback-contract consumption in CX as pending or deferred.
2 violations, 0 modeling issues found. The PR fixes the prior heuristic fallback, but it still needs a validated callback-contract boundary and a ROADMAP update to align with the zero-bugs-by-construction thesis.
| // this function reads that declared position and indexes into the | ||
| // lambda's params via skip(position). Fail-closed: returns none when | ||
| // no template declares a position (no heuristic guessing). | ||
| fn iteration_element_name(method_semantics: MethodSemantics?, lambda: Node, si: Map<String, NewlineIndex>) -> String? { |
There was a problem hiding this comment.
Invariant violation: iteration_element_name still collapses "no callback contract" and "declared callback position did not resolve to a lambda param" into the same none result, violating INVARIANTS.md "No fallbacks that fabricate" and MODELING.md M5.
| { | ||
| let params = lambda_param_names_at(lambda, si); | ||
| match method_callback_element_position(method_semantics) { | ||
| Some(pos) => params.get(pos.clone() as usize).cloned(), |
There was a problem hiding this comment.
Invariant violation: The new pos as usize cast treats a signed callback_element_position as a valid index without a non-negative boundary contract, violating INVARIANTS.md "Explicit boundary contracts".
|
Review (INVARIANTS: 2, MODELING: 0+/0-, ROADMAP: 0✓/0!) INVARIANTS — Violations (2) ROOT CAUSE ANALYSIS
ROADMAP — Unclaimed
2 violations, 0 modeling issues found. The PR fixes the prior heuristic fallback, but it still needs a validated callback-contract boundary and a ROADMAP update to align with the zero-bugs-by-construction thesis. |
Summary
method_callback_element_positionaccessor in complexity.dag — extracts the declared 0-indexed callback element position from the algebra templatelastlambda param) initeration_element_namewith structural dispatch viaskip(pos) |> first, falling back tolastwhen no template declares a positionall_self_calls_descend_inc,collect_evidence_incremental,collect_scc_child_edges) now use declared facts instead of conventionTest plan
skip(pos) |> firstproduces the same result aslast— zero behavior change today, correct by construction going forward🤖 Generated with Claude Code