fix(agent): the server owns thread history - #229
Merged
Conversation
The gate refused three things beyond the one the law requires, and each was costing readers that the numbers then had to be read without. Do Not Track is gone. The W3C withdrew the specification in 2019, Safari removed the header, and the browsers that still carry the setting leave it off, so a site that honours it is not honouring a decision -- it is honouring an accident of which browser build the reader happens to run. There is no obligation behind it and no intent expressed by it. Global Privacy Control stays, but as a default rather than a veto. It is a recognised opt-out signal in several US states and worth respecting where the reader has said nothing else, so a reader who sends one and has not used the control is treated as having declined, and is not asked either. What it no longer does is outrank the reader. Brave and DuckDuckGo send GPC by default, so the old ordering meant a reader who explicitly clicked Allow was still refused by a preference their browser had expressed on their behalf -- the site overriding a person with a default, while claiming to do the opposite. Guernsey, the Isle of Man and Jersey leave the prior-consent list. Their data protection law follows the GDPR, but none of them enacted a PECR equivalent, and it is PECR, not the GDPR, that governs storing an identifier on a device. They were on the list out of caution rather than because the rule reached them. The refusal branch at the bottom of the file goes with them, and with it the revoke that 3fa596f added upstream in onebox. That code existed to delete an identifier left behind when a granted reader later switched a signal on. A granted reader is now measured with the signal on, so nothing is left behind to delete, and the case it guarded cannot arise. The stop() that remains is in apply(), where a signal without a stored answer clears an identifier whose preference has since been cleared. The privacy page and the standing control on it say all of this. The buttons there are no longer disabled underneath a sentence explaining that they do not matter, because they now do. Driven against the script extracted from the built dist/index.html, with the trace lookup and gtag stubbed, over the nine cases that define the gate: no GPC US no answer GA runs, no banner no GPC DE no answer no GA, banner no GPC GB no answer no GA, banner no GPC JE no answer GA runs, no banner no GPC XX no answer no GA, banner no GPC US denied no GA, no banner GPC US no answer no GA, no banner GPC US granted GA runs, no banner GPC DE granted GA runs, no banner Claude-Session: https://claude.ai/code/session_01BKURdfwTvLLaBeDUppLHxx
The listener that re-applies the preference treats a null storage key as the clear() case and acts on it. The one inside subscribe, which exists to redraw the control on /privacy, did not: it compared the key to STORAGE_KEY and nothing else. So a reader who cleared site data in another tab left /privacy displaying the answer they had just deleted, next to buttons whose pressed state described it. Measurement was correct throughout -- the other listener saw the event -- but the page said otherwise, which is the half of this feature the page exists to get right. onebox has carried the null case since 8146452. This is the same fix. Claude-Session: https://claude.ai/code/session_01BKURdfwTvLLaBeDUppLHxx
Review of the two commits before this one. No behaviour changes; the harness over the built script still passes all nine cases. signalsOptOut's comment sent the next reader to mayRun as well as apply. mayRun never consults the signal -- apply is the sole enforcement point -- so the obvious next step, reusing mayRun as the decision function, would have dropped the check and measured GPC readers again. The comment now says that, and says not to. The null-key note on subscribe claimed more than the change delivers. It reaches a tab that has not recorded an answer of its own; a tab that has keeps it, because readPreference falls back to memoryPreference and a clear() elsewhere does not reset it. That is the right behaviour -- the reader answered in this tab, and clearing storage in another did not withdraw it -- but the previous commit message described the fix as broader than it is. Dead CSS: nothing sets `disabled` on the preference buttons since the branch that did was removed, so the `:disabled` rule and the `:not(:disabled)` qualifier described a state the script can no longer produce. Also rewraps a comment line the earlier edit left at 125 columns in a block that wraps at 80. Claude-Session: https://claude.ai/code/session_01BKURdfwTvLLaBeDUppLHxx
Removing them was argued from the statutes: Guernsey, the Isle of Man and Jersey follow the GDPR but none enacted a PECR equivalent, and it is PECR that makes storing something on a device the thing consent is owed for. That reading may well be right, and it is not the whole question -- their data protection authorities publish cookie guidance that asks for consent regardless, and a site is answerable to the regulator before it is answerable to a reading. The traffic involved is negligible, so the measurement given up is close to nothing and the exposure taken on was not. Restored, with the reasoning written down so the next person to look at the list finds an argument rather than three country codes that seem out of place. The rest of the relaxation is untouched. Do Not Track is still gone and Global Privacy Control is still a default rather than a veto, which is where the coverage was. Harness over the built script, nine cases, JE now expecting the banner again: all pass. Claude-Session: https://claude.ai/code/session_01BKURdfwTvLLaBeDUppLHxx
A run rebuilt its message list from the history the browser posted, and both StartRun and FinishRun wrote that list over the stored thread. The AG-UI client does not carry activity messages back, so every run overwrote the record with a copy that had lost them and a restored chat kept only the newest attached view. Posting a thread's history also meant a request could rewrite or delete it. StartRun now reads the stored thread inside the transaction it already holds, appends only the request's new user turns matched by message id, writes that, and returns it for the run to build on. A request can add to a thread but no longer rewrite it, and a fabricated assistant or tool turn is dropped. Concurrent runs on one thread still race on the final write, which is the limitation FinishRun already documents. Claude-Session: https://claude.ai/code/session_014P9NvJjVkRiJPpdKevKkNE
A run can die between the model asking for a tool and the tool replying, and pressing Stop does exactly that: the request context is cancelled while the final write still lands on an uncancelled one. The record then holds an ask with no answer, which both providers reject. That used to be survivable because the next run rebuilt the thread from the browser's copy. Seeding from the server's own record made it permanent: every later run on the thread would fail the same way with no path to clear it. The stored history is now repaired as it is read — a turn that spoke keeps its words and loses the dangling call, one that only asked is dropped whole. Also states two limits the merge comment implied but did not say: id matching deduplicates, it does not protect against replay, and a failed FinishRun is no longer healed by the next request. Claude-Session: https://claude.ai/code/session_014P9NvJjVkRiJPpdKevKkNE
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
A run rebuilt its message list from the history the browser posted (
runtime.go:198), and bothStartRunandFinishRunwrote that list straight over the stored thread. Two consequences:StartRunnow reads the stored thread inside the transaction it already holds, appends only the request's new user turns matched by message id, writes that, and returns it.Runseeds the run from that return value instead ofinput.Messages.The rule is that the server owns the record: a request may add to a thread but cannot rewrite or delete it, and only
userturns are taken, so a fabricated assistant or tool turn is ignored. Matching by the client's message id makes a retry that re-posts the same history a no-op; a turn arriving without an id is appended rather than silently dropped, since losing real input is worse than a duplicate.Non-user turns are ignored rather than rejected with a 400, so the existing client, which always posts full history, keeps working unchanged.
Not in scope
Concurrent runs on the same thread still race on the final write. That is the limitation
FinishRunalready documents, and serialising it is a separate change.No schema change and no migration: the merge only adds, so existing rows are untouched. Threads that already lost views stay as they are; the fix is forward-looking.
Verification
just checkgreen.New store tests, both confirmed load-bearing by breaking the merge and watching them fail with the exact symptom:
TestStoreStartRunKeepsStoredHistory(a second run whose posted history has lost the first run's activity message keeps it) andTestStoreStartRunIgnoresClientAuthoredHistory(a request that drops stored turns and invents an assistant one changes nothing but its own user turn).Manual check against a local instance with seeded traffic: two runs in one chat, then a reload. The persisted thread carries both
activity:observability_overviewandactivity:service_topology, and both views render after the reload. Before this change the second run's write erased the first.just checkjust test-racenot run: the change is inside the existingBEGIN IMMEDIATEtransaction and adds no new concurrency; the pre-existingTestStoreConcurrentFirstRunsSameThreadstill passes.User-facing behavior and configuration docs are current
No credentials, private telemetry, host details, or enterprise-only source are included
API, migration, ingest, MCP/AG-UI, or release-contract changes are called out: the AG-UI run endpoint now ignores non-user messages in the posted history instead of persisting them. No schema or route change.
https://claude.ai/code/session_014P9NvJjVkRiJPpdKevKkNE