Skip to content

docs(evaluator): PR-E E5 loop readiness audit - #1401

Merged
briansrls merged 3 commits into
mainfrom
feat/r3-evaluator-e5-loop-readiness
May 1, 2026
Merged

briansrls merged 3 commits into
mainfrom
feat/r3-evaluator-e5-loop-readiness

Conversation

@briansrls

Copy link
Copy Markdown
Contributor

Summary

  • add the PR-E E5 loop readiness/blocker audit
  • cross-link the E5 dispatch brief to the audit
  • document LoopNode fields, Cardinality/Descent handling, frame-state interactions, and the exact resume gate before executable loop work

Scope

  • docs-only readiness receipt
  • no Transform/Branch/Bind implementation
  • no strategy carrier widening
  • no substrate, runner, or bootstrap changes

Validation

  • git diff --check
  • pre-push: cargo fmt --all --check

@briansrls
briansrls marked this pull request as ready for review May 1, 2026 11:53
@briansrls

Copy link
Copy Markdown
Contributor Author

Manager pass: scope looks correct for the E5 dispatch. This is docs-only, names the concrete blocker without pretending zero-iteration or fixed-body support proves loop execution, and preserves the STOP+PING boundaries: no Transform/Branch/Bind implementation, no strategy widening, no substrate edits, no runner changes.\n\nNo manager-blocking changes requested. Hold for CI / scheduled reviews.\n\n— sent from snappy-moth-795

@briansrls

Copy link
Copy Markdown
Contributor Author

Review metadata

  • Provider / model: claude / claude-opus-4-7
  • Commit: 66ff1145 · Trigger: schedule
  • Comparison: origin/main @ 8cd53594 ... review/pr-1401-66ff1145 @ 66ff1145
  • Thinking: 17s wall

Docs-only PR. Cleanly scoped, faithfully describes the live substrate, and explicitly defers implementation.

Verdict: APPROVE — Docs-only readiness audit; accurately cites the live LoopNode/LoopBound shape, names the blocker (E1-only eval_node coverage), keeps scope to a receipt, and lists explicit non-goals. No invariant or modeling-discipline concerns in the diff.

@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: 66ff1145 · Trigger: schedule
  • Thinking: 193s wall

BLOCKING (1)

Root Cause

  • docs/briefs/r3-pr-e5-loop-readiness-audit.md Loop count decoding is specified as Int-shaped value decoding instead of a Nat/cardinality witness contract → require non-negative finite count decoding and add a negative-count fail-closed test before E5 can implement from this receipt.

⚠️ One blocking readiness gap remains in the future E5 acceptance contract.

1. Evaluate `loop_node.init` through `eval_port`.
2. Evaluate `count` through `eval_port`.
3. Decode the count witness fail-closed. The first executable slice should
accept only a runtime integer literal count; missing or non-integer counts

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: The rule allows a runtime integer literal count but never requires negative Int values to fail closed, so E5 could accept a non-cardinality loop bound despite the Decidability invariant's bounded-iteration contract.

@briansrls

Copy link
Copy Markdown
Contributor Author

Review metadata

  • Provider / model: cursor / composer-2
  • Commit: ac8710fa · Trigger: schedule
  • Comparison: origin/main @ 8895a5e4 ... review/pr-1401-ac8710fa @ ac8710fa
  • Thinking: 23s wall

Findings: None. The change is documentation only: a new readiness brief plus a short cross-link in r3-evaluator-dispatch.md. Nothing here conflicts with INVARIANTS / modeling-discipline (it explicitly records fail-closed behavior, defers Descent, rejects parallel loop carriers, and names a resume gate and test bar). CODING.md and TESTING.md govern Rust/tests; nothing in this diff touches them.

Verdict: APPROVE — The diff is small, honest about scope (“docs-only receipt”), and documents the E5 blocker and acceptance tests without landing substrate or implementation debt. No violations of the referenced rubric grounded in these edits.

@briansrls

Copy link
Copy Markdown
Contributor Author

Review metadata

  • Provider / model: openai-pro / gpt-5-5-pro
  • Commit: a8262c71 · Trigger: manual
  • Comparison: main @ 8895a5e4 ... feat/r3-evaluator-e5-loop-readiness @ a8262c71
  • Conversation: View conversation

1. Story of the diff

This PR turns E5 loop execution from a vague future acceptance target into an explicit readiness audit. docs/briefs/r3-evaluator-dispatch.md:107-110 now points readers to the new audit and states the core blocker: E5 cannot honestly prove accumulator threading until the body evaluator can run a loop body that consumes the iteration accumulator binding. The new audit documents the live LoopNode / LoopBound shape, defines the intended cardinality-loop execution rule, requires fail-closed count decoding, leaves descent loops as a named residual, and sets a resume gate plus required tests for the later implementation PR. The main mechanism is deliberately not implementation: docs/briefs/r3-pr-e5-loop-readiness-audit.md:3-5 says this is a docs-only receipt and does not implement eval_loop, change substrate carriers, widen strategy carriers, or add runner behavior.

2. Invariant categories

  1. LAYER MODEL — Compliant. The diff discusses substrate-facing loop carriers but does not modify substrate; it explicitly says E5 should consume the existing LoopNode / LoopBound shape directly and “must not add a parallel loop carrier or reinterpret LoopBound with string labels” at docs/briefs/r3-pr-e5-loop-readiness-audit.md:34-37, with substrate edits also excluded in the non-goals at docs/briefs/r3-pr-e5-loop-readiness-audit.md:124-128.
  2. INVARIANTS.md + modeling-discipline.md — Compliant. Fail-closed is handled directly: count decoding must accept only non-negative runtime integer literal counts, while missing, non-integer, or negative counts must produce typed diagnostics rather than defaulting to zero or wrapping unsigned at docs/briefs/r3-pr-e5-loop-readiness-audit.md:57-60. Single-authority / facts-flow-forward is also preserved by naming LoopNode.source as the candidate accumulator binding authority while requiring STOP+PING if lowering says another port owns it, rather than inventing a second authority, at docs/briefs/r3-pr-e5-loop-readiness-audit.md:81-87.
  3. CODING.md — Compliant. Although this is docs-only, the proposed implementation shape follows the repo’s data + free-functions style: eval_loop is specified as a free function over explicit inputs &Dag, &LoopNode, &mut EvalStateStack<Value>, and &EvalStrategy, returning Result<Value, EvalError> at docs/briefs/r3-pr-e5-loop-readiness-audit.md:44-50.
  4. TESTING.md — Compliant. No tests are required in this PR because no executable behavior changes, and the audit avoids dead API. The future implementation is constrained with behavior-driven required tests covering zero iteration, multiple iteration, accumulator visibility/threading, missing/non-integer/negative counts, descent residual, and stack restoration at docs/briefs/r3-pr-e5-loop-readiness-audit.md:107-118.
  5. LOCKED DESIGN DECISIONS — N/A. The diff references parent dispatch/scaffold authorities at docs/briefs/r3-pr-e5-loop-readiness-audit.md:7-9, but it does not reference or alter anything marked locked.
  6. TRACKED vs UNTRACKED DEBT — Compliant. The new blocker is tracked rather than left as ambient TODO debt: it is documented as an implementation blocker at docs/briefs/r3-pr-e5-loop-readiness-audit.md:3-5, bounded by scope/non-goals at docs/briefs/r3-pr-e5-loop-readiness-audit.md:120-128, and given a concrete dissolution/resume trigger at docs/briefs/r3-pr-e5-loop-readiness-audit.md:99-105.

3. Verdict

APPROVE. The PR is a docs-only readiness receipt that makes the E5 blocker explicit, preserves existing substrate authority, avoids fake partial implementation, and gives the later implementation PR a bounded resume gate and test checklist. I found no diff-anchored issue requiring changes.

@briansrls

Copy link
Copy Markdown
Contributor Author

Review metadata

  • Provider / model: claude / claude-opus-4-7
  • Commit: a8262c71 · Trigger: schedule
  • Comparison: origin/main @ 8895a5e4 ... review/pr-1401-a8262c71 @ a8262c71
  • Thinking: 11s wall

APPROVE — Docs-only PR adding a readiness audit for E5 plus a cross-link from the parent dispatch brief. Scope is clearly bounded ("does not implement…", explicit Non-Goals), the loop shape it describes matches the substrate it cites (LoopNode/LoopBound), and it correctly defers Descent as a named fail-closed residual rather than papering over it. No invariant, modeling, coding, or testing rules are touched by the diff.

@briansrls

Copy link
Copy Markdown
Contributor Author

Verified the earlier codex blocker against current head a8262c71: resolved. The readiness audit now requires count decoding to accept only a non-negative runtime integer literal count, and explicitly says missing, non-integer, or negative counts must fail closed rather than defaulting to zero or wrapping. The future implementation test list also includes LoopBound::Cardinality with negative integer count fails closed.\n\nCurrent checks: fmt, ci, and v3 are passing; self_host_ratchet is still pending. No manager patch needed.\n\n— sent from snappy-moth-795

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