diff --git a/CLAUDE.md b/CLAUDE.md index b2376fe..9a350c5 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -13,6 +13,10 @@ Repo `codegen-compare-tool`, local folder `code-review`, package `compare_tool`. Where this file and `~/.claude/CLAUDE.md` disagree, this one wins. +`docs/architecture.md` explains how the pieces fit and why — layering, the two +diff passes, the shared seams, the result-dict contract. Read it before a change +that crosses module boundaries; this file is the rules, that one is the map. + ## 1. Fail-safe is not negotiable **If it cannot be *proven* to be noise, it is a real change.** A rule that is @@ -74,7 +78,7 @@ any dev script take whatever they need; no need to ask first. The exception is **what ships in `compare_tool.pyz`**: the scan, the rules, the diff, the report, the review store and `gitsource` import stdlib only. That -zipapp is 78 KB, needs nothing installed, and is the documented fallback for +zipapp is ~110 KB, needs nothing installed, and is the documented fallback for the machines where antivirus blocks the `.exe` — one third-party import in `scanner.py` and it stops running there, which is a shipped promise broken by an import nobody reviewed. @@ -102,8 +106,12 @@ Two more promises the same machines depend on: Tests pass on layouts that look broken. Every UI change gets looked at: -```bash -QT_QPA_PLATFORM=offscreen python smoke.py # widget.grab().save(png), then read the png +Write a throwaway script under `%TEMP%` that builds the widget, grabs it and +saves a png — then read the png: + +```powershell +$env:QT_QPA_PLATFORM = 'offscreen' +python $env:TEMP\smoke.py # widget.grab().save(png) ``` and, for anything about colour or legibility, a real window on the desktop. @@ -180,6 +188,7 @@ decision; the frozen entry point asks it rather than re-deriving it. ```bash python -m compare_tool --report out.html python -m unittest discover -s tests # unittest, NOT pytest -.\build.ps1 # dist/compare_tool.pyz -.\build.ps1 -Exe # plus the .exe (needs pyinstaller) +.\build.ps1 # dist\compare-tool.exe (needs pyinstaller + PySide6) +.\build.ps1 -Pyz # plus dist\compare_tool.pyz +.\build.ps1 -PyzOnly # zipapp only, no build dependencies ``` diff --git a/README.md b/README.md index 16abc00..e080005 100644 --- a/README.md +++ b/README.md @@ -86,7 +86,7 @@ scan did find is still printed. | `--exclude PATTERN` | Skip files matching a glob (relative path or bare file name), repeatable. Example: `--exclude compare_report.html` | | `--exit-zero` | Always exit 0 even when real changes exist (report-only mode for pipelines). Compare errors still exit 2 | | `--arxml-only` | Scan only `.arxml`/`.xml`/`.a2l` and write a compact per-type report (default `arxml_update.html`) — always written, even when nothing changed | -| `--review FILE` | Render notes and sign-offs from a review file (`codegen-review.json`, written by the viewer) next to the changes they belong to, with a `Reviewed` badge. Must be named explicitly; no effect with `--arxml-only` | +| `--review FILE` | Render notes and sign-offs from a review file (`codegen-review.json`, written by the viewer) next to the changes they belong to, plus a `Reviewed` badge that hides the changes already signed off. Must be named explicitly — a report must not pick up someone else's sign-off by accident; no effect with `--arxml-only` | | `--qt`, `--viewer` | Open the side-by-side viewer on folders named on the command line, instead of comparing them in the terminal. Needs the `viewer` extra (see below) | Omitting `old_dir`/`new_dir` opens the viewer. `--gui` (the tkinter panel) was removed in 1.1.0. @@ -114,6 +114,8 @@ Reading a scan: `Review mode` adds the note box and a `Review` column in the tree — green when every change in a row is signed off, amber part way, grey when none is. Sign off one change (`Ctrl+R`) or a whole file (`Ctrl+Shift+R`); the notes travel into the exported report. +`Export report…` (`Ctrl+E`) writes the same self-contained HTML report the CLI writes, with the review notes folded in. It is always built from the **complete scan**, never from what is on screen — a category you collapsed in the tree still appears in the file with its real verdict. + Full walkthrough is built into the app — `Help` → `User guide` (`F1`), which works offline. Standalone `.exe` (no Python needed): see [Single-file build](#single-file-build). ## What counts as noise @@ -166,7 +168,6 @@ A file whose XML fails to parse is skipped from this summary (its text diff stil Files are grouped by **Simulink model** using the Embedded Coder AUTOSAR naming convention (`X.c`, `X.h`, `X.arxml`, `Rte_X.h`, the modular ARXML set, …). Files that match no model land in a final **Shared / other** group. - ## HTML report Self-contained file, one per compare: badge toggles, folder tree, filter box, collapsible diffs per file. Opens `Unimportant` hidden and `Modified` expanded, so it opens on what matters. @@ -189,7 +190,7 @@ See [azure-pipelines.yml](azure-pipelines.yml) for a working example (OLD checke ```powershell .\build.ps1 # dist\compare-tool.exe - one file, nothing to install on the target -.\build.ps1 -Pyz # also dist\compare_tool.pyz (~80 KB) for machines that have Python 3.8+ +.\build.ps1 -Pyz # also dist\compare_tool.pyz (~110 KB) for machines that have Python 3.8+ .\build.ps1 -PyzOnly # zipapp only (building it needs no PyInstaller / PySide6) ``` @@ -203,7 +204,7 @@ See [azure-pipelines.yml](azure-pipelines.yml) for a working example (OLD checke Built as a **console** application so terminal runs keep their exit code (`1` = real changes, `2` = compare incomplete) for the CI gate. The viewer hides the console window at runtime — you'll see a brief flash on double-click. A crash un-hides the console so the error is visible. -- **`.pyz` (zipapp, stdlib)**: `python compare_tool.pyz [flags]`. Prefer it when Python is available — ~80 KB, no build dependencies, not flagged by antivirus. The CLI works anywhere; the viewer additionally needs PySide6 on that machine (without it, the tool says so instead of opening). +- **`.pyz` (zipapp, stdlib)**: `python compare_tool.pyz [flags]`. Prefer it when Python is available — ~110 KB, no build dependencies, not flagged by antivirus. The CLI works anywhere; the viewer additionally needs PySide6 on that machine (without it, the tool says so instead of opening). - **`.exe` (PyInstaller onefile, ~47 MB)**: no Python needed on the target. Building needs `pyinstaller` and `PySide6` on the dev machine (`build.ps1` installs them), and the binary only runs on the OS it was built on. PyInstaller executables are sometimes blocked by antivirus or AppLocker — fall back to the `.pyz` there. Every CLI flag behaves identically in the packaged builds. `build/` and `dist/` are already in `.gitignore`. @@ -226,10 +227,16 @@ compare_tool/ ├── c_rules.py # C/H rules: strip comments, tokenize, detect renames, extract RTE access points ├── arxml_rules.py # ARXML rules: UUID, ADMIN-DATA, DATE, comments + extract port interfaces, SWCs (ports/runnables/events) ├── a2l_rules.py # A2L rules: strip C-style comments + extract CHARACTERISTIC/MEASUREMENT +├── view_model.py # renderer-agnostic view model (paint mode, intra-line span, row alignment) shared by the report and the viewer +├── syntax.py # line-at-a-time C / XML token spans, Qt-free so it ships in the .pyz +├── review.py # reviewer notes and sign-offs, keyed by change content so they survive a rescan +├── gitsource.py # read-only `git archive` of a commit into a temp folder, so a commit can be the OLD side └── report.py # self-contained HTML report (badge toggles, model overview, grouping, filter, collapsible diffs) ``` -To add a rule: write the strip function in `c_rules.py` / `arxml_rules.py`, then register it in the shadow builder and `_build_variants` in `diff_engine.py`. +[docs/architecture.md](docs/architecture.md) covers how these fit together and why: the two diff passes, where a verdict is decided, the shared seams and the result-dict contract. Anything both renderers need lives in `view_model.py` — reimplementing a mapping inline lets the HTML report and the viewer drift apart about what changed. + +To add a rule: write the strip function in `c_rules.py` / `arxml_rules.py` / `a2l_rules.py`, join it into that ruleset's shadow, register one labelled variant in `_build_variants` in `diff_engine.py`, and add both tests — the pattern alone is noise, and the same pattern *beside* a real change still reports the real change. Issues and pull requests are welcome. Please keep the **compare core stdlib-only** — it has to run on locked-down build servers, so PySide6 stays confined to `compare_tool/qtviewer/` and is imported only when the viewer opens — and add a test under `tests/` for any new rule. diff --git a/build.ps1 b/build.ps1 index 4e34897..1101198 100644 --- a/build.ps1 +++ b/build.ps1 @@ -10,7 +10,7 @@ # and the exit code; the viewer hides the console # window at runtime (see packaging\entry.py). # -# dist\compare_tool.pyz optional tiny zipapp (~80 KB) for machines that +# dist\compare_tool.pyz optional tiny zipapp (~110 KB) for machines that # already have Python 3.8+. Stdlib only, so the CLI # works anywhere; the viewer additionally needs # PySide6 installed on that machine. diff --git a/docs/architecture.md b/docs/architecture.md new file mode 100644 index 0000000..64d72e6 --- /dev/null +++ b/docs/architecture.md @@ -0,0 +1,274 @@ +# Architecture + +How the compare tool is put together, and why. For what it *does* — flags, +noise rules, screenshots — read the [README](../README.md) first; this document +is for someone about to change the code. + +## The claim the design has to protect + +The product is one sentence: **"you can ignore what I hid."** + +Regenerating a Simulink model rewrites timestamps, UUIDs, comment banners and +auto-generated identifiers even when the behaviour is identical. The tool +classifies every difference so a reviewer is not drowned in that churn. The +moment it hides one real change, a reviewer stops trusting the filter and the +tool is worse than `diff`. + +Every structural decision below follows from that: **anything not *proven* to +be noise is a real change**, and anything that could not be compared at all is +louder still. + +## Layers + +```mermaid +flowchart TD + subgraph front[Front ends] + CLI[main.py
CLI + exit code] + QT[qtviewer/
PySide6 viewer] + end + subgraph core[Compare core — stdlib only] + SC[scanner.py
walk + pair + fold] + DE[diff_engine.py
two-pass diff + verdict] + RULES[c_rules · arxml_rules · a2l_rules
strip, tokenize, extract] + end + subgraph shared[Shared seams] + VM[view_model.py
mode_of · char_span · aligned_rows] + RV[review.py
notes keyed by content] + SY[syntax.py
token spans, Qt-free] + end + RP[report.py
self-contained HTML] + GS[gitsource.py
commit → temp folder] + + CLI --> SC + QT --> SC + GS --> QT + SC --> DE + DE --> RULES + CLI --> RP + QT --> RP + RP --> VM + QT --> VM + QT --> SY + RP --> RV + QT --> RV +``` + +Two rules hold this shape: + +**The core imports nothing but the standard library.** `scanner`, `diff_engine`, +the three rule modules, `report`, `review`, `view_model`, `syntax` and +`gitsource` are what ships in `compare_tool.pyz` — ~110 KB, no install, the +documented fallback for machines where antivirus blocks the `.exe`. One +third-party import in `scanner.py` and the zipapp stops running there. PySide6 +lives only under `compare_tool/qtviewer/` and is imported lazily, when the +viewer opens, so the test suite runs headless. + +**Arrows only point down.** The core never imports a front end. `syntax.py` +says *what* a stretch of text is and never what colour it gets, so the Qt layer +and any second surface can reuse it without the mapping being written twice. + +## Data flow of one compare + +```mermaid +sequenceDiagram + participant F as Front end + participant S as scanner.scan + participant D as diff_engine.compare_pair + participant R as Renderer + + F->>S: old_root, new_root, exclude/include + S->>S: list_files() both sides, capture listing errors + loop each relative path + alt in both trees + S->>D: old_text, new_text, rel + D->>D: pass 1 — raw line diff + D->>D: pass 2 — shadow diff (+ rename map) + D->>D: label each raw hunk, then _status_of + D-->>S: {status, hunks, renames, notes} + else one side only + S->>S: added / deleted + semantic extras + else unreadable + S->>S: status 'error' — loud, never silent + end + end + S-->>F: {rel_path: result} + F->>R: the SAME dict, unfiltered +``` + +### The two passes + +`compare_pair` diffs the files twice. + +- **Pass 2 decides the truth.** Each side is reduced to a *shadow*: comments + stripped, whitespace collapsed, UUIDs and dates and version stamps removed, + and for C a verified 1-to-1 rename map applied. Whatever still differs + between the two shadows is a real change. The rename map is best-effort and + then *checked* — it is applied to the old shadow and re-diffed, and any line + it does not fully explain stays real. +- **Pass 1 decides what you see.** The raw line diff keeps every textual + difference, so the viewer can show the churn instead of pretending the files + were identical. A raw hunk that intersects no real hunk is ignorable, and is + *labelled* by `_build_variants`: a list of shadows each with exactly **one** + rule applied. The first variant under which the hunk's two slices are equal + names it (`comment`, `uuid`, `timestamp`, `sw-version`, `rename`, + `whitespace`). A hunk that no single rule explains is `mixed` — still + ignorable, but honest that more than one rule combined to explain it. + +This is why the noise-rule checklist is what it is: a new rule has to be +text-based and preserve line count (or the two passes stop lining up), joined +into the ruleset's shadow, *and* given a labelled variant. Miss the variant and +the rule silently becomes `mixed`. + +### The verdict lives in one function + +`diff_engine._status_of` is the only place a status is decided: + +| Status | Meaning | Foldable | +|---|---|---| +| `identical` | no difference at all | — | +| `comment-only` | every labelled hunk is `comment` | yes | +| `ignorable-only` | noise, but not comments alone | yes | +| `real-change` | at least one hunk survived pass 2 | **no** | +| `added` / `deleted` | present on one side only | **no** | +| `error` | could not be listed, read or compared | **no** | + +`comment-only` is deliberately separate from `ignorable-only`: "the banner +moved" triages differently from "an identifier was renamed". A file mixing +comments *with* other noise stays `ignorable-only` — the narrower claim has to +be exact. + +`scanner.FOLDABLE` names the only two statuses a UI toggle may collapse. Real +changes, one-sided files and errors are absent from that tuple **by +construction**, so no caller mistake can hide one. + +### Folding is a pure function, not a rescan + +`scanner.apply_fold` re-judges an already scanned tree under different rules +with no disk access: the hunks already say what kind each difference is, so +re-reading every file to learn the same thing is pure waste. It copies rather +than mutates, so the rules can be toggled back and forth. The viewer keeps the +untouched scan in `MainWindow._raw_results` and folds into `self.results` for +display. + +## The result dict is the contract + +Everything downstream — CLI summary, HTML report, viewer tree, review store — +consumes one dict per compared path: + +```python +{ + 'status': 'real-change', + 'hunks': [{'kind': 'real', 'old_range': [12, 15], 'new_range': [12, 14]}, + {'kind': 'moved', 'old_range': [40, 60], 'new_range': [40, 40], + 'moved_to': 91}], + 'renames': {'rtb_AND_c4nxjoom3d': 'rtb_AND_j2kqp1wxab'}, + 'notes': ['line-endings'], + 'binary': False, + # semantic extras, only on real changes and one-sided files: + 'ifaces': ..., 'swc': ..., 'rte': ..., 'a2l': ..., +} +``` + +Ranges are 0-based, end-exclusive, into the **raw** lines of each side. +`kind` is one of `real`, `moved`, `comment`, `rename`, `uuid`, `timestamp`, +`sw-version`, `whitespace`, `mixed`. + +The semantic extras are computed only where they can matter: a shadow-equal +file has the same content, so it cannot have moved the AUTOSAR surface. + +## Shared seams + +Any fact two renderers need lives in one module they both import. The failure +mode this prevents is quiet: two copies of a four-line mapping agree perfectly +until someone adds a new kind to one of them. + +- **`view_model.mode_of`** — hunk kind → paint mode (`real`, `moved`, + `comment`, `minor`). The HTML report and the Qt panes cannot disagree about + how a kind is coloured or whether it can be hidden. +- **`view_model.char_span`** — the intra-line highlight as plain character + offsets. The report wraps them in a ``; the viewer applies a + `QTextCharFormat` over the same numbers. +- **`view_model.aligned_rows` / `collapse_rows`** — whole-file two-pane + alignment, and folding a run of noise rows into one `⋯ N uuid lines hidden` + placeholder that reads as context on both sides so the panes stay in scroll + lockstep. The count is always stated: this hides noise, it does not drop it. +- **`review.py`** — notes and sign-offs keyed by a hash of the change's own + text, not by line number, so an unrelated edit elsewhere in the file does not + detach them on the next scan. + +## Front ends + +`main.viewer_requested(argv)` owns the "which front end does this argv want" +decision, and answers it **without running the compare** — the frozen entry +point calls it to hide the console window before Qt starts. The rule: the +terminal compare runs only when both folders are named on the command line; +everything else opens the viewer. + +### CLI + +`run_compare` deletes any leftover report *before* scanning — if this run dies, +a stale file from the previous one must not pass for its result. A report path +that cannot be written raises `ReportWriteError` carrying the scan it could not +write, so the terminal still prints what was found, and the run exits `2`: + +| Code | Meaning | +|---|---| +| 0 | No real change | +| 1 | Real changes found (the CI gate) | +| 2 | Compare INCOMPLETE — a path could not be listed, read or compared, or the report could not be written | + +The exit code is a contract with somebody's pipeline. `--exit-zero` suppresses +`1`, never `2` — an incomplete compare must never look green. + +### Viewer + +The scan walks the disk, so it runs on a `QThread` (`qtviewer/worker.py`) and +results cross back through signals only. Widgets stay dumb: they walk a model +and paint it. `tree.py`, `summary_model.py` and `compare_tool/resources.py` +have no PySide6 import at all, so their tests run on a box with no Qt. + +`Git compare…` is not a second compare mode. `gitsource.py` runs `git archive` +— read-only, touching neither HEAD, the index nor the working tree — to lay one +commit down in a temp folder, and everything downstream sees two directories as +usual. That matters because the folder being reviewed is normally the one the +engineer is still editing. + +## Decisions worth knowing before you change something + +**The record is never the filtered view.** `MainWindow._export_report` builds +from `_raw_results`, not from what is on screen. A category the reviewer +collapsed must still appear in the exported file with its real verdict — +otherwise an export could show a file as Identical when it is not. Same for the +quick-changes rollup. + +**The HTML report is self-contained.** CSS and JS inline, no CDN, nothing +fetched when the file is opened. It gets mailed around and opened on machines +with no internet; a report that renders blank there is worse than no report. + +**Cosmetic failures degrade, the compare does not.** A missing icon leaves a +button with its text label (`resources.py` getters return `None` and callers +cope); no PySide6 gives a plain sentence about the `viewer` extra, not a +traceback; Windows legacy codepages are handled with +`stream.reconfigure(errors='replace')` so a print can never kill a run. The +compare itself is the exception — a scan or render failure is loud, never an +empty, clean-looking result. + +**The `.exe` is a console build on purpose.** A windowed build makes the shell +stop waiting and throws the exit code away, which breaks the CI gate. So the +console *window* is hidden at runtime instead (`packaging/entry.py`), and +un-hidden on a crash. + +**Python 3.8 is a shipped promise.** No `match`, no `X | Y` at runtime; +`list[str]` in an annotation needs `from __future__ import annotations`. CI runs +3.8 and 3.11 on Linux and Windows, so a 3.10-ism passes locally and fails there. + +## Where to change what + +| Change | Touch | +|---|---| +| New noise rule | the rule module's strip function → that ruleset's shadow → one labelled variant in `_build_variants` → two tests (alone it is noise; beside a real change it stays real) | +| New file type | `RULES` in `diff_engine.py`, a `*_rules.py` module, shadow + variants | +| New semantic extraction | `*_rules.py` extractor, wire into `scanner.compare_file` and `_single_info`, then a `summarize_*` rollup | +| Anything both renderers show | `view_model.py` — never inline in one of them | +| New verdict | `diff_engine._status_of`, and decide explicitly whether it belongs in `scanner.FOLDABLE` (default: no) | +| Viewer layout or colour | render it and look at it (`widget.grab().save(png)` under `QT_QPA_PLATFORM=offscreen`), then a real window |