Adversarial review: a cached summary still fetched the reader's identifiers - #284
Merged
Merged
Conversation
adamjohnwright
force-pushed
the
011-phase7-review
branch
from
September 20, 2026 18:30
72c09d1 to
776077a
Compare
…ifiers The cache was looked up *after* the disclosure fetch, so every reload of an already-summarised analysis went to the Analysis Service for the reader's unmatched identifiers and then discarded them. Not a disclosure beyond what they chose, but a pointless request on the one endpoint where pointless requests are worth avoiding -- and it meant the cache did nothing for the path it should protect most. The lookup now precedes the fetch, under the tier the reader *asked* for, while the write still uses the tier that *applied*. The asymmetry is deliberate and was worth working out rather than assuming: a request whose disclosure failed stores an aggregate summary under `aggregate`, so the next disclosing request misses and gets another attempt at the identifiers, instead of being served the fallback forever. Both directions are now tested. One thing the review confirmed rather than changed, and it is worth writing down because it looks like waste. The analysis result is fetched on every request, before the cache is consulted, and that is what keeps `gone` correct: a release deletes the analysis, and a reader must be told to re-run rather than handed a confident summary of something that no longer exists. A cache that skipped the fetch would serve stale summaries of deleted results indefinitely. Now pinned by a test. Verified by sabotage, with the sabotage asserted to have applied. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
adamjohnwright
force-pushed
the
011-phase7-review
branch
from
September 20, 2026 18:33
776077a to
5723e23
Compare
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.
Follow-up review of #283. Builds on it, so merge that first.
The cache did nothing for the path it should protect most
The lookup happened after the disclosure fetch. So every reload of an already-summarised analysis went to the Analysis Service for the reader's unmatched identifiers and then discarded them.
Not a disclosure beyond what they chose — but a pointless request on the one endpoint where pointless requests are worth avoiding, repeated on every reload.
The fix, and an asymmetry worth understanding
The lookup now precedes the fetch, under the tier the reader asked for, while the write still uses the tier that applied.
That asymmetry is deliberate. A request whose disclosure failed stores an aggregate summary under
aggregate— so the next disclosing request misses, and gets another attempt at the identifiers rather than being served the fallback forever. Both directions are tested.One thing the review confirmed rather than changed
The analysis result is fetched on every request, before the cache is consulted, and that looks like waste. It is what keeps
gonecorrect: a release deletes the analysis, and a reader must be told to re-run rather than handed a confident summary of something that no longer exists.A cache that skipped the fetch would serve stale summaries of deleted results indefinitely. Now pinned by a test, so nobody optimises it away.
Verified by sabotage, with the sabotage asserted to have applied. All checks gated with
set -e.🤖 Generated with Claude Code