Skip to content

fix: _capture_fork_prefix no longer forks onto an array that was never sent (#jnt) - #28

Merged
Brian Krabach (bkrabach) merged 1 commit into
mainfrom
lane/jnt-fork-prefix-capture
Sep 3, 2026
Merged

fix: _capture_fork_prefix no longer forks onto an array that was never sent (#jnt)#28
Brian Krabach (bkrabach) merged 1 commit into
mainfrom
lane/jnt-fork-prefix-capture

Conversation

@bkrabach

Copy link
Copy Markdown
Collaborator

DONE-NOTE - model_performance-jnt

_capture_fork_prefix() appends to an array that was never sent (11 of 20
forks measured).

Verdict: CORRUPTING, not cosmetic. It is not dead code and it produces a
record. It silently substitutes a message array that never went on the wire
for one that did, on a path whose entire purpose is byte-parity with the wire.
Fixed. No 6da measured number is invalidated — see §3.

1. The array, named, at file:line

Everything below is amplifier_module_context_simple/__init__.py at the
pre-fix commit a877b36.

the array self._last_request_view
created :1015 (__init__), written :1778 inside _finalize_view()
what it holds this module's own last RETURNED view, pre-strip
why it is "never sent" it is written on every view served, and a view served is not a request sent
consumed by :4187 _capture_fork_prefix(), as the fallback source
flows to _maybe_trigger_summary_compaction :4040_run_summary_compaction_task_build_fork_request :4422 → the provider

The selection at :4187–4198 preferred the caller's recorded wire array
(_sent_messages, :1017/:1210) only while
_sent_serial == _view_serial (:4189), and substituted
_last_request_view whenever that equality failed (:4187, :4192–4197
logger.debug, not warning).

Why the equality fails in production. _view_serial counts views
served
, not requests sent, and the two are not 1:1. The real
orchestrator serves the view more than once per sent request:
amplifier-module-loop-streaming/__init__.py calls
context.get_messages_for_request() at :3215 and then re-fetches at
:3329 and :3453 after persisting an ephemeral injection — up to three
views for one ChatRequest. The summary trigger is evaluated inside every
one of them (:1364, which runs before that call's _finalize_view at
:1480/:1482). So:

  • trigger on the first view of a request → serials match → the wire array
    is used → correct fork (wire offset 0 or 1);
  • trigger on a re-fetched view → serials differ by 1–2 → the wire array is
    discarded in favour of _last_request_view, which at that instant holds
    the view the re-fetch just superseded — built, thrown away, never sent.

That is the 9-vs-11 split 6da measured, and it reproduces exactly.

2. Consequence — unambiguous

Corrupting to fork-mode behaviour, and self-concealing.

  1. Guaranteed cache miss. A superseded view is not a prefix any provider
    holds. Fork mode's only justification is appending onto a cached prefix; a
    fork that misses pays full price for the whole conversation, which is
    strictly worse than the standalone call it replaces.
  2. Silent. last_summary_call_stats["mode_used"] reported "fork" for
    all 20 calls. The substitution was logger.debug. There was no field
    distinguishing the two sources — this is precisely why 6da had to
    reconstruct the distinction from the provider's request log.
  3. Backwards under uncertainty. The check traded the array with positive
    evidence
    of having been sent (the caller said so) for one with none
    (fate unknown to this module), and did so exactly when uncertainty was
    highest.

Not affected: history, _seq allocation, span selection, tool-pair
integrity, the served view, or any default-mode behaviour. The blast radius
is fork mode's cache economics and the honesty of its self-report.

3. Does this invalidate any of 6da's measured numbers? NO.

Stated plainly for the manager, because the item asked for it loudly:

  • G-FORK-PREFIX (2/7/11 offset distribution, 45% aligned) — VALID, and is
    the direct measurement of this bug.
    Scored from the wire, not from the
    module's self-report.
  • G-FORK-CACHED, G-FORK-NOBOUNDARY, the Anthropic guardrail, quality
    parity, and the −0.8% run-cost delta — VALID.
    All are wire/usage-derived
    and none depend on _capture_fork_prefix() having chosen correctly.
  • The §7 correction (summarizer share ≈30%, not 2.4%/8.3–10.9%) — VALID
    and untouched.
    Independent of the fork path.

One caveat, in 6da's favour, not against it: the −0.8% run-cost delta was
measured with only ~45% of forks byte-aligned. It is a lower bound on
what a correctly-aligned fork arm would deliver, not an upper bound. 6da's
"the mechanism works and the lever does not pay / DON'T-SHIP as-is" verdict
therefore still stands as written, but its cost figure is now known to
have been measured on a partially-broken treatment and should be re-measured
before the DON'T-SHIP call is made final
. 6da itself flagged this
("one of them has a cheap fix worth a follow-up item"); this is that fix.

4. The fix (minimal)

_capture_fork_prefix() (:4176) no longer substitutes:

  • a recorded wire array, when one exists, is used — it is the only source
    carrying positive evidence it was on the wire, so it is never traded for one
    that carries none. Extra views served since the send are reported, not
    acted on
    ;
  • the module's own view is used only when the caller has never supplied a
    message array (the documented explicit-breakpoint/Anthropic path, unchanged);
  • staleness in the wire record is still caught, but by an exact check
    instead of a proxy: a record too old to contain the span fails
    _prefix_contains_span and refuses LOUDLY — standalone call, WARNING,
    _summary_fork_fallbacks incremented, named reason. "A fork that silently
    missed" is no longer reachable on this path.

Also added, because 6da needed it and could not get it: prefix_source
("wire_record" / "module_view" / None) and prefix_views_since_send
on last_summary_call_stats. The next arm can separate the two populations
from the module's own report instead of reconstructing them from the wire.

Config surface: unchanged. Default summary_call_mode remains
"standalone".

5. Tests

286 passed, 1 skipped (was 281 passed at a877b36). ruff check: clean.

New Group F in tests/test_summary_call_mode_fork.py:

Test Pins
test_a_re_fetched_view_does_not_displace_the_recorded_wire_array THE regression: a superseding re-fetch must not displace the wire array
test_the_module_view_is_never_substituted_when_a_wire_record_exists same defect from the other side: never-sent content cannot reach the fork
test_prefix_source_names_the_module_view_path_honestly the module-view path is allowed but reported as what it is
test_prefix_source_is_none_when_the_call_did_not_fork a refused fork claims no alignment
test_tool_pair_integrity_and_seq_stability_survive_the_re_fetch_path no _seq consumed, history byte-identical, same span absorbed, served view identical to an unforked control

These three fail against the pre-fix selection logic and pass against the
fix
— verified by temporarily restoring the old branch and re-running; they
are load-bearing, not decoration.

One existing test changed: test_a_stale_caller_message_record_is_ignored_not_trusted
test_a_stale_caller_message_record_refuses_loudly_not_silently. It
asserted the substitution as correct behaviour; it now asserts the loud
refusal. The rewritten docstring records why the original resolution was
wrong, so the reversal is not silent.

Default-mode byte-identity re-verified by the pre-existing Group A/C tests,
strengthened with assert context._fork_prefix_source is None in
test_default_mode_never_records_a_fork_prefix.

6. Residual, disclosed

The module-view path (note_request_sent(tools=...) with no messages)
can still append to a superseded view — this module genuinely cannot know
whether its own view was sent. Not silently, now: prefix_source == "module_view" says so on every call. A caller that wants byte-parity must
pass messages.
Closing this properly needs a caller-side confirmation
signal, which is an orchestrator change and out of this lane's scope.

Unchanged and still true: fork mode cannot fork the first summarization of
a CLI turn (each turn is a fresh amplifier run --resume process, and the
trigger is evaluated before any request is sent). 6da measured 12 of 24
refusals from this; this fix does not address it.

7. Deliverable ledger

Deliverable Status
DRAFT PR on origin, branch lane/jnt-fork-prefix-capture, tests green, default inline byte-identical DONE
The array named at file:line with why it was never sent + cosmetic-vs-corrupting verdict DONE — §1, §2 (corrupting)
Explicit statement of whether any of 6da's measured fork numbers are invalidated DONE — §3 (none invalidated; −0.8% is a lower bound and warrants re-measurement)
DONE-NOTE.md in the PR body DONE — this section

Spend: $0.00. No API calls, no DTU, no containers, no infrastructure
created — the item was answerable from the code, the shipped tests, and 6da's
existing evidence files. Nothing to tear down; nothing registered in the infra
ledger. No PII or team-internal data. No merge to main. No files touched
outside this module.

…r sent (#jnt)

`_capture_fork_prefix()` preferred the caller's recorded wire array
(`_sent_messages`) only while `_sent_serial == _view_serial`, and silently
substituted `_last_request_view` whenever that equality failed.

`_view_serial` counts VIEWS SERVED, not REQUESTS SENT, and the two are not
1:1: loop-streaming re-fetches the view (get_messages_for_request at :3215,
:3329, :3453) up to three times per sent request, and the summary trigger is
evaluated inside every one of them. On a re-fetch the serials differ, and the
substituted `_last_request_view` holds the view the re-fetch just superseded
-- built, discarded, never on the wire.

Measured (model_performance-6da, 20 forked calls): 9 appended to a request the
provider actually saw; 11 appended to an array never sent as any request.
`mode_used` reported "fork" for all 20.

Fix: the recorded wire array is the only source with positive evidence of
having been sent, so it is never traded for one with none. Extra views are
reported, not acted on. Staleness is caught exactly (span-presence) and
refuses LOUDLY -- standalone + WARNING + counter -- instead of substituting.
The module-view path (no `messages` supplied) is unchanged.

Adds `prefix_source` and `prefix_views_since_send` to
`last_summary_call_stats` so a measurement can separate the two prefix
populations from the module's own report.

Default `summary_call_mode` remains "standalone"; the default path is
untouched. 286 passed (was 281), ruff clean.
@bkrabach
Brian Krabach (bkrabach) marked this pull request as ready for review September 3, 2026 00:03
@bkrabach

Copy link
Copy Markdown
Collaborator Author

Merge-queue verification — PASS

Scratch clone: fresh checkout of lane/jnt-fork-prefix-capture (7af38cf) on top of main (a877b36, no divergence — direct fast-forward, git merge-base confirms). uv sync + uv run pytest / uv run ruff check ..

Gate table

# Gate Method Result
1 Default mode byte-identical Ran test_default_mode_summarizer_request_is_byte_identical (asserts the standalone request is independently-rebuilt-identical: 2 messages, system prompt + formatted span, no tools) and test_default_mode_never_records_a_fork_prefix (now also asserts context._fork_prefix_source is None). Both pass. No literal field named default_mode_byte_identical exists in the repo — this is the concrete test pair that backs the claim; naming that plainly since the spec asked for it. PASS
2 3 regression tests genuinely fail before the fix git checkout a877b36 (parent commit, pre-fix source) + git checkout pr-28 -- tests/test_summary_call_mode_fork.py (test file only). Ran the 5 new Group F tests against pre-fix source: all 5 fail (test_a_re_fetched_view_does_not_displace_the_recorded_wire_array and test_the_module_view_is_never_substituted_when_a_wire_record_exists fail on the actual substitution behavior / wrong mode_used; the other 3 fail on missing prefix_source instrumentation). Re-checked out pr-28 (fix + tests together): all 5 pass. The two tests the PR calls out as pinning the core defect fail with content-level assertions, not just KeyError — confirmed by reading the failure output, not just the pass/fail count. PASS
3 Suite green, 286/1 (was 281) Ran full suite on pr-28: 286 passed, 1 skipped. Checked out a877b36 alone and ran full suite: 281 passed, 1 skipped. Delta of +5 matches the 5 new Group F tests exactly. PASS
4 Tool-pair integrity / _seq/prefix stability invariants Ran tests/test_tool_pair_compaction.py (5/5 pass) plus the relevant tests in test_summary_call_mode_fork.py: test_fork_mode_never_serves_an_orphaned_tool_result, test_fork_consumes_no_seq_and_appends_nothing_to_history, test_call_mode_does_not_change_which_span_is_selected, test_fork_does_not_resend_the_span, test_forked_and_unforked_sessions_serve_identical_views, test_fork_refuses_when_the_prefix_ends_on_unanswered_tool_calls, and the new test_tool_pair_integrity_and_seq_stability_survive_the_re_fetch_path — all pass, and are included in the full 286-pass run. PASS
5 Honest scope claim Read PR body §3 verbatim: invalidates_6da_numbers is stated as NO, with the caveat present and correctly framed — the −0.8% run-cost delta was measured on ~45% aligned forks and is called a lower bound, explicitly not an upper bound, and the body states the DON'T-SHIP verdict "still stands as written" while its cost figure "should be re-measured before the DON'T-SHIP call is made final." Nowhere does the PR claim the fix makes fork mode shippable — it explicitly disclaims that (§6 residual: module-view path still can't guarantee byte-parity; first-turn-of-CLI-run forks still refuse, unaddressed). PASS
6 No unvalidated performance claim All cited numbers (9/11 split, −0.8%, 45% alignment, 12/24 refusals) are attributed to model_performance-6da's prior measurement, not asserted as new results of this fix. No new performance number is claimed here. PASS

Scope / diff check

git diff a877b36 pr-28 --stat: DONE-NOTE.md (+174, new), amplifier_module_context_simple/__init__.py (+116/−22), tests/test_summary_call_mode_fork.py (+221/−8). Read the full source diff: confined to _capture_fork_prefix(), the new FORK_PREFIX_SOURCE_* constants/_fork_prefix_source field, last_summary_call_stats's two new keys, and docstrings. _prefix_contains_span/_fork_refusal_reason/_note_fork_fallback (the loud-refusal machinery from #27) are unmodified and reused as-is — matches the "minimal fix" claim. ruff check .: clean.

Concurrency check

No conflict with main (branch sits directly on the current tip, git merge-base == main HEAD). Checked for lane pmt-fork-span-predicate: no open PR (gh pr list) and no matching remote branch (git ls-remote --heads origin) at verification time, so there is nothing to union yet — noting this explicitly per the concurrency instruction rather than silently skipping it.

Disclosure

Merging with --admin — repo has a required-review ruleset and I am the PR author (bkrabach), so admin override is expected/necessary per the task's own instruction.

@bkrabach
Brian Krabach (bkrabach) merged commit 5cdbc62 into main Sep 3, 2026
1 check passed
Brian Krabach (bkrabach) added a commit that referenced this pull request Sep 3, 2026
* Revert "fix: _capture_fork_prefix no longer forks onto an array that was never sent (#jnt) (#28)"

This reverts commit 5cdbc62253ca80cb93f11a2fa6f7a5d3940394c78.

Not in the owner's literal revert list (#20-27), but merged AFTER #27 and
exists solely to fix a bug in the summary_call_mode="fork" code path that
#27 introduced. Once #27 (the fork feature itself) is reverted next, this
fix has no surviving target -- keeping it would leave dead code / an
orphaned test file referencing a feature no longer on main. Reverting it
first, before #27, keeps the revert of #27 itself clean.

* Revert "feat: summary_call_mode -- cache-safe fork of the summarization call (#27)"

This reverts commit a877b36.

Merge policy: main carries wins only. summary_call_mode's fork mode was
shipped with benefit explicitly labeled unmeasured (see the original merge
note: "benefit correctly labeled unmeasured"). Unproven default-off
feature -- belongs on a branch for evaluation, not on main.

* Revert "feat: clear_at_least -- a worth-the-rebuild predicate in front of compaction (#2o9) (#26)"

This reverts commit f851d12.

Merge policy: main carries wins only. clear_at_least was an unproven,
default-off predicate. Belongs on a branch for evaluation, not on main.

* Revert "feat: replay_last_user_on_compaction -- opt-in last-user replay at the tail (#4i3) (#24)"

This reverts commit 3972070.

Merge policy: main carries wins only. replay_last_user_on_compaction was
an unproven, opt-in feature. Belongs on a branch for evaluation, not on
main.

* Revert "feat: tool-result budget (token-denominated, head+tail, per-tool) + spill-to-disk -- all defaults no-op (#21)"

This reverts commit 49e2799.

Merge policy: main carries wins only. Tool-result budget + spill-to-disk
was an unproven, all-defaults-no-op feature. Belongs on a branch for
evaluation, not on main.

* Revert "feat: token_meter \"hybrid\" -- provider-anchored counts with provenance (+ the never-run estimate-vs-hybrid-vs-actual measurement) (#22)"

This reverts commit f47c894.

Merge policy: main carries wins only. The hybrid token_meter mode's own
measurement was never run. Unproven, opt-in feature -- belongs on a
branch for evaluation, not on main.

* Revert "feat: compaction_strategy \"summary\" -- LLM rolling-summary compaction (opt-in) (#20)"

This reverts commit c6dfbba.

Merge policy: main carries wins only. The summary compaction strategy's
own gate measurement showed a +83% cost regression with summarizer share
26-35%% -- falsified, not a win. Belongs on a branch for evaluation, not
on main.
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