Repository navigation
Lane 2 Stage 2a follow-up: collapse DerivedOpEffect into OperationEffect - #521
Conversation
DerivedOpEffect's `method` and `path_template` fields were never read downstream — the modifier check and obligation generator both project through `shape` alone, and the `ReadEffect` variant already encodes "method was GET/HEAD/OPTIONS" structurally. Collapsing to OperationEffect aligns `derive_op_effect` output with what `compose_effects` already consumes, so Stage 2b walks one carrier shape rather than two parallel records. Clears half the §Lane 2 Stage 2a / Track 17a boundary deferral; ComposedEffect reshape remains the one open gate before Stage 2b. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
ChatGPT review in progress... (view conversation) Check back in ~30 minutes for the full review. |
briansrls
left a comment
There was a problem hiding this comment.
Clean execution of the brief. Three files, no surprise scope, ROADMAP and lane2 docs updated to track the remaining gate. Approve.
What's right
- Option (a) chosen as recommended.
DerivedOpEffectdeleted entirely;derive_op_effectreturnsOperationEffect?directly. The transport fields (method,path_template) are gone — the rationale in the doc updates is correct: nothing downstream consumed them ("the modifier check and obligation generator both project throughshapealone, andReadEffectalready encodes 'method was GET/HEAD/OPTIONS' structurally"). ✅ - Single-authority restored.
OperationEffectis now the durable carrier — same shapecompose_effectswalks, same shapederive_op_effectproduces. No more two-types-for-one-fact at the Stage 2a → Stage 2b boundary. ✅ - Consumer migrations are minimal because the bodies didn't actually depend on the deleted fields.
check_modifier_vs_derivation(op: OperationEffect, ...)andgenerate_idempotency_obligations(ops: List<OperationEffect>)— signature changes only, body shapes unchanged. That confirms the originalDerivedOpEffectshape was over-wide; this PR's evidence backs the original "lossy compression at substrate" diagnosis. ✅ - Doc updates are explicit and honest.
docs/lane2-compile-time-proofs.md: boundary note rewritten as "(cleared forDerivedOpEffect)" with the rationale inline; pre-start gate at line 90 narrowed from "DerivedOpEffect or ComposedEffect" to "ComposedEffect" only.ROADMAP.md: deferral row title narrowed fromDerivedOpEffect/ComposedEffectboundary cleanup to justComposedEffect; "Cleared (this PR)" subsection appended with the structural rationale.
ComposedEffectcorrectly preserved as the remaining gate. The PR doesn't try to do too much.ComposedEffect's reshape is its own work item; this PR doesn't touch it. Scope discipline good. ✅
Forward-looking note (not a blocker for this PR)
The closing sentence in the ROADMAP "Cleared" entry is the right framing for future debt avoidance: "Future diagnostic rendering that wants the originating method/path should attach a separate evidence carrier to the diagnostic, not smuggle transport facts onto the effect record." That's exactly the principle that should govern the eventual ComposedEffect reshape too — separate concerns, don't bundle.
Verdict
APPROVE. Smallest of tonight's PRs, cleanest scope, most directly executes its brief. Ready to merge.
briansrls
left a comment
There was a problem hiding this comment.
codex · gpt-5.4 · 01fb105d
✅ Review (blocking: 0, non-blocking: 2+/0-)
Non-blocking — Strengths
src/v3/std/effects.dagCollapsingDerivedOpEffectintoOperationEffectremoves a parallel substrate carrier and keeps downstream consumers on the same authority they already read:operation_nameplusshape.docs/lane2-compile-time-proofs.mdThe Stage 2a boundary note is honest about the discard: it explains whymethodandpath_templatedo not need to survive on the effect record instead of letting the convenience shape harden by inertia.
ROADMAP — Verified
- DerivedOpEffect boundary cleanup:
src/v3/ROADMAP.mdanddocs/lane2-compile-time-proofs.mdnow agree that this deferral is cleared and thatComposedEffectremains the only Stage 2a boundary cleanup still open.
✅ This looks clean; the PR simplifies the effect substrate without dropping any fact that current downstream consumers actually use, and it keeps the remaining debt explicitly tracked.
|
✅ Review (blocking: 0, non-blocking: 2+/0-) Non-blocking — Strengths
ROADMAP — Verified
✅ This looks clean; the PR simplifies the effect substrate without dropping any fact that current downstream consumers actually use, and it keeps the remaining debt explicitly tracked. |
ChatGPT ReviewPrinciple audit. Fail-closed. Satisfied for this follow-up. The diff does not introduce a new silent-success path; it narrows the boundary so derivation emits the same carrier the downstream checks already consume. The carrier alignment is structural, even though the underlying partial derivation remains inherited Stage 2a behavior rather than something this PR changes. Illegal states unrepresentable. Improved. Facts flow forward. Satisfied. The PR does not silently lose Coproduct dissolution. Satisfied. No new substrate enum is introduced, and the modeling move is in the right direction: a short-lived bridge carrier is dissolved before Stage 2b learns it as canonical. The remaining provisional shape, Single-authority metadata. Strongly satisfied. Before this change, derivation produced one carrier and composition/checking walked another; after it, derivation, modifier falsification, obligation generation, and future composition all share the same API-level enforcement. Improved. The important invariant—“derivation outputs the same shape composition consumes”—is now enforced by the API instead of depending on discipline across two parallel records. The remaining Design question. Non-blocking: when Stage 2b or Track 17a eventually wants richer diagnostics for a misderived effect, where will method/path provenance live without reintroducing a second effect carrier? The stake is preserving the win this PR banks: transport provenance should attach as separate diagnostic/test evidence, not grow Verdict. APPROVE. This is the right cleanup: it removes a tracked bridge, improves illegal-state modeling, and makes the Stage 2a/2b seam use one durable carrier. I don’t see a real concern in this commit beyond the already-tracked LOOP HEALTH: converging — this round dissolves one explicitly tracked scaffold ( |
Summary
DerivedOpEffect;derive_op_effect(...)now returnsOperationEffect?directly. Themethodandpath_templatefields were never read downstream — modifier falsification and obligation generation both project throughshapealone, andReadEffectalready encodes "method was GET/HEAD/OPTIONS" structurally.check_modifier_vs_derivationandgenerate_idempotency_obligationsnow takeOperationEffect— bodies unchanged (they only ever read.operation_nameand.shape).src/v3/ROADMAP.md;ComposedEffectreshape remains the one open gate before Stage 2b.docs/lane2-compile-time-proofs.mdStage 2b pre-start gate updated accordingly.Scope is v3-only:
dsl/std/effects.dag+src/v2/effect_derivation.daguntouched. v2 is retiring andModifierCheckhas already diverged structurally from v3 (singleagreementvs two-axisModifierAxisCheck), so the "keep in sync" note in the v3 file header is aspirational for this shape.Test plan
cargo test -p v3-compiler --test lane2_stage_2a_effects_smoke— 4/4 pass (smoke test asserts function-name presence, not arg types;DerivedOpEffectwasn't in the asserted type list so deletion is safe)cargo test -p v3-compiler --test real_stdlib_parse_smoke— 10/10 pass (effects_dag_parsesgreen)cargo test -p v3-compiler— full suite green, 0 failurescargo test --workspace --exclude v2-compiler-tests— 34 "ok" test-result lines, 0 failurescargo fmt --all --check— cleancargo clippy --all-targets -- -D warnings— clean🤖 Generated with Claude Code