refactor(structural): stop the model echoing the symbol delta, and make the delta worth having - #22
Merged
Merged
Conversation
…elta Measured on cpg-infrastructure#373 (6 files, 13 hunks): the overview pass was 136 s of a 237 s run, generation-bound at 16,066 output tokens, and 89% of its structured output was `symbols_modified` — 262 entries, set-identical to a freshly recomputed `SymbolDelta`. Nothing invented, nothing dropped, and nothing read it: `symbols_added/modified/removed` were declared in `types.d.ts` and referenced by no `.ts` module, because the viewer's Symbols axis is built from the deterministic delta (ADR 0001 stance C). Every augment pass but the console already passed `include_symbols=False`, and the console already receives the delta as its own seed section. Retire `OverviewSymbol` and the three fields from `Overview`, `OverviewSubmission`, the SSE payload, the viewer JSON, `augmented.diff`, and `types.d.ts`; drop `overview_to_prompt_json`'s `include_symbols` parameter. The prompt's `# Symbols changed (deterministic …)` seed section stays — input is cheap where output is not, and it grounds `summary` / `themes` / `groups`. Its instruction changes from "populate the fields verbatim" to "use it as context", since the fields it named no longer exist. `format/parse._build_overview` reads named keys, so a run directory written before this change parses fine and simply drops them on re-emit; covered by a new test. No `PROMPT_VERSION` bump: the cache key is `(pass, model, system, *cache_inputs)`, and the overview pass is the only one whose system text or user text moved, so it self-invalidates. Bumping would evict the expensive hunk/fold/explainer entries for nothing.
`modified` was "same qualified name, differing range", which on a real 6-file diff meant 262 entries of which 6 were genuine API changes. 244 had shifted only because lines above them moved. The bucket was 93% noise, so every consumer had to treat the whole thing as noise. Compare span *text*, not span, and split the result: - `modified` — the code differs. `reason` is `signature` (the declared header moved: an API change) or `body`. - `moved` — byte-identical text in a new position. Its own bucket, not a `reason`, because it is the bulk and says nothing about the change: `added + removed + modified` is now "what changed" with no filtering. Text, not line count, decides. A body edit that adds and removes the same number of lines preserves `end - start`; on the measured diff a length comparison filed `CPGDatasetCloudInfrastructure` as a pure shift when its `create_group` had gained a keyword-only marker — one of the six real API changes, hidden. The same reasoning retires the old range-equality gate: an in-place edit that leaves the span unmoved is a change and is now flagged. `merge` collapses cross-file moves. A qualified name that is `removed` at one path and `added` at another *with identical text* is one event, so it becomes a single `moved` entry carrying `from_path`. Identity is required rather than similarity: `qualified_name` is unique only within a file, so two files each defining `helper` must not be linked, and a symbol that both moved and changed stays two entries — calling it one would be an inference, which ADR 0001 keeps out of this layer. `diff_file` now takes the sources its forests were parsed from; the comparison key rides on `ChangedSymbol.body_sha`, excluded from every serialisation since `merge` needs it across files but no consumer does. Counts on cpg-infrastructure#373: 264 entries before, 264 after — but "what changed" drops from 264 to 20 (2 added, 6 signature, 12 body), with 244 moved. On metamist#1276: 30 -> 11 (9 signature, 2 body, 19 moved). Consumers move with it, in one commit because each breaks the other's type check on its own: - The overview and explainer-skeleton seeds tag a `modified` entry with its reason and omit `moved` entirely — the same 244 lines of nothing, now off the prompt as well. - The Symbols axis treats a moved symbol as context, exactly like an unchanged ancestor: it renders only when a changed descendant keeps it alive. A pill labelled "modified" that filtered to a hunk the symbol merely neighboured was actively misleading. A `modified` pill now says which of the declaration or the body changed. - `changed_symbols` gains `moved`; a `path` filter matches either end of a cross-file move, so the base-side file still sees it leave.
Two `Amended:` paragraphs in place, as ADR 0002 and 0004 do — the original decision stands, two of its clauses moved. Stance B: the seed stays, the model's echo of it goes. Stance C already meant nothing read `Overview.symbols_*`, so asking for them bought 16,066 output tokens of transcription and no consumer. Parsing: range-inequality was the wrong test for `modified` — it was 93% line shifts, and it missed an in-place edit that left the span unmoved. Span text decides, `modified` carries a reason, `moved` is its own bucket, and `merge` collapses cross-file moves by text identity. CONTEXT.md's Symbol / SymbolDelta / Overview seed / Symbols axis entries follow, and TREE_SITTER.md stops naming the retired fields.
Swept 110 cached run directories: 11 carry a cross-file move, and the matches are genuine module splits — five constants and three classes out of `litfetch/sources.py`, a `FixtureBackend` out of a literature backend, `_segment_block` out of `build_json.py`. All were two unrelated events before. Among them, a one-line `log = logging.getLogger(__name__)` deleted from one module and present in another matches too. Deterministically a move by the stated rule; a span-length threshold to suppress it would be an arbitrary line the rule does not need. Say so instead.
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.
Two halves of one problem, found by timing a cold
scr proncpg-infrastructure#373(6 files, 13 hunks, 237 s).The measurement
The overview pass is 136 s — 58% of the run — and generation-bound: 31,678
input tokens against 16,066 output. Input prefills fast and is half
cache-read; output generates serially. So the cheapest pass in tokens is the most
expensive in time.
89% of that output was
symbols_modified— 262 entries, 29,253 of 32,894chars of structured output. Recomputing the deterministic
SymbolDeltaanddiffing it against what the model emitted: 262 vs 262, set-identical, zero
invented, zero dropped. Pure transcription of a list already in its own prompt,
which told it to copy the list "verbatim".
And nothing read the result. The three fields were declared in
types.d.tsandreferenced by no
.tsmodule — the viewer's Symbols axis is builtindependently from the deterministic delta (ADR 0001 stance C). Every augment
pass but the console passed
include_symbols=False, and the console — the onlyTruecaller — already received the same delta as its own seed section.Half 1 — delete the echo, keep the seed
Overview.symbols_*andOverviewSymbolare gone, along with theirserialisation into the SSE payload, the viewer JSON,
augmented.diffandtypes.d.ts, andoverview_to_prompt_json'sinclude_symbolsparameter.The prompt's seed section stays. It is useful grounding for
summary/themes/groups, and input is cheap where output is not. Itsinstruction changes from "populate
symbols_*from THIS list verbatim" to "useit to ground the summary; do not restate the list back".
Stale
augmented.difffiles are tolerated rather than migrated:format/parse._build_overviewreads named keys, so a retired field in an oldartefact is dropped silently and a re-emit writes the current shape. Covered by a
test.
PROMPT_VERSIONis deliberately not bumped. The cache key is(pass_name, model, system, *cache_inputs); only the overview's system and usertext moved, so it self-invalidates on its own key, while every other pass's
prompt bytes are unchanged. A bump would evict the expensive hunk, fold-summary
and explainer entries for nothing.
Half 2 — make the delta worth having
modifiedwas a range comparison, so inserting two lines near the top of afile flagged everything below it.
driver.pyreported 150 of its 177 symbols.Now
modifiedcarries areason—SIGNATUREorBODY— and a pure relocationmoves to its own
movedbucket. Cross-file moves are detected inmergeandcollapse a
removed+addedpair into onemovedentry carryingfrom_path.The six
SIGNATUREentries on #373 are exactly that PR's subject —create_groupgaining
group_settingsacross five classes plus the provider, anddescription/
cache_membersbecoming keyword-only.Two corrections to the brief this was written from
ChangedSymbolalready carriedsignature. The brief said to add it; it wasthere. Nothing to do.
Span text, not span length. The brief proposed classifying by span length —
same length plus a moved start means a pure shift. That is wrong on real data:
CPGDatasetCloudInfrastructureshifted 7 lines with an identical line count, solength calls it moved-only, while its
create_grouphad gained a*— one of thesix real API changes. Exact text comparison catches it. Text also retires the old
range-equality gate, so an in-place edit that leaves the span unmoved is no longer
invisible by design (zero extra entries across the four runs measured — the gate
was right in principle and free in practice).
Design calls
movedis a bucket, not a reason. It is 93% of the oldmodifiedand saysnothing about the change; as a separate bucket,
added + removed + modifiedis"what changed" with nothing left to filter.
removedhalf wouldsend a reviewer chasing a symbol that still exists.
qualified_name—which is only unique within a file. That keeps it a set intersection rather
than an inference, and a symbol that both moved and changed stays two entries.
log = logging.getLogger(__name__)deleted from one module and present inanother matches by text and reports as a move. It is deterministically true; the
alternative was an arbitrary span-length threshold. Swept across 110 cached
runs, 11 have cross-file moves and the rest are genuine module splits.
Wired through
The Symbols axis treats a
movedsymbol as context — it renders only when achanged descendant keeps it alive, which the existing subtree prune already does —
and a
modifiedpill now says which kind. Both prompt seeds tagmodifiedbyreason and omit
movedentirely, taking those 244 lines of nothing off the inputas well as the output.
RepoTools.changed_symbolsreturns the four buckets, andits
pathfilter matches either end of a move so the base-side file still sees itleave.
ADR 0001 amended in place with two
Amended:paragraphs, in the style ADR 0002and 0004 already use: the seed stays, the model's echo goes, and
modifiedgainsa reason.
Gate
914 passed / 3 skipped (was 903, net +11, none removed),
pyright0,ruffand
ruff formatclean,npm run test:js362.