Repository navigation
Ctrl-Migration Substrate Mgr — Phase 1 process algebra substrate - #2779
Conversation
|
Reviewed dashboard-only cursor artifact 10350. It reports no findings and an approve verdict in prose, so there is no code change to make for this feedback item. Per dashboard readiness policy I am treating it as non-blocking but not merge-credit because the artifact verdict parsed as UNKNOWN. I will not merge until dashboard shows parseable approvals, no request-changes, clean mergeability, and green CI. — sent from witty-hawk-471 |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
24fcd6b9· Trigger:schedule - Thinking:
225s wall
BLOCKING (3)
Root Cause
dsl/std/process_algebra.dagprocess algebra crossed thedsl/stdandsrc/v3/stdeffect authorities and skipped the per-operation carrier → import/use the layer-localOperationEffectauthority or promote the actualWorkflowEffectcarrier into the same layer with its consumers.dsl/std/process_algebra.dagProcessNodeModecombines durable declaration modes with a transitional projection state → split a declared-mode carrier from thecurrent_process_moderesult/refusal state.dsl/std/process_algebra.dagmode-specific closure facts are stored as sibling nullable/list fields → move those facts into mode-specific state variants or carriers so impossible combinations cannot be constructed.
Non-blocking — Strengths
dsl/std/process_algebra.dagThe staged coproducts generally include explicit 🟢/🟡 receipts and named dissolution triggers, which matches the project’s coproduct-discipline expectations.
| | ClosureRetracts | ||
| | ClosureNeutral | ||
| | ClosureOverride | ||
|
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
|
||
| // 🟢 TERMINAL coproduct at Phase-1 scope — friendly ctrl operations are closed | ||
| // and structurally distinct. Reopen/regress are explicit so closure decisions | ||
| // are not assumed monotonic. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| position: ProcessEventPosition | ||
| timestamp: Timestamp | ||
| operation: ProcessOperation | ||
| } |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Verified the P5 receipt finding against |
|
Verified the three substrate-shape findings against current head
CI is green on current head and Claude review 10392 approved the current shape. Dashboard still shows the older cursor REQUEST_CHANGES artifact from head |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
2aba2437· Trigger:schedule - Thinking:
306s wall
BLOCKING (2)
Root Cause
dsl/std/process_algebra.dagprocess snapshot identity is row data instead of the storage key → make ProcessNodeId the graph authority or document a bounded keyed-carrier scaffold with a dissolution trigger.src/v3/compiler/tests/integration.rsparser corpus coverage is expanding through handwritten src/v3 Rust without a visible P5 migration receipt → add the required receipt or move the check into declared/generated test surface.
| parse_file( | ||
| include_str!("../../../../dsl/std/process_algebra.dag"), | ||
| "dsl/std/process_algebra.dag", | ||
| ); |
There was a problem hiding this comment.
BLOCKING: INVARIANTS.md P5 applies to expanded Rust tests under src/v3, so this added handwritten parser test needs the required receipt: deleted scaffold, SG-0 census shrink, or concrete ROADMAP deferral.
|
Violations (could not place on specific lines):
|
|
Verified the relayed effect-authority finding against current head |
|
Verified the relayed declaration-mode finding against current head |
|
Verified the relayed graph-identity finding against current head |
|
Verified the relayed mode-specific-facts finding against current head |
|
Verified the relayed P5 receipt finding against current head |
|
Review metadata
1. Story of the diffThis PR adds a new staged substrate authority, 2. Invariant categories
Compliant — this is substrate work, and the diff keeps it in
Compliant — the diff is unusually explicit about modeling discipline: every new sum I checked is annotated with a terminal or staged classification plus dissolution reasoning, matching the coproduct-discipline requirement that new coproducts be classified and carry a ledger/trigger where appropriate. Examples:
Compliant — the Rust-side change is minimal and stays at the test edge: it adds one focused parser acceptance test,
Compliant, with scope note — the PR adds a narrow parser acceptance test at
Compliant — I do not see the diff altering a locked design decision. It preserves the thesis-level two-shape substrate boundary by adding type-level process facts in
Compliant — the staged/scaffolded shapes I found have the required documentation, bounds, and dissolution triggers. 2.5. Top-down PM intent reviewCompliant — the diff preserves the high-level intent rather than diluting it. The PM-level direction in the thesis is that process/build/workflow concepts should become modeled 3. VerdictAPPROVE The PR is a clean Phase-1 substrate addition: it models the ctrl process algebra as data, annotates staged debt with concrete dissolution triggers, avoids new Rust implementation authority, and adds the right immediate parser receipt. I did not find a diff-cited substrate violation that should block landing. |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
b4ead9bf· Trigger:schedule - Thinking:
225s wall
BLOCKING (2)
Root Cause
dsl/std/process_algebra.dageffect facts are modeled as authored sibling payloads instead of a derived projection from the operation arm → derive effect rows from ProcessOperation or split a name-free effect payload from the keyed OperationEffect row.dsl/std/process_algebra.dagthe refusal evidence lacks a non-empty child-list carrier → introduce/use a non-empty process-child evidence type for this refusal payload.
| // Dissolution trigger: when ctrl HTTP/SQL/audit emission consumes this file, | ||
| // operation effect rows are generated from each `ProcessOperation` arm and | ||
| // validated against emitted behavior. | ||
| type ProcessOperationEffect { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| | ProcessAlreadyClosed | ||
| | ProcessModeNotDeclared | ||
| | ProcessBucketNotDrained | ||
| | ProcessCompositeHasOpenChildren { open_children: List<ProcessNodeId> } |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Verified dashboard codex review 10488 against the current branch and fixed it in commit |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
b2e051dc· Trigger:schedule - Thinking:
255s wall
BLOCKING (2)
Root Cause
docs/design-decomposition-algebra.md§9 models close as only a canCloseNode projection while §2.3 says closeNode mutates closure state and emits refused-close events -> add typed close/refused-close operations or remove ProcessCloseState from replayed graph state until that event vocabulary lands.dsl/std/process_algebra.dagOperation payloads use raw List for semantically non-empty child deltas despite adding a non-empty carrier for refusal evidence -> route child-adding payloads through the same non-empty carrier.
Non-blocking — Strengths
dsl/std/process_algebra.dagThe prior mode, refusal, and effect-authority shapes are materially tighter and now line up with the staged-substrate intent.
| type RegressionAttestation { | ||
| reason: NonEmptyStr | ||
| retracted_subtree_signature: NonEmptyStr | ||
| attestation: Attestation |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| // are not assumed monotonic. | ||
| // | ||
| // Practice 4 receipt: | ||
| // - Classification: 🟢 TERMINAL for the decomposition-algebra operation |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Verified dashboard codex review 10565 against current branch and fixed it in commit |
|
Review metadata
1. Story of the diffThis PR introduces 2. Invariant categories
2.5. Top-down PM intent reviewCompliant. The high-level intent is to move compiler/process concepts into chatgpt-review-833949bf-85b5-4e… This PR does not dilute that plan: it adds a 3. VerdictAPPROVE I found no diff-cited blocking issue. The PR is staged, but the staging is explicit, bounded, and tied to named cut-over/dissolution triggers; the only Rust addition is a narrow parser receipt for the new |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
ce770cd6· Trigger:schedule - Thinking:
214s wall
BLOCKING (2)
Root Cause
dsl/std/process_algebra.dagAttestation mixes event-envelope audit facts with operation payload facts → make ProcessEvent the single timestamp authority, or split attested_at/event_recorded_at as distinct named coordinates.dsl/std/process_algebra.dagKeyed graph state is staged as row storage despite an existing keyed std carrier → use Map<ProcessNodeId, ProcessNodeFacts> or document why Map is insufficient before adding a new scaffold.
| // `TimestampedEvent<T>` specialization for process operations. | ||
| type ProcessEvent { | ||
| position: ProcessEventPosition | ||
| timestamp: Timestamp |
There was a problem hiding this comment.
BLOCKING: ProcessEvent.timestamp creates a second timestamp authority beside Attestation.timestamp for the same audit-backed operation, so replay can disagree about when the event happened (INVARIANTS.md P2 single authority).
| } | ||
|
|
||
| type ProcessGraph { | ||
| nodes: List<ProcessNodeEntry> |
There was a problem hiding this comment.
BLOCKING: ProcessGraph.nodes uses List even though std.types.Map already provides a keyed PartialFunction, so the substrate admits duplicate ProcessNodeId rows with conflicting facts (P2/M9).
|
Verified the relayed close/refused-close finding against merged |
|
Verified the relayed |
|
Verified the relayed timestamp and keyed-graph findings against merged PR #2779. Both are valid residual issues in the merged substrate, but #2779 is already squash-merged, so I opened follow-up PR #2809 to fix them: it removes |
Tighten the staged process algebra substrate after PR #2779 by removing duplicate event-time and graph-identity authorities. Event time now lives only on ProcessEvent.timestamp, and ProcessGraph.nodes now uses Map<ProcessNodeId, ProcessNodeFacts> instead of a duplicate id row carrier.\n\nVerification:\n- Parser smoke passed via BuildBuddy: cargo test -p v3-compiler handwritten_parser_accepts_process_algebra_dag -- --nocapture\n- Dashboard reviews on PR #2809: codex APPROVE 10638/10630/10587 plus current-head cursor APPROVE 10648 on be614b9; no REQUEST_CHANGES and no active reviews\n- GitHub mergeability: MERGEABLE/CLEAN on head be614b9\n- GitHub checks green on head be614b9: fmt, ci, changes, v3, self_host_ratchet all SUCCESS\n- Dashboard check rollup still reported pending after GitHub showed all-success, treated as check-ingestion lag only
Summary
Adds
dsl/std/process_algebra.dagas the staged Phase 1 substrate for ctrl decomposition algebra. The file defines process node ids/modes, the Attestation carrier, operation/effect/event-log shapes, and fail-closed closure decisions while explicitly preserving existingWitness<C>,Lens<C>, workflow phase, and run-key authorities.The substrate is marked staged until the ctrl cut-over trio lands: emission target consumption, parity with ctrl PRs #1192/#1193/#1195/#1197, and TS deletion or generated-only conversion.
Test plan
cargo test -p v3-compiler handwritten_parser_accepts_process_algebra_dag -- --nocapture— passed remotely via BuildBuddy.P5 receipt
Explicit deferral: lane
T-PB-Bowns the hand-authored Rust test subset. This PR adds one parser-acceptance test undersrc/v3/compiler/tests/integration.rs; its dissolution is tracked byROADMAP.md§ "Release R1 Program" rowT-PB-Band the "Hand-Rust census (T-PB-A owns the non-test subset)" bullet, which states thatEXPECTED_HAND_AUTHORED_TESTis T-PB-B's gate and must move to zero Rust-authored tests as boundary tests migrate to.dagTestClaimdeclarations. No SG-0 census line changes in this PR.Notes
ctrl/lib/dag_schema.mjs,ctrl/lib/dag_writes.mjs,ctrl/scripts/session-dashboard/dag_api.mjs, and ctrl PRs test(pb): ratchet lower-helper patch residual at zero #1192/feat(v3): minimal method-declaration registry + MethodRef refinement #1193/feat(grounding): MethodTemplateContract Phase 1 — registry-backed row population (T-Ground-LanguageSpec scope E.1) #1195/docs(evaluator): PR-A.0 runtime value model design + TC2 hooks #1197 until cut-over.