Conversation
b991d3c to
f927d8b
Compare
|
Rebased onto Still valid. Worth noting against v1.23.0's changelog, which says it now folds "effort changes removed by downstream prefix compaction into the retained baseline through a checksum-bound current-boundary anchor". That landed in 1.22.0 and is the tolerance at Pre-existing failures are upstream'sTwo failures on the full opencode suite, both reproducing on clean Typecheck passes; core 295/0. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // A full trim is the `trimmedPrefix === expectedTransitions.length` case: | ||
| // every transition was consumed by the host's prefix trim, so fold them all | ||
| // into the baseline. Without a resolvable plan the request is untrusted and | ||
| // stays fail-closed. |
There was a problem hiding this comment.
If downstream processing removes every marker while retaining an earlier or otherwise non-suffix message, hasCandidate is false even though the body is not a valid complete prefix trim. This branch then folds every transition into the baseline and sends the request with the wrong effort instead of rejecting the non-prefix loss. Verify that the retained body ends at the correlated plan's current boundary before accepting a full trim.
Knowledge Base Used:
…apes The trimmed-prefix tolerance at effort-history.ts:658-688 was unreachable in the two shapes that actually occur, so a legitimate host prefix trim became a 400 and manufactured the contentless assistant that widens the next merge. Shape A (expected N, found 0): the !hasCandidate path threw unconditionally on markerCount !== 0, ~90 lines before the tolerance. A full trim with a resolvable plan is the trimmedPrefix === expectedTransitions.length case, so fold every consumed transition into the baseline and proceed. The throw stays for the untrusted case (markerCount > 0 with no resolvable plan). Shape B (expected N, found 1): the request carried a valid plan header but the tracker could not resolve it back to a plan, so expectedTransitions was null and the exact path made any loss fatal. The tracker held one slot per (sessionId, messageId); when the host trims the history prefix between provider calls the same current message is re-marked with a shorter timeline, and record() overwrote the slot while a header generated from the previous plan was still in flight. Keep a bounded identity-keyed history so resolveHeader can still find the plan a header references. Tests: full-trim fold asserts the folded baseline is the last consumed transition's effort; full trim without a resolvable plan still throws; a re-recorded trimmed timeline no longer loses the in-flight plan; non-prefix loss still throws.
The identity-keyed history map is bounded but was unasserted, so a later refactor could delete the eviction silently. Record past the cap and assert both halves: the size stays at the cap, and insertion-order eviction drops the oldest plan while the newest still resolves (the newest is the one an in-flight header needs).
f927d8b to
c3930f8
Compare
1 participant
Fixes the
correlation failed: expected N, found Mvariant of #237 — the initiating failure. It produces aparts=0contentless assistant, which lowers to nothing on the next request and so cannot separate the user records around it, widening the next merge into the variant #241 fixes. #241 stopped the amplification; this stops the ignition.All four fleet-wide occurrences, every one
found < expected— loss, never duplication:Loss already has correct tolerance at
effort-history.ts:658-688: the host trims a history prefix, survivors must be an exact suffix of the plan, consumed transitions fold into the baseline. Neither occurring shape reached it, for two unrelated reasons.Shape A — total trim short-circuits the tolerance
effort-history.ts:570-576threw unconditionally onexpectedTransitions, ~90 lines before the tolerance. The result was an asymmetry with no justification: trim 3 of 4 markers and the suffix logic accepts it; trim the 4th as well and the same legitimate compaction becomes a 400.A full trim is simply
trimmedPrefix === expectedTransitions.length— every transition consumed. It now folds all of them into the baseline. The throw is retained for the genuinely untrusted case:markerCount > 0with no resolvable plan.Shape B — the plan an in-flight header referenced was overwritten
The tracker held one slot per
(sessionId, messageId). When the host trims the history prefix between provider calls, the same current user message is re-marked with a shorter timeline andrecord()overwrites the slot — while a header generated from the previous plan is still in flight.resolveHeaderre-encodes tracked plans and string-compares, so it matched nothing,expectedTransitionswas null, and the exact path made any loss fatal.Fixed with a bounded identity-keyed history map alongside the existing slot map, so a header can still resolve the plan it was minted from.
The mechanism was demonstrated, not assumed. Two competing candidates were ruled out with evidence: LRU eviction (the in-flight plan is the newest;
record+markHeadersrefresh recency) andclear()on the same message id (the transform returns a plan, not null, for a single Fable user). A probe also corrected the original hypothesis — a growing tool loop alone leaves the plan identical; the trim is what re-records it.Verification
Five mutations, re-run independently of the implementer's report, tree restored to clean between each:
!hasCandidatethrowfolds a full trim into the last consumed transition effort when the plan resolves—expected 2, found 0requestPlan.baselineinstead of the last consumed transitioneffort: "low"where"high"expectedresolveHeaderback to scanning the single-slot mapresolves a plan header after the same message is re-recorded with a trimmed timeline—Expected -13 / Received +1while (false))evicts the oldest plan history entry once the cap is exceeded—Expected: 4096, Received: 4097The last two were not in the original brief. The fourth caught a real gap: the first implementation's
historybound was correct but entirely unasserted — disabling eviction left all 16 tests green. That map is keyed by encoded plan, so it gains an entry per distinct plan — per trim per message — growing faster than the slot map beside it, in a long-lived process. Issue #230 is open on exactly that shape, and introducing a second instance of it inside this fix would have been a poor trade.The fifth separates bounded from bounded correctly: evicting the newest keeps
sizeat the cap, so a size-only assertion would pass while silently reintroducing Shape B — the newest plan is precisely the one an in-flight header needs. Both halves are asserted.Gates: core 199/0 · opencode 1898/0 · pi 114/0 · typecheck clean.
Scope
Untouched: the suffix, digest, and scope checks (
:665-686,:695-706). Non-suffix loss, reordering, mutation, and scope mismatch all stay fail-closed — this makes a designed tolerance reachable rather than adding a new one.Two existing tests encoded the old throw (the unit full-trim assertion and the integration
missingAllMarkerscase) and now assert the folded success instead.Stacking note: this branch is off
mainand deliberately avoids:611-628and:724, which #241 changes, so the two do not conflict.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Makes the effort-marker trim tolerance reachable so legitimate host prefix trims no longer fail as 400 errors. Full trims with a resolvable plan now fold into the last consumed transition's baseline, and in-flight headers can still resolve a plan the tracker has since overwritten.
Bug Fixes
(sessionId, messageId), so re-marking the same message with a trimmed timeline orphaned the header already in flight; an identity-keyed history map now preserves those plans.Written for commit c3930f8. Summary will update on new commits.
The PR is not yet safe to merge because the unresolved complete-trim path can accept non-prefix marker loss and apply the wrong effort.
Findings
Summary
This PR changes OpenCode’s Fable 5.1 effort-history handling:
The previous review’s unresolved concern remains: the zero-marker path treats any body paired with a resolvable plan as a complete prefix trim without correlating the retained body to the plan’s current boundary.
Diagram
%%{init: {'theme': 'neutral'}}%% flowchart LR A[Message transform] --> B[Record effort plan] B --> C[Attach encoded plan header] C --> D[Downstream history trimming] D --> E[Resolve original plan from bounded history] E --> F{Markers remain?} F -->|Yes| G[Validate suffix and apply transitions] F -->|No| H[Fold all transitions into baseline]Reviews (2) · Last reviewed commit: "test(opencode): pin the effort plan hist..."