Skip to content

R3 gate #4: tier3 effect carrier mirror dissolved - #2591

Closed
briansrls wants to merge 1 commit into
mainfrom
session/royal-heron-214
Closed

briansrls wants to merge 1 commit into
mainfrom
session/royal-heron-214

Conversation

@briansrls

Copy link
Copy Markdown
Contributor

Auto-opened by session-dashboard for session royal-heron-214.
Pushing to session/royal-heron-214 advances this PR.

Worker attestation

Before flipping this PR to ready for review, confirm each item:

  • Title describes the change (not the session id or branch).
  • PR body summarises what and why (replace the TODO below).
  • Tests run: name the command (e.g. npm test, cargo test) and the result.
  • If this closes a work item, the body contains a Closes #N directive.
  • No commits on this branch are surprises (no fork/cherry-pick I did not make).
  • No secrets / credentials / large binaries staged.

Summary

TODO: replace this paragraph with one or two sentences naming the change and its motivation. Reviewers read this first.

Test plan

  • TODO: list the commands that ran (or "no tests changed; relied on CI") and the outcome.

@briansrls
briansrls marked this pull request as ready for review May 10, 2026 14:47
@briansrls

Copy link
Copy Markdown
Contributor Author

Review metadata

  • Provider / model: openai-pro / gpt-5-5-thinking
  • Commit: f0cbc5f9 · Trigger: manual
  • Comparison: main @ 77678c04 ... session/royal-heron-214 @ f0cbc5f9
  • Conversation: View conversation

1. Story of the diff

This PR consolidates the Stage 2b workflow-idempotency Rust projection into src/v3/compiler/src/dag/effects.rs, making the effects carrier module the native Dag witness surface for the std.effects algebra rather than keeping a separate effect-carrier mirror. The load-bearing move is that linear workflow idempotency now composes directly over existing OperationEffect / EffectShape carriers via compose_operation_effects, while non-linear workflow forms fail closed into WorkflowIdempotencyReport::IdempotencyUnsupported instead of pretending to have a verdict. The new analyze_workflow entry point reads the existing native Dag projection with lane2_workflow_effect_at and delegates to the same projection logic used by lane2_workflow_idempotency_report, so the API surface is one carrier model with two entry shapes: direct WorkflowEffect and native Dag root.

2. Invariant categories

  1. LAYER MODEL — Compliant. This touches a substrate-adjacent carrier module, but it does not introduce a new Dag substrate type, field, or variant; the new code consumes existing carriers (OperationEffect, WorkflowEffect, EffectShape) and existing native Dag lookup (d.lane2_workflow_effect_at(&workflow_root)) at src/v3/compiler/src/dag/effects.rs:220, src/v3/compiler/src/dag/effects.rs:259, and src/v3/compiler/src/dag/effects.rs:296. That matches Boundary Discipline / single-authority use of declared substrate facts rather than a fresh parallel storage shape. chatgpt-review-0cda0389-3d9b-42…
  2. INVARIANTS.md + modeling-discipline.md — Compliant. Fail-closed and typed-carrier discipline are handled: missing workflow facts return WorkflowIdempotencyReport::IdempotencyUnsupported at src/v3/compiler/src/dag/effects.rs:297, and unsupported non-linear workflow variants return the same typed unsupported carrier at src/v3/compiler/src/dag/effects.rs:263, src/v3/compiler/src/dag/effects.rs:271, and src/v3/compiler/src/dag/effects.rs:279. Facts flow forward through the existing partition: compose_operation_effects reads EffectShape::IsBreaking(_) directly at src/v3/compiler/src/dag/effects.rs:222 and returns either BrokenBy { first_breaker } or IdempotentComposition at src/v3/compiler/src/dag/effects.rs:229 and src/v3/compiler/src/dag/effects.rs:232.
  3. CODING.md — Compliant. The added behavior is data + free functions with explicit dependencies and structured outputs: compose_operation_effects(effects: &[OperationEffect]) -> CompositionVerdict at src/v3/compiler/src/dag/effects.rs:220, project_workflow_idempotency_report(workflow: &WorkflowEffect) -> WorkflowIdempotencyReport at src/v3/compiler/src/dag/effects.rs:256, and analyze_workflow(d: &Dag, workflow_root: NodeId) -> WorkflowIdempotencyReport at src/v3/compiler/src/dag/effects.rs:295. That aligns with the coding rule that lenses/analyses are free functions over data, not methods hidden on Dag. chatgpt-review-56c84d01-3f6e-4a…
  4. TESTING.md — Compliant. The diff itself adds no tests, but the changed surface is explicitly described as an emitted-output export exercised by the existing rustc round-trip in m2_lens_idempotency_migration_test at src/v3/compiler/src/dag/effects.rs:237–src/v3/compiler/src/dag/effects.rs:238. Since this PR is relocating/dissolving a mirror rather than adding a new workflow-idempotency semantic, I do not see a diff-citable missing-test finding. The shape also fits the current testing guidance that lens behavior should be pinned through minimal carrier/API surfaces where possible. chatgpt-review-e05961b6-c509-45…
  5. LOCKED DESIGN DECISIONS — Compliant. The diff preserves the Pure Bootstrap / self-hosting direction rather than weakening it: the module banner names this as a typed Rust witness for currently unparsed std arrow bodies and says the standalone workflow_idempotency.rs mirror is retired at src/v3/compiler/src/dag/effects.rs:5–src/v3/compiler/src/dag/effects.rs:8. That is consistent with the higher-level target that hand-authored Rust remains scaffold pending dissolution, not a permanent parallel authority. chatgpt-review-1500ddde-ea5d-40…
  6. TRACKED vs UNTRACKED DEBT — Compliant. The only bridge-like language is tracked and bounded in the module contract: this is the “typed Rust witness for unparsed std arrow bodies in bootstrap” at src/v3/compiler/src/dag/effects.rs:5, with the dissolution trigger named as the retired standalone mirror / R3 gate at src/v3/compiler/src/dag/effects.rs:6–src/v3/compiler/src/dag/effects.rs:8. I do not see a new TODO, temporary enum, second record shape, or unbounded scaffold introduced in the diff.

2.5. Top-down PM intent review

Compliant. The highest-level intent is to reduce hand-maintained parallel compiler authority and move toward .dag/generated/self-hosted structure; the thesis and Pure Bootstrap docs make “one .dag authority, shrinking Rust scaffold” the direction of travel. chatgpt-review-1500ddde-ea5d-40…

This diff’s concrete mechanism supports that: the module is declared as the “sole compiler-local implementation surface for this algebra” at src/v3/compiler/src/dag/effects.rs:6, and it folds the standalone mirror into the carrier module at src/v3/compiler/src/dag/effects.rs:7–src/v3/compiler/src/dag/effects.rs:8. I do not see a diff-cited semantic dilution where a must-have dissolution target becomes optional or permanent.

3. Verdict

APPROVE. The PR is small and appears to dissolve a mirror into the existing effects carrier authority without adding new substrate variants, duplicate record shapes, or fail-open behavior. The unsupported cases are explicit typed reports, and the native Dag entry point reads the existing workflow projection rather than inventing a second source of truth.

@briansrls briansrls left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review metadata

  • Provider / model: codex / unknown
  • Commit: f0cbc5f9 · Trigger: schedule
  • Thinking: 135s wall

BLOCKING (1)

Root Cause

  • src/v3/compiler/src/dag/effects.rs The projection was copied into the carrier module before the old workflow_idempotency.rs authority was atomically retired → move the implementation and update exports/census in the same diff, or keep one implementation surface.

⚠️ The core logic matches the existing algebra, but the dissolution PR currently adds a second authority instead of completing the retirement.

}

// ── Stage 2b projection (native `Dag` / rustc round-trip exports) ───────────

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BLOCKING: Adding compose_operation_effects here copies the existing Stage 2b authority from src/v3/compiler/src/workflow_idempotency.rs without deleting or rerouting that module/exports, leaving parallel implementations in violation of INVARIANTS.md P2/P5 single-authority dissolution.

@briansrls

Copy link
Copy Markdown
Contributor Author

Closing as superseded by PR #2679 (gate #4 tier3 effect carrier mirror dissolved, warm-ibex-579 re-spawn after royal-heron-214 archive). Re-spawn substance (+130/-147) supersedes original (+100/-9). Closed by Director cleanup.

@briansrls briansrls closed this May 11, 2026
@briansrls
briansrls deleted the session/royal-heron-214 branch May 11, 2026 04:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant