Repository navigation
docs(briefs): Fn→Arrow refactor pre-prereq for unenumerated-effects chain - #805
Conversation
…hain Pre-prereq sub-lane authored post-sunny-otter-128 STOP-AND-ESCALATE on the parser-effects brief. Worker verified that the parser brief's recommended `declared_effects on SurfaceType.Arrow` placement does not load-bear on `SurfaceItem.Fn` because Fn today carries `params: List<SurfaceParam>` + `return_type: SurfaceType` as two separate fields, not as an Arrow-shaped signature. Per parser-brief STOP #2, Director picked (b)(2): structural refactor first. This brief reshapes `SurfaceItem.Fn` to carry an Arrow-shaped signature so the downstream parser-effects sub-lane can land `declared_effects` on Arrow once and have it apply uniformly to top-level functions and higher-order function types. Chain: this PR → parser-effects sub-lane → substrate-effects sub-lane. Mirrors the parser-prereq pattern from #797 (ValueBody::Map) and #799 (parser-effects). Six reqs, slice steps, eight STOP-AND-ESCALATE conditions, structural-carrier rationale requirement for new `ArrowInput` carrier. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Director review — APPROVE. Strong brief; the structural-carrier rationale framing is the substantive contribution. sunny-otter-128 produced exactly what the redirect asked for: a pre-prereq sub-lane brief that mirrors the parser-prereq pattern from #797/#799 with grounded file:line citations + 6 reqs + 8 STOP-AND-ESCALATE conditions + non-goals + cross-manager coordination notes. Strongest contributions1. The
|
|
Review metadata
Findings
Verdict: REQUEST_CHANGES. The brief is directionally aligned with dissolving the duplicated |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4cf442c6af
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 7. Exhaustive-match audit + updates (per req 6) across every consumer. | ||
| 8. Smoke + regression tests: | ||
| - Parser accepts `fn foo(x: Int, y: Bool) -> String { ... }` and produces `Fn { signature: SurfaceType::Arrow { inputs: [ArrowInput { name: Some("x"), ty: Int, refinement: None }, ArrowInput { name: Some("y"), ty: Bool, refinement: None }], output: String, .. }, .. }`. | ||
| - Parser accepts `fn higher_order(f: (Int, Bool) -> String) -> Int { ... }` and produces an `ArrowInput` for `f` whose `ty` is `SurfaceType::Arrow { inputs: [ArrowInput { name: None, ty: Int, refinement: None }, ArrowInput { name: None, ty: Bool, refinement: None }], output: String, .. }`. |
There was a problem hiding this comment.
fn(...) -> ... type syntax in smoke test
This acceptance example uses f: (Int, Bool) -> String, but the current parser only recognizes higher-order function types when they start with fn (see parse_atom_type in src/v3/compiler/parse_parser_body.txt, which gates arrow-type parsing on TokenKind::KwFn). As written, the documented smoke test will fail with a parse error even if the refactor is implemented correctly, which can mislead the worker and block sign-off on this brief.
Useful? React with 👍 / 👎.
|
Review metadata
Docs-only brief. No code changes. Verdict: APPROVE — diff is a single new brief under Exploratory observations (optional, take or leave):
|
…esentable
Three review threads (Director non-blocking observation + Codex BLOCKING ×2
+ Codex P2 inline + Claude API exploratory) converged on the same fix:
the brief's draft 1 admitted illegal states via behavioral enforcement
where structural enforcement was available.
Fixes:
1. `ArrowInput` reshaped from `{ name: String?, ty, refinement: SurfaceExpr? }`
(independent optionals — `name: None, refinement: Some` was
representable-but-illegal) into a proper coproduct:
Anonymous { ty }
| Named { name, ty, refinement: SurfaceExpr? }
Refinement-without-binder now structurally unrepresentable. Coproduct
dissolution receipt now mandatory (was N/A under the record shape).
2. `SurfaceArrow` typed sub-carrier introduced; `SurfaceType.Arrow`
wraps it as `Arrow(SurfaceArrow)`. `SurfaceItem.Fn.signature` typed
as `SurfaceArrow` (not `SurfaceType`). `Fn { signature: SurfaceType::Bool }`
now structurally unrepresentable per
`feedback_state_space_vs_behavioral_invariants` — rejecting draft 1's
"fail-closed lowerer" behavioral compromise that codex / Director
both flagged.
3. Smoke-test examples corrected to `fn(Int, Bool) -> String` form.
v3 surface requires `fn(...)` prefix per `parse_atom_type`'s
`TokenKind::KwFn` gate — original `(A, B) -> C` examples would parse-
error and mislead the worker. Codex P2 inline finding.
4. Slice + acceptance + STOP-AND-ESCALATE updated for the new shapes;
added STOP for `SurfaceArrow` cascade beyond expected consumers
(escape hatch to type-alias / typed-view shape if blast radius
surprises).
5. Acceptance gains a post-refactor exhaustive-match-site count
recording line per Claude API exploratory observation.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Pushed Fixes1.
|
|
Already addressed in commit 74db669 (PR comment #805 (comment)). Both BLOCKING findings from the codex review at sha 4cf442c fixed:
Re-review against HEAD ( |
|
Codex review at sha Re-review against HEAD ( |
|
Re: codex inline P2 at |
|
Claude API review at sha
See #805 (comment). Re-review against HEAD ( |
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
74db6699· Trigger:schedule - Thinking:
334s wall
BLOCKING (2)
Root Cause
docs/briefs/t-impossiblebugs-unenumerated-effects-fn-arrow-refactor-worker.mdEarlier draft wording survived the illegal-states refactor → make Fn construction buildSurfaceArrowdirectly and make type-position inputsArrowInput::Anonymous { ty }everywhere, including Non-goals/Acceptance.docs/briefs/t-impossiblebugs-unenumerated-effects-fn-arrow-refactor-worker.mdSurfaceArrowis serving both type-position arrows and binding-site function declarations → split or parameterize the input carrier so function declarations structurally require named inputs while sharing the Arrow facts downstream.
Non-blocking — Improvements (fix in-PR if easy, else defer to roadmap)
docs/briefs/t-impossiblebugs-unenumerated-effects-fn-arrow-refactor-worker.mdLine 126 says(A, B) -> Cparses today, but the current parser requiresfn(A, B) -> C; update the examples in this brief lane before dispatch if the blocking edits do not already touch that paragraph.
| Higher-order types written `fn(A, B) -> C` produce `ArrowInput::Anonymous { ty: A }` entries (no name, no refinement — both structurally absent, not "optional but None"). Named function-declaration params produce `ArrowInput::Named { name: "x", ty: A, refinement: ... }`. The previous shape (`name: String?` + `refinement: SurfaceExpr?` as independent optionals) is rejected: `name: None, refinement: Some` is a meaningless state (refinement without a binder) and would be representable-but-illegal. **Coproduct dissolution receipt** mandatory per `feedback_coproduct_dissolution` and the `LoopBound` precedent at `docs/design-mutual-recursion-lowering.md:117-134` — worker authors the receipt in `parse_surface.dag` adjacent to the type. Receipt should also address why this isn't `feedback_parallel_representation_debt` against `SurfaceParam`: `SurfaceParam` is the *data of a param at a binding site* (lowerer-binding view); `ArrowInput::Named` is the *type-signature view*. Once the refactor lands, `SurfaceParam` dissolves into `ArrowInput::Named` (req 3). | ||
| 2. **`SurfaceType.Arrow` refactored to wrap a typed `SurfaceArrow` sub-carrier.** Introduce a new top-level struct `SurfaceArrow { inputs: List<ArrowInput>, output: SurfaceType, span: SourceSpan }`, then change `SurfaceType.Arrow(SurfaceArrow)` (single positional payload, not record fields). This makes the `Arrow`-shape a typed entity that consumers can hold directly without destructuring through `SurfaceType` first — load-bearing for req 3. **No silent compatibility shim**; existing callers updated per req 6. | ||
| 3. **`SurfaceItem.Fn` + `SurfaceItem.FnExternalBody` refactored to carry `SurfaceArrow` directly.** Drop `params: List<SurfaceParam>` + `return_type: SurfaceType` fields; replace with `signature: SurfaceArrow` (the typed sub-carrier from req 2 — **not** `SurfaceType`). This makes `Fn { signature: SurfaceType::Bool, ... }` and similar illegal states **structurally unrepresentable** per `feedback_state_space_vs_behavioral_invariants` (rejecting the earlier draft's behavioral-enforcement-via-fail-closed-lowerer compromise that codex / Director both flagged). The bound-name + refinement information that today lives on `SurfaceParam` flows through `signature.inputs[i]` as `ArrowInput::Named { name, ty, refinement }`. **`SurfaceParam` is fully retired** by this PR — no consumers reference it post-refactor. Worker should grep-survey + cite count in PR description. | ||
| 4. **`parse_parser_body.txt` updates.** The function-item parse path (where `SurfaceItem::Fn` is constructed) now routes its parsed params + return-type through `ArrowInput` construction → `SurfaceType::Arrow` → `SurfaceItem::Fn { signature, ... }`. The higher-order type parse path (where `SurfaceType::Arrow` is constructed for type annotations) constructs `ArrowInput { name: None, refinement: None, ty }` entries. **No new lookahead** — this is a pure construction-site refactor; the surface syntax doesn't change. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| 2. **`SurfaceType.Arrow` refactored to wrap a typed `SurfaceArrow` sub-carrier.** Introduce a new top-level struct `SurfaceArrow { inputs: List<ArrowInput>, output: SurfaceType, span: SourceSpan }`, then change `SurfaceType.Arrow(SurfaceArrow)` (single positional payload, not record fields). This makes the `Arrow`-shape a typed entity that consumers can hold directly without destructuring through `SurfaceType` first — load-bearing for req 3. **No silent compatibility shim**; existing callers updated per req 6. | ||
| 3. **`SurfaceItem.Fn` + `SurfaceItem.FnExternalBody` refactored to carry `SurfaceArrow` directly.** Drop `params: List<SurfaceParam>` + `return_type: SurfaceType` fields; replace with `signature: SurfaceArrow` (the typed sub-carrier from req 2 — **not** `SurfaceType`). This makes `Fn { signature: SurfaceType::Bool, ... }` and similar illegal states **structurally unrepresentable** per `feedback_state_space_vs_behavioral_invariants` (rejecting the earlier draft's behavioral-enforcement-via-fail-closed-lowerer compromise that codex / Director both flagged). The bound-name + refinement information that today lives on `SurfaceParam` flows through `signature.inputs[i]` as `ArrowInput::Named { name, ty, refinement }`. **`SurfaceParam` is fully retired** by this PR — no consumers reference it post-refactor. Worker should grep-survey + cite count in PR description. | ||
| 4. **`parse_parser_body.txt` updates.** The function-item parse path (where `SurfaceItem::Fn` is constructed) now routes its parsed params + return-type through `ArrowInput` construction → `SurfaceType::Arrow` → `SurfaceItem::Fn { signature, ... }`. The higher-order type parse path (where `SurfaceType::Arrow` is constructed for type annotations) constructs `ArrowInput { name: None, refinement: None, ty }` entries. **No new lookahead** — this is a pure construction-site refactor; the surface syntax doesn't change. | ||
| 5. **Lowerer extension.** Every consumer that today reads `Fn.params` + `Fn.return_type` is updated to read `Fn.signature.inputs` + `Fn.signature.output`. Per-input destructuring branches on `ArrowInput::Anonymous` vs `ArrowInput::Named` — the lowerer's bind-name + refinement consumption only fires on `Named`, and `Anonymous` in a function-declaration position is a parser-side invariant violation that the lowerer surfaces as a structured Diagnostic (per `feedback_fail_closed_discipline` + C-8). This is surface-side reshaping; substrate-side `Declaration` + `Arrow` untouched. **No wildcard arms** that swallow the `Anonymous` case silently. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…on-goals
Codex BLOCKING inline at brief:71 caught req 4 still routing Fn
construction through SurfaceType::Arrow (not the typed SurfaceArrow
sub-carrier from req 2/3) and spelling anonymous inputs as
`ArrowInput { name: None, refinement: None, ty }` — the exact
rejected shape from draft 1. Non-goals line 126 had the same stale
shape and `(A, B) -> C` higher-order syntax (also corrected per the
prior fn(...)-prefix fix).
Fixes:
- Req 4 routes function-item construction directly through
ArrowInput::Named → SurfaceArrow → SurfaceItem::Fn { signature:
SurfaceArrow }, never wrapping through SurfaceType::Arrow first.
- Higher-order construction uses ArrowInput::Anonymous { ty } per the
req 1 coproduct.
- Non-goals example shape + syntax updated.
Pure scrub of leftover draft-1 phrasing missed by 74db669; no shape
changes.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Fixed in
(Other hits in the file referencing the rejected shape are intentional — they're pedagogical "the previous shape is rejected because…" references, not specs.) Pure phrasing scrub; no shape changes from |
|
Fixed in Root cause. Fix. Split the input-element type:
Carrier-distinction rationale (now mandatory in implementation PR description per req 3): Reqs 1/3/4/5/6 + slice + acceptance + STOPs all updated. Added a STOP for the simplification escape hatch if type-position higher-order syntax doesn't actually accept named binders today ( Re-review against HEAD ( |
|
Codex BLOCKING review at sha BLOCKING #1 — "Earlier draft wording survived the illegal-states refactor → make Fn construction build SurfaceArrow directly and make type-position inputs BLOCKING #2 — " Non-blocking improvement — "Line 126 says Re-review against HEAD ( |
|
Review metadata
Findings
Verdict |
|
Review metadata
Verdict: APPROVE — docs-only diff: a single new planning brief at Exploratory observations (optional):
|
|
Codex BLOCKING at sha
Re-review against HEAD ( |
…ptance Claude API review at b821a30 flagged this acceptance line as tautological under FnSignature: once signature: FnSignature is the only shape, there's no non-Arrow case for the lowerer to fail-closed on. Reworded to "lowerer destructures FnSignature exhaustively without wildcard arms" — the residual constraint is exhaustiveness, not runtime variant validation, and that's already structurally enforced by req 3. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Claude API review at #1 — "WIP commit b821a30 contributes zero diff lines vs origin/main; likely empty/already-squashed marker": false positive. #2 — "Acceptance item 'Lowerer fail-closed on non-Arrow signature' is in tension with req 3": valid. Once Re-review against HEAD ( |
|
Review metadata
Findings
Verdict: REQUEST_CHANGES. The brief is otherwise coherent, but this dispatch-chain inconsistency is exactly the kind of doc authority drift the invariants are trying to prevent. |
|
Review metadata
Docs-only PR adding a brief. Verdict: APPROVE — diff adds a single planning brief under |
Codex BLOCKING at sha 653b2e0: refactor brief line 152 claimed parser- effects brief didn't need editing, but parser-effects brief still pointed workers at SurfaceType.Arrow / SurfaceItem.Fn (pre-refactor shapes) and framed Arrow-vs-Fn as a worker placement choice. Refactor brief req 3 mandates declared_effects lands on BOTH FnSignature and SurfaceArrow as a co-invariant. Two-file fix: 1. Parser-effects brief: removed worker-placement-choice language; declared_effects now mandated on both FnSignature + SurfaceArrow. Read-first cites the refactor brief as pre-prereq. Pre-flight check added as slice step 1. Coproduct dissolution receipt requirement removed (no new variants — purely additive field on two existing carriers); replaced with structural-carrier rationale requirement. Title + smoke test + acceptance updated to reference FnSignature. 2. Refactor brief: line 152 reworded to reflect that parser-effects brief WAS updated alongside, not "doesn't need editing." P1 Documentation Describes Live State / P2 Boundary Discipline violation closed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Fixed in Two-file fix: Parser-effects brief ():
Refactor brief ( line 152):
Re-review against HEAD ( |
|
Correction to prior comment: shell ate the filenames in parens. The two files edited:
|
|
Review metadata
Findings
Verdict: APPROVE_WITH_COMMENTS. The diff is docs-only and the actual requirements/acceptance largely enforce the intended illegal-states-unrepresentable shape; I only found stale “Arrow once” wording that could mislead a later worker. |
|
Review metadata
APPROVE — Docs-only diff. New brief plus a coordinated update to the sibling parser-effects brief; no code, dag, or generated artifacts touched. The new brief itself reads as discipline-aligned planning: it explicitly invokes Exploratory observations (optional):
|
|
Fixed in Re-review against HEAD ( |
… variant span Claude API exploratory at sha fbdd0fb flagged two pre-dispatch tightenings: 1. The STOP for "ArrowInput simplification if type-position doesn't accept named binders today" was a worker-time deferral. Verified at HEAD (src/v3/compiler/parse_parser_body.txt:864-877 parse_atom_type reads inputs via parse_type_expr_list_until — pure type expressions, no binding): type position is anonymous-only today. Pre-resolved. - ArrowInput coproduct dropped entirely. - SurfaceArrow.inputs simplifies to List<SurfaceType> (today's shape preserved, just routed through the typed sub-carrier). - Coproduct dissolution receipt requirement removed; replaced with structural-carrier rationale for NamedArrowInput. - STOP rephrased: STOP if dispatch grep surfaces a parse path that does accept fn(x: A) -> B (would invalidate the simplification). 2. Two-spans-per-arrow risk (SurfaceType::Arrow wrapper variant + wrapped SurfaceArrow.span). Resolved: the wrapper variant carries no separate span; SurfaceArrow.span is source-of-truth. Brief is now leaner and pre-resolves the worker-discretion question that Claude API recommended pre-resolving. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Both Claude API exploratory observations resolved in Observation #1 — ArrowInput simplification escape hatch. Verified at HEAD: Pre-resolved by:
Observation #2 — two-spans-per-arrow risk. Net: brief is leaner — single new carrier ( Re-review against HEAD ( |
|
Review metadata
1. Story of the diffThis PR is a docs-only sequencing change for the unenumerated-effects chain. It adds a new pre-prerequisite worker brief that reframes the parser surface so 2. Invariant categories
The level choice is right, but the expected value is wrong for that level:
3. VerdictREQUEST_CHANGES The main refactor brief is structurally solid and does the right modeling work around illegal states and carrier distinction. I would fix the stale parser-effects smoke-test expectation before merge, because it directly contradicts the parser/lowerer authority split the brief is trying to protect. |
…stage tests OpenAI-pro BLOCKING at sha 6a5a9f5: parser-effects brief smoke test asserted declared_effects = [ReadEffect] at the parser-output stage, but req 1 declares the parser carrier as List<SurfaceType> and req 5 assigns OperationEffect resolution to the lowerer. Asserting the resolved variant at the parser-output boundary either makes the parser do semantic resolution too early (boundary violation) or pins a post-lowerer fact at the parse boundary (facts-don't-flow-forward). Fix: split into two smoke tests at the right stage boundaries — parser smoke asserts the SurfaceType::Named "Read" reference; lowerer smoke asserts resolution to ReadEffect. Acceptance bullet updated to match. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Fixed in The contradiction. Parser-effects brief req 1 declares the parser carrier as Fix. Split into two smoke tests at the right stage boundaries:
Acceptance bullet updated:
Re-review against HEAD ( |
|
Review metadata
Loop summary5 review rounds over ~50 minutes: first review at 2026-04-25 15:34:22Z, last at 16:24:03Z. The attached history contains 5 codex reviews and 4 non-codex API reviews; it does not contain a separately labeled chatgpt-review-5ab904d4-ab03-44… Forward progress evidenceThe loop made real modeling progress. The first codex finding blocked on exactly the right class of issue: chatgpt-review-16e3e559-06c3-44… Those findings did graduate into a better brief shape: the final design moves declaration-position inputs to chatgpt-review-5ab904d4-ab03-44… The loop also caught cross-brief authority drift. A codex round found that the sibling parser-effects brief still pointed workers at Consumers enabled: none mechanical. No parser, lowerer, emitter, interpreter, or test consumer lands in this PR. The current diff is still a dispatch/planning artifact. The brief’s own text says effects do not land here and are left to the next sub-lane ( Scaffolds dissolved: no code scaffolds dissolved. Design-review scaffolds were paid down: Invariants graduated: no new Debt accumulation evidenceThe PR adds one new planning scaffold: a pre-prereq brief whose only real consumer is a future worker. It has accounting—STOP conditions, acceptance checks, and a stated handoff chain—but it is still substrate-growing prose with no mechanical consumer yet. P5 allows scaffolds only when they have named dissolution triggers; this brief mostly has those, but the actual dissolution is deferred to later implementation PRs. chatgpt-review-7c4cfb62-911d-43… The newest debt is stale authority residue, not code unsoundness. The final diff still contains wording that appears inconsistent with the resolved design: it says type-position The review loop is also trending toward cheaper fixes. Early rounds forced structural modeling changes. The last round found a non-blocking stale “Arrow once” wording issue after the sibling brief had been updated. Claude’s final exploratory note still sees an “open door” around No consumer test protects this. The brief asks a future worker to add smoke tests, grep counts, DB-8 fixed-point checks, and full workspace checks, but those are future acceptance criteria, not present receipts ( Cheating signalMostly documented, not hidden. The implementer is not silently smuggling a shortcut into code. The compromises are surfaced as STOP-AND-ESCALATE conditions, carrier-distinction rationale, “no generated hand edits,” no-wildcard requirements, grep-count requirements, and future smoke tests. But the most recent fixes are prose-local, not structural. The loop has moved from “make invalid states unrepresentable” to “remove stale words from the dispatch brief.” That is not a failure yet, but it is the point where another broad review round has diminishing returns unless it is tightly scoped. The cheating signal is therefore: low hidden-cheating risk, moderate exhaustion risk. The debt is visible, but the loop is starting to accept “good enough wording” in a document whose entire purpose is to be unambiguous dispatch authority. Path to convergenceThe smallest next actions that justify KEEP_ITERATING are:
Do not run another open-ended review round after that. If those three actions land and the next review still finds only wording polish, stop reviewing this docs PR and move to the implementation PR where real consumers can define correctness. Meta-verdict📈 KEEP_ITERATING — the loop is still making real progress, but only one tightly scoped iteration is justified. The next round must be a carrier-authority cleanup and handoff receipt, not another general design review. |
|
Review metadata
This is a docs-only PR adding a refactor brief and updating a sibling brief. No Rust code changes; INVARIANTS / CODING / TESTING discipline anchors don't directly apply to brief contents, but internal consistency does — these briefs are dispatch authority for downstream PRs. Findings
VerdictAPPROVE_WITH_COMMENTS — the briefs are coherent and the Director's (b)(2) sequencing rationale is sound. The two stale Exploratory observationThe carrier-distinction rationale (req 3) is doing real work — |
…y scrub OpenAI-pro meta-review at sha 91504d8 gave a tight three-action path to convergence. Executing. Action 1: Live carrier table added to refactor brief frame section. Single source of truth for which carrier sits at which position (declaration vs. type), what input element shape each takes, what's retired, what's absent in this PR, what's deferred to a future PR. Action 2: Mechanical stale-authority sweep across both briefs: - Refactor brief: line 35 `(A, B) -> C` → `fn(A, B) -> C` (parser requires fn(...) prefix per parse_atom_type KwFn gate). - Refactor brief: line 46 same fix. - Refactor brief: dropped duplicate STOP at line 134 referencing removed ArrowInput::Named(NamedArrowInput) shape — same condition already covered by line 129 STOP in the cleaned-up form. - Refactor brief: line 144 non-goals cleaned up "wraps each entry in ArrowInput::Anonymous { ty }" → describes actual post-PR shape (List<SurfaceType> at type-position layer). - Parser brief: lines 22 + 57 dropped `ArrowInput` from the pre-flight carrier list (it's not introduced). Action 3 (PR body handoff receipt) follows in the next push. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Convergence sweep landed in Action 1 — live carrier table. Added to the refactor brief frame section (and mirrored in PR body). Single source of truth for which carrier sits at which position, what's retired, what's absent in this PR, what's deferred. Action 2 — mechanical stale-authority sweep. Across both touched briefs:
Remaining Action 3 — PR body handoff receipt. Updated PR description with the live carrier table + a concrete consumer-obligation checklist for the implementation worker (parser smoke for top-level + higher-order, post-refactor grep counts, Per the meta-review's instruction: stopping further open-ended review rounds on this docs PR. Re-review against HEAD ( |
|
Review metadata
Findings
Verdict: REQUEST_CHANGES. The intended |
|
Both findings at sha
Verified at HEAD: Exploratory observation noted — asymmetric Re-review against HEAD ( |
|
Both BLOCKING findings at sha
Verified at HEAD ( Re-review against HEAD ( |
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
a30f3b19· Trigger:schedule - Thinking:
184s wall
Non-blocking — Strengths
docs/briefs/t-impossiblebugs-unenumerated-effects-fn-arrow-refactor-worker.mdThe carrier table and requirements now give one coherent post-refactor model forFnSignature,SurfaceArrow, retiredSurfaceParam, and absentArrowInput.docs/briefs/t-impossiblebugs-unenumerated-effects-parser-worker.mdThe parser-effects brief now preserves the parser-vs-lowerer stage boundary with separate smoke tests for surface references and resolvedOperationEffects.
✅ Docs-only diff looks clean; no blocking concerns.
|
Verified at HEAD Current line 71 is part of the live carrier table, not the rejected shape. Searched at HEAD:
The "rejected illegal-state shape" the finding describes is No action. |
|
Verified at HEAD Current state at HEAD — req 5 lowerer text (line 95):
There is no "planned lowerer diagnostic for Searched at HEAD: No action. |
|
Review metadata
1. Story of the diffThis PR is a docs-only coordination change for the unenumerated-effects chain. It adds a new pre-prereq worker brief that says the parser-effects work should not bolt 2. Invariant categories
3. VerdictAPPROVE_WITH_COMMENTS. The diff is structurally sound as a docs/briefs refactor plan: it moves the chain toward typed carriers, illegal-state exclusion, and stage-specific tests. I would clean up the stale “Arrow-vs-Fn placement choices” reporting sentence before or after merge, but the load-bearing requirements now point to the right model. |
…parser-effects reporting OpenAI-pro non-blocking at sha a30f3b1: parser-effects brief reporting line still asked the worker to document Arrow-vs-Fn placement choices, but the same diff already fixed placement as co-invariant on both FnSignature + SurfaceArrow per refactor-brief req 3 ("not a worker placement choice"). Boundary-discipline / single-authority drift on a one-liner. Updated to ask for the carrier-distinction rationale instead. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Fixed in Re-review against HEAD ( |
…cted scope (codex P2 on #835) Codex P2 inline at r2-impossible-bugs-manager.md:63: design-brief filenames were missing the canonical -design suffix; the actual files are t-impossiblebugs-*-design.md. Audit revealed a bigger correction needed than just filename suffix: 1. Worker briefs ALREADY EXIST for all three classes (I had said 'needs Director conversion to worker briefs' — wrong). Correct state: - Nested-optional flatten: design + worker (DESIGN/SCOPING shape) authored - Unhandled diagnostic paths: design + worker (DESIGN/SCOPING shape) authored - Unenumerated effects: design authored, prior worker briefs SUPERSEDED 2026-04-25 by design doc 2. The two non-effects workers are DESIGN/SCOPING shape — they produce substrate proposals, not direct implementation. Manager role is dispatch + Substrate-Manager-handoff coordination, not convert-design-to-worker. 3. Effects has SUPERSEDED workers (closed-system framing dissolved the prior lens-vs-declaration framing). Manager owns design-doc routing + post-supersede implementation worker authoring against the canonical design. 4. Fn→Arrow refactor (PR #805) reframed as independent vestigial- syntax cleanup, not direct effects-framing prereq. Three coordinated fixes in r2-impossible-bugs-manager.md: - Program scope table: canonical filenames + per-class authored-status + SUPERSEDED notes - Owned deliverables: 'Manager dispatches existing worker' (not 'convert design to worker') - Sub-briefs section: explicit Authored/SUPERSEDED/Pending tri-state with full canonical paths Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… readiness) (#835) * docs(briefs): pre-stage 6 R2 manager briefs (PM portion of R2 spin-up readiness) Per user direction: every lane/brief/design must be authored before R2 managers spawn. PR #827 (merged) named the 6-manager structure; Transition mechanics step 4 said "pre-stage skeletons during R1 final week" — accelerated to "pre-stage now." This PR lands all 6 R2 manager briefs as one bundle, structured consistently: - Status (PROPOSAL pre-spawn, spawns on R1 close) - Orient before reading (R2 structure authority, scope source, cross-program coordination, demo coordination) - Program scope (the lane/sub-program scope this manager owns) - Owned deliverables (table of lanes/sub-lanes with status) - Cross-program dependencies (produces/consumes signals) - Autonomous dispatch authority (what manager does without Director) - Reporting cadence (where signals flow) - Sub-briefs (authored / pending) - Working state (placeholder for fill on spawn) - Cross-refs Six briefs: 1. r2-grounding-manager.md — T-Ground sub-program (the one true R2 critical path: Pilot → Rust → Engine → Tests → Dissolve, with Python/Go fill). Migrates from grounding-manager.md (which archives on R2 promotion). Names Engine sharpened-(b) consumer dependency on Substrate Manager's ValueBody-list/sum carrier. 2. r2-substrate-manager.md — T-Substrate (4 sub-lanes) + B4 Identity-Carrier Substrate Pass program (12 sub-briefs). Largest single program in R2; produces 4 carriers consumed by Modeling (3 sub-lanes) + Grounding (Engine sharpened-(b)). Names watch condition for B4 split if Substrate becomes the new bottleneck. 3. r2-modeling-manager.md — T-Modeling (3 Goal 2 items + tokenizer charclass phase-2 added per shared T-Substrate dependency). All gated on Substrate Manager carrier readiness. 4. r2-impossible-bugs-manager.md — T-ImpossibleBugs (3 R2+ classes: nested-optional flatten, unhandled diagnostic paths, unenumerated effects). Design docs already authored (#798, #801, #808+#805 prereq); needs Director conversion to worker briefs. 5. r2-pure-bootstrap-manager.md — POST-R1 only per gate-vs-program resolution in PR #827. Migrates from pure-bootstrap-zero-manager.md with scope narrowed (does NOT duplicate R1 T-PB-A/T-PB-B census- reduction work). Owns Tier 3 mirror dissolutions + Tier 2 patch_lower_helpers retirement + post-R1 emergent dissolutions. 6. r2-release-manager.md — Goal 5 (§6a metadata-pick) + Goal 6 (R2 demo coordination) + B-wave Tier 0/2 dispatch (#810) + discipline framework central reporting + thesis-claim coverage mapping (Open call 1) + R2 closure ledger + v2 retirement. Single authority for closure ledger and demo coordination. Each brief explicitly defers to ROADMAP/THESIS/r2-structure.md for upstream authority; does not duplicate gate semantics or scope decisions. Cross-program coordination via R1 `Cross-manager notifications queued` brief pattern. Coordination split with Director on inbox #828: Director takes the worker-level briefs (B4.2/B4.3/B4.4 + T-Substrate sub-lane scoping + T-Modeling worker briefs + T-ImpossibleBugs design→worker conversion); PM takes §6a + B5/B6/B7 + thesis-claim mapping in follow-up PRs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(briefs): r2-impossible-bugs-manager — canonical filenames + corrected scope (codex P2 on #835) Codex P2 inline at r2-impossible-bugs-manager.md:63: design-brief filenames were missing the canonical -design suffix; the actual files are t-impossiblebugs-*-design.md. Audit revealed a bigger correction needed than just filename suffix: 1. Worker briefs ALREADY EXIST for all three classes (I had said 'needs Director conversion to worker briefs' — wrong). Correct state: - Nested-optional flatten: design + worker (DESIGN/SCOPING shape) authored - Unhandled diagnostic paths: design + worker (DESIGN/SCOPING shape) authored - Unenumerated effects: design authored, prior worker briefs SUPERSEDED 2026-04-25 by design doc 2. The two non-effects workers are DESIGN/SCOPING shape — they produce substrate proposals, not direct implementation. Manager role is dispatch + Substrate-Manager-handoff coordination, not convert-design-to-worker. 3. Effects has SUPERSEDED workers (closed-system framing dissolved the prior lens-vs-declaration framing). Manager owns design-doc routing + post-supersede implementation worker authoring against the canonical design. 4. Fn→Arrow refactor (PR #805) reframed as independent vestigial- syntax cleanup, not direct effects-framing prereq. Three coordinated fixes in r2-impossible-bugs-manager.md: - Program scope table: canonical filenames + per-class authored-status + SUPERSEDED notes - Owned deliverables: 'Manager dispatches existing worker' (not 'convert design to worker') - Sub-briefs section: explicit Authored/SUPERSEDED/Pending tri-state with full canonical paths Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * WIP: gunbc PM * fix(briefs): add pre-spawn vs post-spawn authority subsection to all 6 R2 manager briefs (codex P2 on #835) Codex flagged ownership ambiguity in r2-impossible-bugs-manager.md: the brief said design/scoping docs would be 'converted to worker briefs by Director' but elsewhere said the manager authors all worker briefs autonomously. Without an explicit phase boundary (pre-spawn vs post-spawn), ownership is ambiguous and dispatch can stall. Resolution applied uniformly to all 6 briefs: new 'Pre-spawn vs post-spawn authority' subsection inserted before 'Autonomous dispatch authority': - Pre-spawn (now, before R1 close): Director + PM coordinate on brief authoring per inbox #828 split. PM authors the manager skeleton; Director authors worker-level briefs not yet existing. Both stop authoring once R2 spawns. - Post-spawn (R2 promotion onward): Manager owns all worker-brief authoring autonomously per Autonomous dispatch authority. Director narrows to cross-program conflict resolution + scope-change escalation. Release Manager variant has the same boundary plus an explicit note that PM also authors the §6a / B5 / B6 / B7 / thesis-claim-mapping briefs as Release-Manager-portion PM deliverables (per inbox #828). The phase boundary is now structurally explicit: no dispatch stall from both Director and Manager assuming the other owns authoring. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(briefs): tighten Pending-line authority qualifier (codex BLOCKING on #835 sha 3803266 :90) Codex flagged the 'Pending — Director-authored per coordination on inbox #828:' lines as creating dual authority — the line read in isolation contradicted the 'Manager authors autonomously' framing elsewhere. The d42f17e phase-boundary subsection resolved this contextually, but a reader scanning just the Pending line could still read it as a permanent assignment. Surgical tightening: add explicit pre-spawn qualifier inline so the Pending line is self-resolving without requiring the reader to cross-reference the phase-boundary subsection. Old: 'Pending — Director-authored per coordination on inbox #828:' New: 'Pending — pre-spawn Director-authored per inbox #828 coordination split; post-spawn manager-authored autonomously per "Pre-spawn vs post-spawn authority" subsection above:' Applied to 4 briefs (Modeling, Substrate, Pure Bootstrap, Release). Release variant uses 'PM-authored' instead of 'Director-authored' since R2 Release Manager's pre-spawn portion is PM-owned per inbox #828 split (the §6a / B5 / B6 / B7 / thesis-claim-mapping briefs). The Pending line now reads cleanly in isolation: pre-spawn / post- spawn boundary is explicit at the line itself, not deferred to a cross-reference. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(briefs): resolve openai-pro REQUEST_CHANGES on #835 sha bfaab66 Two surgical fixes for the two BLOCKING findings (P2 + P5): 1. r2-release-manager.md:67 — B7 dual-authority contradiction. Was: "Authors all T-Release worker briefs without Director (§6a pick, B5/B6/B7, ...)" But B7 is "Cross-manager signal, not a worker brief" per :33 + :86. Now: "Authors all T-Release owned deliverables ...: worker briefs (§6a pick, B5, B6, thesis-claim coverage mapping) and cross-manager signals (B7 priority-hint relay)." — distinguishes briefs from signals, no item carries two contracts. 2. r2-grounding-manager.md:62 — Pending line unbounded across pre/post spawn. The other 4 briefs got the "pre-spawn Director-authored; post-spawn manager-authored" temporal qualifier in bfaab66; Grounding was missed. Same pattern applied here. Both fixes mechanical; no scope or authority change beyond removing the ambiguity openai-pro flagged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(briefs): resolve codex BLOCKING on #835 sha bfaab66 — stale §6a + B4.1 status Two codex BLOCKING findings, both about briefs copying status from earlier state without verifying against live receipts: 1. r2-release-manager.md §6a — DECISION already locked. docs/design-substrate-carrier-port-program.md §6a:171 says "pick **Option 3, unified MethodContract carrier**." :173 names the live receipt (src/v3/std/algebra.dag declares MethodContract; src/v3/lenses/cost.dag imports it via method_contract_cost_shape). :175 names the dissolution trigger (size_effect / cost_shape / callback_element_position field-by-field retirement). Brief was framing this as "DECISION BRIEF NOT YET AUTHORED — write up the 4 options ... recommend one based on E-I evidence." Stale. Fix: rename "pick decision brief" → "follow-through brief"; status from "NOT YET AUTHORED" to "DECISION LOCKED — Option 3 ... live receipt landed"; describe remaining work as bulk migration + dissolution-trigger tracking. Updated the deliverable table row, the Core deliverables list, the Autonomous dispatch authority line, the Sub-briefs Pending list, and the Cross-refs §6a source. 2. r2-substrate-manager.md B4.1 — BLOCKING already resolved. PR #819 ("docs(briefs): add B4.1a DeclarationRef runner migration brief") merged 2026-04-26 01:13:32. The §0.2 scope gap was resolved in 6f564f5 BEFORE merge per Director receipt on inbox #828. B4.1a follow-on brief landed in the same PR. Real open residual is the first-consumer migration at PR #826 (regen drift on r1_gates.dag — worker CI-fix, not brief authoring). Brief was still saying "DRAFTED (with §0.2 BLOCKING outstanding — codex finding on PR #819)" and "with outstanding BLOCKING ... resolution pending." Stale on both the BLOCKING and the residual shape. Fix: status to "BRIEF LANDED (PR #819, merged 2026-04-26 — §0.2 scope gap resolved in 6f564f5 before merge); B4.1a runner-migration follow-on brief landed same PR. Real residual: first-consumer migration #826 OPEN with regen drift (worker CI-fix)." Updated the deliverable table row, the Sub-briefs Authored list, and the Cross-refs adjacent line. Both findings: feedback_verify_thesis_claims violation on the PM authoring side. Two surgical text updates per finding; no scope or authority change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(briefs): anchor §6a follow-through against existing pick worker brief Codex inline BLOCKING on r2-release-manager.md:30 surfaced that docs/briefs/t-permethodmetadata-pick-worker.md (landed PR #794) already exists as the pick-worker brief. My prior fix (74b679b) reframed "pick decision brief" → "follow-through brief" but didn't reference the existing worker, leaving readers to wonder if the follow-through was re-picking. Two precision tightenings: - "Pick is closed." Names the worker brief explicitly + cites its scope-closure clause ("Do not migrate all consumer lenses ... bulk migration is post-pick work"). - "No duplicate decision authority — pick is closed; follow-through is post-pick scope." Closes the P2 single-authority concern codex named. Surface change only; no scope expansion. The follow-through scope (bulk migration + dissolution-trigger tracking) is unchanged from the 74b679b state — what's added is the explicit worker-brief anchor. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(briefs): resolve openai-pro APPROVE_WITH_COMMENTS on #835 sha 3260d71 Finding (P2 single-authority): T-Ground-Rust had two contradictory states — deliverables table at :23 said DISPATCHED, but Sub-briefs Pending list at :62-63 listed "T-Ground-Rust full implementation" as pending pre-spawn work. Same lane, two authoritative states. Audit: T-Ground-Rust full lane (Rust target-spec primitive declarations end-to-end) has not been authored. Pilot (PR #765) and Engine Phase 1 typestructure (PR #788) are separate dispatched lanes (their own rows in the table); the "DISPATCHED (Engine implementation parked pending loader-close)" parenthetical was a status leak from the Engine row's parking note. Fix: row status now reads "NOT YET AUTHORED — listed under Sub-briefs Pending below; gated on pre-spawn Director scope refinement per inbox #828. (Pilot PR #765 + Engine Phase 1 typestructure PR #788 are separate dispatched lanes — see those rows; the prior 'DISPATCHED' status here was a parenthetical leak from the Engine row's loader-close parking note.)" Now table status matches Sub-briefs Pending list. Single authority restored. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(briefs): refresh impossible-bugs manager against PR #836 merge Codex BLOCKING on r2-impossible-bugs-manager.md:78 (sha bfaab66) was correct in spirit and now newly actionable: the brief's Pending section re-dispatched the older DESIGN/SCOPING workers (t-impossiblebugs-nested-optional-flatten-worker.md + t-impossiblebugs-unhandled-diagnostic-paths-worker.md) even though their design docs (PR #798 + PR #801) had landed with next-step recommendations + PR #836 just authored the IMPLEMENTATION workers (r2-impossible-bugs-{nested-optional-flatten,unhandled-diagnostic-paths, unenumerated-effects}-worker.md). Re-dispatching DESIGN/SCOPING workers when implementation workers are authored = duplicate decision authority under P2 + accumulating ad-hoc state under P5. Codex was right. Three sections updated to reflect PR #836-merged state: ## Program scope table (lines 17-19) Reframed columns: "Design authority + implementation worker (post PR #836 merge)" / "Implementation status" / "Substrate gating". Each class row now names: - Design doc PR + closed-in-scope status - Implementation worker filename (PR #836) + IMPLEMENTATION WORKER LANDED - UNGATED status per design-doc audit (Director's reframes #1, #2 confirmed no substrate gates — substrate-constructor invariant for nested-optional; totality-by-omission for unhandled-diagnostic; closed-system for effects) The OLD DESIGN/SCOPING workers are explicitly named SUPERSEDED for unenumerated-effects already; nested-optional + unhandled-diagnostic older workers are now also marked superseded by their PR #836 implementation counterparts. ## Owned deliverables (lines 25-31) Reframed from "Worker brief is already authored ... DESIGN/SCOPING shape" to "Implementation worker brief landed on main via PR #836 merge ... do not re-dispatch the older workers." Substrate-gap escalation reframed as the exception path (was the expected path under the older DESIGN/SCOPING worker assumption); expected path is direct implementation per design-doc Director-actionable recommendation. ## Sub-briefs Pending (lines 78-86) Reframed from "Dispatch nested-optional-flatten worker (DESIGN/SCOPING produces substrate proposal → escalate)" to "Dispatch nested-optional-flatten implementation worker (ungated; dispatchable Day-1 post-spawn)" + same pattern for the other two classes. PR #836's 3 implementation workers are now the canonical dispatch targets. Added explicit SUPERSEDED list for the older workers (4 entries: 2 DESIGN/SCOPING + 2 effects-worker variants) with their respective implementation-worker successors named. ## Discipline note This finding was real, not an echo. PR #836 merging changed the substrate of facts the manager brief grounds against. Same class as the §6a stale framing on Release Manager + the B4.1 stale BLOCKING on Substrate Manager: brief authored against pre-merge state; merge surfaces the staleness. The matrix's pre-author verification invariant catches state-drift at authoring time; the matrix's status-consistency rule catches dual-state within a single brief. This finding is a third class: cross-PR state drift (brief A's Pending list cites brief B's content; brief B merges and brief A's content goes stale). Worth noting as a refresh-discipline trigger separately from authoring discipline. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 participant
Summary
Pre-prereq sub-lane brief authored post-
sunny-otter-128STOP-AND-ESCALATE on the parser-effects brief (#799). Worker verified the parser brief'sdeclared_effects on SurfaceType.Arrowplacement does not load-bear onSurfaceItem.Fnbecause Fn today carriesparams+return_typeas two separate fields, not as anArrow-shaped signature.Per parser-brief STOP #2, Director picked (b)(2): structural refactor first.
Chain
Live carrier table (post-this-PR)
fnitems)FnSignature { inputs, output, span }NamedArrowInput { name, ty, refinement: SurfaceExpr? }namemandatory; refinement-without-binder structurally unrepresentablefn(...)higher-order types)SurfaceType::Arrow(SurfaceArrow { inputs, output, span })(no span on variant)SurfaceTypeSurfaceParamNamedArrowInput; zero references post-refactorArrowInputfn(x: A) -> Bsyntax is added, that PR introduces the type-position carrier with its own receiptWhat this PR does (docs-only)
t-impossiblebugs-unenumerated-effects-fn-arrow-refactor-worker.md(this brief).t-impossiblebugs-unenumerated-effects-parser-worker.mdto match the new shapes (declared_effects co-invariant on bothFnSignature+SurfaceArrow; pre-flight check; parser/lowerer-stage smoke split).Handoff receipt — first real consumer obligations
The implementation worker dispatched against this brief MUST:
fn foo(x: Int, y: Bool) -> String { ... }→Fn { signature: FnSignature { inputs: [NamedArrowInput { name: "x", ... }, NamedArrowInput { name: "y", ... }], output: String, .. } }.fn higher_order(f: fn(Int, Bool) -> String) -> Int { ... }→NamedArrowInputforfwhosetyisSurfaceType::Arrow(SurfaceArrow { inputs: [Int, Bool], output: String, .. }).SurfaceType::Arrow+SurfaceItem::Fn/FnExternalBodyin PR body (snapshot grep at brief-time was ~5 + ~20; verify at dispatch).SurfaceParamhas zero references post-refactor (grep + cite count in PR body).NamedArrowInput+FnSignaturevsSurfaceArrowcarrier-distinction rationale required in PR body.FnSignatureexhaustively without wildcard arms (no runtime non-Arrow check needed — typed sub-carrier eliminates the case structurally per req 3).cargo test --workspace --exclude v2-compiler-tests/clippy --all-targets -- -D warnings/fmt --all --checkclean.Why (b)(2) and not (a) or (c)
Fn-only effects weakensfeedback_no_annotationsdiscipline anchor (effects can't appear in higher-order function types).Arrow+Fncarry effects isfeedback_parallel_representation_debt— two carriers for the same concept.feedback_construction_over_ratchets. Function declarations are arrows; the param/return split is a vestige. Cost lands once.Test plan
🤖 Generated with Claude Code