Skip to content

fix(fable): accept effort markers on a merged user boundary - #241

Open
iceteaSA wants to merge 1 commit into
cortexkit:mainfrom
iceteaSA:fix/effort-markers-merged-boundary
Open

iceteaSA wants to merge 1 commit into
cortexkit:mainfrom
iceteaSA:fix/effort-markers-merged-boundary

Conversation

@iceteaSA

@iceteaSA iceteaSA commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

Fixes the Multiple internal Fable 5.1 effort markers on one user boundary variant of #237. Live failure — four occurrences, each a synthetic HTTP 400 that kills the turn:

2026-09-15 09:29:50 · 2026-09-16 18:06:37 · 2026-09-18 20:43:06 · 2026-09-18 20:44:04

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 recordeffort-history.ts:391:

const token = transitionMarker(scope, info.id, effort)
item.parts.push({ type: 'text', text: token })

Markers are validated per wire messageeffort-history.ts:611:

for (const message of consumed.messages) {
  if (message.transitions.length > 1) { throw  }

consumeInternalMarkers groups 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:

  • 26 consecutive-user-record pairs in the affected session. Coordinator seats receive peer session frames and subagent completions as ordinary user-role messages.
  • A failed turn leaves an assistant record with zero partsparts=0, textparts=0 for 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:

09:29:08  user low
09:29:08  user low
09:29:08  assistant ERR   ← "correlation failed"        (0 parts)
09:29:50  user high
09:29:50  assistant ERR   ← "Multiple internal markers" (0 parts)

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 ordered planDigest (: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:

mutation result
restore the > 1 throw accepts a merged boundary whose transitions are a correctly-ordered run and emits the last effort — throws Multiple internal Fable 5.1 effort markers on one user boundary
revert emission to transitions[0] same test — effort: "high" received where "max" expected
scope-check only [0] rejects a foreign-scope marker at index 1 of a merged boundaryExpected 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, as non-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.ts asserted the removed guard's message for a duplicated marker. It now includes the anchor so it exercises the flat count check, and expects correlation failed: expected 1, found 2. Duplication still fails closed — 400, request never sent.


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with 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

  • Dropped the per-message count guard; the flat checks already pin ordering, duplication, and position across all messages.
  • Scope-check now runs on every transition on a boundary, not just the first.
  • Emission uses the last transition (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 72618a7. Summary will update on new commits.

Review in cubic

…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).
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