diff --git a/CONTEXT.md b/CONTEXT.md index 8c6668d..a8f0b73 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -431,37 +431,65 @@ This is the single internal currency the structural consumers read: the `RepoTools.outline` / `symbol_at` tools, the diff-wide delta, the overview-prompt seed, and the sidebar Symbols axis. It is deliberately *not* reconciled with the LLM-derived -`Overview.symbols_*` / `FileSymbols` — those answer "why did this -change" (semantic, fallible); `Symbol` answers "where is the code and -what does it literally declare" (structural, exact). The two coexist as -separate layers by design (ADR 0001). +per-file `FileSymbols` — that answers "why did this change" (semantic, +fallible); `Symbol` answers "where is the code and what does it +literally declare" (structural, exact). The two coexist as separate +layers by design (ADR 0001). The PR-level `Overview.symbols_*` used to +be the other half of that pairing and is gone: it was the model +transcribing the [[SymbolDelta]] out of its own prompt, 89% of the +overview pass's output, read by nothing. **SymbolDelta** The deterministic base→head structural delta — `{added, removed, -modified}` lists of flat `ChangedSymbol`s, defined in +modified, moved}` lists of flat `ChangedSymbol`s, defined in `structural/diff.py`. Computed by a `qualified_name` set-diff over the flattened base and head `Symbol` forests (`diff_file` per file, `merge` -diff-wide): added = head-only name, removed = base-only, **modified = -same name on both sides with a differing range** (a same-span body edit -is not flagged — the range is the signal; finer "what changed" meaning -stays the LLM's). Each `ChangedSymbol` carries its `path` and the span -on its live side (head for added/modified, base for removed). Computed -by `RepoTools.compute_symbol_delta()`, which reads base via `git show` -and head from the worktree for every changed file in a supported -language; `changed_symbols()` is its JSON wrapper for the LLM tool -surface. +diff-wide), refined by comparing the two sides' **span text**: + +- `added` / `removed` — the name exists on one side only. +- `modified` — the text differs. `reason` is `ChangeReason.SIGNATURE` + when the declared header moved (an API change) or `BODY` when only the + implementation did. +- `moved` — the text is byte-identical in a new position. Its own bucket + rather than a reason, because it is the bulk of the delta and says + nothing about the change: `added + removed + modified` is "what + changed" with nothing to filter. Measured on cpg-infrastructure#373, + 244 of 262 same-name-both-sides symbols were moves. + +Text is the comparison, not the span: a body edit that adds and removes +the same number of lines preserves `end - start`, and an in-place edit +preserves the span outright — both would read as no-change under a range +or length test. `merge` also collapses **cross-file** moves diff-wide: a +qualified name `removed` at one path and `added` at another with +identical text becomes one `moved` entry carrying `from_path`. Identity +is required, not similarity — `qualified_name` is unique only within a +file — so a symbol that both moved and changed stays two entries. +`ChangedSymbol.body_sha` is the comparison key `merge` needs across +files; it is excluded from every serialisation. + +Each `ChangedSymbol` carries its `path` and the span on its live side +(head for added/modified/moved, base for removed). Computed by +`RepoTools.compute_symbol_delta()`, which reads base via `git show` and +head from the worktree for every changed file in a supported language; +`changed_symbols()` is its JSON wrapper for the LLM tool surface. **Overview seed** -Before the overview pass, the pipeline computes the `SymbolDelta` and +Before the overview pass, the pipeline computes the [[SymbolDelta]] and passes it to `format_overview_prompt`, which appends a `# Symbols changed (deterministic …)` section listing each changed symbol by kind -and `qualified_name`. The overview system prompt instructs the model to -populate `Overview.symbols_*` from that section verbatim — turning the -symbol fields from inference into a deterministic seed (ADR 0001 Slice -3). The seed is our own tree-sitter parse rather than LLM tool access, -and best-effort (a failure leaves the overview unseeded). When the delta is empty — every changed file is in -an unsupported language — no section is appended and the prompt is -byte-identical to the pre-seed form. +and `qualified_name`, tagging a `modified` entry with its reason. It is +context, not an order: the model uses it to ground `summary` / `themes` +/ `groups` and reports none of it back. Asking for it back was 89% of +the pass's output tokens and a pure transcription of the prompt (ADR +0001, amended). The `moved` bucket is omitted — byte-identical code that +shifted lines is prompt weight with no signal. The explainer skeleton's +`_format_symbol_section` renders the same shape for the same reasons. + +The seed is our own tree-sitter parse rather than LLM tool access, and +best-effort (a failure leaves the overview unseeded). When every +rendered bucket is empty — e.g. every changed file is in an unsupported +language — no section is appended and the prompt is byte-identical to +the pre-seed form. **Symbols axis** The third sidebar grouping axis (after Themes and Files), built @@ -472,7 +500,11 @@ parses each changed file's base/head worktree, takes the per-file The changed symbols are then nested by `qualified_name` into a forest of `GroupBlock` nodes (id `SY`, class ▸ method): a changed method hangs off its enclosing class, and an unchanged ancestor is synthesized as a -context node from the live forest. A parent's `hunk_ids` is its subtree +context node from the live forest. A `moved` symbol is context too — its +text is byte-identical across the revisions, so it earns no pill of its +own and renders only when a changed descendant keeps it alive. A +`modified` pill's rationale names its reason ("function signature +changed in …"). A parent's `hunk_ids` is its subtree union (clicking it filters to every changed descendant) and the count is the distinct hunks beneath it; a leaf carries only its own. Any node whose whole subtree touches no hunk yields no block. The viewer's diff --git a/TREE_SITTER.md b/TREE_SITTER.md index 2759b2c..a6f65ba 100644 --- a/TREE_SITTER.md +++ b/TREE_SITTER.md @@ -41,8 +41,9 @@ HTTP server with a back-channel: a shipped grammar. - **Deterministic overview seed.** The overview pass is seeded with the base→head `SymbolDelta` (`augment/overview.py` `_format_symbol_seed`), - so `symbols_added` / `symbols_modified` come from the parse rather than - the LLM guessing. + so the symbol inventory comes from the parse rather than the LLM + guessing — and the model no longer restates it, which is what its + `symbols_*` fields were (ADR 0001, amended). Grammars ship as three individual packages — `tree-sitter-python`, `tree-sitter-javascript`, `tree-sitter-typescript`. Other languages diff --git a/docs/adr/0001-tree-sitter-structural-layer.md b/docs/adr/0001-tree-sitter-structural-layer.md index f21b2ed..342f414 100644 --- a/docs/adr/0001-tree-sitter-structural-layer.md +++ b/docs/adr/0001-tree-sitter-structural-layer.md @@ -40,6 +40,21 @@ to help. it starts from truth rather than re-deriving `symbols_*`). Seeding is per-language; unsupported languages fall back to today's behaviour. + Amended: the seed stays; the model's echo of it goes. Seeding was + specified as "populate `symbols_*` from this list verbatim", which + made the model transcribe its own prompt. Measured on + cpg-infrastructure#373, the overview pass was 58% of the run and + generation-bound at 16,066 output tokens, 89% of its structured output + was `symbols_modified`, and the 262 entries were set-identical to a + freshly recomputed `SymbolDelta` — nothing invented, nothing dropped. + Nothing read the result: stance C means the viewer builds its Symbols + axis from the delta, and `Overview.symbols_*` reached no consumer. + The three fields are retired from the schema, the prompt, the SSE + payload, the viewer JSON and `augmented.diff`. The prompt section + stays as grounding for `summary` / `themes` / `groups` — input is + cheap where output is not — and now says so instead of ordering a + transcription. + ### Parsing - **Engine:** `tree-sitter` core + curated, individually @@ -54,6 +69,26 @@ to help. `RepoTools.read_file_at` → `git show :`). Added / removed / modified are derived by `qualified_name` set-diff (modified = same qualified name present on both sides with a differing range). + + Amended: the range was the wrong signal and `modified` gains a reason. + Range-inequality made `modified` 93% noise — 244 of 262 entries on the + measured diff had shifted only because lines above them moved, and it + missed an in-place edit that left the span unmoved. The comparison is + now the span *text*, and the result splits four ways: `added`, + `removed`, `modified` (text differs; `reason` is `signature` when the + declared header moved, else `body`), and `moved` (text identical, new + position). `moved` is a bucket rather than a reason so that + `added + removed + modified` is "what changed" with no filtering. + Line count cannot substitute for text: a body edit that adds and + removes equally preserves the span length, and on the measured diff + that hid one of the six real API changes. + + `merge` additionally resolves cross-file moves diff-wide: a qualified + name `removed` at one path and `added` at another *with identical + text* collapses to one `moved` entry carrying `from_path`. Identity, + not similarity — `qualified_name` is unique only within a file, and a + symbol that both moved and changed stays two entries rather than + becoming an inference this layer does not make. - **Extraction:** tree-sitter **`tags.scm` tag queries** (the established convention; vendor a curated query where a grammar ships none) → a normalized `Symbol{kind, name, qualified_name, range, @@ -106,7 +141,8 @@ LLM-derived layer is unaffected. No hard failure, no empty UI noise. ## Consequences - The model can no longer hallucinate the symbol delta on supported - languages — it is handed the truth and verifies against it. + languages — it is handed the truth and verifies against it, and (per + the amendment above) is no longer asked to repeat it back. - The pinned-dependency surface grows by one wheel per language; each is auditable in `requirements.lock`. - Two "symbol" notions coexist by design (LLM-semantic, tree-sitter- diff --git a/semantic_code_review/augment/explainer.py b/semantic_code_review/augment/explainer.py index fbbaf55..58ff5ad 100644 --- a/semantic_code_review/augment/explainer.py +++ b/semantic_code_review/augment/explainer.py @@ -219,8 +219,11 @@ def _format_file_list(diff: AnnotatedDiff) -> str: def _format_symbol_section(delta: SymbolDelta | None) -> str: """Render the deterministic symbol delta compactly, or `""`. - Kind and qualified name only: the skeleton is ordering files, so a - symbol's line range would be weight it cannot spend. + Kind, qualified name and — for a `modified` entry — whether the + declaration or only the body changed. No line ranges: the skeleton is + ordering files, so a symbol's span would be weight it cannot spend. + `delta.moved` is omitted for the same reason it is omitted from the + overview seed: byte-identical code that shifted lines is not a change. """ if delta is None: return "" @@ -233,7 +236,8 @@ def _format_symbol_section(delta: SymbolDelta | None) -> str: continue lines.append(f"{label}:") for c in items: - lines.append(f" {c.kind} {c.qualified_name} ({c.path})") + tag = f" [{c.reason}]" if c.reason is not None else "" + lines.append(f" {c.kind} {c.qualified_name}{tag} ({c.path})") return "\n".join(lines) @@ -384,7 +388,7 @@ async def generate_explainer_skeleton( system_text, user_prefix = carry_guidance(client, guidance) user_text = format_skeleton_prompt( diff, - overview_json=overview_to_prompt_json(diff, include_symbols=False), + overview_json=overview_to_prompt_json(diff), delta=_symbol_delta(run_dir, diff), ) if user_prefix: diff --git a/semantic_code_review/augment/explainer_section.py b/semantic_code_review/augment/explainer_section.py index 01cd7c7..c60b0ce 100644 --- a/semantic_code_review/augment/explainer_section.py +++ b/semantic_code_review/augment/explainer_section.py @@ -874,7 +874,7 @@ async def generate_explainer_section( diff, doc, targets, - overview_json=overview_to_prompt_json(diff, include_symbols=False), + overview_json=overview_to_prompt_json(diff), ) if user_prefix: user_text = f"{user_prefix}\n\n{user_text}" diff --git a/semantic_code_review/augment/fold_summary.py b/semantic_code_review/augment/fold_summary.py index e8ce2c9..cea49e1 100644 --- a/semantic_code_review/augment/fold_summary.py +++ b/semantic_code_review/augment/fold_summary.py @@ -330,7 +330,7 @@ async def apply_fold_summary_to_run( run_dir=run_dir, file_path=fp.path, file_summary=(fp.ann.summary or "").strip(), - overview_json=overview_to_prompt_json(diff, include_symbols=False), + overview_json=overview_to_prompt_json(diff), context=context, right_range=right_range, left_range=left_range, diff --git a/semantic_code_review/augment/hunks.py b/semantic_code_review/augment/hunks.py index 357b33e..671b80e 100644 --- a/semantic_code_review/augment/hunks.py +++ b/semantic_code_review/augment/hunks.py @@ -522,20 +522,13 @@ def _ref(d: dict[str, Any]) -> dict[str, Any]: return {"path": d["path"], "line": int(d["line"]), "reason": d.get("reason", "") or ""} -def overview_to_prompt_json(diff: AnnotatedDiff, *, include_symbols: bool = True) -> str: +def overview_to_prompt_json(diff: AnnotatedDiff) -> str: """Serialize the overview into a compact JSON string for a prompt. - `include_symbols=False` drops the three symbol inventories, leaving - the prose. They are the bulk of the overview — measured at 92% of it - on a 47-file diff, 14,353 of 15,656 characters — and a per-hunk - prompt re-sends the whole thing for every hunk of the file, so the - cost grows with the diff rather than with the hunk. The passes that - repeat per hunk therefore leave them out and let the model pull what - it needs from the `changed_symbols` tool, which can filter by path - and carries more per entry than the overview did. - - The console keeps them: it is one long-lived conversation, not a - call per hunk, so there is nothing to amortise. + Prose only. The symbol inventory lives in the deterministic + `SymbolDelta` — reachable through the `changed_symbols` tool, which + filters by path and carries more per entry than the overview ever + did — so nothing here restates it. """ if not isinstance(diff.overview, Overview): return "{}" @@ -552,8 +545,4 @@ def overview_to_prompt_json(diff: AnnotatedDiff, *, include_symbols: bool = True "themes": list(diff.overview.themes), "callgraph_edges": [e.model_dump(by_alias=True) for e in diff.overview.callgraph_edges], } - if include_symbols: - payload["symbols_added"] = [s.model_dump() for s in diff.overview.symbols_added] - payload["symbols_modified"] = [s.model_dump() for s in diff.overview.symbols_modified] - payload["symbols_removed"] = [s.model_dump() for s in diff.overview.symbols_removed] return json.dumps(payload, ensure_ascii=False) diff --git a/semantic_code_review/augment/overview.py b/semantic_code_review/augment/overview.py index 9a799e5..f85be72 100644 --- a/semantic_code_review/augment/overview.py +++ b/semantic_code_review/augment/overview.py @@ -19,7 +19,6 @@ OverviewEdge, OverviewGroup, OverviewGroupMember, - OverviewSymbol, ) from ..cache.store import CacheStore from ..structural import SymbolDelta @@ -41,10 +40,10 @@ def format_overview_prompt( """Produce the user-message text for the overview call. `delta`, when present and non-empty, seeds the prompt with the - deterministic tree-sitter symbol delta (ADR 0001) so the model - reports `symbols_*` from ground truth rather than guessing from hunk - headers. An absent or empty delta (no supported-language change) - appends nothing — the prompt is byte-identical to the pre-seed form. + deterministic tree-sitter symbol delta (ADR 0001) — grounding for the + summary, themes and groups, not something the model reports back. An + absent or empty delta (no supported-language change) appends nothing + — the prompt is byte-identical to the pre-seed form. """ parts: list[str] = [] title = meta.get("title", "") @@ -81,9 +80,15 @@ def format_overview_prompt( def _format_symbol_seed(delta: SymbolDelta | None) -> str: """Render the deterministic symbol delta as a prompt section, or `""`. + `delta.moved` is omitted: byte-identical code that only shifted lines + says nothing about the change, and it is the bulk of the delta. + A `modified` entry is labelled by its `reason`, so the model can see + which changes touched a declaration. + Returns the empty string when there's nothing to seed (no delta, or - every bucket empty) so callers can keep the prompt byte-identical to - the unseeded form for all-unsupported-language diffs. + every rendered bucket empty) so callers can keep the prompt + byte-identical to the unseeded form for all-unsupported-language + diffs. """ if delta is None: return "" @@ -93,10 +98,11 @@ def _format_symbol_seed(delta: SymbolDelta | None) -> str: lines = [ "\n# Symbols changed (deterministic — tree-sitter, not your inference)", "These are the exact definitions that changed between base and head, by " - "name and kind. Populate `symbols_added` / `symbols_modified` / " - "`symbols_removed` from THIS list verbatim — one entry per line below, " - "using its `qualified_name` as the symbol `name`. Do not add, drop, or " - "rename entries; languages absent here are simply not yet parseable.", + "name and kind — ground truth, not your inference. A `modified` entry is " + "tagged `signature` when its declaration changed (an API change) or `body` " + "when only the implementation did. Definitions that merely shifted lines are " + "omitted. Use these to ground the summary, themes and groups; do not restate " + "the list back. Languages absent here are simply not yet parseable.", ] for label, items in buckets: lines.append(f"{label}:") @@ -104,7 +110,8 @@ def _format_symbol_seed(delta: SymbolDelta | None) -> str: lines.append(" (none)") continue for c in items: - lines.append(f" {c.kind} {c.qualified_name} ({c.path})") + tag = f" [{c.reason}]" if c.reason is not None else "" + lines.append(f" {c.kind} {c.qualified_name}{tag} ({c.path})") return "\n".join(lines) @@ -153,9 +160,6 @@ def apply_overview_to_diff(diff: AnnotatedDiff, submit_args: dict[str, Any]) -> """ overview = Overview( summary=submit_args.get("summary", ""), - symbols_added=[OverviewSymbol(**s) for s in submit_args.get("symbols_added", [])], - symbols_modified=[OverviewSymbol(**s) for s in submit_args.get("symbols_modified", [])], - symbols_removed=[OverviewSymbol(**s) for s in submit_args.get("symbols_removed", [])], callgraph_edges=[OverviewEdge.model_validate(e) for e in submit_args.get("callgraph_edges", [])], themes=list(submit_args.get("themes", [])), groups=_resolve_groups(diff, submit_args.get("groups") or []), diff --git a/semantic_code_review/augment/pipeline.py b/semantic_code_review/augment/pipeline.py index 8324960..0139db4 100644 --- a/semantic_code_review/augment/pipeline.py +++ b/semantic_code_review/augment/pipeline.py @@ -214,7 +214,7 @@ async def augment_run_dir( mcp_host.start() client.set_mcp_endpoint(mcp_host.mcp_config()) - overview_json = overview_to_prompt_json(diff, include_symbols=False) + overview_json = overview_to_prompt_json(diff) # Per-file definition spans, parsed once from the worktrees, so the # per-hunk SSE re-emits below carry symbol-aware `fold_regions` @@ -709,9 +709,6 @@ def _overview_event_payload(diff: AnnotatedDiff) -> dict[str, Any]: "pr": { "summary": ov.summary if ov else "", "themes": ov.themes if ov else [], - "symbols_added": [s.model_dump() for s in (ov.symbols_added if ov else [])], - "symbols_modified": [s.model_dump() for s in (ov.symbols_modified if ov else [])], - "symbols_removed": [s.model_dump() for s in (ov.symbols_removed if ov else [])], "callgraph_edges": [e.model_dump(by_alias=True) for e in (ov.callgraph_edges if ov else [])], }, "groups": groups, diff --git a/semantic_code_review/augment/prompts.py b/semantic_code_review/augment/prompts.py index dfcacbd..796b7f0 100644 --- a/semantic_code_review/augment/prompts.py +++ b/semantic_code_review/augment/prompts.py @@ -53,12 +53,9 @@ "Produce a concise overview.\n\n" "Guidelines:\n" "- Lead with WHY, not WHAT.\n" - "- Symbol kinds are: function, method, class, constant.\n" "- If the user prompt has a `# Symbols changed` section, it is a deterministic " - " parse of what actually changed. Populate `symbols_added`/`symbols_modified`/" - " `symbols_removed` from it verbatim — that list is ground truth; do not invent, " - " drop, or rename entries. With no such section, infer symbols from the hunk " - " headers as before.\n" + " parse of what actually changed — ground truth, not your inference. Use it to " + " ground `summary`, `themes` and `groups`; do not restate it as a list.\n" "- `callgraph_edges` are introduced or modified calls (best-effort — omit if unsure).\n" "- `themes` are short keyword tags (e.g. 'pagination', 'api-surface').\n" "- Per-file `summary` is one sentence; `lang` only when the extension is ambiguous.\n" diff --git a/semantic_code_review/augment/schemas.py b/semantic_code_review/augment/schemas.py index b9e51cc..c631b8c 100644 --- a/semantic_code_review/augment/schemas.py +++ b/semantic_code_review/augment/schemas.py @@ -137,12 +137,6 @@ class FileSymbols(BaseModel): removed: list[str] = Field(default_factory=list) -class OverviewSymbol(BaseModel): - path: str - kind: str - name: str - - class OverviewEdge(BaseModel): src: str = Field(alias="from") dst: str = Field(alias="to") @@ -179,9 +173,6 @@ class OverviewGroup(BaseModel): class Overview(BaseModel): summary: str = "" - symbols_added: list[OverviewSymbol] = Field(default_factory=list) - symbols_modified: list[OverviewSymbol] = Field(default_factory=list) - symbols_removed: list[OverviewSymbol] = Field(default_factory=list) callgraph_edges: list[OverviewEdge] = Field(default_factory=list) themes: list[str] = Field(default_factory=list) groups: list[OverviewGroup] = Field(default_factory=list) @@ -344,9 +335,6 @@ class OverviewSubmission(BaseModel): """Wire format of `submit_overview`. Consumed by `apply_overview_to_diff`.""" summary: str = Field(description="1-3 sentence summary of the PR's intent.") - symbols_added: list[OverviewSymbol] = Field(default_factory=list) - symbols_modified: list[OverviewSymbol] = Field(default_factory=list) - symbols_removed: list[OverviewSymbol] = Field(default_factory=list) callgraph_edges: list[OverviewEdge] = Field( default_factory=list, description="Introduced or modified calls (best-effort — omit if unsure).", diff --git a/semantic_code_review/augment/tools.py b/semantic_code_review/augment/tools.py index 643fb67..4d72863 100644 --- a/semantic_code_review/augment/tools.py +++ b/semantic_code_review/augment/tools.py @@ -255,7 +255,7 @@ def compute_symbol_delta(self) -> structural.SymbolDelta: head_src = self._read_source(path, None) base_syms = self._outline_symbols(path, self.base_sha, base_src, lang) if base_src is not None else [] head_syms = self._outline_symbols(path, None, head_src, lang) if head_src is not None else [] - deltas.append(structural.diff_file(path, base_syms, head_syms)) + deltas.append(structural.diff_file(path, base_syms, head_syms, base_src=base_src, head_src=head_src)) return structural.merge(deltas) @_tool @@ -263,15 +263,22 @@ def changed_symbols(self, path: str | None = None) -> str: """Deterministic structural delta of the diff, as JSON. Compares the base commit against the head worktree for every - changed file in a supported language, returning - `{added, removed, modified}` lists of symbols by `qualified_name` - set-diff — no LLM, no hallucination. `modified` means the same - qualified name on both sides with a differing line range; a - same-span body edit is not flagged. Each entry carries its - `path`, `kind`, `name`, `qualified_name`, declared `signature`, - and the line `range` on its live side (head for added/modified, - base for removed). Changed files in unsupported languages are - silently absent. + changed file in a supported language — no LLM, no hallucination. + Four buckets by `qualified_name` set-diff: + + - `added` / `removed` — the name exists on one side only. + - `modified` — same name, and the code differs. `reason` is + `signature` (the declared header changed — an API change) or + `body` (header unchanged, implementation differs). + - `moved` — same name and byte-identical text: the definition + only shifted. `from_path` names the base-side file when the + move crossed files; absent for a line shift within one file. + + Read `modified` first: it is what actually changed. Each entry + carries its `path`, `kind`, `name`, `qualified_name`, declared + `signature`, and the line `range` on its live side (head for + added/modified/moved, base for removed). Changed files in + unsupported languages are silently absent. This is the whole-PR symbol inventory the hunk prompt no longer carries inline: on a large diff it ran to tens of thousands of @@ -292,6 +299,9 @@ def changed_symbols(self, path: str | None = None) -> str: added=[s for s in delta.added if s.path == path], removed=[s for s in delta.removed if s.path == path], modified=[s for s in delta.modified if s.path == path], + # A cross-file move is in `moved` under its head path; the + # base-side file wants to see it leave, so match either end. + moved=[s for s in delta.moved if path in (s.path, s.from_path)], ) return _cap(delta.model_dump_json()) diff --git a/semantic_code_review/format/emit.py b/semantic_code_review/format/emit.py index 1c18274..ee450d1 100644 --- a/semantic_code_review/format/emit.py +++ b/semantic_code_review/format/emit.py @@ -54,9 +54,6 @@ def _emit_preamble(diff: AnnotatedDiff) -> list[str]: def _overview_to_jsonable(ov: Overview) -> dict[str, Any]: return { "summary": ov.summary, - "symbols_added": [s.model_dump() for s in ov.symbols_added], - "symbols_modified": [s.model_dump() for s in ov.symbols_modified], - "symbols_removed": [s.model_dump() for s in ov.symbols_removed], "callgraph_edges": [e.model_dump(by_alias=True) for e in ov.callgraph_edges], "themes": list(ov.themes), "groups": [g.model_dump() for g in ov.groups], diff --git a/semantic_code_review/format/parse.py b/semantic_code_review/format/parse.py index 0c4c329..83670a5 100644 --- a/semantic_code_review/format/parse.py +++ b/semantic_code_review/format/parse.py @@ -43,7 +43,6 @@ Overview, OverviewEdge, OverviewGroup, - OverviewSymbol, ParsedDiff, ParsedFile, ParsedHunk, @@ -259,9 +258,6 @@ def _apply_preamble(directives: list[_Directive]) -> _PreambleResult: def _build_overview(data: dict) -> Overview: return Overview( summary=data.get("summary", ""), - symbols_added=[OverviewSymbol(**s) for s in data.get("symbols_added", [])], - symbols_modified=[OverviewSymbol(**s) for s in data.get("symbols_modified", [])], - symbols_removed=[OverviewSymbol(**s) for s in data.get("symbols_removed", [])], callgraph_edges=[OverviewEdge.model_validate(e) for e in data.get("callgraph_edges", [])], themes=list(data.get("themes", [])), groups=[OverviewGroup.model_validate(g) for g in data.get("groups", [])], diff --git a/semantic_code_review/structural/__init__.py b/semantic_code_review/structural/__init__.py index d95fd78..0e7e9bb 100644 --- a/semantic_code_review/structural/__init__.py +++ b/semantic_code_review/structural/__init__.py @@ -15,7 +15,7 @@ from __future__ import annotations -from .diff import ChangedSymbol, SymbolDelta, diff_file, flatten, merge +from .diff import ChangedSymbol, ChangeReason, SymbolDelta, diff_file, flatten, merge from .parse import ( enclosing_symbol, language_for_path, @@ -26,6 +26,7 @@ from .symbols import Symbol, SymbolRange __all__ = [ + "ChangeReason", "ChangedSymbol", "Symbol", "SymbolDelta", diff --git a/semantic_code_review/structural/diff.py b/semantic_code_review/structural/diff.py index 1c6e094..25c682e 100644 --- a/semantic_code_review/structural/diff.py +++ b/semantic_code_review/structural/diff.py @@ -1,24 +1,53 @@ """Deterministic base→head symbol delta (ADR 0001). The `Symbol` forest carries *where the code is*; this module carries -*what moved between two revisions*. Added / removed / modified are a -`qualified_name` set-diff over the flattened base and head forests: +*what changed between two revisions*. Buckets are a `qualified_name` +set-diff over the flattened base and head forests, refined by comparing +the two sides' declared signature and span text: - * added — qualified name present on head only + * added — qualified name present on head only * removed — present on base only - * modified — present on both, with a **differing range** - -A same-line body edit that leaves every span identical is therefore -*not* flagged — by design (ADR 0001): the range is the deterministic -signal, and the LLM layer owns finer "what actually changed" meaning. + * modified — present on both, and the code differs; `reason` says + whether the *declaration* moved (`SIGNATURE` — an API + change) or only the body (`BODY`) + * moved — present on both with byte-identical span text: a pure + relocation, no code change + +`moved` is a bucket rather than a `reason` on `modified` because it is +the overwhelming majority and carries no information about the change: +on a measured 6-file diff, 244 of 262 same-name-both-sides symbols had +shifted only because lines above them moved. Keeping them out of +`modified` means "what changed" needs no filtering by every consumer. + +Span *text* is what every comparison here turns on — not the span. A +body edit that adds and removes the same number of lines preserves +`end - start`, so a length comparison reports it as a pure shift; +measured on the same diff, that misfiled one of the six real API +changes. Conversely an in-place edit that leaves the span byte-for-byte +unmoved *is* flagged: the range is not the signal. + +Cross-file relocation is resolved in `merge`, diff-wide: a qualified +name that is `removed` at one path and `added` at another *with +identical span text* is one move, not two events, and collapses into a +single `moved` entry carrying `from_path`. Identity is required, not +similarity — `qualified_name` is only unique within a file, so two +unrelated files each defining `helper` must not be linked. A symbol +that both moved file and changed stays as separate added/removed +entries; calling those one event would be an inference, and ADR 0001 +keeps this layer to parse-and-compare. The rule reads a one-line +boilerplate definition (`log = logging.getLogger(__name__)` deleted from +one module, present in another) as a move. Deterministically true, and +not worth a span-length threshold to suppress. Each `ChangedSymbol` carries its `path` (the delta is diff-wide, across -files) and the span on its *live* side: head for added/modified, base -for removed. +files) and the span on its *live* side: head for added/modified/moved, +base for removed. """ from __future__ import annotations +import enum +import hashlib from collections.abc import Iterable from pydantic import BaseModel, Field @@ -26,12 +55,30 @@ from .symbols import Symbol, SymbolRange +class ChangeReason(enum.StrEnum): + """Why a symbol is in `SymbolDelta.modified`.""" + + SIGNATURE = "signature" + """The declared header differs — an API change.""" + + BODY = "body" + """The header is unchanged; the implementation differs.""" + + class ChangedSymbol(BaseModel): """One symbol that changed between base and head. Flat (the tree is flattened by `qualified_name` before diffing), so `children` is intentionally absent. `range` is the symbol's span on - its live side — head for added/modified, base for removed. + its live side — head for added/modified/moved, base for removed. + `reason` is set on `modified` entries only. `from_path` is set on a + `moved` entry whose relocation crossed files, and names the base-side + path. + + `body_sha` digests the symbol's span text. It is the comparison key + for `moved` — including across files, where `merge` has the entries + but not the sources — and is excluded from every serialisation: it is + an internal identity, not part of the `Symbol` currency the ADR pins. """ path: str @@ -40,14 +87,18 @@ class ChangedSymbol(BaseModel): qualified_name: str range: SymbolRange signature: str | None = None + reason: ChangeReason | None = None + from_path: str | None = None + body_sha: str = Field(exclude=True) class SymbolDelta(BaseModel): - """The diff-wide structural delta: three `qualified_name` set-diff buckets.""" + """The diff-wide structural delta, bucketed by what changed.""" added: list[ChangedSymbol] = Field(default_factory=list) removed: list[ChangedSymbol] = Field(default_factory=list) modified: list[ChangedSymbol] = Field(default_factory=list) + moved: list[ChangedSymbol] = Field(default_factory=list) def flatten(symbols: list[Symbol]) -> dict[str, Symbol]: @@ -68,7 +119,29 @@ def walk(syms: list[Symbol]) -> None: return out -def _changed(path: str, sym: Symbol) -> ChangedSymbol: +def _span_text(src: str, rng: SymbolRange) -> str: + """The source lines the symbol spans, 1-indexed inclusive.""" + return "\n".join(src.splitlines()[rng.start_line - 1 : rng.end_line]) + + +def _body_sha(src: str | None, sym: Symbol) -> str: + """Digest of a symbol's span text. + + `src` is `None` only when the file is absent on that side, in which + case no symbol from it can exist. + """ + if src is None: + raise ValueError(f"no source for {sym.qualified_name}: a symbol cannot come from an absent file") + return hashlib.sha256(_span_text(src, sym.range).encode("utf-8")).hexdigest() + + +def _changed( + path: str, + sym: Symbol, + src: str | None, + *, + reason: ChangeReason | None = None, +) -> ChangedSymbol: return ChangedSymbol( path=path, kind=sym.kind, @@ -76,27 +149,80 @@ def _changed(path: str, sym: Symbol) -> ChangedSymbol: qualified_name=sym.qualified_name, range=sym.range, signature=sym.signature, + reason=reason, + body_sha=_body_sha(src, sym), ) -def diff_file(path: str, base: list[Symbol], head: list[Symbol]) -> SymbolDelta: +def diff_file( + path: str, + base: list[Symbol], + head: list[Symbol], + *, + base_src: str | None, + head_src: str | None, +) -> SymbolDelta: """Per-file `qualified_name` set-diff between two `Symbol` forests. - An added file passes `base=[]`; a deleted file passes `head=[]`. + An added file passes `base=[]` and `base_src=None`; a deleted file + passes `head=[]` and `head_src=None`. The sources are the text the + forests were parsed from — `moved` vs `modified` compares span text, + which the `Symbol` tree does not carry. + + Cross-file relocation is not visible here; `merge` resolves it. """ b = flatten(base) h = flatten(head) - added = [_changed(path, h[q]) for q in h if q not in b] - removed = [_changed(path, b[q]) for q in b if q not in h] - modified = [_changed(path, h[q]) for q in h if q in b and h[q].range != b[q].range] - return SymbolDelta(added=added, removed=removed, modified=modified) + delta = SymbolDelta( + added=[_changed(path, h[q], head_src) for q in h if q not in b], + removed=[_changed(path, b[q], base_src) for q in b if q not in h], + ) + for q, hs in h.items(): + bs = b.get(q) + if bs is None: + continue + if _body_sha(head_src, hs) == _body_sha(base_src, bs): + if hs.range != bs.range: + delta.moved.append(_changed(path, hs, head_src)) + continue + reason = ChangeReason.SIGNATURE if hs.signature != bs.signature else ChangeReason.BODY + delta.modified.append(_changed(path, hs, head_src, reason=reason)) + return delta def merge(deltas: Iterable[SymbolDelta]) -> SymbolDelta: - """Concatenate per-file deltas into one diff-wide delta, order preserved.""" + """Concatenate per-file deltas into one diff-wide delta, order preserved. + + Collapses cross-file relocations: an `added` entry whose + `(qualified_name, body_sha)` also appears in some other file's + `removed` is the same code in a new home, so both entries are + replaced by one `moved` entry carrying `from_path`. Pairing is + one-to-one in source order. + """ out = SymbolDelta() for d in deltas: out.added.extend(d.added) out.removed.extend(d.removed) out.modified.extend(d.modified) + out.moved.extend(d.moved) + + candidates: dict[tuple[str, str], list[ChangedSymbol]] = {} + for r in out.removed: + candidates.setdefault((r.qualified_name, r.body_sha), []).append(r) + + relocated: list[ChangedSymbol] = [] + paired: set[int] = set() + kept_added: list[ChangedSymbol] = [] + for a in out.added: + queue = candidates.get((a.qualified_name, a.body_sha)) + source = next((r for r in queue if r.path != a.path and id(r) not in paired), None) if queue else None + if source is None: + kept_added.append(a) + continue + paired.add(id(source)) + relocated.append(a.model_copy(update={"from_path": source.path})) + + out.added = kept_added + out.removed = [r for r in out.removed if id(r) not in paired] + out.moved.extend(relocated) return out diff --git a/semantic_code_review/viewer/assets/types.d.ts b/semantic_code_review/viewer/assets/types.d.ts index 6d72672..c65c12c 100644 --- a/semantic_code_review/viewer/assets/types.d.ts +++ b/semantic_code_review/viewer/assets/types.d.ts @@ -58,18 +58,9 @@ interface PRBlock { url: string; summary: string; themes: string[]; - symbols_added: OverviewSymbol[]; - symbols_modified: OverviewSymbol[]; - symbols_removed: OverviewSymbol[]; callgraph_edges: OverviewEdge[]; } -interface OverviewSymbol { - kind: string; - name: string; - path: string; -} - interface OverviewEdge { /** dump_by_alias=True so the wire format uses the original `from` * / `to` keys rather than the Python-side `src` / `dst` attrs. */ diff --git a/semantic_code_review/viewer/build_json.py b/semantic_code_review/viewer/build_json.py index df56353..325ad92 100644 --- a/semantic_code_review/viewer/build_json.py +++ b/semantic_code_review/viewer/build_json.py @@ -166,9 +166,11 @@ class _SymNode: """A node in the per-file changed-symbol tree (slice 5 nesting). Directly-changed symbols carry a `status`; ancestors synthesized only - to give a changed descendant its context have `status=None`. `name` - is the short segment (the title shown in the tree); `hunk_ids` is the - distinct subtree union, filled bottom-up by `_rollup`. + to give a changed descendant its context have `status=None`, as does a + symbol that merely moved. `reason` refines a `modified` status into + `signature` or `body`. `name` is the short segment (the title shown in + the tree); `hunk_ids` is the distinct subtree union, filled bottom-up + by `_rollup`. """ qn: str @@ -176,6 +178,7 @@ class _SymNode: name: str = "" kind: str = "" status: str | None = None + reason: str | None = None start_line: int = 0 own_hunks: list[str] = field(default_factory=list) children: list[str] = field(default_factory=list) @@ -184,15 +187,19 @@ class _SymNode: @dataclass class _FileSymbols: - """The parsed base/head `Symbol` forests for one file. + """The parsed base/head `Symbol` forests for one file, and their source. Both lists are empty for an unsupported language or an unavailable worktree — the same graceful-degradation guard the changed-symbol - delta uses, so a file with no parse simply carries no spans. + delta uses, so a file with no parse simply carries no spans. The + sources ride along because `structural.diff_file` compares span text + to tell a moved definition from an edited one. """ base: list[structural.Symbol] = field(default_factory=list) head: list[structural.Symbol] = field(default_factory=list) + base_src: str | None = None + head_src: str | None = None def _file_symbols( @@ -209,6 +216,8 @@ def _file_symbols( return _FileSymbols( base=structural.outline_symbols(base_src, lang) if base_src is not None else [], head=structural.outline_symbols(head_src, lang) if head_src is not None else [], + base_src=base_src, + head_src=head_src, ) @@ -264,13 +273,19 @@ def _symbol_blocks( its enclosing class even when the class itself is unchanged — those ancestors are synthesized as context nodes from the live forest. + A `moved` symbol is context, not a change: its text is byte-identical + across the revisions, so it renders only when a changed descendant + keeps it alive, exactly like an unchanged ancestor. Without that the + axis is mostly noise — on a measured 6-file diff, 245 of 262 + same-name-both-sides symbols had shifted only because lines above + them moved. + A parent's `hunk_ids` is the union of its subtree's hunks (clicking it filters to every changed descendant); a leaf carries only its own. - Any node whose whole subtree touches no hunk — e.g. a symbol that only - shifted because lines moved above it, with no changed children — - yields no block, so every pill filters to at least one hunk. Absent - entirely when neither worktree is available or the language is - unsupported — those files carry empty `file_syms` and are skipped. + Any node whose whole subtree touches no hunk yields no block, so every + pill filters to at least one hunk. Absent entirely when neither + worktree is available or the language is unsupported — those files + carry empty `file_syms` and are skipped. """ out: list[dict[str, Any]] = [] counter = [0] @@ -278,7 +293,7 @@ def _symbol_blocks( base_syms, head_syms = syms.base, syms.head if not base_syms and not head_syms: continue - delta = structural.diff_file(f.path, base_syms, head_syms) + delta = structural.diff_file(f.path, base_syms, head_syms, base_src=syms.base_src, head_src=syms.head_src) head_spans = [ (h.parsed.new_start, h.parsed.new_start + h.parsed.new_count - 1, f"H{fi}_{hi}") for hi, h in enumerate(f.hunks) @@ -301,7 +316,8 @@ def _symbol_tree_blocks( counter: list[int], ) -> list[dict[str, Any]]: """Build one file's nested changed-symbol blocks (see `_symbol_blocks`).""" - # Live side is head for added/modified, base for removed. + # Live side is head for added/modified, base for removed. `delta.moved` + # is deliberately absent: byte-identical code is context, not a change. changed: dict[str, tuple[str, structural.ChangedSymbol, list[str]]] = {} for status, spans, syms in ( ("added", head_spans, delta.added), @@ -331,6 +347,7 @@ def ensure(qn: str) -> _SymNode: for qn, (status, cs, hids) in changed.items(): node = ensure(qn) node.status, node.kind, node.name = status, cs.kind, cs.name + node.reason = cs.reason.value if cs.reason is not None else None node.start_line, node.own_hunks = cs.range.start_line, hids # Fill metadata for synthesized ancestors from the live forests so the @@ -366,9 +383,7 @@ def emit(qn: str) -> dict[str, Any] | None: node = nodes[qn] if not node.hunk_ids: # nothing in this subtree touches a hunk return None - rationale = ( - f"{node.status} {node.kind} in {path}" if node.status is not None else f"{node.kind} (unchanged) in {path}" - ) + rationale = _symbol_rationale(node, path) block: dict[str, Any] = { "id": f"SY{counter[0]}", "title": node.name, @@ -384,6 +399,15 @@ def emit(qn: str) -> dict[str, Any] | None: return [b for r in roots if (b := emit(r)) is not None] +def _symbol_rationale(node: _SymNode, path: str) -> str: + """One line of context under a Symbols-axis pill.""" + if node.status is None: + return f"{node.kind} (unchanged) in {path}" + if node.reason is not None: + return f"{node.kind} {node.reason} changed in {path}" + return f"{node.status} {node.kind} in {path}" + + def _hunk_sort_key(hid: str) -> tuple[int, int]: """Sort key for "H{file}_{hunk}" ids → (file_idx, hunk_idx).""" try: @@ -430,9 +454,6 @@ def _pr_block(diff: AnnotatedDiff, meta: dict[str, Any]) -> dict[str, Any]: "url": meta.get("url", ""), "summary": ov.summary if ov else "", "themes": ov.themes if ov else [], - "symbols_added": [s.model_dump() for s in (ov.symbols_added if ov else [])], - "symbols_modified": [s.model_dump() for s in (ov.symbols_modified if ov else [])], - "symbols_removed": [s.model_dump() for s in (ov.symbols_removed if ov else [])], "callgraph_edges": [e.model_dump(by_alias=True) for e in (ov.callgraph_edges if ov else [])], } diff --git a/tests/fixtures/sample.augmented.diff b/tests/fixtures/sample.augmented.diff index 2e35718..d5c09eb 100644 --- a/tests/fixtures/sample.augmented.diff +++ b/tests/fixtures/sample.augmented.diff @@ -5,21 +5,6 @@ #scr: scr-model: claude-opus-4-7 2026-04-22 #scr: scr-overview: { #scr> "summary": "Introduces pagination on /users; callers updated.", -#scr> "symbols_added": [ -#scr> { -#scr> "path": "src/users.py", -#scr> "kind": "function", -#scr> "name": "paginate" -#scr> } -#scr> ], -#scr> "symbols_modified": [ -#scr> { -#scr> "path": "src/users.py", -#scr> "kind": "function", -#scr> "name": "list_users" -#scr> } -#scr> ], -#scr> "symbols_removed": [], #scr> "callgraph_edges": [ #scr> { #scr> "from": "src/users.py:list_users", diff --git a/tests/js/viewer.test.ts b/tests/js/viewer.test.ts index e8d7268..3de2488 100644 --- a/tests/js/viewer.test.ts +++ b/tests/js/viewer.test.ts @@ -271,7 +271,7 @@ function makeData(overrides: Partial = {}): ViewerData { return { version: "1", pending: true, - pr: { title: "test", themes: [], symbols_added: [], symbols_modified: [], symbols_removed: [], callgraph_edges: [] }, + pr: { title: "test", themes: [], callgraph_edges: [] }, smells_catalogue: {}, files: [{ id: "F0", @@ -488,7 +488,7 @@ describe("streaming events", () => { const es = lastEventSource(); es.dispatch("overview", { - pr: { summary: "bumps return values", themes: ["constants"], symbols_added: [], symbols_modified: [], symbols_removed: [], callgraph_edges: [] }, + pr: { summary: "bumps return values", themes: ["constants"], callgraph_edges: [] }, groups: [ { id: "G0", title: "return value bumps", rationale: "two related edits", hunk_ids: ["H0_0", "H0_1"] }, ], diff --git a/tests/test_batch_annotations.py b/tests/test_batch_annotations.py index 1c7c479..23cd9f6 100644 --- a/tests/test_batch_annotations.py +++ b/tests/test_batch_annotations.py @@ -6,6 +6,8 @@ from __future__ import annotations +import hashlib + import pytest from semantic_code_review.augment.hunks import split_batch_annotations @@ -191,6 +193,7 @@ def _removed(name: str = "mod._mcp_config_for", start: int = 159, end: int = 196 qualified_name=name, range=SymbolRange(start_line=start, end_line=end, start_col=0, end_col=0), signature=f"def {name.rsplit('.', maxsplit=1)[-1]}(self)", + body_sha=hashlib.sha256(name.encode()).hexdigest(), ) @@ -330,7 +333,7 @@ def test_the_overview_block_carries_the_base_sha() -> None: overview=Overview(summary="s"), ) - payload = json.loads(overview_to_prompt_json(diff, include_symbols=False)) + payload = json.loads(overview_to_prompt_json(diff)) assert payload["base_sha"] == "deadbeefcafe" diff --git a/tests/test_explainer_skeleton.py b/tests/test_explainer_skeleton.py index 9f0ecd3..b00a3e2 100644 --- a/tests/test_explainer_skeleton.py +++ b/tests/test_explainer_skeleton.py @@ -108,6 +108,7 @@ def test_prompt_seeds_the_symbol_delta_when_there_is_one(diff: AnnotatedDiff) -> name="cursor", qualified_name="ListRequest.cursor", range=SymbolRange(start_line=3, end_line=3, start_col=0, end_col=0), + body_sha="0" * 64, ) ] ) diff --git a/tests/test_format_roundtrip.py b/tests/test_format_roundtrip.py index e19f641..e7e449c 100644 --- a/tests/test_format_roundtrip.py +++ b/tests/test_format_roundtrip.py @@ -28,6 +28,27 @@ def test_fixture_lint_passes() -> None: assert result.ok, result.errors +def test_preamble_tolerates_retired_symbol_inventories() -> None: + """A run directory written before the LLM symbol echo was retired + still carries `symbols_added` / `symbols_modified` / `symbols_removed` + in `scr-overview`. `_build_overview` reads named keys, so the retired + ones are skipped rather than rejected; re-emitting drops them.""" + from semantic_code_review.augment.schemas import Overview + + text = FIXTURE.read_text(encoding="utf-8") + stale = text.replace( + '#scr> "summary": "Introduces pagination on /users; callers updated.",', + '#scr> "summary": "Introduces pagination on /users; callers updated.",\n' + '#scr> "symbols_added": [{"path": "src/users.py", "kind": "function", "name": "paginate"}],\n' + '#scr> "symbols_modified": [],\n' + '#scr> "symbols_removed": [],', + ) + diff = parse_augmented_diff(stale) + assert isinstance(diff.overview, Overview) + assert diff.overview.summary.startswith("Introduces pagination") + assert emit_augmented_diff(diff) == text + + def test_fixture_has_expected_structure() -> None: from semantic_code_review.augment.schemas import Overview @@ -113,7 +134,6 @@ def test_handwritten_annotated_diff_round_trips() -> None: HunkAnnotations, LineNote, Overview, - OverviewSymbol, ParsedHunk, PRInfo, Ref, @@ -130,7 +150,6 @@ def test_handwritten_annotated_diff_round_trips() -> None: ), overview=Overview( summary="Round-trip fixture.", - symbols_added=[OverviewSymbol(path="m.py", kind="function", name="f")], themes=["round-trip"], ), files=[ diff --git a/tests/test_overview_seed.py b/tests/test_overview_seed.py index 2df12d4..f5b3ba0 100644 --- a/tests/test_overview_seed.py +++ b/tests/test_overview_seed.py @@ -8,6 +8,8 @@ from __future__ import annotations +import hashlib + from semantic_code_review.augment.overview import format_overview_prompt from semantic_code_review.augment.schemas import ( AnnotatedDiff, @@ -16,7 +18,7 @@ ParsedHunk, PRInfo, ) -from semantic_code_review.structural import ChangedSymbol, SymbolDelta, SymbolRange +from semantic_code_review.structural import ChangedSymbol, ChangeReason, SymbolDelta, SymbolRange def _hunk(header: str) -> AnnotatedHunk: @@ -48,13 +50,20 @@ def _make_diff() -> AnnotatedDiff: _META = {"title": "T", "body": ""} -def _changed(name: str, qn: str, kind: str = "function") -> ChangedSymbol: +def _changed( + name: str, + qn: str, + kind: str = "function", + reason: ChangeReason | None = None, +) -> ChangedSymbol: return ChangedSymbol( path="a.py", kind=kind, name=name, qualified_name=qn, range=SymbolRange(start_line=1, end_line=2, start_col=0, end_col=0), + reason=reason, + body_sha=hashlib.sha256(qn.encode()).hexdigest(), ) @@ -74,19 +83,29 @@ def test_non_empty_delta_appends_seed_section() -> None: diff = _make_diff() delta = SymbolDelta( added=[_changed("bar", "bar")], - modified=[_changed("baz", "Foo.baz", kind="method")], + modified=[_changed("baz", "Foo.baz", kind="method", reason=ChangeReason.SIGNATURE)], removed=[_changed("gone", "gone")], ) out = format_overview_prompt(diff, _META, delta) assert "# Symbols changed (deterministic" in out - # Each entry rendered as `kind qualified_name (path)`. + # Each entry rendered as `kind qualified_name[ [reason]] (path)`. assert " function bar (a.py)" in out - assert " method Foo.baz (a.py)" in out + assert " method Foo.baz [signature] (a.py)" in out assert " function gone (a.py)" in out # The seed extends — never replaces — the existing prompt body. assert "# Hunk headers" in out +def test_moved_only_symbols_are_left_out_of_the_seed() -> None: + """A definition whose text is byte-identical says nothing about the + change, and it is the bulk of the delta — 244 of 262 entries on a + measured 6-file diff.""" + diff = _make_diff() + baseline = format_overview_prompt(diff, _META) + delta = SymbolDelta(moved=[_changed("shifted", "shifted")]) + assert format_overview_prompt(diff, _META, delta) == baseline + + def test_seed_section_is_strict_suffix_of_unseeded() -> None: """The seed only appends; the pre-seed prefix is unchanged.""" diff = _make_diff() diff --git a/tests/test_repo_tool_fns.py b/tests/test_repo_tool_fns.py index 93ed2f8..f7bca9d 100644 --- a/tests/test_repo_tool_fns.py +++ b/tests/test_repo_tool_fns.py @@ -236,7 +236,7 @@ def test_changed_symbols_skips_untouched_file(diff_repo: RepoTools) -> None: def test_changed_symbols_empty_when_base_equals_head(repo: RepoTools) -> None: delta = json.loads(mcp_dispatch(repo, "changed_symbols", {})) - assert delta == {"added": [], "removed": [], "modified": []} + assert delta == {"added": [], "removed": [], "modified": [], "moved": []} def test_changed_symbols_takes_no_query_args() -> None: diff --git a/tests/test_schema_unification.py b/tests/test_schema_unification.py index c7626b3..65fe0cb 100644 --- a/tests/test_schema_unification.py +++ b/tests/test_schema_unification.py @@ -18,22 +18,15 @@ def test_overview_submission_dump_has_keys_apply_overview_reads() -> None: - """`apply_overview_to_diff` reads keys: summary, symbols_added, - symbols_modified, symbols_removed, callgraph_edges, themes, files, - groups. The model's dump must produce those same keys.""" + """`apply_overview_to_diff` reads keys: summary, callgraph_edges, + themes, files, groups. The model's dump must produce those same keys + — and must not carry the retired symbol inventories, which the + deterministic `SymbolDelta` owns (ADR 0001).""" sub = OverviewSubmission(summary="hi", files=[]) dump = sub.model_dump(by_alias=True) - expected = { - "summary", - "symbols_added", - "symbols_modified", - "symbols_removed", - "callgraph_edges", - "themes", - "files", - "groups", - } + expected = {"summary", "callgraph_edges", "themes", "files", "groups"} assert expected <= dump.keys() + assert not {"symbols_added", "symbols_modified", "symbols_removed"} & dump.keys() def test_hunk_annotations_dump_has_keys_apply_hunk_reads() -> None: diff --git a/tests/test_structural.py b/tests/test_structural.py index d18b314..7105c79 100644 --- a/tests/test_structural.py +++ b/tests/test_structural.py @@ -5,7 +5,9 @@ import json from semantic_code_review.structural import ( + ChangeReason, Symbol, + SymbolDelta, diff_file, enclosing_symbol, language_for_path, @@ -193,47 +195,123 @@ def m(self): """ +def _delta(base: str = _BASE, head: str = _HEAD, path: str = "m.py") -> SymbolDelta: + return diff_file( + path, + outline_symbols(base, "python"), + outline_symbols(head, "python"), + base_src=base, + head_src=head, + ) + + def test_diff_added_removed_by_qualified_name() -> None: - delta = diff_file("m.py", outline_symbols(_BASE, "python"), outline_symbols(_HEAD, "python")) + delta = _delta() assert [c.qualified_name for c in delta.added] == ["added"] assert [c.qualified_name for c in delta.removed] == ["gone"] -def test_diff_modified_is_differing_range() -> None: - delta = diff_file("m.py", outline_symbols(_BASE, "python"), outline_symbols(_HEAD, "python")) - # C.m gained a comment line → its range differs → modified. C's range - # also shifts. `keep` and `X` are byte-identical on both sides. - qns = {c.qualified_name for c in delta.modified} - assert "C.m" in qns - assert "keep" not in qns and "X" not in qns +def test_diff_modified_is_a_real_code_change() -> None: + delta = _delta() + # C.m gained a comment line → its text differs → modified. C's text + # differs too (its child grew). `keep` and `X` are byte-identical. + assert {c.qualified_name for c in delta.modified} == {"C", "C.m"} def test_diff_carries_path_and_live_side_range() -> None: - delta = diff_file("m.py", outline_symbols(_BASE, "python"), outline_symbols(_HEAD, "python")) - added = delta.added[0] + added = _delta().added[0] assert added.path == "m.py" assert added.kind == "function" and added.signature == "def added()" +def test_modified_reason_is_signature_when_the_declaration_moves() -> None: + base = "def f(a):\n return a\n" + head = "def f(a, *, b=1):\n return a\n" + assert [(c.qualified_name, c.reason) for c in _delta(base, head).modified] == [("f", ChangeReason.SIGNATURE)] + + +def test_modified_reason_is_body_when_only_the_implementation_moves() -> None: + base = "def f(a):\n return a\n" + head = "def f(a):\n return a\n # trailing\n" + assert [(c.qualified_name, c.reason) for c in _delta(base, head).modified] == [("f", ChangeReason.BODY)] + + +def test_a_pure_line_shift_is_moved_not_modified() -> None: + base = "def f():\n return 1\n" + head = "X = 0\n\n\ndef f():\n return 1\n" + delta = _delta(base, head) + assert not delta.modified + assert [(c.qualified_name, c.from_path) for c in delta.moved] == [("f", None)] + + +def test_a_line_count_neutral_body_edit_under_a_shift_is_body_not_moved() -> None: + """Span *length* cannot tell these apart — the text can. + + `f` shifts down two lines and edits one line in place, so + `end - start` is identical on both sides. Measured on a real diff, a + length comparison misfiled one of six genuine API changes this way. + """ + base = "def f():\n return 1\n" + head = "X = 0\n\ndef f():\n return 2\n" + delta = _delta(base, head) + assert not delta.moved + assert [(c.qualified_name, c.reason) for c in delta.modified] == [("f", ChangeReason.BODY)] + + def test_diff_added_file_is_all_added() -> None: - delta = diff_file("new.py", [], outline_symbols(_HEAD, "python")) - assert not delta.removed and not delta.modified + delta = diff_file("new.py", [], outline_symbols(_HEAD, "python"), base_src=None, head_src=_HEAD) + assert not delta.removed and not delta.modified and not delta.moved assert {c.qualified_name for c in delta.added} >= {"X", "keep", "added", "C", "C.m"} def test_diff_deleted_file_is_all_removed() -> None: - delta = diff_file("old.py", outline_symbols(_BASE, "python"), []) - assert not delta.added and not delta.modified + delta = diff_file("old.py", outline_symbols(_BASE, "python"), [], base_src=_BASE, head_src=None) + assert not delta.added and not delta.modified and not delta.moved assert "gone" in {c.qualified_name for c in delta.removed} def test_merge_concatenates_per_file_deltas() -> None: - d1 = diff_file("a.py", [], outline_symbols("def a():\n pass\n", "python")) - d2 = diff_file("b.py", [], outline_symbols("def b():\n pass\n", "python")) + d1 = _delta("", "def a():\n pass\n", path="a.py") + d2 = _delta("", "def b():\n pass\n", path="b.py") merged = merge([d1, d2]) assert {(c.path, c.qualified_name) for c in merged.added} == {("a.py", "a"), ("b.py", "b")} +def test_body_sha_stays_out_of_the_wire_format() -> None: + """It is an internal comparison key, not part of the `Symbol` currency.""" + dumped = json.loads(_delta().model_dump_json()) + assert all("body_sha" not in c for bucket in dumped.values() for c in bucket) + + +# --- cross-file moves (resolved diff-wide, in `merge`) --------------------- + +_FN = "def helper(x: int) -> int:\n return x + 1\n" + + +def test_merge_collapses_a_cross_file_move() -> None: + merged = merge([_delta(_FN, "", path="old.py"), _delta("", _FN, path="new.py")]) + assert not merged.added and not merged.removed + assert [(c.path, c.from_path, c.qualified_name) for c in merged.moved] == [("new.py", "old.py", "helper")] + + +def test_merge_leaves_a_moved_and_edited_symbol_as_two_events() -> None: + """Only byte-identical code is a move by construction. Pairing an + edited definition with a same-named removal would be an inference, + which this layer does not make (ADR 0001). + """ + merged = merge([_delta(_FN, "", path="old.py"), _delta("", _FN.replace("x + 1", "x + 2"), path="new.py")]) + assert not merged.moved + assert [c.path for c in merged.added] == ["new.py"] + assert [c.path for c in merged.removed] == ["old.py"] + + +def test_merge_does_not_link_same_named_symbols_with_different_bodies() -> None: + gone = _delta("def helper():\n return 1\n", "", path="a.py") + arrived = _delta("", "def helper():\n return 2\n", path="b.py") + merged = merge([gone, arrived]) + assert not merged.moved and len(merged.added) == 1 and len(merged.removed) == 1 + + # --- TypeScript / TSX / JavaScript (Slice 6) ------------------------------- _TS_SAMPLE = """interface Foo { @@ -315,10 +393,14 @@ def test_js_outline_has_no_signature() -> None: assert box_method.qualified_name == "Box.open" and box_method.signature is None +_TS_BASE = "function keep(): void {}\nfunction gone(): void {}\n" +_TS_HEAD = "function keep(): void {}\nfunction added(): void {}\n" + + def test_ts_changed_symbols_diff() -> None: - base = outline_symbols("function keep(): void {}\nfunction gone(): void {}\n", "typescript") - head = outline_symbols("function keep(): void {}\nfunction added(): void {}\n", "typescript") - delta = diff_file("m.ts", base, head) + base = outline_symbols(_TS_BASE, "typescript") + head = outline_symbols(_TS_HEAD, "typescript") + delta = diff_file("m.ts", base, head, base_src=_TS_BASE, head_src=_TS_HEAD) assert [c.qualified_name for c in delta.added] == ["added"] assert [c.qualified_name for c in delta.removed] == ["gone"] diff --git a/tests/test_viewer_json.py b/tests/test_viewer_json.py index 3b5b96b..16baf0a 100644 --- a/tests/test_viewer_json.py +++ b/tests/test_viewer_json.py @@ -281,7 +281,7 @@ def test_symbol_blocks_nest_methods_under_their_class(run_dir: paths.RunDir) -> foo = syms[0] assert foo["id"] == "SY0" assert foo["title"] == "Foo" - assert "modified" in foo["rationale"] + assert foo["rationale"] == "class body changed in a.py" assert foo["hunk_ids"] == ["H0_0"] # subtree union # baz nests under Foo as the only child. children = foo["children"] @@ -294,6 +294,51 @@ def test_symbol_blocks_nest_methods_under_their_class(run_dir: paths.RunDir) -> assert "children" not in baz # leaf carries no children key +_MOVED_DIFF = """diff --git a/a.py b/a.py +index 0123456..89abcde 100644 +--- a/a.py ++++ b/a.py +@@ -1,3 +1,5 @@ ++HEADER = 1 ++ + def keep(): + return 1 + def other(): +""" + + +def test_a_moved_only_symbol_is_context_not_a_pill(run_dir: paths.RunDir) -> None: + """Byte-identical code that shifted lines is not a change, so it earns + no pill of its own — the same treatment an unchanged ancestor gets. + Without this the axis is mostly noise: on a measured 6-file diff, 244 + of 262 same-name-both-sides symbols had only shifted.""" + run_dir.raw_diff.write_text(_MOVED_DIFF, encoding="utf-8") + run_dir.meta.write_text( + json.dumps( + { + "title": "Add a header constant", + "author": {"login": "t"}, + "url": "", + "baseRefOid": "aaa", + "headRefOid": "bbb", + } + ), + encoding="utf-8", + ) + base = run_dir.base + head = run_dir.head + base.mkdir() + head.mkdir() + body = "def keep():\n return 1\ndef other():\n return 2\n" + (base / "a.py").write_text(body, encoding="utf-8") + (head / "a.py").write_text("HEADER = 1\n\n" + body, encoding="utf-8") + + data = build_pending_viewer_json(run_dir) + + # `keep` and `other` both shifted two lines down; only HEADER is new. + assert [b["title"] for b in data["symbols"]] == ["HEADER"] + + def test_symbol_blocks_absent_without_worktrees(run_dir: paths.RunDir) -> None: """No base/head worktree available ⇒ empty Symbols axis, no raise.""" run_dir.raw_diff.write_text(_SYMBOL_DIFF, encoding="utf-8")