From 4376f656637508b6dca66cca64b08660a817278c Mon Sep 17 00:00:00 2001 From: Lars Pohlmann Date: Tue, 11 Aug 2026 14:32:09 +0200 Subject: [PATCH 01/14] docs(#7): expand Phase 4 into a step-by-step plan Diff-aware mode against the shipped Phase 3 interfaces, plus two items pulled forward at the maintainer's request: a composer tramp script backed by the repo's own phptramp.dist.json, and the CI dogfood step becoming a real gate. Thresholds are measured, not guessed: the current tree has one 4-hop finding (CallResolver's $caller chain, to be fixed by refactoring, not suppressed) and six 2-hop chains, so limit 3 / warn-limit 2 bites without noise. The --no-config escape hatch lands first because the fixture harness's folder mode runs from the repo root and must not inherit the repo's own config. --- docs/plan.md | 12 +- docs/plans/phase-4.md | 387 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 397 insertions(+), 2 deletions(-) create mode 100644 docs/plans/phase-4.md diff --git a/docs/plan.md b/docs/plan.md index adaf828..6a42482 100644 --- a/docs/plan.md +++ b/docs/plan.md @@ -712,11 +712,19 @@ not yet consumed by analysis. Not a Phase 3 deliverable; tracked in their own ph --- -## Phase 4 — Diff-aware mode (flagship) +## Phase 4 — Diff-aware mode (flagship) (in progress — issue #7) **Deliverable:** `phptramp --changed-only --git-base origin/main` reports only chains intersecting the diff and marks which hops are the user's. +**Detailed step-by-step plan: [docs/plans/phase-4.md](plans/phase-4.md)** — written +against the shipped Phase 1–3 interfaces; where it refines the sketches below +(diff via `--diff ` instead of a positional `-`, a `--no-config` escape +hatch, marking via immutable Finding rebuild), the detailed plan wins. Two items +are pulled forward into this phase at the maintainer's request: the repo's own +`composer tramp` script + `phptramp.dist.json`, and the CI dogfood step becoming a +real gate (limit 3, warn-limit 2 — thresholds measured on the actual tree). + - **4.1 DiffParser + ChangedLines:** parse unified diff from stdin (`--changed-only -`) or by executing `git diff --unified=0 ...HEAD` (note: three-dot, merge-base semantics, matching CI expectations); produce `file -> set` of *added/modified* @@ -756,7 +764,7 @@ intersecting the diff and marks which hops are the user's. worker count from `nproc`, flag `--jobs`. - **6.3 Docs:** PhpStorm External Tool + File Watcher recipe (`docs/phpstorm.md`), finalize `docs/ci.md`, README rewrite against the real tool output. -- **6.4 Release:** dogfood job gating (`--limit 3`, real), Packagist submission, tag +- **6.4 Release:** (dogfood gating shipped early, in Phase 4) Packagist submission, tag `v0.1.0`. Roadmap notes (explicit maybe-laters): `--follow-all-implementations`, field/property tramping, native PhpStorm plugin, composer-plugin command package. diff --git a/docs/plans/phase-4.md b/docs/plans/phase-4.md new file mode 100644 index 0000000..1b46426 --- /dev/null +++ b/docs/plans/phase-4.md @@ -0,0 +1,387 @@ +# Phase 4 Implementation Plan — Diff-aware mode + self-gating dogfood + +> **For agentic workers:** Self-contained; assumes no conversation context. Read +> `CLAUDE.md` (toolchain, Clean Code house style, git-flow workflow) and the +> "Frozen core semantics" + Phase 4 sections of `docs/plan.md` first — this plan +> expands them into steps and repeats what it relies on. Work task-by-task with +> checkboxes; strict TDD; one task per commit. + +**Goal:** the flagship feature — `phptramp --changed-only --git-base origin/main` +reports only chains touching the diff and marks *which hops are yours* — plus two +items pulled forward at the maintainer's request: a `composer tramp` script backed +by the repo's own `phptramp.dist.json`, and the CI dogfood step becoming a real +gate at limit 3 / warn-limit 2. + +**Architecture:** A new `src/Diff/` layer, fully decoupled from the chain walk: +`DiffParser` turns a unified diff into `ChangedLines`; `GitDiffRunner` produces +that diff from git when none is piped in; `ChangedChainFilter` post-processes the +finished `Finding` list — dropping chains that don't intersect the diff and +rebuilding the kept ones with `Hop->changed` set (immutably; the chain layer never +learns about diffs). Reporters render the mark. Self-gating rides on a new +`--no-config` escape hatch so the repo's own config cannot leak into tests. + +**Tech stack:** unchanged — PHP ≥ 8.2, nikic/php-parser ^5 (only prod dependency). +`GitDiffRunner` shells out to `git` via `proc_open` — that is an invocation, not a +dependency; it only runs when `--changed-only` needs a diff nobody piped in. + +## Global constraints + +- Everything in `CLAUDE.md` applies: `composer check` green before every commit, + PHPMD-clean for every touched `src/` file, Clean Code house style, strict TDD, + one task per commit. +- **GitHub issue for this phase: #7.** Branch `feature/7-diff-aware-mode` off + `develop` (already created, carries this plan). Commits `type(#7): …`. PR into + `develop` with `Closes #7`; diff-scoped Infection gate (minMsi 80) applies. +- Frozen semantics rule 9 is the contract: any-hop intersection at changed-line + granularity — a hop matches if its *declaration line* or its *forwarding + call-site line* falls in the diff; a chain is reported iff ≥ 1 hop matches; + terminal nodes are not hops and never qualify a chain. +- Diff-aware mode is a **reporting filter**: the whole configured project is + still indexed and chains still stitch across unchanged files. Never narrow the + analysis scope to the diff. +- Exit-code contract unchanged (`0`/`1`/`2`); thresholds/severity apply *after* + the changed-only filter. + +## Existing interfaces this phase consumes (Phase 1–3, verbatim) + +- `Finding{ string $param, string $origin, ?string $terminal, TerminalKind $terminalKind, int $hops, list $chain, int $classes, list $notes, list $trace }`. +- `Hop{ string $fqmn, ?string $class, string $file, int $line, ?int $forwardLine }` + — `file` realpath-absolute; `forwardLine` null on the terminal node; the chain + includes the terminal node only for kinds `used|stored|&-terminated|unused-end`. +- `Options` — `changedOnly` and `gitBase` ('origin/main' default) already parse; + both currently unused downstream. README documents them as "reserved". +- `Thresholds::severityOf(Finding): ?Severity`; `Reporter` interface; + `ReporterFactory::create(Options): Reporter`; reporters receive **unfiltered** + findings and apply thresholds themselves. +- Reporter output contracts pinned in `tests/Report/*Test.php` (exact-string) and + `tests/fixtures/*/expected-findings.json` (json, end-to-end). +- Fixture harness (`tests/FixtureTest.php`): folder mode runs from the **repo + root** with `--folder /src --format json` + optional + `phptramp-args.json`; config mode chdirs into the fixture dir when a + `phptramp.json` is present. **Folder mode's repo-root cwd is why Task 6 needs + `--no-config` before the repo gets its own config file.** +- `Application::__construct($stdout = null, $stderr = null)` — streams injected + for tests; `analyze()` builds index → findings → thresholds → reporter. +- Self-run baseline (measured on the Phase 3 tree, `bin/phptramp --folder src`): + 70 one-hop chains, 6 two-hop, **one 4-hop finding** — + `$caller` through `CallResolver::resolve → resolveFunction → functionTarget → + functionCandidates → namespaceOf [used]`. Task 6 resolves it. + +## File structure (this phase) + +``` +src/Diff/DiffException.php NEW (Task 1) +src/Diff/ChangedLines.php NEW file -> changed-line set, intersection (Task 1) +src/Diff/DiffParser.php NEW unified diff text -> ChangedLines (Task 1) +src/Diff/GitDiffRunner.php NEW git diff --unified=0 base...HEAD (Task 2) +src/Chain/Hop.php MOD + readonly bool $changed = false (Task 3) +src/Diff/ChangedChainFilter.php NEW drop non-intersecting, mark kept hops (Task 3) +src/Console/ArgvParser.php MOD --diff (implies --changed-only) (Task 4) +src/Console/Options.php MOD + readonly ?string $diff = null (Task 4) +src/Console/Application.php MOD diff acquisition + filter wiring; $stdin (Task 4) +src/Report/TextReporter.php MOD *YOURS* mark (Task 5) +src/Report/JsonReporter.php MOD "changed" key in changed-only runs (Task 5) +src/Report/GithubReporter.php MOD anchor annotation at first changed hop (Task 5) +src/Console/ArgvParser.php MOD --no-config (Task 6) +phptramp.dist.json NEW the repo's own config (Task 6) +composer.json MOD "tramp" script (Task 6) +src/Resolve/CallResolver.php MOD resolve the 4-hop $caller chain (Task 6) +.github/workflows/ci.yml MOD dogfood becomes composer tramp (real gate)(Task 6) +docs/ci.md NEW CI cookbook (Task 7) +tests/… mirrors all of the above +tests/fixtures/changed-* NEW diff-aware end-to-end cases (Task 7) +``` + +--- + +### Task 1: `DiffParser` + `ChangedLines` + +**Files:** create `src/Diff/{DiffException,ChangedLines,DiffParser}.php`; +tests `tests/Diff/DiffParserTest.php`, `tests/Diff/ChangedLinesTest.php`. + +**Produces:** + +```php +final class DiffException extends \RuntimeException +{ +} + +final class ChangedLines +{ + /** @param array> $linesByFile paths exactly as the diff names them */ + public function __construct(private readonly array $linesByFile) {} + + public function isEmpty(): bool; + + public function containsLine(string $file, int $line): bool; + + /** + * Re-key every path via realpath($baseDirectory . '/' . $path); paths that + * do not resolve (deleted files) are dropped. Hop->file is realpath-absolute, + * so intersection must happen against a resolved instance. + */ + public function resolveAgainst(string $baseDirectory): self; +} + +final class DiffParser +{ + /** @throws DiffException when the text is not a unified diff */ + public function parse(string $unifiedDiff): ChangedLines; +} +``` + +Parsing rules (pin each as a test): + +- Only the **new-file side** matters: file from `+++ b/` (strip `a/`/`b/` + prefixes; tolerate none), lines from hunk headers `@@ -o,n +s,c @@` → lines + `s … s+c-1`. Omitted count (`+s`) means `c = 1`; `c = 0` (pure deletion hunk) + contributes nothing. +- `+++ /dev/null` (file deletion) → file skipped entirely. +- Rename/copy/mode/index header lines and `\ No newline at end of file` are + tolerated noise. CRLF input is accepted (`\r` stripped before matching). +- Text containing no `+++`/`@@` structure at all → `DiffException` — except the + empty string, which parses to an empty `ChangedLines` (an empty diff is a + legitimate "nothing changed" answer from git, not garbage). + +- [ ] **Step 1:** failing tests from literal diff strings covering every rule + above, plus `ChangedLinesTest` for `containsLine` and `resolveAgainst` + (temp files; a diff path that doesn't exist disappears). +- [ ] **Step 2:** red. **Step 3:** implement. **Step 4:** green + `composer check`. +- [ ] **Step 5:** commit `feat(#7): unified diff parser and changed-line set`. + +### Task 2: `GitDiffRunner` + +**Files:** create `src/Diff/GitDiffRunner.php`; +test `tests/Diff/GitDiffRunnerTest.php`. + +**Produces:** + +```php +final class GitDiffRunner +{ + /** + * `git diff --unified=0 ...HEAD` in $workingDirectory (three-dot: + * merge-base semantics, matching what CI reviews). + * @throws DiffException with git's stderr when git fails (no repo, unknown ref) + */ + public function run(string $base, string $workingDirectory): string; +} +``` + +`proc_open` with stdout/stderr pipes; non-zero exit → `DiffException` carrying +stderr. No shell string interpolation — pass an argv array (base ref is +user-supplied input). + +- [ ] **Step 1:** failing integration tests against a throwaway repo built in a + temp dir (`git init -b main`, commit, edit on a branch, commit): diff text + contains the expected `@@` hunk; unknown base ref throws with git's message; + non-repo directory throws. (git exists on all CI runners and dev machines — + acceptable test dependency; guard with a `git --version` skip-check.) +- [ ] **Step 2:** red. **Step 3:** implement. **Step 4:** green + check. +- [ ] **Step 5:** commit `feat(#7): git diff runner`. + +### Task 3: `Hop->changed` + `ChangedChainFilter` + +**Files:** modify `src/Chain/Hop.php` (additive last constructor arg +`public readonly bool $changed = false` — existing call sites untouched); +create `src/Diff/ChangedChainFilter.php`; +test `tests/Diff/ChangedChainFilterTest.php`. + +**Produces:** + +```php +final class ChangedChainFilter +{ + public function __construct(private readonly ChangedLines $changedLines) {} + + /** + * Frozen rule 9. Keeps findings with >= 1 changed hop, rebuilt so each kept + * finding's hops carry `changed`. Hop matches iff its declaration `line` or + * its `forwardLine` is in the diff for its file. The terminal node (the + * chain entry with forwardLine === null and a terminal-kind that keeps it + * in the chain) never matches and is never marked. + * @param list $findings + * @return list + */ + public function filter(array $findings): array; +} +``` + +Immutability: never mutate — construct new `Hop`s/`Finding`s for kept chains. + +- [ ] **Step 1:** failing tests with hand-built findings: middle hop's + forwardLine in diff → kept, exactly that hop marked; declaration line match → + kept; only the terminal's line in diff → dropped; no intersection → dropped; + order and every non-hop `Finding` field preserved on rebuild; empty + `ChangedLines` → everything dropped. +- [ ] **Step 2:** red. **Step 3:** implement. **Step 4:** green + check. +- [ ] **Step 5:** commit `feat(#7): changed-chain filter with hop marking`. + +### Task 4: CLI wiring — `--diff`, stdin, `Application` + +**Files:** modify `src/Console/ArgvParser.php`, `src/Console/Options.php`, +`src/Console/Application.php`; tests extend `ArgvParserTest`, `ApplicationTest`. + +Contract: + +- New value flag `--diff `: read the unified diff from a file, or from + stdin when the value is `-`, instead of executing git. **`--diff` implies + `--changed-only`** (specifying a diff source and not wanting the filter is + contradictory). `Options` gains `public readonly ?string $diff = null`. +- `Application::__construct` gains a third optional stream param + `$stdin = null` (defaults to `\STDIN`), matching the existing + stdout/stderr injection pattern — tests feed `php://memory` streams. +- `analyze()` pipeline order: build index → findings → **changed-only filter** + (when active) → thresholds → reporter. Diff acquisition: `Options->diff` + null → `GitDiffRunner->run($options->gitBase, getcwd())`; `'-'` → + `stream_get_contents($stdin)`; path → `file_get_contents` (unreadable → + error). `DiffException` → message on stderr, exit 2. +- `--changed-only` with an empty diff is a **success**: `No tramp data found`, + exit 0 (a doc-only PR must not fail). + +- [ ] **Step 1:** failing tests — parser: `--diff x.patch` sets `diff` and + forces `changedOnly`; `--diff` without value throws. Application (temp dir + with a 3-hop chain file): `--changed-only --diff ` where the diff + touches hop 2's forwarding line → finding reported, exit 1; + diff touching only an unrelated line → `No tramp data found`, exit 0; + `--diff -` reads the same diff from the injected stdin stream; unreadable + diff path → exit 2; malformed diff text → exit 2. +- [ ] **Step 2:** red. **Step 3:** implement. **Step 4:** green + check. +- [ ] **Step 5:** commit `feat(#7): --changed-only pipeline with --diff and stdin input`. + +### Task 5: Reporter marking — "this hop is yours" + +**Files:** modify `src/Report/{TextReporter,JsonReporter,GithubReporter}.php`; +tests extend the three reporter test files. + +Contracts (pin byte-for-byte): + +- **text:** changed hops get ` *YOURS*` appended after the location column: + +``` +FINDING $config: 3 pass-through hops across 4 classes + origin Demo\Controller::handle($config) src/Demo.php:12 + hop 2 Demo\ServiceA::process($config) src/Demo.php:18 *YOURS* + hop 3 Demo\ServiceB::run($config) src/Demo.php:24 + terminal Demo\Mailer::__construct($config) src/Demo.php:32 (stored) +``` + +- **json:** every chain entry gains `"changed": true|false` — **only in + changed-only runs** (outside them the key is absent, keeping every Phase 3 + fixture byte-stable; the field would be meaningless noise there anyway — + document this conditionality in the README schema note). +- **github:** the `::error`/`::warning` annotation anchors at the **first changed + hop** (fall back to the origin if — impossible after the filter, but code + defensively — none is marked); its title gains the suffix + ` (hop N of the chain, changed by this diff)` when anchored off-origin. + Notices for the remaining hops as before. +- **checkstyle / sarif:** deliberately unchanged this phase — their consumers + (CI annotation ingestion, code scanning) key on stable locations; revisit with + real usage feedback. State this in the commit body. + +- [ ] Steps: failing exact-string tests (marked text block; json with and + without the key; github anchored at hop 2) → red → implement → green + check → + commit `feat(#7): mark changed hops in text, json and github output`. + +### Task 6: `composer tramp`, self-config, dogfood gate + +The maintainer's two requests, plus the prerequisite that makes them safe. +Sub-steps are ordered so the suite is green after every commit. + +**Files:** modify `src/Console/ArgvParser.php`, `src/Console/Options.php`, +`src/Console/Application.php`, `tests/FixtureTest.php` (folder mode), +`composer.json`, `.github/workflows/ci.yml`, `src/Resolve/CallResolver.php`; +create `phptramp.dist.json`. + +- [ ] **Step 1 — `--no-config`:** bool flag; `Options->noConfig`; `Application` + skips `ConfigLoader` when set. Test-first in `ArgvParserTest` + + `ApplicationTest` (a config file in cwd is ignored under `--no-config`). + The fixture harness's **folder mode** and every `ApplicationTest` invocation + that runs from the repo root add `--no-config` — this is what keeps the root + config (next step) out of the tests. Config mode stays config-driven on + purpose. Commit `feat(#7): --no-config escape hatch`. +- [ ] **Step 2 — the repo's own config + script:** create `phptramp.dist.json` + (`.dist` so a developer can locally override with an uncommitted + `phptramp.json`): + +```json +{ + "paths": ["src"], + "limit": 3, + "warnLimit": 2 +} +``` + + and in `composer.json` scripts: `"tramp": "@php bin/phptramp"` — config + supplies paths and thresholds; args still pass through + (`composer tramp -- --format summary`). Thresholds are measured, not guessed: + the current tree has one 4-hop finding (error at limit 3) and six 2-hop + chains (warnings at warn-limit 2) — the gate is strict enough to bite and + quiet enough to stay credible. Run `composer tramp` — it must **fail** with + exactly the `CallResolver` finding; that red run is this step's "failing + test". Commit `feat(#7): composer tramp script and self-config` (red gate is + fine — the gate is not in CI until Step 4). +- [ ] **Step 3 — resolve the finding:** refactor `CallResolver` so `$caller` + stops tramping through `resolveFunction → functionTarget → + functionCandidates` (the chain's terminal only wants the caller's namespace — + derive it once in `resolve()` and pass the namespace, or introduce a narrow + value object; pick what reads best against the house style). **Prefer the + refactor; `#[TrampIgnore]` on our own flagship finding is the last resort** + and needs a written justification in the commit body. `composer tramp` goes + green (warnings may remain); full `composer check` green. + Commit `refactor(#7): stop trampling caller context through CallResolver`. +- [ ] **Step 4 — the gate:** replace the report-only CI step with the real one: + +```yaml + # Dogfood: phptramp gates itself via its own config (limit 3, warn-limit 2). + - name: Dogfood + run: composer tramp +``` + + Update the stale "Phase 6 lowers it" comment. README: document that the repo + gates itself and that `composer tramp` inside this repo is the same + invocation consumers get via the documented script snippet — the README's + consuming-project snippet is now *also* how this repo actually runs. Commit + `ci(#7): dogfood becomes a real gate via composer tramp`. + +### Task 7: Diff fixtures, CI cookbook, close-out + +**Files:** create fixture dirs, `docs/ci.md`; modify `README.md`, +`docs/plan.md` (status), help text in `Application` (`--changed-only`, +`--git-base`, `--diff` lose their "reserved" wording). + +- **Fixtures** (folder mode + `phptramp-args.json` using `--diff` with a + committed `changes.diff`; paths in the diff are relative to the repo root + because folder mode runs from there): + `changed-hop-reported` (diff touches a middle hop's forwarding line; expected + json carries `"changed"` flags), `changed-only-filters-rest` (two chains, diff + touches one — the other must not appear), `changed-terminal-not-enough` (diff + touches only the terminal's line; expects zero findings). +- **docs/ci.md:** copy-paste recipes — GitHub Actions PR job (`fetch-depth: 0`, + fetch the base ref, `--changed-only --git-base "origin/$BASE" --format + github`), GitLab MR equivalent, piping any diff via + `git diff … | phptramp --changed-only --diff - --format json`, and the + full-scan + `composer tramp` pattern this repo itself uses. Note baseline + (Phase 5) as the upcoming piece for legacy adoption. +- **README:** status → Phase 4; flagship section becomes present-tense with the + `*YOURS*` example; json `"changed"` conditionality documented. +- **docs/plan.md:** check off Phase 4, mark ✅, note that the dogfood gate and + `composer tramp` shipped here (pulled forward from Phase 6). + +- [ ] Steps: fixtures test-first → green + check → + commit `test(#7): diff-aware end-to-end fixtures` → docs → + commit `docs(#7): CI cookbook and phase 4 status`. + +--- + +## Done when + +1. Every task committed on `feature/7-diff-aware-mode`; `composer check` green at + every commit; `composer infection:diff` ≥ 80 MSI locally (`git fetch origin + main` first). +2. `composer tramp` is green and gating in CI; the CallResolver chain is fixed, + not suppressed (or the suppression is justified in writing). +3. All Phase 4 bullets in `docs/plan.md` checked off; README/help/docs/ci.md + match actual behavior. +4. PR into `develop` with `Closes #7`; CI (matrix + static + dogfood + mutation) + green; after merge, verify issue #7 auto-closed. From 69b5bc73fdea57dd6bf42edde9fbc5f932447271 Mon Sep 17 00:00:00 2001 From: Lars Pohlmann Date: Tue, 11 Aug 2026 14:40:57 +0200 Subject: [PATCH 02/14] feat(#7): unified diff parser and changed-line set Task 1 of Phase 4 (diff-aware mode): DiffParser turns unified diff text into a ChangedLines set of added/modified line numbers keyed by the diff's new-file paths. ChangedLines exposes containsLine/isEmpty plus resolveAgainst() to re-key paths to realpath-absolute form, matching Hop->file, for the intersection filter landing in a later task. The parser recognizes "+++ "/"@@ " structure only outside a hunk body, tracking the old-side/new-side line counts a hunk header declares and consuming exactly that many body lines before looking for structure again. Otherwise a hunk-body ADDED line whose own content starts with "++ " renders as "+++ ..." once the diff's leading marker is prepended, indistinguishable from a real file header, and would silently misattribute or drop every subsequent hunk until the next real header. --- src/Diff/ChangedLines.php | 47 ++++++ src/Diff/DiffException.php | 9 ++ src/Diff/DiffParser.php | 202 ++++++++++++++++++++++++++ tests/Diff/ChangedLinesTest.php | 95 ++++++++++++ tests/Diff/DiffParserTest.php | 247 ++++++++++++++++++++++++++++++++ 5 files changed, 600 insertions(+) create mode 100644 src/Diff/ChangedLines.php create mode 100644 src/Diff/DiffException.php create mode 100644 src/Diff/DiffParser.php create mode 100644 tests/Diff/ChangedLinesTest.php create mode 100644 tests/Diff/DiffParserTest.php diff --git a/src/Diff/ChangedLines.php b/src/Diff/ChangedLines.php new file mode 100644 index 0000000..b2375c6 --- /dev/null +++ b/src/Diff/ChangedLines.php @@ -0,0 +1,47 @@ +> $linesByFile paths exactly as the diff names them */ + public function __construct(private readonly array $linesByFile) + { + } + + public function isEmpty(): bool + { + return $this->linesByFile === []; + } + + public function containsLine(string $file, int $line): bool + { + return in_array($line, $this->linesByFile[$file] ?? [], true); + } + + /** + * Re-keys every path via realpath($baseDirectory . '/' . $path); paths that + * do not resolve (deleted files) are dropped. Hop->file is realpath-absolute, + * so intersection must happen against a resolved instance. + */ + public function resolveAgainst(string $baseDirectory): self + { + $resolvedLinesByFile = []; + + foreach ($this->linesByFile as $file => $lines) { + $realPath = realpath($baseDirectory . '/' . $file); + if ($realPath === false) { + continue; + } + + $resolvedLinesByFile[$realPath] = $lines; + } + + return new self($resolvedLinesByFile); + } +} diff --git a/src/Diff/DiffException.php b/src/Diff/DiffException.php new file mode 100644 index 0000000..c20dfb8 --- /dev/null +++ b/src/Diff/DiffException.php @@ -0,0 +1,9 @@ +` + * lines (the `a/`/`b/` prefix is stripped, and its absence is tolerated), and + * the changed line numbers come from hunk headers (`@@ -o,n +s,c @@`). Every + * other diff line — `diff --git`, `index`, rename/copy/mode headers, hunk + * body lines, `\ No newline at end of file` — is noise this parser ignores. + * + * Structural lines (`+++ `, `@@ `) are only recognized outside a hunk body: a + * hunk body can itself contain a line whose *content* starts with `++ ` (an + * added source line "++ i" becomes the diff line "+++ i" once the leading `+` + * marker is prepended), which would otherwise be misread as a new file + * header. The parser instead tracks how many old-side and new-side body + * lines the current hunk header declared, and only resumes looking for + * structure once that many have been consumed. + */ +final class DiffParser +{ + private const HUNK_HEADER_PATTERN = '/^@@ -\d+(?:,(\d+))? \+(\d+)(?:,(\d+))? @@/'; + private const NO_NEWLINE_MARKER = '\\'; + private const CONTEXT_MARKER = ' '; + private const REMOVED_MARKER = '-'; + private const ADDED_MARKER = '+'; + + /** @throws DiffException when the text is not a unified diff */ + public function parse(string $unifiedDiff): ChangedLines + { + if ($unifiedDiff === '') { + return new ChangedLines([]); + } + + $lines = $this->splitIntoLines($unifiedDiff); + $this->assertHasDiffStructure($lines); + + return new ChangedLines($this->collectChangedLinesByFile($lines)); + } + + /** @return list */ + private function splitIntoLines(string $unifiedDiff): array + { + return explode("\n", str_replace("\r", '', $unifiedDiff)); + } + + /** @param list $lines */ + private function assertHasDiffStructure(array $lines): void + { + foreach ($lines as $line) { + $isDiffTargetLine = str_starts_with($line, '+++ '); + $isHunkHeaderLine = $this->matchHunkHeader($line) !== null; + + if ($isDiffTargetLine || $isHunkHeaderLine) { + return; + } + } + + throw new DiffException('text is not a unified diff: no "+++" or "@@" hunk header found'); + } + + /** + * @param list $lines + * @return array> + */ + private function collectChangedLinesByFile(array $lines): array + { + $linesByFile = []; + $currentFile = null; + $totalLines = count($lines); + $index = 0; + + while ($index < $totalLines) { + $line = $lines[$index]; + + if (str_starts_with($line, '+++ ')) { + $currentFile = $this->targetFile($line); + $index++; + continue; + } + + $hunk = $this->matchHunkHeader($line); + if ($hunk === null) { + $index++; + continue; + } + + [$oldLineCount, $newStartLine, $newLineCount] = $hunk; + $hunkLines = $this->linesInRange($newStartLine, $newLineCount); + if ($hunkLines !== [] && $currentFile !== null) { + $linesByFile[$currentFile] = [...($linesByFile[$currentFile] ?? []), ...$hunkLines]; + } + + $index = $this->skipHunkBody($lines, $index + 1, $oldLineCount, $newLineCount); + } + + return $linesByFile; + } + + /** @return list */ + private function linesInRange(int $startLine, int $lineCount): array + { + if ($lineCount === 0) { + return []; + } + + return range($startLine, $startLine + $lineCount - 1); + } + + /** + * Advances past the body lines a hunk header declared: exactly + * $remainingOldLines old-side lines (markers ' '/'-') and $remainingNewLines + * new-side lines (markers ' '/'+'), in whatever order/interleaving they + * appear. Returns the index of the first line after the body. + * + * @param list $lines + */ + private function skipHunkBody(array $lines, int $index, int $remainingOldLines, int $remainingNewLines): int + { + $totalLines = count($lines); + + while ($index < $totalLines && ($remainingOldLines > 0 || $remainingNewLines > 0)) { + $remaining = $this->consumeHunkBodyLine($lines[$index], $remainingOldLines, $remainingNewLines); + if ($remaining === null) { + break; + } + + [$remainingOldLines, $remainingNewLines] = $remaining; + $index++; + } + + return $index; + } + + /** + * Accounts one hunk-body line against the still-owed old-side/new-side + * counts, based on its leading marker. Null means the line does not belong + * to the hunk body (an unexpected marker, or a count already exhausted) — + * the caller stops consuming and treats it as ordinary diff text again. + * + * @return array{0: int, 1: int}|null + */ + private function consumeHunkBodyLine(string $line, int $remainingOldLines, int $remainingNewLines): ?array + { + $marker = $line[0] ?? ''; + + if ($marker === self::NO_NEWLINE_MARKER) { + return [$remainingOldLines, $remainingNewLines]; + } + + if ($marker === self::CONTEXT_MARKER && $remainingOldLines > 0 && $remainingNewLines > 0) { + return [$remainingOldLines - 1, $remainingNewLines - 1]; + } + + if ($marker === self::REMOVED_MARKER && $remainingOldLines > 0) { + return [$remainingOldLines - 1, $remainingNewLines]; + } + + if ($marker === self::ADDED_MARKER && $remainingNewLines > 0) { + return [$remainingOldLines, $remainingNewLines - 1]; + } + + return null; + } + + /** Null for a deleted file (`+++ /dev/null`); the `a/`/`b/` prefix, if present, is stripped. */ + private function targetFile(string $plusPlusPlusLine): ?string + { + $path = substr($plusPlusPlusLine, strlen('+++ ')); + + if ($path === '/dev/null') { + return null; + } + + if (str_starts_with($path, 'a/') || str_starts_with($path, 'b/')) { + return substr($path, 2); + } + + return $path; + } + + /** @return array{0: int, 1: int, 2: int}|null old-side line count, new-side start line, new-side line count */ + private function matchHunkHeader(string $line): ?array + { + if (preg_match(self::HUNK_HEADER_PATTERN, $line, $matches) !== 1) { + return null; + } + + // Group 1 is always present in $matches (a required group follows it, so + // PCRE fills it with '' when unmatched); group 3 is trailing, so PCRE + // omits it entirely when unmatched instead of filling it with ''. + $oldLineCount = $matches[1] !== '' ? (int) $matches[1] : 1; + $newStartLine = (int) $matches[2]; + $newLineCount = isset($matches[3]) ? (int) $matches[3] : 1; + + return [$oldLineCount, $newStartLine, $newLineCount]; + } +} diff --git a/tests/Diff/ChangedLinesTest.php b/tests/Diff/ChangedLinesTest.php new file mode 100644 index 0000000..4ecd065 --- /dev/null +++ b/tests/Diff/ChangedLinesTest.php @@ -0,0 +1,95 @@ +baseDirectory = $baseDirectory; + } + + protected function tearDown(): void + { + unlink($this->baseDirectory . '/src/Foo.php'); + rmdir($this->baseDirectory . '/src'); + rmdir($this->baseDirectory); + } + + public function testIsEmptyIsTrueForNoFiles(): void + { + self::assertTrue((new ChangedLines([]))->isEmpty()); + } + + public function testIsEmptyIsFalseWhenAFileHasLines(): void + { + self::assertFalse((new ChangedLines(['src/Foo.php' => [1]]))->isEmpty()); + } + + public function testContainsLineIsTrueForAKnownLine(): void + { + $changedLines = new ChangedLines(['src/Foo.php' => [1, 2, 3]]); + + self::assertTrue($changedLines->containsLine('src/Foo.php', 2)); + } + + public function testContainsLineIsFalseForAnUnlistedLine(): void + { + $changedLines = new ChangedLines(['src/Foo.php' => [1, 2, 3]]); + + self::assertFalse($changedLines->containsLine('src/Foo.php', 5)); + } + + public function testContainsLineIsFalseForAnUnknownFile(): void + { + $changedLines = new ChangedLines(['src/Foo.php' => [1, 2, 3]]); + + self::assertFalse($changedLines->containsLine('src/Bar.php', 1)); + } + + public function testResolveAgainstRekeysPathsToRealpath(): void + { + $changedLines = new ChangedLines(['src/Foo.php' => [1, 2]]); + + $resolved = $changedLines->resolveAgainst($this->baseDirectory); + + $expectedRealPath = realpath($this->baseDirectory . '/src/Foo.php'); + self::assertIsString($expectedRealPath); + self::assertTrue($resolved->containsLine($expectedRealPath, 1)); + self::assertFalse($resolved->containsLine('src/Foo.php', 1)); + } + + public function testResolveAgainstDropsFilesThatDoNotResolve(): void + { + $changedLines = new ChangedLines(['src/Deleted.php' => [1]]); + + $resolved = $changedLines->resolveAgainst($this->baseDirectory); + + self::assertTrue($resolved->isEmpty()); + } + + public function testResolveAgainstKeepsResolvableFilesAndDropsDeletedOnes(): void + { + $changedLines = new ChangedLines([ + 'src/Foo.php' => [1], + 'src/Deleted.php' => [7], + ]); + + $resolved = $changedLines->resolveAgainst($this->baseDirectory); + + $expectedRealPath = realpath($this->baseDirectory . '/src/Foo.php'); + self::assertIsString($expectedRealPath); + self::assertTrue($resolved->containsLine($expectedRealPath, 1)); + self::assertFalse($resolved->isEmpty()); + } +} diff --git a/tests/Diff/DiffParserTest.php b/tests/Diff/DiffParserTest.php new file mode 100644 index 0000000..759694e --- /dev/null +++ b/tests/Diff/DiffParserTest.php @@ -0,0 +1,247 @@ +parse($unifiedDiff); + } + + public function testEmptyStringParsesToEmptyChangedLines(): void + { + self::assertTrue($this->parse('')->isEmpty()); + } + + public function testTextWithoutDiffStructureThrows(): void + { + $this->expectException(DiffException::class); + + $this->parse("hello world\nnot a diff at all\n"); + } + + public function testSingleHunkWithFullNewRangeStripsBPrefix(): void + { + $diff = <<parse($diff); + + self::assertTrue($changedLines->containsLine('src/Foo.php', 10)); + self::assertTrue($changedLines->containsLine('src/Foo.php', 13)); + self::assertFalse($changedLines->containsLine('src/Foo.php', 14)); + self::assertFalse($changedLines->containsLine('src/Foo.php', 9)); + } + + public function testAPrefixIsAlsoStripped(): void + { + $diff = <<parse($diff)->containsLine('src/Bar.php', 1)); + } + + public function testNoPrefixIsTolerated(): void + { + $diff = <<parse($diff)->containsLine('path/no/prefix.php', 2)); + self::assertFalse($this->parse($diff)->containsLine('path/no/prefix.php', 1)); + } + + public function testOmittedNewCountMeansSingleLine(): void + { + $diff = <<parse($diff); + + self::assertTrue($changedLines->containsLine('src/Baz.php', 5)); + self::assertFalse($changedLines->containsLine('src/Baz.php', 6)); + } + + public function testPureDeletionHunkContributesNothing(): void + { + $diff = <<parse($diff)->isEmpty()); + } + + public function testDevNullNewFileSkipsFileEntirely(): void + { + $diff = <<parse($diff)->isEmpty()); + self::assertFalse($this->parse($diff)->containsLine('src/Removed.php', 1)); + } + + public function testRenameCopyModeIndexAndNoNewlineNoiseAreTolerated(): void + { + $diff = <<parse($diff); + + self::assertTrue($changedLines->containsLine('new.php', 1)); + self::assertTrue($changedLines->containsLine('new.php', 2)); + } + + public function testCrlfLineEndingsAreAccepted(): void + { + $diff = implode("\r\n", [ + '--- a/src/Foo.php', + '+++ b/src/Foo.php', + '@@ -10,3 +10,4 @@ class Foo', + ' line1', + '+line2', + ' line3', + ' line4', + '', + ]); + + $changedLines = $this->parse($diff); + + self::assertTrue($changedLines->containsLine('src/Foo.php', 10)); + self::assertTrue($changedLines->containsLine('src/Foo.php', 13)); + } + + public function testMultipleHunksInSameFileAccumulate(): void + { + $diff = <<parse($diff); + + self::assertTrue($changedLines->containsLine('src/Multi.php', 1)); + self::assertTrue($changedLines->containsLine('src/Multi.php', 20)); + self::assertTrue($changedLines->containsLine('src/Multi.php', 21)); + self::assertFalse($changedLines->containsLine('src/Multi.php', 15)); + } + + public function testAddedLineLookingLikeAFileHeaderIsNotMistakenForOne(): void + { + // The hunk body's added line has source content "++ i", which the diff + // renders as "+++ i" once its own "+" marker is prepended — indistinguishable + // from a real "+++ " header unless the parser tracks hunk-body extent. + $diff = "--- a/src/Foo.php\n" + . "+++ b/src/Foo.php\n" + . "@@ -1,1 +1,2 @@\n" + . " context\n" + . "+++ i\n" + . "@@ -50,1 +50,1 @@\n" + . "+second hunk line\n"; + + $changedLines = $this->parse($diff); + + self::assertTrue($changedLines->containsLine('src/Foo.php', 50)); + self::assertFalse($changedLines->containsLine('i', 50)); + } + + public function testDeletionOnlyHunkBodyIsSkippedBeforeANonDeletionHunkInTheSameFile(): void + { + $diff = <<parse($diff); + + self::assertTrue($changedLines->containsLine('src/Mixed.php', 10)); + self::assertTrue($changedLines->containsLine('src/Mixed.php', 11)); + self::assertFalse($changedLines->isEmpty()); + } + + public function testMultipleFilesAreKeptSeparate(): void + { + $diff = <<parse($diff); + + self::assertTrue($changedLines->containsLine('src/One.php', 1)); + self::assertFalse($changedLines->containsLine('src/One.php', 9)); + self::assertTrue($changedLines->containsLine('src/Two.php', 9)); + self::assertFalse($changedLines->containsLine('src/Two.php', 1)); + } +} From b4841d7085db4cf0444bfbad244465f593917138 Mon Sep 17 00:00:00 2001 From: Lars Pohlmann Date: Tue, 11 Aug 2026 15:02:04 +0200 Subject: [PATCH 03/14] feat(#7): git diff runner GitDiffRunner shells out to `git diff --unified=0 ...HEAD` (three-dot merge-base semantics) via proc_open with an argv array, so the user-supplied base ref is never shell-interpolated. A $base starting with '-' is rejected up front, closing the remaining argument-injection hole: `git` itself would otherwise parse a leading-dash argv element as an option (e.g. `--output=`) rather than a revision, even though the shell never gets a chance to interpret it. Non-zero exit raises DiffException carrying git's stderr, falling back to a message naming the exit code when git wrote nothing to stderr (e.g. killed by a signal). Stdout is returned on success. Reuses the existing PhpTramp\Diff\DiffException from Task 1. Tests build a real throwaway git repo per test in a temp dir (init, commit, branch, commit) rather than mocking git, covering the success path, an unknown base ref, a non-repository working directory, and a leading-dash base (asserting git is never invoked with the injected option). Skips itself via `git --version` when git is unavailable. --- src/Diff/GitDiffRunner.php | 71 +++++++++++++++++++ tests/Diff/GitDiffRunnerTest.php | 113 +++++++++++++++++++++++++++++++ 2 files changed, 184 insertions(+) create mode 100644 src/Diff/GitDiffRunner.php create mode 100644 tests/Diff/GitDiffRunnerTest.php diff --git a/src/Diff/GitDiffRunner.php b/src/Diff/GitDiffRunner.php new file mode 100644 index 0000000..5091ee8 --- /dev/null +++ b/src/Diff/GitDiffRunner.php @@ -0,0 +1,71 @@ +...HEAD` (three-dot: merge-base semantics, + * matching what CI reviews) in a given working directory. + */ +final class GitDiffRunner +{ + /** + * @throws DiffException with git's stderr when git fails (no repo, unknown + * ref), or when $base looks like an option instead of a ref + */ + public function run(string $base, string $workingDirectory): string + { + if (str_starts_with($base, '-')) { + throw new DiffException("base ref must not start with '-': {$base}"); + } + + $descriptorSpec = [ + 0 => ['pipe', 'r'], + 1 => ['pipe', 'w'], + 2 => ['pipe', 'w'], + ]; + + $process = proc_open( + ['git', 'diff', '--unified=0', $base . '...HEAD'], + $descriptorSpec, + $pipes, + $workingDirectory + ); + + if ($process === false) { + throw new DiffException("failed to start git in {$workingDirectory}"); + } + + // stdin is never written to; closing it immediately avoids git ever + // blocking on it and lets us read the output pipes without deadlock. + fclose($pipes[0]); + $standardOutput = $this->readAndClosePipe($pipes[1]); + $standardError = $this->readAndClosePipe($pipes[2]); + $exitCode = proc_close($process); + + if ($exitCode !== 0) { + throw new DiffException($this->failureMessage($standardError, $exitCode)); + } + + return $standardOutput; + } + + /** @param resource $pipe */ + private function readAndClosePipe($pipe): string + { + $contents = stream_get_contents($pipe); + fclose($pipe); + + return $contents !== false ? $contents : ''; + } + + /** Falls back to the exit code when git wrote nothing to stderr (e.g. killed by a signal). */ + private function failureMessage(string $standardError, int $exitCode): string + { + $trimmedError = trim($standardError); + + return $trimmedError !== '' ? $trimmedError : "git exited with status {$exitCode} and no stderr output"; + } +} diff --git a/tests/Diff/GitDiffRunnerTest.php b/tests/Diff/GitDiffRunnerTest.php new file mode 100644 index 0000000..88fb8d0 --- /dev/null +++ b/tests/Diff/GitDiffRunnerTest.php @@ -0,0 +1,113 @@ +repository = sys_get_temp_dir() . '/phptramp-gitdiff-' . uniqid('', true); + mkdir($this->repository, 0777, true); + $this->buildRepositoryWithChangeOnFeatureBranch(); + } + + protected function tearDown(): void + { + $iterator = new \RecursiveIteratorIterator( + new \RecursiveDirectoryIterator($this->repository, \FilesystemIterator::SKIP_DOTS), + \RecursiveIteratorIterator::CHILD_FIRST + ); + foreach ($iterator as $item) { + $path = (string) $item; + is_dir($path) ? rmdir($path) : unlink($path); + } + rmdir($this->repository); + } + + private function buildRepositoryWithChangeOnFeatureBranch(): void + { + $this->runGit(['init', '-q', '-b', 'main']); + $this->runGit(['config', 'user.email', 'phptramp-test@example.com']); + $this->runGit(['config', 'user.name', 'phptramp test']); + + file_put_contents($this->repository . '/tracked.php', "runGit(['add', 'tracked.php']); + $this->runGit(['commit', '-q', '-m', 'initial commit']); + + $this->runGit(['checkout', '-q', '-b', 'feature']); + file_put_contents($this->repository . '/tracked.php', "runGit(['add', 'tracked.php']); + $this->runGit(['commit', '-q', '-m', 'second commit']); + } + + /** @param list $arguments */ + private function runGit(array $arguments): void + { + $command = array_merge(['git'], $arguments); + $descriptorSpec = [1 => ['pipe', 'w'], 2 => ['pipe', 'w']]; + $process = proc_open($command, $descriptorSpec, $pipes, $this->repository); + self::assertNotFalse($process, 'failed to start git while setting up the test repository'); + + fclose($pipes[1]); + fclose($pipes[2]); + $exitCode = proc_close($process); + self::assertSame(0, $exitCode, 'git command failed while setting up the test repository'); + } + + public function testReturnsUnifiedDiffContainingTheChangedFilesHunk(): void + { + $diff = (new GitDiffRunner())->run('main', $this->repository); + + self::assertStringContainsString('+++ b/tracked.php', $diff); + self::assertStringContainsString('@@ -2,0 +3 @@', $diff); + } + + public function testUnknownBaseRefThrowsWithGitsStderr(): void + { + $this->expectException(DiffException::class); + $this->expectExceptionMessageMatches('/this-branch-does-not-exist/'); + + (new GitDiffRunner())->run('this-branch-does-not-exist', $this->repository); + } + + public function testNonRepositoryDirectoryThrows(): void + { + $nonRepository = sys_get_temp_dir() . '/phptramp-gitdiff-non-repo-' . uniqid('', true); + mkdir($nonRepository); + + try { + $this->expectException(DiffException::class); + (new GitDiffRunner())->run('main', $nonRepository); + } finally { + rmdir($nonRepository); + } + } + + public function testLeadingDashBaseIsRejectedBeforeReachingGit(): void + { + $injectedOutputPath = $this->repository . '-injected-output.txt'; + + try { + $this->expectException(DiffException::class); + (new GitDiffRunner())->run('--output=' . $injectedOutputPath, $this->repository); + } finally { + self::assertFileDoesNotExist( + $injectedOutputPath, + 'a leading-dash base must never reach git as an option' + ); + } + } +} From 6af419692be88ca3ecee77be12a960bbe6e02b70 Mon Sep 17 00:00:00 2001 From: Lars Pohlmann Date: Tue, 11 Aug 2026 15:11:54 +0200 Subject: [PATCH 04/14] feat(#7): changed-chain filter with hop marking Implements frozen rule 9: diff-aware mode keeps a finding iff at least one of its hops intersects the diff, marking which hops are the user's. Hop->changed is a purely additive, default-false constructor parameter so every existing call site is untouched. ChangedChainFilter is a pure post-processor over the finished Finding list; the terminal node (forwardLine === null) never matches and is never marked. --- src/Chain/Hop.php | 2 + src/Diff/ChangedChainFilter.php | 97 +++++++++++++++++ tests/Diff/ChangedChainFilterTest.php | 144 ++++++++++++++++++++++++++ 3 files changed, 243 insertions(+) create mode 100644 src/Diff/ChangedChainFilter.php create mode 100644 tests/Diff/ChangedChainFilterTest.php diff --git a/src/Chain/Hop.php b/src/Chain/Hop.php index 1e7f6b5..63cf9e5 100644 --- a/src/Chain/Hop.php +++ b/src/Chain/Hop.php @@ -18,6 +18,8 @@ public function __construct( public readonly int $line, /** Line of the forwarding call site; null on the terminal node. */ public readonly ?int $forwardLine, + /** Whether this hop intersects the diff in diff-aware mode. */ + public readonly bool $changed = false, ) { } } diff --git a/src/Diff/ChangedChainFilter.php b/src/Diff/ChangedChainFilter.php new file mode 100644 index 0000000..b4eaaf1 --- /dev/null +++ b/src/Diff/ChangedChainFilter.php @@ -0,0 +1,97 @@ += 1 changed hop, rebuilt so each kept + * finding's hops carry `changed`. A hop matches iff its declaration `line` + * or its `forwardLine` is in the diff for its file. The terminal node (the + * chain entry with forwardLine === null) never matches and is never marked. + * @param list $findings + * @return list + */ + public function filter(array $findings): array + { + $kept = []; + + foreach ($findings as $finding) { + $rebuilt = $this->rebuildIfChanged($finding); + if ($rebuilt !== null) { + $kept[] = $rebuilt; + } + } + + return $kept; + } + + private function rebuildIfChanged(Finding $finding): ?Finding + { + $markedChain = $this->markChangedHops($finding->chain); + if (!$this->hasChangedHop($markedChain)) { + return null; + } + + return new Finding( + $finding->param, + $finding->origin, + $finding->terminal, + $finding->terminalKind, + $finding->hops, + $markedChain, + $finding->classes, + $finding->notes, + $finding->trace, + ); + } + + /** + * @param list $chain + * @return list + */ + private function markChangedHops(array $chain): array + { + return array_map(fn (Hop $hop): Hop => $this->markHop($hop), $chain); + } + + private function markHop(Hop $hop): Hop + { + return new Hop($hop->fqmn, $hop->class, $hop->file, $hop->line, $hop->forwardLine, $this->hopMatches($hop)); + } + + private function hopMatches(Hop $hop): bool + { + if ($hop->forwardLine === null) { + return false; + } + + return $this->changedLines->containsLine($hop->file, $hop->line) + || $this->changedLines->containsLine($hop->file, $hop->forwardLine); + } + + /** @param list $chain */ + private function hasChangedHop(array $chain): bool + { + foreach ($chain as $hop) { + if ($hop->changed) { + return true; + } + } + + return false; + } +} diff --git a/tests/Diff/ChangedChainFilterTest.php b/tests/Diff/ChangedChainFilterTest.php new file mode 100644 index 0000000..51e5392 --- /dev/null +++ b/tests/Diff/ChangedChainFilterTest.php @@ -0,0 +1,144 @@ + */ + private function threeHopChain(): array + { + return [ + new Hop('Demo\A::go', 'Demo\A', 'src/A.php', 5, 7), + new Hop('Demo\B::step', 'Demo\B', 'src/B.php', 9, 11), + new Hop('Demo\C::sink', 'Demo\C', 'src/C.php', 13, null), + ]; + } + + private function findingWithChain(array $chain): Finding + { + return new Finding( + 'config', + 'Demo\A::go', + 'Demo\C::sink', + TerminalKind::Used, + 2, + $chain, + 3, + ['a note'], + ['a trace line'], + ); + } + + public function testMiddleHopForwardLineInDiffKeepsFindingAndMarksOnlyThatHop(): void + { + $finding = $this->findingWithChain($this->threeHopChain()); + $filter = new ChangedChainFilter(new ChangedLines(['src/B.php' => [11]])); + + $kept = $filter->filter([$finding]); + + self::assertCount(1, $kept); + $chain = $kept[0]->chain; + self::assertFalse($chain[0]->changed); + self::assertTrue($chain[1]->changed); + self::assertFalse($chain[2]->changed); + } + + public function testHopDeclarationLineInDiffKeepsFindingAndMarksThatHop(): void + { + $finding = $this->findingWithChain($this->threeHopChain()); + $filter = new ChangedChainFilter(new ChangedLines(['src/A.php' => [5]])); + + $kept = $filter->filter([$finding]); + + self::assertCount(1, $kept); + $chain = $kept[0]->chain; + self::assertTrue($chain[0]->changed); + self::assertFalse($chain[1]->changed); + self::assertFalse($chain[2]->changed); + } + + public function testOnlyTerminalLineInDiffDropsFinding(): void + { + $finding = $this->findingWithChain($this->threeHopChain()); + $filter = new ChangedChainFilter(new ChangedLines(['src/C.php' => [13]])); + + $kept = $filter->filter([$finding]); + + self::assertSame([], $kept); + } + + public function testNoIntersectionDropsFinding(): void + { + $finding = $this->findingWithChain($this->threeHopChain()); + $filter = new ChangedChainFilter(new ChangedLines(['src/Z.php' => [1]])); + + $kept = $filter->filter([$finding]); + + self::assertSame([], $kept); + } + + public function testEmptyChangedLinesDropsEveryFinding(): void + { + $finding = $this->findingWithChain($this->threeHopChain()); + $filter = new ChangedChainFilter(new ChangedLines([])); + + $kept = $filter->filter([$finding]); + + self::assertSame([], $kept); + } + + public function testKeptFindingPreservesOrderAndEveryNonHopField(): void + { + $originalChain = $this->threeHopChain(); + $finding = $this->findingWithChain($originalChain); + $filter = new ChangedChainFilter(new ChangedLines(['src/A.php' => [5]])); + + $kept = $filter->filter([$finding]); + + self::assertCount(1, $kept); + $rebuilt = $kept[0]; + self::assertSame($finding->param, $rebuilt->param); + self::assertSame($finding->origin, $rebuilt->origin); + self::assertSame($finding->terminal, $rebuilt->terminal); + self::assertSame($finding->terminalKind, $rebuilt->terminalKind); + self::assertSame($finding->hops, $rebuilt->hops); + self::assertSame($finding->classes, $rebuilt->classes); + self::assertSame($finding->notes, $rebuilt->notes); + self::assertSame($finding->trace, $rebuilt->trace); + + self::assertCount(3, $rebuilt->chain); + self::assertSame('Demo\A::go', $rebuilt->chain[0]->fqmn); + self::assertSame('Demo\B::step', $rebuilt->chain[1]->fqmn); + self::assertSame('Demo\C::sink', $rebuilt->chain[2]->fqmn); + + // The original Finding/Hops must be untouched — the filter rebuilds + // rather than mutates. + self::assertFalse($originalChain[0]->changed); + self::assertNotSame($originalChain[0], $rebuilt->chain[0]); + } + + public function testKeepsFindingsThatMatchAndDropsThoseThatDoNotAmongSeveral(): void + { + $keptFinding = $this->findingWithChain($this->threeHopChain()); + $droppedFinding = $this->findingWithChain([ + new Hop('Demo\X::go', 'Demo\X', 'src/X.php', 1, 2), + new Hop('Demo\Y::sink', 'Demo\Y', 'src/Y.php', 3, null), + ]); + $filter = new ChangedChainFilter(new ChangedLines(['src/A.php' => [5]])); + + $result = $filter->filter([$droppedFinding, $keptFinding]); + + self::assertCount(1, $result); + self::assertSame('Demo\A::go', $result[0]->chain[0]->fqmn); + self::assertTrue($result[0]->chain[0]->changed); + } +} From 2b2f8917c8323851c6b3ecbb315210a833bbb6e0 Mon Sep 17 00:00:00 2001 From: Lars Pohlmann Date: Tue, 11 Aug 2026 15:26:15 +0200 Subject: [PATCH 05/14] feat(#7): --changed-only pipeline with --diff and stdin input Wires the Task 1-3 diff-aware building blocks into the CLI: a new --diff flag reads a unified diff from a file or stdin instead of shelling out to git, and Application's analyze() pipeline now runs the changed-only filter (when active) between chain-building and reporting. --diff implies --changed-only, since naming a diff source with no filter to apply it to is contradictory. An empty diff is a success (no findings, exit 0), not a failure, so a doc-only PR never fails the gate. Also introduces DiffAwareFlags, grouping ArgvParser's changedOnly/gitBase/diff fields behind one holder: adding $diff as a fourth flat field would have pushed ArgvParser's field count from 15 to 16, tripping PHPMD's TooManyFields ceiling. The three flags already form a conceptual unit (the CLI help text's own "Diff-aware mode:" section), so grouping them is a real fix rather than a threshold tweak. --- src/Console/Application.php | 65 ++++++++- src/Console/ArgvParser.php | 27 +++- src/Console/DiffAwareFlags.php | 21 +++ src/Console/Options.php | 1 + tests/Console/ApplicationTest.php | 234 ++++++++++++++++++++++++++++++ tests/Console/ArgvParserTest.php | 23 +++ 6 files changed, 360 insertions(+), 11 deletions(-) create mode 100644 src/Console/DiffAwareFlags.php diff --git a/src/Console/Application.php b/src/Console/Application.php index ef34ea5..2cb99e2 100644 --- a/src/Console/Application.php +++ b/src/Console/Application.php @@ -8,6 +8,10 @@ use PhpTramp\Chain\Finding; use PhpTramp\Config\ConfigException; use PhpTramp\Config\ConfigLoader; +use PhpTramp\Diff\ChangedChainFilter; +use PhpTramp\Diff\DiffException; +use PhpTramp\Diff\DiffParser; +use PhpTramp\Diff\GitDiffRunner; use PhpTramp\Discovery\FileLocator; use PhpTramp\Index\ForwardSite; use PhpTramp\Index\Indexer; @@ -31,14 +35,19 @@ final class Application /** @var resource */ private $stderr; + /** @var resource */ + private $stdin; + /** * @param resource|null $stdout * @param resource|null $stderr + * @param resource|null $stdin */ - public function __construct($stdout = null, $stderr = null) + public function __construct($stdout = null, $stderr = null, $stdin = null) { $this->stdout = $stdout ?? \STDOUT; $this->stderr = $stderr ?? \STDERR; + $this->stdin = $stdin ?? \STDIN; } /** @@ -87,18 +96,67 @@ private function analyze(Options $options): int $thresholds = new Thresholds($options->limit, $options->warnLimit); $index = $this->buildIndex($options); $reporter = (new ReporterFactory($this->workingDirectory()))->create($options); - } catch (InvalidArgsException | ParseException $e) { + $findings = $this->changedOnlyFindings($this->findChains($index), $options); + } catch (InvalidArgsException | ParseException | DiffException $e) { fwrite($this->stderr, 'phptramp: ' . $e->getMessage() . "\n"); return 2; } - $findings = $this->findChains($index); fwrite($this->stdout, $reporter->render($findings)); return $this->hasError($findings, $thresholds) ? 1 : 0; } + /** + * @param list $findings + * @return list + */ + private function changedOnlyFindings(array $findings, Options $options): array + { + if (! $options->changedOnly) { + return $findings; + } + + $changedLines = (new DiffParser())->parse($this->acquireDiffText($options)) + ->resolveAgainst($this->workingDirectory()); + + return (new ChangedChainFilter($changedLines))->filter($findings); + } + + private function acquireDiffText(Options $options): string + { + if ($options->diff === null) { + return (new GitDiffRunner())->run($options->gitBase, $this->workingDirectory()); + } + + if ($options->diff === '-') { + return $this->readStdin(); + } + + return $this->readDiffFile($options->diff); + } + + private function readDiffFile(string $path): string + { + $contents = @file_get_contents($path); + if ($contents === false) { + throw new DiffException("unable to read diff file: {$path}"); + } + + return $contents; + } + + private function readStdin(): string + { + $contents = stream_get_contents($this->stdin); + if ($contents === false) { + throw new DiffException('unable to read diff from stdin'); + } + + return $contents; + } + /** * @param list $findings */ @@ -203,6 +261,7 @@ private static function helpText(): string Diff-aware mode: --changed-only Only report chains touching changed lines --git-base Diff base for --changed-only (default: origin/main) + --diff Read the diff from a file, or stdin with '-' (implies --changed-only) Baseline: --baseline Ignore findings recorded in the baseline file diff --git a/src/Console/ArgvParser.php b/src/Console/ArgvParser.php index 24d234c..a523e8a 100644 --- a/src/Console/ArgvParser.php +++ b/src/Console/ArgvParser.php @@ -19,6 +19,7 @@ final class ArgvParser '--warn-limit', '--format', '--git-base', + '--diff', '--baseline', '--generate-baseline', ]; @@ -43,8 +44,7 @@ final class ArgvParser private ?int $warnLimit = null; private string $format = 'text'; private bool $explain = false; - private bool $changedOnly = false; - private string $gitBase = 'origin/main'; + private DiffAwareFlags $diffAware; private ?string $baseline = null; private ?string $generateBaseline = null; private bool $dumpIndex = false; @@ -74,8 +74,9 @@ public function parse(array $args, Options $defaults = new Options()): Options warnLimit: $this->warnLimit, format: $this->format, explain: $this->explain, - changedOnly: $this->changedOnly, - gitBase: $this->gitBase, + changedOnly: $this->diffAware->changedOnly, + gitBase: $this->diffAware->gitBase, + diff: $this->diffAware->diff, baseline: $this->baseline, generateBaseline: $this->generateBaseline, dumpIndex: $this->dumpIndex, @@ -93,8 +94,7 @@ private function reset(Options $defaults): void $this->warnLimit = $defaults->warnLimit; $this->format = $defaults->format; $this->explain = $defaults->explain; - $this->changedOnly = $defaults->changedOnly; - $this->gitBase = $defaults->gitBase; + $this->diffAware = new DiffAwareFlags($defaults->changedOnly, $defaults->gitBase, $defaults->diff); $this->baseline = $defaults->baseline; $this->generateBaseline = $defaults->generateBaseline; $this->dumpIndex = $defaults->dumpIndex; @@ -146,7 +146,8 @@ private function applyValueFlag(string $name, string $value): void '--limit' => $this->limit = $this->toInt($name, $value), '--warn-limit' => $this->warnLimit = $this->toInt($name, $value), '--format' => $this->format = $this->validateFormat($value), - '--git-base' => $this->gitBase = $value, + '--git-base' => $this->diffAware->gitBase = $value, + '--diff' => $this->applyDiffFlag($value), '--baseline' => $this->baseline = $value, '--generate-baseline' => $this->generateBaseline = $value, default => throw new InvalidArgsException("unknown option: {$name}"), @@ -157,7 +158,7 @@ private function applyBoolFlag(string $name): void { match ($name) { '--explain' => $this->explain = true, - '--changed-only' => $this->changedOnly = true, + '--changed-only' => $this->diffAware->changedOnly = true, '--dump-index' => $this->dumpIndex = true, '--help', '-h' => $this->help = true, '--version', '-V' => $this->version = true, @@ -165,6 +166,16 @@ private function applyBoolFlag(string $name): void }; } + /** + * A diff source with no filter to apply it to is contradictory, so + * --diff always turns --changed-only on. + */ + private function applyDiffFlag(string $value): void + { + $this->diffAware->diff = $value; + $this->diffAware->changedOnly = true; + } + private function appendFolder(string $value): void { $this->clearSeededPathsOnce(); diff --git a/src/Console/DiffAwareFlags.php b/src/Console/DiffAwareFlags.php new file mode 100644 index 0000000..8f4dd49 --- /dev/null +++ b/src/Console/DiffAwareFlags.php @@ -0,0 +1,21 @@ +stdout)); } + public function testChangedOnlyWithDiffFileTouchingForwardingLineReportsFindingAndExitsOne(): void + { + $folder = $this->fixtureWithLineControlledThreeHopChain(); + $diffFile = $this->writeDiffFile($folder, 'touches-hop.diff', $this->diffTouchingHopTwoForwardLine()); + + $exitCode = $this->runInFolder($folder, [ + 'phptramp', '--folder', $folder, '--changed-only', '--diff', $diffFile, + ]); + + self::assertSame(1, $exitCode); + self::assertStringContainsString('FINDING', self::contents($this->stdout)); + } + + public function testChangedOnlyWithDiffFileTouchingUnrelatedLineReportsNothingAndExitsZero(): void + { + $folder = $this->fixtureWithLineControlledThreeHopChain(); + $diffFile = $this->writeDiffFile($folder, 'touches-unrelated.diff', $this->diffTouchingUnrelatedLine()); + + $exitCode = $this->runInFolder($folder, [ + 'phptramp', '--folder', $folder, '--changed-only', '--diff', $diffFile, + ]); + + self::assertSame(0, $exitCode); + self::assertStringContainsString('No tramp data found', self::contents($this->stdout)); + } + + public function testDiffDashReadsTheSameDiffFromInjectedStdin(): void + { + $folder = $this->fixtureWithLineControlledThreeHopChain(); + $stdin = $this->memoryStreamContaining($this->diffTouchingHopTwoForwardLine()); + $app = new Application($this->stdout, $this->stderr, $stdin); + + $exitCode = $this->runAppInFolder($app, $folder, [ + 'phptramp', '--folder', $folder, '--changed-only', '--diff', '-', + ]); + + self::assertSame(1, $exitCode); + self::assertStringContainsString('FINDING', self::contents($this->stdout)); + } + + public function testUnreadableDiffPathExitsWithToolErrorCode(): void + { + $folder = $this->fixtureWithLineControlledThreeHopChain(); + $missingDiffFile = $folder . '/does-not-exist.diff'; + + $exitCode = $this->runInFolder($folder, [ + 'phptramp', '--folder', $folder, '--changed-only', '--diff', $missingDiffFile, + ]); + + self::assertSame(2, $exitCode); + self::assertStringContainsString('phptramp:', self::contents($this->stderr)); + } + + public function testMalformedDiffTextExitsWithToolErrorCode(): void + { + $folder = $this->fixtureWithLineControlledThreeHopChain(); + $diffFile = $this->writeDiffFile($folder, 'malformed.diff', "not a diff at all\n"); + + $exitCode = $this->runInFolder($folder, [ + 'phptramp', '--folder', $folder, '--changed-only', '--diff', $diffFile, + ]); + + self::assertSame(2, $exitCode); + self::assertStringContainsString('phptramp:', self::contents($this->stderr)); + } + + public function testEmptyDiffFileIsSuccessWithNoFindings(): void + { + $folder = $this->fixtureWithLineControlledThreeHopChain(); + $diffFile = $this->writeDiffFile($folder, 'empty.diff', ''); + + $exitCode = $this->runInFolder($folder, [ + 'phptramp', '--folder', $folder, '--changed-only', '--diff', $diffFile, + ]); + + self::assertSame(0, $exitCode); + self::assertStringContainsString('No tramp data found', self::contents($this->stdout)); + } + + public function testEmptyStdinDiffIsSuccessWithNoFindings(): void + { + $folder = $this->fixtureWithLineControlledThreeHopChain(); + $stdin = $this->memoryStreamContaining(''); + $app = new Application($this->stdout, $this->stderr, $stdin); + + $exitCode = $this->runAppInFolder($app, $folder, [ + 'phptramp', '--folder', $folder, '--changed-only', '--diff', '-', + ]); + + self::assertSame(0, $exitCode); + self::assertStringContainsString('No tramp data found', self::contents($this->stdout)); + } + + /** + * @param list $argv + */ + private function runInFolder(string $folder, array $argv): int + { + return $this->runAppInFolder($this->app, $folder, $argv); + } + + /** + * @param list $argv + */ + private function runAppInFolder(Application $app, string $folder, array $argv): int + { + $previousCwd = getcwd(); + self::assertIsString($previousCwd); + + try { + chdir($folder); + + return $app->run($argv); + } finally { + chdir($previousCwd); + } + } + + /** @return resource */ + private function memoryStreamContaining(string $contents) + { + $stream = fopen('php://memory', 'w+'); + self::assertIsResource($stream); + + fwrite($stream, $contents); + rewind($stream); + + return $stream; + } + + private function writeDiffFile(string $folder, string $filename, string $contents): string + { + $path = $folder . '/' . $filename; + file_put_contents($path, $contents); + + return $path; + } + + /** + * Unified diff whose only hunk touches Demo.php line 21 — the forwarding + * call site inside ServiceA::process, i.e. hop 2 of the fixture chain. + */ + private function diffTouchingHopTwoForwardLine(): string + { + return <<<'DIFF' + diff --git a/Demo.php b/Demo.php + index 1111111..2222222 100644 + --- a/Demo.php + +++ b/Demo.php + @@ -21 +21 @@ + - (new ServiceB())->run($config); + + (new ServiceB())->run($config); + + DIFF; + } + + /** + * Unified diff whose only hunk touches Demo.php line 6 — the closing brace + * of class Cfg, which is not any hop's declaration or forwarding line. + */ + private function diffTouchingUnrelatedLine(): string + { + return <<<'DIFF' + diff --git a/Demo.php b/Demo.php + index 1111111..2222222 100644 + --- a/Demo.php + +++ b/Demo.php + @@ -6 +6 @@ + -{ + +{ + + DIFF; + } + + /** + * A PHP file whose exact line numbers are hand-controlled: a 3-hop + * pure-forward chain (Controller -> ServiceA -> ServiceB -> Mailer) where + * every declaration and forwarding-call-site line is known, so diff + * fixtures can target them precisely. + */ + private function fixtureWithLineControlledThreeHopChain(): string + { + $folder = sys_get_temp_dir() . '/phptramp-diff-' . uniqid(); + mkdir($folder); + $this->folders[] = $folder; + + $lines = [ + 'process($config);', + ' }', + '}', + '', + 'class ServiceA', + '{', + ' public function process(Cfg $config): void', + ' {', + ' (new ServiceB())->run($config);', + ' }', + '}', + '', + 'class ServiceB', + '{', + ' public function run(Cfg $config): void', + ' {', + ' new Mailer($config);', + ' }', + '}', + '', + 'class Mailer', + '{', + ' private Cfg $c;', + '', + ' public function __construct(Cfg $config)', + ' {', + ' $this->c = $config;', + ' }', + '}', + ]; + file_put_contents($folder . '/Demo.php', implode("\n", $lines) . "\n"); + + return $folder; + } + private function fixtureWithOneHopChain(): string { $folder = sys_get_temp_dir() . '/phptramp-app-' . uniqid(); diff --git a/tests/Console/ArgvParserTest.php b/tests/Console/ArgvParserTest.php index 0ee8371..a11ff7e 100644 --- a/tests/Console/ArgvParserTest.php +++ b/tests/Console/ArgvParserTest.php @@ -27,6 +27,7 @@ public function testDefaults(): void self::assertFalse($o->explain); self::assertFalse($o->changedOnly); self::assertSame('origin/main', $o->gitBase); + self::assertNull($o->diff); self::assertNull($o->baseline); self::assertFalse($o->dumpIndex); } @@ -111,6 +112,28 @@ public function testGitBaseParsed(): void self::assertSame('develop', $this->parse('--git-base', 'develop')->gitBase); } + public function testDiffSetsValueAndForcesChangedOnly(): void + { + $options = $this->parse('--diff', 'x.patch'); + + self::assertSame('x.patch', $options->diff); + self::assertTrue($options->changedOnly); + } + + public function testDiffEqualsSyntaxWorks(): void + { + $options = $this->parse('--diff=x.patch'); + + self::assertSame('x.patch', $options->diff); + self::assertTrue($options->changedOnly); + } + + public function testDiffWithoutValueThrows(): void + { + $this->expectException(InvalidArgsException::class); + $this->parse('--diff'); + } + public function testBaselineParsed(): void { self::assertSame('bl.json', $this->parse('--baseline', 'bl.json')->baseline); From bf2a9f0cbf777d3ef307fced710cd461f63d7fd1 Mon Sep 17 00:00:00 2001 From: Lars Pohlmann Date: Tue, 11 Aug 2026 15:36:55 +0200 Subject: [PATCH 06/14] feat(#7): mark changed hops in text, json and github output Surface Hop::changed (set by ChangedChainFilter in changed-only runs) in three reporters: - TextReporter: a changed non-terminal hop gets " *YOURS*" appended after its location column, via the existing per-row `annotation` slot the terminal already uses for "(kind)". - JsonReporter: chain entries gain a trailing "changed": true|false key, but only when the new constructor flag `changedOnly` is true (wired from Options::$changedOnly via ReporterFactory). Outside changed-only runs the key is omitted entirely, so every Phase 3 JSON fixture stays byte-identical. - GithubReporter: the ::error/::warning annotation now anchors at the first changed non-terminal hop instead of unconditionally at the origin, falling back to the origin when nothing is marked (every normal run). The title gains a " (hop N of the chain, changed by this diff)" suffix only when anchored off-origin. Every other non-terminal hop, including the origin when it's not the anchor, becomes a ::notice in chain order. When nothing is changed this reproduces today's output exactly. CheckstyleReporter, SarifReporter and SummaryReporter are deliberately unchanged this phase: their consumers (CI annotation ingestion, code scanning) key on stable locations, and revisiting that is left for real usage feedback. --- src/Report/GithubReporter.php | 73 ++++++++++++++++++----- src/Report/JsonReporter.php | 29 ++++++--- src/Report/ReporterFactory.php | 2 +- src/Report/TextReporter.php | 15 ++++- tests/Report/GithubReporterTest.php | 33 +++++++++++ tests/Report/JsonReporterTest.php | 91 +++++++++++++++++++++++++++++ tests/Report/TextReporterTest.php | 36 ++++++++++++ 7 files changed, 256 insertions(+), 23 deletions(-) diff --git a/src/Report/GithubReporter.php b/src/Report/GithubReporter.php index d7f8d74..2bc4f56 100644 --- a/src/Report/GithubReporter.php +++ b/src/Report/GithubReporter.php @@ -9,8 +9,13 @@ /** * Renders findings as GitHub Actions workflow commands: one `::error` or - * `::warning` annotation at the origin per finding, plus one `::notice` per - * subsequent hop (the terminal node, when present, is not annotated). See + * `::warning` annotation per finding, plus one `::notice` per remaining + * non-terminal hop (the terminal node, when present, is never annotated). The + * `::error`/`::warning` annotation anchors at the first hop marked `changed` + * (diff-aware mode) so the annotation lands on the line the diff touched; when + * no hop is changed — every normal run, and defensively if the filter ever + * left a finding with nothing marked — it falls back to the origin, which is + * today's unconditional behavior. See * https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions * for the property-value escaping this reporter applies to every `file=` and * `title=` value. @@ -54,25 +59,36 @@ public function render(array $findings): string */ private function annotationLines(Finding $finding, Severity $severity): array { - $lines = [$this->originAnnotation($finding, $severity)]; - foreach ($this->subsequentHops($finding) as $index => $hop) { + $anchorIndex = $this->anchorIndex($finding); + + $lines = [$this->anchorAnnotation($finding, $severity, $anchorIndex)]; + foreach ($this->noticeHops($finding, $anchorIndex) as $index => $hop) { $lines[] = $this->hopNotice($finding, $hop, $index); } return $lines; } - private function originAnnotation(Finding $finding, Severity $severity): string + private function anchorAnnotation(Finding $finding, Severity $severity, int $anchorIndex): string { - $origin = $finding->chain[0]; - $title = 'phptramp::' . $this->message->describe($finding); + $anchor = $finding->chain[$anchorIndex]; + $title = 'phptramp::' . $this->message->describe($finding) . $this->anchorSuffix($anchorIndex); return '::' . $severity->label() - . ' file=' . $this->escapeProperty($this->paths->relativize($origin->file)) - . ',line=' . $origin->line + . ' file=' . $this->escapeProperty($this->paths->relativize($anchor->file)) + . ',line=' . $anchor->line . ',title=' . $this->escapeProperty($title); } + private function anchorSuffix(int $anchorIndex): string + { + if ($anchorIndex === 0) { + return ''; + } + + return ' (hop ' . ($anchorIndex + 1) . ' of the chain, changed by this diff)'; + } + private function hopNotice(Finding $finding, Hop $hop, int $index): string { $title = 'phptramp::hop ' . ($index + 1) . ' of $' . $finding->param . ' chain from ' . $finding->origin; @@ -83,13 +99,42 @@ private function hopNotice(Finding $finding, Hop $hop, int $index): string } /** - * @return array subsequent-hop entries keyed by their chain index - * (1 .. hops-1) — the terminal node, at index - * hops, is never included + * The chain index of the first hop marked `changed`, restricted to the + * non-terminal hops (0 .. hops-1). Falls back to the origin (index 0) when + * none is marked, which is the only case a normal (non diff-aware) run + * ever produces. + */ + private function anchorIndex(Finding $finding): int + { + foreach ($this->nonTerminalHops($finding) as $index => $hop) { + if ($hop->changed) { + return $index; + } + } + + return 0; + } + + /** + * @return array the anchor hop excluded, remaining non-terminal + * hops in chain order, keyed by their chain index + */ + private function noticeHops(Finding $finding, int $anchorIndex): array + { + $hops = $this->nonTerminalHops($finding); + unset($hops[$anchorIndex]); + + return $hops; + } + + /** + * @return array non-terminal-hop entries keyed by their chain + * index (0 .. hops-1) — the terminal node, at + * index hops, is never included */ - private function subsequentHops(Finding $finding): array + private function nonTerminalHops(Finding $finding): array { - return array_slice($finding->chain, 1, $finding->hops - 1, true); + return array_slice($finding->chain, 0, $finding->hops, true); } /** diff --git a/src/Report/JsonReporter.php b/src/Report/JsonReporter.php index abb868d..2c30f13 100644 --- a/src/Report/JsonReporter.php +++ b/src/Report/JsonReporter.php @@ -5,6 +5,7 @@ namespace PhpTramp\Report; use PhpTramp\Chain\Finding; +use PhpTramp\Chain\Hop; /** * Renders findings as a single JSON document: `{limit, warnLimit, findings}`. @@ -18,6 +19,7 @@ final class JsonReporter implements Reporter public function __construct( private readonly Thresholds $thresholds, private readonly Paths $paths, + private readonly bool $changedOnly = false, ) { $this->encoder = new JsonEncoder(); } @@ -82,18 +84,31 @@ private function chainDocument(Finding $finding): array $chain = []; foreach ($finding->chain as $index => $hop) { $isTerminal = $hasTerminalNode && $index === $finding->hops; - $chain[] = [ - 'method' => $hop->fqmn, - 'role' => $this->role($index, $isTerminal), - 'file' => $this->paths->relativize($hop->file), - 'line' => $hop->line, - 'forwardLine' => $hop->forwardLine, - ]; + $chain[] = $this->hopDocument($hop, $index, $isTerminal); } return $chain; } + /** + * @return array + */ + private function hopDocument(Hop $hop, int $index, bool $isTerminal): array + { + $document = [ + 'method' => $hop->fqmn, + 'role' => $this->role($index, $isTerminal), + 'file' => $this->paths->relativize($hop->file), + 'line' => $hop->line, + 'forwardLine' => $hop->forwardLine, + ]; + if ($this->changedOnly) { + $document['changed'] = $hop->changed; + } + + return $document; + } + private function role(int $index, bool $isTerminal): string { if ($isTerminal) { diff --git a/src/Report/ReporterFactory.php b/src/Report/ReporterFactory.php index 05dc2fc..f36d511 100644 --- a/src/Report/ReporterFactory.php +++ b/src/Report/ReporterFactory.php @@ -19,7 +19,7 @@ public function create(Options $options): Reporter return match ($options->format) { 'text' => new TextReporter($thresholds, new Paths($this->workingDirectory), $options->explain), - 'json' => new JsonReporter($thresholds, new Paths($this->workingDirectory)), + 'json' => new JsonReporter($thresholds, new Paths($this->workingDirectory), $options->changedOnly), 'github' => new GithubReporter($thresholds, new Paths($this->workingDirectory)), 'checkstyle' => new CheckstyleReporter($thresholds, new Paths($this->workingDirectory)), 'sarif' => new SarifReporter($thresholds, new Paths($this->workingDirectory)), diff --git a/src/Report/TextReporter.php b/src/Report/TextReporter.php index 95268c3..0373711 100644 --- a/src/Report/TextReporter.php +++ b/src/Report/TextReporter.php @@ -6,6 +6,7 @@ use PhpTramp\Chain\Finding; use PhpTramp\Chain\Hop; +use PhpTramp\Chain\TerminalKind; /** * Renders findings as aligned plain text. Columns are two-space separated; the @@ -114,13 +115,25 @@ private function hopRows(Finding $finding): array 'label' => $this->label($index, $isTerminal), 'method' => $hop->fqmn . '($' . $finding->param . ')', 'location' => $this->location($hop), - 'annotation' => $isTerminal ? '(' . $finding->terminalKind->value . ')' : '', + 'annotation' => $this->annotation($hop, $finding->terminalKind, $isTerminal), ]; } return $rows; } + private function annotation(Hop $hop, TerminalKind $terminalKind, bool $isTerminal): string + { + if ($isTerminal) { + return '(' . $terminalKind->value . ')'; + } + if ($hop->changed) { + return '*YOURS*'; + } + + return ''; + } + private function label(int $index, bool $isTerminal): string { if ($isTerminal) { diff --git a/tests/Report/GithubReporterTest.php b/tests/Report/GithubReporterTest.php index 37d6c62..6597558 100644 --- a/tests/Report/GithubReporterTest.php +++ b/tests/Report/GithubReporterTest.php @@ -55,6 +55,39 @@ public function testEmitsErrorAnnotationAtOriginAndNoticePerSubsequentHop(): voi self::assertSame($expected, $reporter->render([$finding])); } + public function testAnchorsAtFirstChangedNonTerminalHopWithSuffixAndOriginAsNotice(): void + { + $chain = [ + new Hop('Demo\Controller::handle', 'Demo\Controller', 'src/Demo.php', 12, 14), + new Hop('Demo\ServiceA::process', 'Demo\ServiceA', 'src/Demo.php', 18, 20, true), + new Hop('Demo\ServiceB::run', 'Demo\ServiceB', 'src/Demo.php', 24, 26), + new Hop('Demo\Mailer::__construct', 'Demo\Mailer', 'src/Demo.php', 32, null), + ]; + $finding = new Finding( + 'config', + 'Demo\Controller::handle', + 'Demo\Mailer::__construct', + TerminalKind::Stored, + 3, + $chain, + 4, + [], + [], + ); + + $expected = "::error file=src/Demo.php,line=18,title=phptramp%3A%3A\$config%3A 3 pass-through hops " + . "across 4 classes (terminal%3A Demo\\Mailer%3A%3A__construct [stored]) (hop 2 of the chain%2C " + . "changed by this diff)\n" + . "::notice file=src/Demo.php,line=12,title=phptramp%3A%3Ahop 1 of \$config chain from " + . "Demo\\Controller%3A%3Ahandle\n" + . "::notice file=src/Demo.php,line=24,title=phptramp%3A%3Ahop 3 of \$config chain from " + . "Demo\\Controller%3A%3Ahandle\n"; + + $reporter = new GithubReporter(new Thresholds(3, null), $this->paths()); + + self::assertSame($expected, $reporter->render([$finding])); + } + public function testEmitsWarningAnnotationAndOmitsNoticeWhenNoSubsequentHops(): void { $chain = [ diff --git a/tests/Report/JsonReporterTest.php b/tests/Report/JsonReporterTest.php index 9a9cf3f..35fda68 100644 --- a/tests/Report/JsonReporterTest.php +++ b/tests/Report/JsonReporterTest.php @@ -99,6 +99,97 @@ public function testRendersExactDocumentForThreeHopStoredErrorCase(): void self::assertSame($expected, $reporter->render([$finding])); } + public function testChangedOnlyRunAppendsChangedKeyAsLastEntryInEachChainEntry(): void + { + $chain = [ + new Hop('Demo\Controller::handle', 'Demo\Controller', 'src/Demo.php', 12, 14), + new Hop('Demo\ServiceA::process', 'Demo\ServiceA', 'src/Demo.php', 18, 20, true), + new Hop('Demo\ServiceB::run', 'Demo\ServiceB', 'src/Demo.php', 24, 26), + new Hop('Demo\Mailer::__construct', 'Demo\Mailer', 'src/Demo.php', 32, null), + ]; + $finding = new Finding( + 'config', + 'Demo\Controller::handle', + 'Demo\Mailer::__construct', + TerminalKind::Stored, + 3, + $chain, + 4, + [], + [], + ); + + $expected = <<<'JSON' + { + "limit": 3, + "warnLimit": 2, + "findings": [ + { + "param": "config", + "severity": "error", + "origin": "Demo\\Controller::handle", + "terminal": "Demo\\Mailer::__construct", + "terminalKind": "stored", + "hops": 3, + "classes": 4, + "chain": [ + { + "method": "Demo\\Controller::handle", + "role": "origin", + "file": "src/Demo.php", + "line": 12, + "forwardLine": 14, + "changed": false + }, + { + "method": "Demo\\ServiceA::process", + "role": "hop", + "file": "src/Demo.php", + "line": 18, + "forwardLine": 20, + "changed": true + }, + { + "method": "Demo\\ServiceB::run", + "role": "hop", + "file": "src/Demo.php", + "line": 24, + "forwardLine": 26, + "changed": false + }, + { + "method": "Demo\\Mailer::__construct", + "role": "terminal", + "file": "src/Demo.php", + "line": 32, + "forwardLine": null, + "changed": false + } + ], + "notes": [] + } + ] + } + + JSON; + + $reporter = new JsonReporter(new Thresholds(3, 2), $this->paths(), true); + + self::assertSame($expected, $reporter->render([$finding])); + } + + public function testChangedOnlyFlagDefaultsToFalseAndOmitsChangedKey(): void + { + $chain = [ + new Hop('Demo\A::go', 'Demo\A', 'src/A.php', 5, 7, true), + ]; + $finding = new Finding('p', 'Demo\A::go', 'Demo\A::go', TerminalKind::Used, 1, $chain, 1, [], []); + + $reporter = new JsonReporter(new Thresholds(1, null), $this->paths()); + + self::assertStringNotContainsString('"changed"', $reporter->render([$finding])); + } + public function testRendersWarningSeverityAndOmitsBelowThresholdFinding(): void { $warningChain = [ diff --git a/tests/Report/TextReporterTest.php b/tests/Report/TextReporterTest.php index 9c230dc..f9e1e36 100644 --- a/tests/Report/TextReporterTest.php +++ b/tests/Report/TextReporterTest.php @@ -50,6 +50,42 @@ public function testRendersStoredChainWithAlignedColumns(): void self::assertSame($expected, $reporter->render([$finding])); } + public function testMarksChangedNonTerminalHopWithYoursAnnotation(): void + { + $chain = [ + new Hop('Demo\Controller::handle', 'Demo\Controller', 'src/Demo.php', 12, 14), + new Hop('Demo\ServiceA::process', 'Demo\ServiceA', 'src/Demo.php', 18, 20, true), + new Hop('Demo\ServiceB::run', 'Demo\ServiceB', 'src/Demo.php', 24, 26), + new Hop('Demo\Mailer::__construct', 'Demo\Mailer', 'src/Demo.php', 32, null), + ]; + $finding = new Finding( + 'config', + 'Demo\Controller::handle', + 'Demo\Mailer::__construct', + TerminalKind::Stored, + 3, + $chain, + 4, + [], + [], + ); + + $expected = <<<'TXT' + FINDING $config: 3 pass-through hops across 4 classes + origin Demo\Controller::handle($config) src/Demo.php:12 + hop 2 Demo\ServiceA::process($config) src/Demo.php:18 *YOURS* + hop 3 Demo\ServiceB::run($config) src/Demo.php:24 + terminal Demo\Mailer::__construct($config) src/Demo.php:32 (stored) + + 1 finding (limit: 3 hops). + + TXT; + + $reporter = new TextReporter(new Thresholds(3, null), new Paths('/nonexistent-root')); + + self::assertSame($expected, $reporter->render([$finding])); + } + public function testRendersTruncatedChainWithNoteLineAndNoTerminal(): void { $chain = [ From f813e9be78e0b8a8a57160c40540b2157b45b93f Mon Sep 17 00:00:00 2001 From: Lars Pohlmann Date: Tue, 11 Aug 2026 15:41:23 +0200 Subject: [PATCH 07/14] docs(#7): note the changed-only json chain-entry field The task-5 brief required documenting the new JsonReporter "changed" key in the README's output-formats section; it was omitted from the original commit. Add a two-sentence note after the format table: each chain entry gains "changed": true|false only in a --changed-only run, absent otherwise. --- README.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/README.md b/README.md index 67a1af9..4ecee45 100644 --- a/README.md +++ b/README.md @@ -124,6 +124,10 @@ which always runs with `--format json`. | `sarif` | `{"ruleId":"phptramp.trampData","level":"error","message":{"text":"$config: 3 pass-through hops across 4 classes (terminal: Demo\\Mailer::__construct [stored])"}}` | | `summary` | `12 chains total; 5 at or over the limit (limit: 3 hops).` | +In a `--changed-only` run, each `json` chain entry additionally carries a `"changed": +true|false` field marking whether that hop intersects the diff; a normal full run omits +the field entirely rather than sending it as always-`false` noise. + ## Suppression Mark a specific false positive as intentional instead of raising `--limit` project-wide: From af4ca6c2a7cc49f28cc3dbfed86bf920c0474a8c Mon Sep 17 00:00:00 2001 From: Lars Pohlmann Date: Tue, 11 Aug 2026 15:49:27 +0200 Subject: [PATCH 08/14] feat(#7): --no-config escape hatch Adds a --no-config bool flag that bypasses ConfigLoader entirely. Because config seeds the parser's defaults, run() detects it from the raw args via a small loadDefaults() helper before parsing exists. The fixture harness's folder mode and --dump-index helper, plus every repo-root ApplicationTest analysis run, now pass --no-config so the repo's own root config (next commit) can never leak into the tests. --- src/Console/Application.php | 19 ++++++++++++-- src/Console/ArgvParser.php | 5 ++++ src/Console/Options.php | 1 + tests/Console/ApplicationTest.php | 43 ++++++++++++++++++++++++++----- tests/Console/ArgvParserTest.php | 10 +++++++ tests/FixtureTest.php | 9 +++++-- 6 files changed, 76 insertions(+), 11 deletions(-) diff --git a/src/Console/Application.php b/src/Console/Application.php index 2cb99e2..4c47a46 100644 --- a/src/Console/Application.php +++ b/src/Console/Application.php @@ -58,8 +58,7 @@ public function run(array $argv): int $args = array_slice($argv, 1); try { - $defaults = (new ConfigLoader())->load($this->workingDirectory()); - $options = (new ArgvParser())->parse($args, $defaults); + $options = (new ArgvParser())->parse($args, $this->loadDefaults($args)); } catch (ConfigException | InvalidArgsException $e) { fwrite($this->stderr, 'phptramp: ' . $e->getMessage() . "\n"); @@ -85,6 +84,22 @@ public function run(array $argv): int return $this->analyze($options); } + /** + * Config seeds the parser's defaults, so it must be resolved before parsing + * — which means --no-config is detected from the raw args here, not from the + * parsed Options that do not exist yet. + * + * @param list $args + */ + private function loadDefaults(array $args): Options + { + if (in_array('--no-config', $args, true)) { + return new Options(); + } + + return (new ConfigLoader())->load($this->workingDirectory()); + } + private function workingDirectory(): string { return getcwd() ?: '.'; diff --git a/src/Console/ArgvParser.php b/src/Console/ArgvParser.php index a523e8a..315e144 100644 --- a/src/Console/ArgvParser.php +++ b/src/Console/ArgvParser.php @@ -28,6 +28,7 @@ final class ArgvParser '--explain', '--changed-only', '--dump-index', + '--no-config', '--help', '-h', '--version', @@ -48,6 +49,7 @@ final class ArgvParser private ?string $baseline = null; private ?string $generateBaseline = null; private bool $dumpIndex = false; + private bool $noConfig = false; private bool $help = false; private bool $version = false; private bool $clearedSeededPaths = false; @@ -80,6 +82,7 @@ public function parse(array $args, Options $defaults = new Options()): Options baseline: $this->baseline, generateBaseline: $this->generateBaseline, dumpIndex: $this->dumpIndex, + noConfig: $this->noConfig, help: $this->help, version: $this->version, exclude: $this->exclude, @@ -98,6 +101,7 @@ private function reset(Options $defaults): void $this->baseline = $defaults->baseline; $this->generateBaseline = $defaults->generateBaseline; $this->dumpIndex = $defaults->dumpIndex; + $this->noConfig = $defaults->noConfig; $this->help = $defaults->help; $this->version = $defaults->version; $this->exclude = $defaults->exclude; @@ -160,6 +164,7 @@ private function applyBoolFlag(string $name): void '--explain' => $this->explain = true, '--changed-only' => $this->diffAware->changedOnly = true, '--dump-index' => $this->dumpIndex = true, + '--no-config' => $this->noConfig = true, '--help', '-h' => $this->help = true, '--version', '-V' => $this->version = true, default => throw new InvalidArgsException("unknown option: {$name}"), diff --git a/src/Console/Options.php b/src/Console/Options.php index ea8b329..95fbb8e 100644 --- a/src/Console/Options.php +++ b/src/Console/Options.php @@ -32,6 +32,7 @@ public function __construct( public readonly ?string $baseline = null, public readonly ?string $generateBaseline = null, public readonly bool $dumpIndex = false, + public readonly bool $noConfig = false, public readonly bool $help = false, public readonly bool $version = false, public readonly array $exclude = [], diff --git a/tests/Console/ApplicationTest.php b/tests/Console/ApplicationTest.php index 90642d8..231535a 100644 --- a/tests/Console/ApplicationTest.php +++ b/tests/Console/ApplicationTest.php @@ -85,7 +85,7 @@ public function testDefaultRunReportsChainsAndExitsOne(): void { $folder = $this->fixtureWithThreeHopChain(); - self::assertSame(1, $this->app->run(['phptramp', '--folder', $folder])); + self::assertSame(1, $this->app->run(['phptramp', '--folder', $folder, '--no-config'])); self::assertStringContainsString('FINDING', self::contents($this->stdout)); } @@ -93,7 +93,7 @@ public function testLimitAboveChainLengthReportsNothingAndExitsZero(): void { $folder = $this->fixtureWithThreeHopChain(); - self::assertSame(0, $this->app->run(['phptramp', '--folder', $folder, '--limit', '4'])); + self::assertSame(0, $this->app->run(['phptramp', '--folder', $folder, '--no-config', '--limit', '4'])); self::assertStringContainsString('No tramp data found', self::contents($this->stdout)); } @@ -101,7 +101,7 @@ public function testUnknownFormatExitsWithToolErrorCode(): void { $folder = $this->fixtureWithThreeHopChain(); - self::assertSame(2, $this->app->run(['phptramp', '--folder', $folder, '--format', 'xml'])); + self::assertSame(2, $this->app->run(['phptramp', '--folder', $folder, '--no-config', '--format', 'xml'])); self::assertStringContainsString('unknown format: xml', self::contents($this->stderr)); self::assertSame('', self::contents($this->stdout)); } @@ -110,7 +110,7 @@ public function testSummaryFormatReportsAllChainsAndExitsOne(): void { $folder = $this->fixtureWithThreeHopChain(); - self::assertSame(1, $this->app->run(['phptramp', '--folder', $folder, '--format', 'summary'])); + self::assertSame(1, $this->app->run(['phptramp', '--folder', $folder, '--no-config', '--format', 'summary'])); self::assertStringContainsString('at or over the limit', self::contents($this->stdout)); } @@ -118,7 +118,7 @@ public function testJsonFormatReportsFindingsAndExitsOne(): void { $folder = $this->fixtureWithThreeHopChain(); - self::assertSame(1, $this->app->run(['phptramp', '--folder', $folder, '--format', 'json'])); + self::assertSame(1, $this->app->run(['phptramp', '--folder', $folder, '--no-config', '--format', 'json'])); $output = self::contents($this->stdout); self::assertStringContainsString('"findings"', $output); @@ -131,7 +131,7 @@ public function testWarnOnlyRunExitsZeroButPrintsWarning(): void self::assertSame( 0, - $this->app->run(['phptramp', '--folder', $folder, '--limit', '3', '--warn-limit', '2']), + $this->app->run(['phptramp', '--folder', $folder, '--no-config', '--limit', '3', '--warn-limit', '2']), ); self::assertStringContainsString('WARNING', self::contents($this->stdout)); } @@ -147,7 +147,9 @@ public function testInvalidWarnLimitFailsFastWithThresholdError(): void { $folder = $this->fixtureWithOneHopChain(); - $exitCode = $this->app->run(['phptramp', '--folder', $folder, '--limit', '3', '--warn-limit', '5']); + $exitCode = $this->app->run( + ['phptramp', '--folder', $folder, '--no-config', '--limit', '3', '--warn-limit', '5'], + ); self::assertSame(2, $exitCode); self::assertStringContainsString( @@ -183,6 +185,33 @@ public function testConfigFileLimitAppliesWithoutCliFlag(): void self::assertStringContainsString('FINDING', self::contents($this->stdout)); } + public function testNoConfigIgnoresConfigFileInWorkingDirectory(): void + { + $directory = sys_get_temp_dir() . '/phptramp-cwd-' . uniqid(); + mkdir($directory); + $this->folders[] = $directory; + + file_put_contents($directory . '/phptramp.json', '{"limit": 1}'); + + $code = 'c = $config; } }'; + file_put_contents($directory . '/Demo.php', $code); + + $previousCwd = getcwd(); + self::assertIsString($previousCwd); + + try { + chdir($directory); + $exitCode = $this->app->run(['phptramp', '--folder', $directory, '--no-config']); + } finally { + chdir($previousCwd); + } + + self::assertSame(0, $exitCode); + self::assertStringContainsString('No tramp data found', self::contents($this->stdout)); + } + public function testChangedOnlyWithDiffFileTouchingForwardingLineReportsFindingAndExitsOne(): void { $folder = $this->fixtureWithLineControlledThreeHopChain(); diff --git a/tests/Console/ArgvParserTest.php b/tests/Console/ArgvParserTest.php index a11ff7e..65bdca2 100644 --- a/tests/Console/ArgvParserTest.php +++ b/tests/Console/ArgvParserTest.php @@ -159,6 +159,16 @@ public function testDumpIndexFlag(): void self::assertTrue($this->parse('--dump-index')->dumpIndex); } + public function testNoConfigFlagSetsNoConfig(): void + { + self::assertTrue($this->parse('--no-config')->noConfig); + } + + public function testNoConfigDefaultsToFalse(): void + { + self::assertFalse($this->parse()->noConfig); + } + public function testHelpFlags(): void { self::assertTrue($this->parse('--help')->help); diff --git a/tests/FixtureTest.php b/tests/FixtureTest.php index 7bc64c0..f7e0673 100644 --- a/tests/FixtureTest.php +++ b/tests/FixtureTest.php @@ -125,7 +125,10 @@ private function runFindingsJson(string $caseDir): array */ private function runFolderMode(string $caseDir): array { - $argv = ['phptramp', '--folder', $caseDir . '/src', '--format', 'json', ...$this->extraCliArgs($caseDir)]; + $argv = [ + 'phptramp', '--folder', $caseDir . '/src', '--no-config', '--format', 'json', + ...$this->extraCliArgs($caseDir), + ]; [$exitCode, $decoded] = $this->runApplication($argv); return [$exitCode, $this->normalizeFindingsDocument($decoded, $caseDir)]; @@ -267,7 +270,9 @@ private function dumpIndex(string $srcDir): array self::assertIsResource($stdout); self::assertIsResource($stderr); - $exit = (new Application($stdout, $stderr))->run(['phptramp', '--folder', $srcDir, '--dump-index']); + $exit = (new Application($stdout, $stderr))->run( + ['phptramp', '--folder', $srcDir, '--no-config', '--dump-index'], + ); self::assertSame(0, $exit); rewind($stdout); From 3e1f396af279ec4889beb37e8face4fbefd9eefc Mon Sep 17 00:00:00 2001 From: Lars Pohlmann Date: Tue, 11 Aug 2026 15:57:53 +0200 Subject: [PATCH 09/14] feat(#7): composer tramp script and self-config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds phptramp.dist.json (paths: src, limit 3, warn-limit 2 — thresholds measured against the actual tree) and a "tramp": "@php bin/phptramp" composer script so the repo gates itself the config-driven way, args still passing through (composer tramp -- --format summary). Semantics refinement (flagged per CLAUDE.md): a no-argument invocation now analyzes when config supplies paths; help is shown only when there is nothing to analyze (no folders and no files, whether from args or config). run()'s dispatch is reordered so --help/--version/--dump-index are unaffected and the old fragile $args === [] guard is replaced by a nothingToAnalyze() predicate. composer tramp is deliberately red here (exit 1, the single 4-hop $caller chain through CallResolver); the next commit refactors that finding away. The gate does not enter CI until a later commit, so the suite stays green. --- composer.json | 1 + phptramp.dist.json | 5 +++++ src/Console/Application.php | 18 +++++++++++++++++- tests/Console/ApplicationTest.php | 31 ++++++++++++++++++++++++++++++- 4 files changed, 53 insertions(+), 2 deletions(-) create mode 100644 phptramp.dist.json diff --git a/composer.json b/composer.json index 6963ed1..da68eb5 100644 --- a/composer.json +++ b/composer.json @@ -51,6 +51,7 @@ "@md", "@test" ], + "tramp": "@php bin/phptramp", "infection": "infection --threads=max", "infection:diff": "infection --git-diff-filter=AM --git-diff-base=origin/main --threads=max --ignore-msi-with-no-mutations" }, diff --git a/phptramp.dist.json b/phptramp.dist.json new file mode 100644 index 0000000..8c5f4d2 --- /dev/null +++ b/phptramp.dist.json @@ -0,0 +1,5 @@ +{ + "paths": ["src"], + "limit": 3, + "warnLimit": 2 +} diff --git a/src/Console/Application.php b/src/Console/Application.php index 4c47a46..efcb25e 100644 --- a/src/Console/Application.php +++ b/src/Console/Application.php @@ -65,7 +65,7 @@ public function run(array $argv): int return 2; } - if ($args === [] || $options->help) { + if ($options->help) { fwrite($this->stdout, self::helpText()); return 0; @@ -81,9 +81,25 @@ public function run(array $argv): int return $this->dumpIndex($options); } + if ($this->nothingToAnalyze($options)) { + fwrite($this->stdout, self::helpText()); + + return 0; + } + return $this->analyze($options); } + /** + * A run with no folders and no files has nothing to work on. That happens + * for a bare `phptramp` with no config; when config supplies `paths` the + * folders are populated, so the same argument-less invocation analyzes. + */ + private function nothingToAnalyze(Options $options): bool + { + return $options->folders === [] && $options->files === []; + } + /** * Config seeds the parser's defaults, so it must be resolved before parsing * — which means --no-config is detected from the raw args here, not from the diff --git a/tests/Console/ApplicationTest.php b/tests/Console/ApplicationTest.php index 231535a..c4bdac4 100644 --- a/tests/Console/ApplicationTest.php +++ b/tests/Console/ApplicationTest.php @@ -77,10 +77,39 @@ public function testHelpDocumentsTheCliContract(): void public function testNoArgumentsPrintsHelpAndExitsZero(): void { - self::assertSame(0, $this->app->run(['phptramp'])); + self::assertSame(0, $this->app->run(['phptramp', '--no-config'])); self::assertStringContainsString('Usage:', self::contents($this->stdout)); } + public function testNoArgumentsAnalyzeWhenConfigSuppliesPaths(): void + { + $directory = sys_get_temp_dir() . '/phptramp-cwd-' . uniqid(); + mkdir($directory); + $this->folders[] = $directory; + + file_put_contents($directory . '/phptramp.json', '{"paths": ["."], "limit": 1}'); + + $code = 'c = $config; } }'; + file_put_contents($directory . '/Demo.php', $code); + + $previousCwd = getcwd(); + self::assertIsString($previousCwd); + + try { + chdir($directory); + $exitCode = $this->app->run(['phptramp']); + } finally { + chdir($previousCwd); + } + + $output = self::contents($this->stdout); + self::assertSame(1, $exitCode); + self::assertStringContainsString('FINDING', $output); + self::assertStringNotContainsString('Usage:', $output); + } + public function testDefaultRunReportsChainsAndExitsOne(): void { $folder = $this->fixtureWithThreeHopChain(); From 37d35da50f05e5d5148eaf67f5b101b164161c60 Mon Sep 17 00:00:00 2001 From: Lars Pohlmann Date: Tue, 11 Aug 2026 15:58:58 +0200 Subject: [PATCH 10/14] refactor(#7): stop trampling caller context through CallResolver MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit functionTarget/functionCandidates only ever needed the caller's namespace, yet the whole MethodInfo $caller was threaded four levels deep (resolve -> resolveFunction -> functionTarget -> functionCandidates) just to reach namespaceOf() at the bottom — phptramp's own flagship tramp-data finding. Derive the namespace once in resolveFunction via namespaceOf($caller) and pass the resulting string down. The 4-hop error collapses to a 2-hop chain (resolve -> resolveFunction -> namespaceOf), below the limit-3 threshold, so composer tramp now exits 0. Resolution semantics are unchanged: the existing tests/Resolve suite stays byte-green and no fixtures move. --- src/Resolve/CallResolver.php | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/Resolve/CallResolver.php b/src/Resolve/CallResolver.php index c9aaa61..7af7eec 100644 --- a/src/Resolve/CallResolver.php +++ b/src/Resolve/CallResolver.php @@ -39,7 +39,7 @@ public function resolve(ForwardSite $site, MethodInfo $caller): Resolution private function resolveFunction(ForwardSite $site, MethodInfo $caller): Resolution { - $target = $this->functionTarget($site->callee->name, $caller); + $target = $this->functionTarget($site->callee->name, $this->namespaceOf($caller)); if ($target === null) { return new ExternalTarget('function not in index'); } @@ -47,9 +47,9 @@ private function resolveFunction(ForwardSite $site, MethodInfo $caller): Resolut return $this->bind($target, $site); } - private function functionTarget(string $name, MethodInfo $caller): ?MethodInfo + private function functionTarget(string $name, string $callerNamespace): ?MethodInfo { - foreach ($this->functionCandidates($name, $caller) as $candidate) { + foreach ($this->functionCandidates($name, $callerNamespace) as $candidate) { $target = $this->index->get($candidate); if ($target !== null) { return $target; @@ -65,18 +65,17 @@ private function functionTarget(string $name, MethodInfo $caller): ?MethodInfo * * @return list */ - private function functionCandidates(string $name, MethodInfo $caller): array + private function functionCandidates(string $name, string $callerNamespace): array { if (str_contains($name, '\\')) { return [$name]; } - $namespace = $this->namespaceOf($caller); - if ($namespace === '') { + if ($callerNamespace === '') { return [$name]; } - return [$namespace . '\\' . $name, $name]; + return [$callerNamespace . '\\' . $name, $name]; } private function namespaceOf(MethodInfo $caller): string From b1239dcbffe5ec518c652dfde1141fa186970926 Mon Sep 17 00:00:00 2001 From: Lars Pohlmann Date: Tue, 11 Aug 2026 16:00:01 +0200 Subject: [PATCH 11/14] ci(#7): dogfood becomes a real gate via composer tramp MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The dogfood step was report-only (--limit 99, never failing). Now that the tree is deliberately chain-clean it runs composer tramp, which gates on phptramp's own phptramp.dist.json (limit 3, warn-limit 2) and fails the build on any finding at or over the limit. Updates the stale 'Phase 6 lowers it' comment. README: documents that this repo gates itself with composer tramp — the same script a consuming project adds, here driven by config paths rather than an inline --folder. --- .github/workflows/ci.yml | 8 +++----- README.md | 10 +++++++--- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 011a132..319909c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -38,11 +38,9 @@ jobs: run: composer stan - name: PHPMD (codesize) run: composer md - # Dogfood: run phptramp on its own source. Report-only for now (--limit 99 - # can never fail) so the chain output is visible in CI logs; Phase 6 lowers - # it to a real gate once the codebase is deliberately chain-clean. - - name: Dogfood (report-only) - run: bin/phptramp --folder src --limit 99 + # Dogfood: phptramp gates itself via its own config (limit 3, warn-limit 2). + - name: Dogfood + run: composer tramp # Line coverage says a line ran; it does not say a test would notice if the # line were wrong. Infection changes the line and reports whether anything diff --git a/README.md b/README.md index 4ecee45..a90051d 100644 --- a/README.md +++ b/README.md @@ -59,9 +59,13 @@ composer tramp composer tramp -- --format json --warn-limit 2 ``` -phptramp itself does not ship a `composer.json` script of this name — `vendor/bin/phptramp` -(or `bin/phptramp` inside this repo) is the invocation; the snippet above is a convenience -you add to a *consuming* project. +This repository gates itself with exactly such a script: `composer tramp` runs +`bin/phptramp` against the paths and thresholds in its own `phptramp.dist.json` +(`paths: src`, `limit: 3`, `warn-limit: 2`), and CI fails the build on any finding at or +over the limit. That is the same `composer tramp` invocation a consuming project gets from +the snippet above — here the paths come from config, so no `--folder` is needed; a consumer +can configure `paths` the same way or pass `--folder src` inline. (`--no-config` bypasses +the config file entirely when you want to drive everything from flags.) ## CLI From 131d588d7bc1e4be5a6925e2ac28b838e0f3e816 Mon Sep 17 00:00:00 2001 From: Lars Pohlmann Date: Tue, 11 Aug 2026 16:10:51 +0200 Subject: [PATCH 12/14] test(#7): diff-aware end-to-end fixtures Three FixtureTest cases exercise --diff end-to-end through the real CLI: a changed middle hop keeps the finding and marks it, a two-chain source proves --changed-only drops the untouched chain entirely, and a diff that only touches the terminal's declaration line yields zero findings (the terminal is never a hop and never matches). --- .../changed-hop-reported/changes.diff | 5 ++ .../expected-findings.json | 22 ++++++ .../changed-hop-reported/phptramp-args.json | 1 + .../changed-hop-reported/src/Demo.php | 41 ++++++++++ .../changed-only-filters-rest/changes.diff | 5 ++ .../expected-findings.json | 22 ++++++ .../phptramp-args.json | 1 + .../changed-only-filters-rest/src/Demo.php | 75 +++++++++++++++++++ .../changed-terminal-not-enough/changes.diff | 5 ++ .../expected-findings.json | 5 ++ .../phptramp-args.json | 1 + .../changed-terminal-not-enough/src/Demo.php | 41 ++++++++++ 12 files changed, 224 insertions(+) create mode 100644 tests/fixtures/changed-hop-reported/changes.diff create mode 100644 tests/fixtures/changed-hop-reported/expected-findings.json create mode 100644 tests/fixtures/changed-hop-reported/phptramp-args.json create mode 100644 tests/fixtures/changed-hop-reported/src/Demo.php create mode 100644 tests/fixtures/changed-only-filters-rest/changes.diff create mode 100644 tests/fixtures/changed-only-filters-rest/expected-findings.json create mode 100644 tests/fixtures/changed-only-filters-rest/phptramp-args.json create mode 100644 tests/fixtures/changed-only-filters-rest/src/Demo.php create mode 100644 tests/fixtures/changed-terminal-not-enough/changes.diff create mode 100644 tests/fixtures/changed-terminal-not-enough/expected-findings.json create mode 100644 tests/fixtures/changed-terminal-not-enough/phptramp-args.json create mode 100644 tests/fixtures/changed-terminal-not-enough/src/Demo.php diff --git a/tests/fixtures/changed-hop-reported/changes.diff b/tests/fixtures/changed-hop-reported/changes.diff new file mode 100644 index 0000000..da6e4ef --- /dev/null +++ b/tests/fixtures/changed-hop-reported/changes.diff @@ -0,0 +1,5 @@ +--- a/tests/fixtures/changed-hop-reported/src/Demo.php ++++ b/tests/fixtures/changed-hop-reported/src/Demo.php +@@ -21 +21 @@ +- (new ServiceB())->run($config); ++ (new ServiceB())->run($config); diff --git a/tests/fixtures/changed-hop-reported/expected-findings.json b/tests/fixtures/changed-hop-reported/expected-findings.json new file mode 100644 index 0000000..2bf70e3 --- /dev/null +++ b/tests/fixtures/changed-hop-reported/expected-findings.json @@ -0,0 +1,22 @@ +{ + "limit": 1, + "warnLimit": null, + "findings": [ + { + "param": "config", + "severity": "error", + "origin": "Demo\\Controller::handle", + "terminal": "Demo\\Mailer::__construct", + "terminalKind": "stored", + "hops": 3, + "classes": 4, + "chain": [ + {"method": "Demo\\Controller::handle", "role": "origin", "file": "src/Demo.php", "line": 11, "forwardLine": 13, "changed": false}, + {"method": "Demo\\ServiceA::process", "role": "hop", "file": "src/Demo.php", "line": 19, "forwardLine": 21, "changed": true}, + {"method": "Demo\\ServiceB::run", "role": "hop", "file": "src/Demo.php", "line": 27, "forwardLine": 29, "changed": false}, + {"method": "Demo\\Mailer::__construct", "role": "terminal", "file": "src/Demo.php", "line": 37, "forwardLine": null, "changed": false} + ], + "notes": [] + } + ] +} diff --git a/tests/fixtures/changed-hop-reported/phptramp-args.json b/tests/fixtures/changed-hop-reported/phptramp-args.json new file mode 100644 index 0000000..4bdb7ad --- /dev/null +++ b/tests/fixtures/changed-hop-reported/phptramp-args.json @@ -0,0 +1 @@ +["--limit", "1", "--diff", "tests/fixtures/changed-hop-reported/changes.diff"] diff --git a/tests/fixtures/changed-hop-reported/src/Demo.php b/tests/fixtures/changed-hop-reported/src/Demo.php new file mode 100644 index 0000000..81a7977 --- /dev/null +++ b/tests/fixtures/changed-hop-reported/src/Demo.php @@ -0,0 +1,41 @@ +process($config); + } +} + +class ServiceA +{ + public function process(Cfg $config): void + { + (new ServiceB())->run($config); + } +} + +class ServiceB +{ + public function run(Cfg $config): void + { + new Mailer($config); + } +} + +class Mailer +{ + private Cfg $config; + + public function __construct(Cfg $config) + { + $this->config = $config; + } +} diff --git a/tests/fixtures/changed-only-filters-rest/changes.diff b/tests/fixtures/changed-only-filters-rest/changes.diff new file mode 100644 index 0000000..8716eff --- /dev/null +++ b/tests/fixtures/changed-only-filters-rest/changes.diff @@ -0,0 +1,5 @@ +--- a/tests/fixtures/changed-only-filters-rest/src/Demo.php ++++ b/tests/fixtures/changed-only-filters-rest/src/Demo.php +@@ -21 +21 @@ +- (new ServiceB())->run($config); ++ (new ServiceB())->run($config); diff --git a/tests/fixtures/changed-only-filters-rest/expected-findings.json b/tests/fixtures/changed-only-filters-rest/expected-findings.json new file mode 100644 index 0000000..2bf70e3 --- /dev/null +++ b/tests/fixtures/changed-only-filters-rest/expected-findings.json @@ -0,0 +1,22 @@ +{ + "limit": 1, + "warnLimit": null, + "findings": [ + { + "param": "config", + "severity": "error", + "origin": "Demo\\Controller::handle", + "terminal": "Demo\\Mailer::__construct", + "terminalKind": "stored", + "hops": 3, + "classes": 4, + "chain": [ + {"method": "Demo\\Controller::handle", "role": "origin", "file": "src/Demo.php", "line": 11, "forwardLine": 13, "changed": false}, + {"method": "Demo\\ServiceA::process", "role": "hop", "file": "src/Demo.php", "line": 19, "forwardLine": 21, "changed": true}, + {"method": "Demo\\ServiceB::run", "role": "hop", "file": "src/Demo.php", "line": 27, "forwardLine": 29, "changed": false}, + {"method": "Demo\\Mailer::__construct", "role": "terminal", "file": "src/Demo.php", "line": 37, "forwardLine": null, "changed": false} + ], + "notes": [] + } + ] +} diff --git a/tests/fixtures/changed-only-filters-rest/phptramp-args.json b/tests/fixtures/changed-only-filters-rest/phptramp-args.json new file mode 100644 index 0000000..c5e9e3f --- /dev/null +++ b/tests/fixtures/changed-only-filters-rest/phptramp-args.json @@ -0,0 +1 @@ +["--limit", "1", "--diff", "tests/fixtures/changed-only-filters-rest/changes.diff"] diff --git a/tests/fixtures/changed-only-filters-rest/src/Demo.php b/tests/fixtures/changed-only-filters-rest/src/Demo.php new file mode 100644 index 0000000..771979b --- /dev/null +++ b/tests/fixtures/changed-only-filters-rest/src/Demo.php @@ -0,0 +1,75 @@ +process($config); + } +} + +class ServiceA +{ + public function process(Cfg $config): void + { + (new ServiceB())->run($config); + } +} + +class ServiceB +{ + public function run(Cfg $config): void + { + new Mailer($config); + } +} + +class Mailer +{ + private Cfg $config; + + public function __construct(Cfg $config) + { + $this->config = $config; + } +} + +class Controller2 +{ + public function handle(Cfg $config): void + { + (new ServiceA2())->process($config); + } +} + +class ServiceA2 +{ + public function process(Cfg $config): void + { + (new ServiceB2())->run($config); + } +} + +class ServiceB2 +{ + public function run(Cfg $config): void + { + new Mailer2($config); + } +} + +class Mailer2 +{ + private Cfg $config; + + public function __construct(Cfg $config) + { + $this->config = $config; + } +} diff --git a/tests/fixtures/changed-terminal-not-enough/changes.diff b/tests/fixtures/changed-terminal-not-enough/changes.diff new file mode 100644 index 0000000..252ebf8 --- /dev/null +++ b/tests/fixtures/changed-terminal-not-enough/changes.diff @@ -0,0 +1,5 @@ +--- a/tests/fixtures/changed-terminal-not-enough/src/Demo.php ++++ b/tests/fixtures/changed-terminal-not-enough/src/Demo.php +@@ -37 +37 @@ +- public function __construct(Cfg $config) ++ public function __construct(Cfg $config) diff --git a/tests/fixtures/changed-terminal-not-enough/expected-findings.json b/tests/fixtures/changed-terminal-not-enough/expected-findings.json new file mode 100644 index 0000000..7bec990 --- /dev/null +++ b/tests/fixtures/changed-terminal-not-enough/expected-findings.json @@ -0,0 +1,5 @@ +{ + "limit": 1, + "warnLimit": null, + "findings": [] +} diff --git a/tests/fixtures/changed-terminal-not-enough/phptramp-args.json b/tests/fixtures/changed-terminal-not-enough/phptramp-args.json new file mode 100644 index 0000000..8ae11b3 --- /dev/null +++ b/tests/fixtures/changed-terminal-not-enough/phptramp-args.json @@ -0,0 +1 @@ +["--limit", "1", "--diff", "tests/fixtures/changed-terminal-not-enough/changes.diff"] diff --git a/tests/fixtures/changed-terminal-not-enough/src/Demo.php b/tests/fixtures/changed-terminal-not-enough/src/Demo.php new file mode 100644 index 0000000..81a7977 --- /dev/null +++ b/tests/fixtures/changed-terminal-not-enough/src/Demo.php @@ -0,0 +1,41 @@ +process($config); + } +} + +class ServiceA +{ + public function process(Cfg $config): void + { + (new ServiceB())->run($config); + } +} + +class ServiceB +{ + public function run(Cfg $config): void + { + new Mailer($config); + } +} + +class Mailer +{ + private Cfg $config; + + public function __construct(Cfg $config) + { + $this->config = $config; + } +} From 583f2cfbf24c5cf9f8bb15efaf2c00482c97f5e3 Mon Sep 17 00:00:00 2001 From: Lars Pohlmann Date: Tue, 11 Aug 2026 16:13:02 +0200 Subject: [PATCH 13/14] docs(#7): CI cookbook and phase 4 status Add docs/ci.md with copy-paste GitHub Actions / GitLab MR / arbitrary-diff / full-scan recipes. Bring README, the CLI help text, and docs/plan.md up to date with diff-aware mode being shipped rather than reserved, including a present-tense *YOURS* example in the README's flagship section. --- README.md | 34 +++++++++--- docs/ci.md | 105 ++++++++++++++++++++++++++++++++++++ docs/plan.md | 24 +++++---- src/Console/Application.php | 10 ++-- 4 files changed, 152 insertions(+), 21 deletions(-) create mode 100644 docs/ci.md diff --git a/README.md b/README.md index a90051d..2c097cd 100644 --- a/README.md +++ b/README.md @@ -4,11 +4,12 @@ > methods that never use them — "this parameter was passed through 4 classes / 5 methods > before being used." -**Status: Phase 3.** Cross-file chain reporting works: `phptramp --folder src` +**Status: Phase 4.** Cross-file chain reporting works: `phptramp --folder src` stitches forwarding chains across files and prints findings in any of six formats, exiting `0`/`1`/`2`. A `phptramp.json` config file, `#[TrampIgnore]`/ `// phptramp-ignore` suppression, and a `--warn-limit` warning tier are all wired -up. The classifier and whole-project index remain inspectable via `--dump-index`. +up. Diff-aware mode (`--changed-only`/`--git-base`/`--diff`) is shipped too — see +below. The classifier and whole-project index remain inspectable via `--dump-index`. See [docs/plan.md](docs/plan.md) for the full implementation plan and current phase. ## What it does @@ -33,8 +34,28 @@ the "tramp data" smell: every method in the middle is coupled to a value it has business knowing about. The fix is usually a parameter object, a context object, or dependency injection at the terminal — this tool tells you *where*. -The flagship feature (Phase 4) is diff-aware CI mode: run it on a pull request and it -reports **"your edit made this chain longer"**, marking exactly which hops are yours. +The flagship feature is diff-aware CI mode: run it on a pull request and it reports +**"your edit made this chain longer"**, marking exactly which hops are yours. +`--changed-only` restricts findings to chains that intersect a diff — a hop matches iff +its declaration line or its forwarding call-site line was touched — and each matching +hop's location line grows a `*YOURS*` annotation: + +```text +$ vendor/bin/phptramp --folder src --changed-only --git-base origin/main + +FINDING $config: 3 pass-through hops across 4 classes + origin App\Http\Controller::handle($config) src/Http/Controller.php:21 + hop 2 App\Service\ServiceA::process($config) src/Service/ServiceA.php:14 *YOURS* + hop 3 App\Service\ServiceB::run($config) src/Service/ServiceB.php:9 + terminal App\Mail\Mailer::__construct($config) src/Mail/Mailer.php:12 (stored) + +1 finding (limit: 3 hops). +``` + +`--git-base ` (default `origin/main`) supplies the diff via +`git diff --unified=0 ...HEAD` (three-dot, merge-base semantics); `--diff ` +reads a unified diff from a file, or from stdin with `-`, instead — either always implies +`--changed-only`. See [docs/ci.md](docs/ci.md) for GitHub Actions and GitLab recipes. ## Installation @@ -79,8 +100,9 @@ phptramp [options] --warn-limit Warn (do not fail CI) on chains with >= n hops --format text|json|github|checkstyle|sarif|summary (default: text) --explain Show why chains ended (call resolution trace) - --changed-only Reserved for diff-aware mode (Phase 4) — accepted, not yet wired up - --git-base Reserved for diff-aware mode (Phase 4) — accepted, not yet wired up + --changed-only Only report chains touching changed lines + --git-base Diff base for --changed-only (default: origin/main) + --diff Read the diff from a file, or stdin with '-' (implies --changed-only) --baseline Reserved for baselining (Phase 5) — accepted, not yet wired up --generate-baseline Reserved for baselining (Phase 5) — accepted, not yet wired up ``` diff --git a/docs/ci.md b/docs/ci.md new file mode 100644 index 0000000..1a0544d --- /dev/null +++ b/docs/ci.md @@ -0,0 +1,105 @@ +# CI cookbook + +phptramp's flagship mode is diff-aware: on a pull/merge request it reports only +the chains your change touches, and marks exactly which hops are yours. Wire +it into a PR pipeline with `--changed-only --git-base `; pipe an +arbitrary diff with `--diff -`; or run it as a full-repo gate the way this +project gates itself. All four patterns below are copy-paste starting points. + +## GitHub Actions — PR job + +`--changed-only --git-base` shells out to `git diff --unified=0 ...HEAD` +(three-dot, merge-base semantics), so the checkout needs history and the base +ref present locally. `--format github` emits `::error`/`::warning`/`::notice` +annotations that GitHub renders inline on the PR diff. + +```yaml +name: phptramp + +on: + pull_request: ~ + +jobs: + phptramp: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + with: + fetch-depth: 0 + - uses: shivammathur/setup-php@v2 + with: + php-version: '8.4' + - run: composer update --no-interaction --no-progress + - name: Fetch the base branch + run: git fetch --no-tags origin "${GITHUB_BASE_REF}" + - name: phptramp (changed lines only) + run: >- + vendor/bin/phptramp --folder src + --changed-only --git-base "origin/${GITHUB_BASE_REF}" + --format github +``` + +## GitLab CI — merge request job + +Same invocation; GitLab has no built-in consumer for `--format github` +annotations, so `text` (readable in the job log) or `json` (for a downstream +step) are the usual picks. `rules` scopes the job to merge request pipelines, +where `CI_MERGE_REQUEST_TARGET_BRANCH_NAME` is available. GitLab's default +clone is shallow (~20 commits), which leaves the merge-base with the target +branch unreachable and breaks `--git-base`'s three-dot diff — `GIT_DEPTH: 0` +forces a full-history clone, the same job GitHub Actions' `fetch-depth: 0` +does above. + +```yaml +phptramp: + stage: test + image: php:8.4-cli + variables: + GIT_DEPTH: 0 + rules: + - if: '$CI_PIPELINE_SOURCE == "merge_request_event"' + before_script: + - git fetch --no-tags origin "$CI_MERGE_REQUEST_TARGET_BRANCH_NAME" + - composer update --no-interaction --no-progress + script: + - >- + vendor/bin/phptramp --folder src + --changed-only --git-base "origin/$CI_MERGE_REQUEST_TARGET_BRANCH_NAME" + --format text +``` + +## Piping an arbitrary diff + +Any unified diff works, not just `git diff ...HEAD` — a range, a +`.patch` file, output from another review tool. Pipe it to stdin with +`--diff -`: + +```bash +git diff main...HEAD | vendor/bin/phptramp --folder src --changed-only --diff - --format json +``` + +`--diff ` reads from a file instead of stdin; either form implies +`--changed-only`, so it can be dropped when `--diff` is present. + +## Full-scan gate: the pattern this repo uses + +Not every project wants diff-only gating — a full-repo scan with a config-driven +threshold is simpler and catches tramp data introduced outside the diff window +(e.g. a rename that lengthens an existing chain without touching the forwarding +line itself). This repository gates itself that way: `composer tramp` runs +`bin/phptramp` against `phptramp.dist.json` (`paths: src`, `limit: 3`, +`warn-limit: 2`), and CI fails the build on any finding at or over the limit — +see the `static` job's `Dogfood` step in +[`.github/workflows/ci.yml`](../.github/workflows/ci.yml). A consuming project +gets the identical invocation from the README's `composer tramp` recipe; the +two approaches (diff-aware PR annotations, full-scan gate) are not mutually +exclusive and commonly run side by side. + +## Baseline for legacy adoption + +Turning phptramp on for an existing codebase full of pre-existing tramp data +means the full-scan gate above fails immediately. `--baseline`/ +`--generate-baseline` — snapshot current findings once, then only fail on +*new* ones — is the answer, and ships in Phase 5. Until then, `--changed-only` +is the practical way to adopt phptramp on a legacy codebase: it never sees +pre-existing chains that a PR doesn't touch. diff --git a/docs/plan.md b/docs/plan.md index 6a42482..a4c7c90 100644 --- a/docs/plan.md +++ b/docs/plan.md @@ -706,13 +706,14 @@ the detailed plan wins. - [x] **3.5 SummaryReporter:** `--format=summary` — histogram of chain lengths, top 10 longest chains, most-forwarded parameters. -`--changed-only`/`--git-base` (Phase 4) and `--baseline`/`--generate-baseline` (Phase 5) -remain accepted-but-inert CLI flags — parsed into `Options` for forward compatibility, -not yet consumed by analysis. Not a Phase 3 deliverable; tracked in their own phases. +`--changed-only`/`--git-base`/`--diff` were, at this point, accepted-but-inert CLI +flags — parsed into `Options` for forward compatibility, not yet consumed by +analysis; Phase 4 below wires them up. `--baseline`/`--generate-baseline` remain +accepted-but-inert pending Phase 5. --- -## Phase 4 — Diff-aware mode (flagship) (in progress — issue #7) +## Phase 4 — Diff-aware mode (flagship) ✅ (issue #7) **Deliverable:** `phptramp --changed-only --git-base origin/main` reports only chains intersecting the diff and marks which hops are the user's. @@ -721,21 +722,22 @@ intersecting the diff and marks which hops are the user's. against the shipped Phase 1–3 interfaces; where it refines the sketches below (diff via `--diff ` instead of a positional `-`, a `--no-config` escape hatch, marking via immutable Finding rebuild), the detailed plan wins. Two items -are pulled forward into this phase at the maintainer's request: the repo's own -`composer tramp` script + `phptramp.dist.json`, and the CI dogfood step becoming a -real gate (limit 3, warn-limit 2 — thresholds measured on the actual tree). +are pulled forward into this phase at the maintainer's request, both shipped here +rather than in Phase 6: the repo's own `composer tramp` script + `phptramp.dist.json`, +and the CI dogfood step becoming a real gate (limit 3, warn-limit 2 — thresholds +measured on the actual tree). -- **4.1 DiffParser + ChangedLines:** parse unified diff from stdin (`--changed-only -`) +- [x] **4.1 DiffParser + ChangedLines:** parse unified diff from stdin (`--changed-only -`) or by executing `git diff --unified=0 ...HEAD` (note: three-dot, merge-base semantics, matching CI expectations); produce `file -> set` of *added/modified* lines, realpath-normalized. -- **4.2 Intersection filter:** frozen rule 9 — hop matches if its declaration line or +- [x] **4.2 Intersection filter:** frozen rule 9 — hop matches if its declaration line or its forwarding call-site line ∈ changed lines of its file. Chain reported iff ≥ 1 hop matches. -- **4.3 Hop marking:** `Hop::isChanged` flows into every reporter; text renders +- [x] **4.3 Hop marking:** `Hop::isChanged` flows into every reporter; text renders `hop 2 *YOURS*`, json gets `"changed": true`, github annotates changed hops as the error location — the "your edit made this chain longer" experience. -- **4.4 CI cookbook page:** `docs/ci.md` with copy-paste GitHub Actions and GitLab +- [x] **4.4 CI cookbook page:** `docs/ci.md` with copy-paste GitHub Actions and GitLab snippets for PR-only gating. --- diff --git a/src/Console/Application.php b/src/Console/Application.php index efcb25e..e125d44 100644 --- a/src/Console/Application.php +++ b/src/Console/Application.php @@ -310,10 +310,12 @@ private static function helpText(): string 1 at least one finding at or over --limit 2 tool error (bad arguments, parse failure, ...) - Status: Phase 3 - cross-file chain reporting works across all six - formats (text/json/github/checkstyle/sarif/summary). phptramp.json - config, #[TrampIgnore]/phptramp-ignore suppression, and --warn-limit - are wired up. See docs/plan.md. + Status: Phase 4 - diff-aware mode is shipped: --changed-only / + --git-base / --diff report only chains touching the diff and mark + which hops are yours, across all six formats + (text/json/github/checkstyle/sarif/summary). phptramp.json config, + #[TrampIgnore]/phptramp-ignore suppression, and --warn-limit are + wired up. See docs/plan.md. TXT; } From 9202e83cb62877ebac1056dd400f470ee9500a5d Mon Sep 17 00:00:00 2001 From: Lars Pohlmann Date: Tue, 11 Aug 2026 16:46:17 +0200 Subject: [PATCH 14/14] refactor(#7): tidy diff-aware internals after review Quality-only cleanups from the /simplify pass; no behavior change (all 321 tests stay green, reporter output byte-identical): - GithubReporter: compute nonTerminalHops once per finding instead of slicing the chain twice (anchorIndex + noticeHops). - ChangedChainFilter: single-pass rebuild via new immutable withers Hop::withChanged() and Finding::withChain(), so the filter no longer hand-reconstructs those value objects positionally (decouples it from their full constructor signatures) and walks each chain once. - ArgvParser: share one ArgvParser::NO_CONFIG_FLAG constant with Application's pre-parse raw-args detection instead of duplicating the literal across files. --- src/Chain/Finding.php | 22 +++++++++++++++ src/Chain/Hop.php | 9 ++++++ src/Console/Application.php | 2 +- src/Console/ArgvParser.php | 11 ++++++-- src/Diff/ChangedChainFilter.php | 49 +++++++-------------------------- src/Report/GithubReporter.php | 24 ++++++---------- 6 files changed, 59 insertions(+), 58 deletions(-) diff --git a/src/Chain/Finding.php b/src/Chain/Finding.php index a72d154..123c70a 100644 --- a/src/Chain/Finding.php +++ b/src/Chain/Finding.php @@ -29,4 +29,26 @@ public function __construct( public readonly array $trace, ) { } + + /** + * A copy of this finding carrying a rebuilt chain — so callers that only + * re-mark hops (the diff filter) need not know this constructor's full + * signature. + * + * @param list $chain + */ + public function withChain(array $chain): self + { + return new self( + $this->param, + $this->origin, + $this->terminal, + $this->terminalKind, + $this->hops, + $chain, + $this->classes, + $this->notes, + $this->trace, + ); + } } diff --git a/src/Chain/Hop.php b/src/Chain/Hop.php index 63cf9e5..a54ea6a 100644 --- a/src/Chain/Hop.php +++ b/src/Chain/Hop.php @@ -22,4 +22,13 @@ public function __construct( public readonly bool $changed = false, ) { } + + /** + * A copy of this hop with its diff-aware `changed` mark set — so the + * diff filter never has to know this constructor's full signature. + */ + public function withChanged(bool $changed): self + { + return new self($this->fqmn, $this->class, $this->file, $this->line, $this->forwardLine, $changed); + } } diff --git a/src/Console/Application.php b/src/Console/Application.php index e125d44..5748827 100644 --- a/src/Console/Application.php +++ b/src/Console/Application.php @@ -109,7 +109,7 @@ private function nothingToAnalyze(Options $options): bool */ private function loadDefaults(array $args): Options { - if (in_array('--no-config', $args, true)) { + if (in_array(ArgvParser::NO_CONFIG_FLAG, $args, true)) { return new Options(); } diff --git a/src/Console/ArgvParser.php b/src/Console/ArgvParser.php index 315e144..0a357cd 100644 --- a/src/Console/ArgvParser.php +++ b/src/Console/ArgvParser.php @@ -11,6 +11,13 @@ */ final class ArgvParser { + /** + * Skips config loading. Public because {@see Application} must detect it in + * the raw args before parsing (config seeds the parser's defaults), so both + * sides share this one spelling. + */ + public const NO_CONFIG_FLAG = '--no-config'; + private const VALUE_FLAGS = [ '--folder', '--file', @@ -28,7 +35,7 @@ final class ArgvParser '--explain', '--changed-only', '--dump-index', - '--no-config', + self::NO_CONFIG_FLAG, '--help', '-h', '--version', @@ -164,7 +171,7 @@ private function applyBoolFlag(string $name): void '--explain' => $this->explain = true, '--changed-only' => $this->diffAware->changedOnly = true, '--dump-index' => $this->dumpIndex = true, - '--no-config' => $this->noConfig = true, + self::NO_CONFIG_FLAG => $this->noConfig = true, '--help', '-h' => $this->help = true, '--version', '-V' => $this->version = true, default => throw new InvalidArgsException("unknown option: {$name}"), diff --git a/src/Diff/ChangedChainFilter.php b/src/Diff/ChangedChainFilter.php index b4eaaf1..67bf2e3 100644 --- a/src/Diff/ChangedChainFilter.php +++ b/src/Diff/ChangedChainFilter.php @@ -41,36 +41,19 @@ public function filter(array $findings): array private function rebuildIfChanged(Finding $finding): ?Finding { - $markedChain = $this->markChangedHops($finding->chain); - if (!$this->hasChangedHop($markedChain)) { - return null; + $anyChanged = false; + $markedChain = []; + foreach ($finding->chain as $hop) { + $matched = $this->hopMatches($hop); + $anyChanged = $anyChanged || $matched; + $markedChain[] = $hop->withChanged($matched); } - return new Finding( - $finding->param, - $finding->origin, - $finding->terminal, - $finding->terminalKind, - $finding->hops, - $markedChain, - $finding->classes, - $finding->notes, - $finding->trace, - ); - } - - /** - * @param list $chain - * @return list - */ - private function markChangedHops(array $chain): array - { - return array_map(fn (Hop $hop): Hop => $this->markHop($hop), $chain); - } + if (!$anyChanged) { + return null; + } - private function markHop(Hop $hop): Hop - { - return new Hop($hop->fqmn, $hop->class, $hop->file, $hop->line, $hop->forwardLine, $this->hopMatches($hop)); + return $finding->withChain($markedChain); } private function hopMatches(Hop $hop): bool @@ -82,16 +65,4 @@ private function hopMatches(Hop $hop): bool return $this->changedLines->containsLine($hop->file, $hop->line) || $this->changedLines->containsLine($hop->file, $hop->forwardLine); } - - /** @param list $chain */ - private function hasChangedHop(array $chain): bool - { - foreach ($chain as $hop) { - if ($hop->changed) { - return true; - } - } - - return false; - } } diff --git a/src/Report/GithubReporter.php b/src/Report/GithubReporter.php index 2bc4f56..f52b484 100644 --- a/src/Report/GithubReporter.php +++ b/src/Report/GithubReporter.php @@ -59,10 +59,12 @@ public function render(array $findings): string */ private function annotationLines(Finding $finding, Severity $severity): array { - $anchorIndex = $this->anchorIndex($finding); + $nonTerminalHops = $this->nonTerminalHops($finding); + $anchorIndex = $this->anchorIndex($nonTerminalHops); $lines = [$this->anchorAnnotation($finding, $severity, $anchorIndex)]; - foreach ($this->noticeHops($finding, $anchorIndex) as $index => $hop) { + unset($nonTerminalHops[$anchorIndex]); + foreach ($nonTerminalHops as $index => $hop) { $lines[] = $this->hopNotice($finding, $hop, $index); } @@ -103,10 +105,12 @@ private function hopNotice(Finding $finding, Hop $hop, int $index): string * non-terminal hops (0 .. hops-1). Falls back to the origin (index 0) when * none is marked, which is the only case a normal (non diff-aware) run * ever produces. + * + * @param array $nonTerminalHops */ - private function anchorIndex(Finding $finding): int + private function anchorIndex(array $nonTerminalHops): int { - foreach ($this->nonTerminalHops($finding) as $index => $hop) { + foreach ($nonTerminalHops as $index => $hop) { if ($hop->changed) { return $index; } @@ -115,18 +119,6 @@ private function anchorIndex(Finding $finding): int return 0; } - /** - * @return array the anchor hop excluded, remaining non-terminal - * hops in chain order, keyed by their chain index - */ - private function noticeHops(Finding $finding, int $anchorIndex): array - { - $hops = $this->nonTerminalHops($finding); - unset($hops[$anchorIndex]); - - return $hops; - } - /** * @return array non-terminal-hop entries keyed by their chain * index (0 .. hops-1) — the terminal node, at