Conversation
72618a7 to
4552477
Compare
|
Rebased onto Still valid — checked, not assumed. A caution on that check: grepping for Pre-existing failures are upstream's, not this PR'sTwo failures on the full opencode suite, both reproducing on clean
Same failing set with and without this branch; counts vary 2→1 across runs on both. Matches #220. Typecheck passes; core 295/0 on the new baseline. |
…tion Consecutive host user records collapse into one wire user message, so a single boundary can legitimately carry several transition markers. The per-message count guard rejected that shape, killing the turn with a synthetic 400; a failed turn leaves a contentless assistant record that cannot separate the user records around it, so the fault self-amplified. Drop the redundant count guard — the flat checks already pin the ordered token identity across all messages. Scope-check every transition on a boundary, not just the first, and emit the last transition's effort for a merged boundary (the first would apply a stale value).
4552477 to
fc305a1
Compare
1 participant
Fixes the
Multiple internal Fable 5.1 effort markers on one user boundaryvariant of #237. Live failure — four occurrences, each a synthetic HTTP 400 that kills the turn:The last two are on a build carrying #238, which fixed the other variant (
anchor_placement). Separate mechanism, same file.Root cause: two sides disagree about what a boundary is
Markers are attached per host record —
effort-history.ts:391:Markers are validated per wire message —
effort-history.ts:611:consumeInternalMarkersgroups by wire message. Anthropic requires strictly alternating roles, so consecutive host user records collapse into one wire user message and both markers land on a single boundary. The invariant is broken by construction.Why it clusters
Measured from the host DB, not inferred:
parts=0, textparts=0for every errored message id checked. A contentless assistant lowers to nothing, so it cannot separate the user records on either side of it.So each failure widens the next request's merge:
Self-amplifying, which is why every occurrence sits in a run rather than alone.
The change
1. Dropped the per-message count guard. Duplication and ordering are already enforced globally on the flat array: the count check (
:690), the orderedplanDigest(:695-706), and positional token comparison in the trimmed path (:666-675). The per-message check added no protection those lack — it encoded a structural assumption the lowering violates.2. Scope-check every transition on a boundary, not just
[0]. With N>1 permitted, a foreign-scope marker at index 1 previously bypassed the explicit check.3. Emission takes the last transition, not the first —
:724,transitions[0]→transitions.at(-1).Point 3 is the one that makes this safe. A marker means "effort became X at this boundary"; when two records merge, the effort going into that wire message is the last one's. Relaxing the guard without this would turn a loud 400 into a silent wrong-effort request — strictly worse than the crash it replaces.
Verification
Three mutations, re-run independently of the implementer's report, tree restored to clean between each:
> 1throwaccepts a merged boundary whose transitions are a correctly-ordered run and emits the last effort— throwsMultiple internal Fable 5.1 effort markers on one user boundarytransitions[0]effort: "high"received where"max"expected[0]rejects a foreign-scope marker at index 1 of a merged boundary—Expected substring: "…scope mismatch"/Received: "…non-prefix loss"The second is the important one: it proves the test asserts the emitted effort value, not merely that nothing threw. Without that assertion the
[0]→at(-1)fix would be untested and the silent-wrong-value path would ship green.On the third — the explicit scope check is defense-in-depth, not the only line. With it narrowed back to
[0], the foreign-scope marker at index 1 is still caught, by the flat check, asnon-prefix loss. The mutation changes which error fires, not whether it fires. Worth stating plainly rather than overclaiming: the check buys a precise diagnostic, not a closed hole.Gates: core 199/0 · opencode 1898/0 · pi 114/0 · typecheck clean.
Scope
Untouched: the anchor logic (
:623-656; anchors are placed on the current boundary only and re-stripped every request, so merging cannot produce two), the attach site (:362-395), and every flat check. Holes, reordering, mutation, duplication, and scope mismatch all stay fail-closed.One existing test in
index.test.tsasserted the removed guard's message for a duplicated marker. It now includes the anchor so it exercises the flat count check, and expectscorrelation failed: expected 1, found 2. Duplication still fails closed — 400, request never sent.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Fixes the crash when multiple effort markers land on one merged user boundary, so those requests no longer die with a synthetic HTTP 400. Consecutive host user records collapse into one wire message, so a boundary legitimately carries several markers; the old per-message guard rejected that shape. A failed turn leaves a contentless assistant record that cannot separate the user records around it, which is why the fault self-amplified.
Key changes
transitions.at(-1)) so a merged boundary applies the correct effort value instead of a stale one.Duplication, reordering, and scope mismatch still fail closed with a 400 before the request is sent. The existing duplicate-marker test now exercises the flat count check and expects the correlation error message.
Written for commit fc305a1. Summary will update on new commits.
The PR appears safe to merge; no actionable correctness, security, or repository-rule violations remain.
Summary
This PR corrects OpenCode effort-history handling when consecutive host user records merge into one wire-level user boundary.
Diagram
%%{init: {'theme': 'neutral'}}%% flowchart LR A[Consecutive host user records] --> B[One merged wire user boundary] B --> C[Consume all effort transitions] C --> D{Correlation, order, scope, and anchor valid?} D -->|No| E[Reject request] D -->|Yes| F[Select last transition] F --> G[Insert one system effort control] G --> H[Remove internal markers and forward]Reviews (2) · Last reviewed commit: "fix(effort): accept merged effort bounda..."