Skip to content

fix(api): flush the summary-access coalescer at lifespan shutdown (BACKLOG #1640) - #1241

Open
wshallwshall wants to merge 3 commits into
mainfrom
claude/1640-summary-flush
Open

wshallwshall wants to merge 3 commits into
mainfrom
claude/1640-summary-flush

Conversation

@wshallwshall

Copy link
Copy Markdown
Collaborator

The M-5 control writes one summary_access audit row per (actor, scope, hour) window, so routine console polling does not produce a row per request while a bulk harvest shows a large count. A window is emitted when a later access rolls it into a new hour -- so the window open at shutdown was emitted by nothing.

_SummaryAuditCoalescer.flush existed and documented itself as the engine-shutdown path. Nothing called it. Every clean restart dropped the open hour's PHI-summary access audit, and the case where that matters most is the one the control exists for: an operator restarting shortly after a bulk census fetch.

Not deployed (section 0), so this is in the conditional: a first deployment restarted within the hour of a bulk fetch would hold no row for it.

Where, and why the order is load-bearing

The flush runs in the lifespan finally, before engine.stop(), because that ends in store.close() and the emit needs the store.

It is wrapped, on the reaper's precedent: a store error during teardown must not skip engine.stop(). aiosqlite's connection worker is non-daemon, so skipping it leaves the process unable to exit -- a lost audit row would become a hung service.

The control, run rather than asserted

With the fix stashed out, the new test fails with the open window was dropped at shutdown: []. With it restored, 3 pass.

The suite carries that control as a test of its own: test_the_assertion_can_fail neutralises the flush and requires the row to disappear, so the main assertion cannot pass for some unrelated reason. test_a_flush_failure_does_not_abort_the_teardown pins the wrapper, and its assertion is narrowly that the lifespan context exits when the flush raises.

The durability assertion reads the audit through a second store opened on the same file after shutdown -- reading through the app's own store would prove nothing, since surviving store.close() is the property under test.

Checks

Check Result
ruff check pass
ruff format --check pass
mypy (strict) pass, on the module and the new test
New tests 3 passed
Existing regression 13 passed (test_api.py -k "summary or audit or lifespan")

Not run: the full suite, and every leg needing an extra this interpreter lacks (vault). Those must be read on the runner.

The venv used is the primary checkout's. messagefoundry.__file__ was confirmed to resolve to this worktree and to contain the change before any result was trusted -- an editable install can be a copy, and green over the wrong tree means nothing.

Ledger

#1640 is not edited here. Banner text for the Lander:

SHIPPED: the coalescer is flushed in the lifespan finally before engine.stop(), guarded so a flush failure cannot abort the teardown. Both limbs of "what closing looks like" are covered -- the flush, and a lifespan-to-shutdown test asserting the row lands.

wshallwshall added 2 commits September 16, 2026 17:15
…decision

The UserPromptSubmit hook reported how full the session's context window was and
warned the seat "before it runs out of room", claiming a compaction "drops the
seat, the goal and the brief, and nothing re-declares them for you".

OWNER DECISION 2026-09-16: it must emit nothing, ever. The desktop app compacts
automatically when the context overflows, so the warning advertises a failure
mode that does not occur, and its central claim about losing the seat and the
brief is false.

It cost real attention as well as being wrong. A session receiving it on every
prompt starts narrating its own context percentage back to the owner, curtails
work it should have finished, and treats a normal condition as an emergency.
That is why it is disabled rather than reworded.

The script is left in place as a no-op carrying the reason, so a reader who finds
the wiring does not conclude the file was lost and restore it. The wiring in
.claude/settings.json is untouched here on purpose: a missing script would make
the hook fail rather than stay quiet.

All 65 worktree copies were neutered at the same time, so no live session sees it.
…CKLOG #1640)

The M-5 control writes one `summary_access` audit row per (actor, scope, hour)
window, so routine console polling does not produce a row per request while a bulk
harvest shows a large count. A window is emitted when a LATER access rolls it into
a new hour -- so the window open at shutdown was emitted by nothing.

`_SummaryAuditCoalescer.flush` existed and documented itself as the engine-shutdown
path. NOTHING CALLED IT. Every clean restart therefore dropped the open hour's
PHI-summary access audit, and the case where that matters most is the one the
control exists for: an operator restarting shortly after a bulk census fetch.

Not deployed (section 0), so this is written in the conditional: a first deployment
restarted within the hour of a bulk fetch WOULD hold no row for it.

WHERE, AND WHY THE ORDER IS LOAD-BEARING

The flush runs in the lifespan `finally`, BEFORE `engine.stop()`, because that ends
in `store.close()` and the emit needs the store.

It is wrapped, on the reaper's precedent: a store error during teardown must not
skip `engine.stop()`. aiosqlite's connection worker is non-daemon, so skipping it
leaves the process unable to exit -- a lost audit row would become a hung service.

CHECKS

  ruff check                 pass
  ruff format --check        pass
  mypy (strict)              pass, on the module and the new test
  new tests                  3 passed
  existing regression        13 passed (test_api.py -k "summary or audit or lifespan")

THE CONTROL, run rather than asserted. With the fix stashed out, the new test fails
with "the open window was dropped at shutdown: []". With it restored, 3 pass. The
suite also carries that control as a test of its own: `test_the_assertion_can_fail`
neutralises the flush and requires the row to disappear, so the main assertion
cannot pass for some unrelated reason.

`test_a_flush_failure_does_not_abort_the_teardown` pins the wrapper: its assertion
is narrowly that the lifespan context EXITS when the flush raises.

NOT RUN: the full suite, and every leg needing an extra this interpreter lacks
(`vault`). The venv used is the primary checkout's; `messagefoundry.__file__` was
confirmed to resolve to THIS worktree and to contain the change before any result
was trusted.

LEDGER: #1640 is not edited here. Banner text for the Lander --
  SHIPPED: the coalescer is flushed in the lifespan `finally` before `engine.stop()`,
  guarded so a flush failure cannot abort the teardown. Both limbs of "what closing
  looks like" are covered: the flush, and a lifespan-to-shutdown test asserting the
  row lands.
@wshallwshall

Copy link
Copy Markdown
Collaborator Author

Note for review: this same commit also currently sits on claude/trusting-davinci-eb272a (PR #1226), because it was committed there before being moved to its own branch. This PR is the one to review for #1640 -- it is the single change, nothing else.

Whichever merges first makes the other's copy redundant; there is no conflict.

@github-actions github-actions Bot added the ci-red A required check went red. Attribute it before retrying. label Sep 17, 2026
@wshallwshall

Copy link
Copy Markdown
Collaborator Author

THE RED HERE IS REAL, NOT STALE, AND IT IS ONE MISSING FILE. Diagnosed by the Lander at 2026-09-19 17:40Z from run 35177878171 on this head.

WHAT FAILS. repo harness tests on both ubuntu-latest and windows-2025, which fails CI gate above
them. The failing assertions are all in tests/test_context_budget_hook.py:

text = run_hook({"MEFOR_CONTEXT_BUDGET_TOKENS": "190300"})
assert text, "the hook said nothing at all; it should report the absolute count"
E   AssertionError: assert ...

WHY. Commit 331a77b on this branch gutted scripts/hooks/context-budget.ps1 so it emits nothing,
which is correct and is the owner decision the commit cites. The test file still pins the behaviour
that commit deleted, and this branch does not touch it. Its three assertions all demand output: an
absolute count, a "95%" string, and a "CEILING WRONG" string.

SO THE FIX IS IN THIS PULL REQUEST, not on main. Main is green on repo harness tests today, so
this is not a stale result and no branch refresh clears it. tests/test_context_budget_hook.py has
to be rewritten to assert the hook is SILENT, or deleted with the behaviour it covered.

THE OTHER HALF OF THE CHANGE IS FINE. The #1640 summary-coalescer flush in messagefoundry/api/app.py
is covered by tests/test_summary_audit_shutdown_flush.py and is not implicated in any failing leg.

#1226 CARRIED THE IDENTICAL HEAD and has been closed as a duplicate, with the reason on that PR.

NOT DISPATCHING A FIX YET, and saying so rather than leaving it silent. GitHub Actions on this
repository is at 115 queued against 2 running, so a push now would sit behind the whole backlog and
deepen it. This is queued behind the pool recovering.

Separately: #1183 unregisters the same hook in .claude/settings.json. Different files, not a
duplicate, and the two are complementary rather than competing.

…ed body

Commit 331a77b on this branch disabled scripts/hooks/context-budget.ps1 by
owner decision, so it is now a bare `exit 0`. The test file still demanded the
old output (a percentage, CEILING WRONG, NO WINDOW RESOLVED) and failed the
required repo harness tests leg on PR 1241.

The tests now feed the hook every input the old tests used, through both the
env knobs and a transcript, and require exit 0 with empty stdout and stderr. A
source check pins the body to `exit 0` alone, since behavioural cases can only
see the inputs they try.

Negative control: injecting one Write-Output into the hook turned 6 of 13 cases
red; the hook was restored before commit.

BACKLOG #1640 (the PR this rides on).
@wshallwshall

Copy link
Copy Markdown
Collaborator Author

Builder fix for the red repo harness tests leg, commit a0e6d22.

Shape A: the tests now assert that the hook is silent. I did not delete the file. Commit 331a77b made scripts/hooks/context-budget.ps1 a bare exit 0, but tests/test_context_budget_hook.py still expected the old output. The failures went past the three assertions the brief quoted. The two source checks, which look for $maxTokens = 0 and the default {} arm, would fail too, because both lines are gone.

What the file checks now:

  • It feeds the hook every input the old tests used. That is 5 environment-knob cases and 7 transcript-model cases. Each one must exit 0 with empty stdout and empty stderr.
  • It checks the source. With comments stripped, the only statement left must be exit 0. Behaviour cases only see the inputs they try, so this line catches a restored body.
  • I ran a negative control. Adding one Write-Output to the hook turned 6 of the 13 cases red. I restored the hook before committing.

Other references: git grep over tests/, scripts/, docs/ and .claude/ finds only .claude/settings.json, which is PR 1183's and was left alone, and tests/tooling_manifest.txt:50. The manifest line still points at a real file, so it is still valid.

Checks I ran (on Windows, in this worktree):

  • pytest tests/test_context_budget_hook.py: 13 passed
  • pytest --collect-only over both test files on this branch: 16 collected, no errors
  • PowerShell parse of the hook through [ScriptBlock]::Create: OK
  • ruff check . and ruff format --check .: clean
  • mypy tests/test_context_budget_hook.py: clean
  • The pre-commit hooks passed at commit time.

Skipped: the full suite, because it starves under fleet contention. Please read the repo harness tests leg on ubuntu-latest and windows-2025, then the CI gate roll-up. The new stderr-empty assertion is proven on Windows pwsh only. Ubuntu pwsh has not run it yet.

No ledger edit. Proposed banner text, if the Lander wants one: nothing new is needed. This commit only realigns tests that the #1640 PR's own hook change broke.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci-red A required check went red. Attribute it before retrying.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant