From 8296f46e02b73551a4ec4dd09914d206d02b1f57 Mon Sep 17 00:00:00 2001 From: Lars Pohlmann Date: Tue, 11 Aug 2026 10:27:19 +0200 Subject: [PATCH 01/26] docs(#5): expand Phase 3 into a step-by-step plan Written against the shipped Phase 2 interfaces (Finding/Hop/TerminalKind, the ArgvParser that already parses --warn-limit and --format, the harness's direct-ChainBuilder findings mode). Pins every reporter contract byte-for-byte, decides the config precedence mechanic (parser-default seeding, all-or-nothing path override) and the suppression semantics (short-name attribute match, comment placement, any-hop chain filter). --- docs/plan.md | 8 +- docs/plans/phase-3.md | 449 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 456 insertions(+), 1 deletion(-) create mode 100644 docs/plans/phase-3.md diff --git a/docs/plan.md b/docs/plan.md index 1d0cd37..de091cf 100644 --- a/docs/plan.md +++ b/docs/plan.md @@ -680,10 +680,16 @@ Tasks (expanded in `docs/plans/phase-2.md`): --- -## Phase 3 — CI & config surface +## Phase 3 — CI & config surface (in progress — issue #5) **Deliverable:** all output formats, config file, suppression, dual thresholds, summary. +**Detailed step-by-step plan: [docs/plans/phase-3.md](plans/phase-3.md)** — written +against the actual Phase 1–2 interfaces; where it refines the sketches below (e.g. +`Severity`/`Thresholds` as reporting-layer concepts, config precedence via +parser-default seeding, suppression exposed through `MethodIndex::suppressions()`), +the detailed plan wins. + - **3.1 ConfigLoader:** `phptramp.json` then `phptramp.dist.json`; keys `paths`, `exclude`, `limit`, `warnLimit`, `format`, `baseline`; strict unknown-key error; precedence CLI > config > defaults (merge into `Options`). `exclude` glob patterns diff --git a/docs/plans/phase-3.md b/docs/plans/phase-3.md new file mode 100644 index 0000000..2529c09 --- /dev/null +++ b/docs/plans/phase-3.md @@ -0,0 +1,449 @@ +# Phase 3 Implementation Plan — CI & config surface + +> **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 3 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:** phptramp becomes CI-shaped: machine formats (`json`, `github`, +`checkstyle`, `sarif`), a `phptramp.json` config file with CLI-over-config +precedence and `exclude` globs, `#[TrampIgnore]` / `// phptramp-ignore` +suppression, `--warn-limit` warning severity, and `--format=summary`. + +**Architecture:** The chain layer stays untouched except for one filter hook +(suppression). New `src/Config/` loads and merges the config file *under* CLI +flags by seeding `ArgvParser` with config-derived defaults. `src/Report/` grows a +`Reporter` interface, a `Thresholds`/`Severity` pair (severity is a *reporting* +concept — `Finding` stays threshold-agnostic), and one reporter per format behind +a factory. Suppression is collected during indexing (attributes + comments are +only visible there) and exposed via `MethodIndex::suppressions()` so existing +call sites keep working. + +**Tech stack:** unchanged — PHP ≥ 8.2, nikic/php-parser ^5 (only prod dependency). + +## 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: #5.** Branch `feature/5-ci-config-surface` off + `develop` (already created, carries this plan). Commits `type(#5): …`. PR into + `develop` with `Closes #5`; diff-scoped Infection gate (minMsi 80) applies. +- Frozen semantics rule 11 (suppression) is the contract for Task 7. Reporter + output contracts pinned in this plan are byte-for-byte — a fixture asserts each. +- Exit-code contract unchanged: `0` no errors, `1` ≥ 1 error-severity finding, + `2` tool error. Warnings never affect the exit code. + +## Existing interfaces this phase consumes (Phase 1–2, verbatim) + +- `Finding{ string $param, string $origin, ?string $terminal, TerminalKind $terminalKind, int $hops, list $chain, int $classes, list $notes, list $trace }` + — chain includes the terminal node only for kinds `used|stored|&-terminated|unused-end`. +- `Hop{ string $fqmn, ?string $class, string $file, int $line, ?int $forwardLine }` + — `file` is realpath-absolute; `forwardLine` null on the terminal node. +- `TerminalKind: string { Used='used', Stored='stored', ByRef='&-terminated', Unused='unused-end', External='external', Truncated='truncated' }`. +- `ChainBuilder::__construct(CallResolver)`, `::build(MethodIndex): list` + (unfiltered; delegates to `ChainTraversal`). +- `TextReporter::__construct(int $limit, bool $explain = false)`, + `::render(list): string` — output format pinned in + `tests/Report/TextReporterTest.php`. +- `ArgvParser::parse(list $args): Options` — already accepts + `--warn-limit`, `--format` with the six valid values, `--baseline`; unknown + flags/formats throw `InvalidArgsException`. `Options` already carries + `warnLimit`, `format`, `baseline` (all currently unused downstream). +- `Application::analyze()` currently hard-rejects `format !== 'text'` with exit 2 + — that guard dies in Task 2. +- `FileLocator::locate(Options): list` — folders + files, realpath, + sorted; no exclude support yet. +- `MethodIndex{ get(), all(), classInfo() }`, built solely by `Indexer::index()`. +- Fixture harness `tests/FixtureTest.php`: `expected-index.json` compares + `--dump-index` through the CLI; `expected-findings.json` currently calls + `ChainBuilder` directly (flattened chains) — Task 3 switches it to the CLI. + +## File structure (this phase) + +``` +src/Report/Severity.php NEW enum error|warning (Task 1) +src/Report/Thresholds.php NEW limit + warnLimit -> severityOf(Finding) (Task 1) +src/Report/Reporter.php NEW interface (Task 1) +src/Report/ReporterFactory.php NEW format string -> Reporter (Task 1) +src/Report/TextReporter.php MOD implements Reporter; renders warnings (Task 1) +src/Console/Application.php MOD thresholds + factory wiring; exit on errors only (Task 1) +src/Report/Paths.php NEW cwd-relativizing helper for all reporters (Task 2) +src/Report/JsonReporter.php NEW (Task 2) +tests/FixtureTest.php MOD findings mode goes through the CLI (Task 3) +src/Config/ConfigException.php NEW (Task 4) +src/Config/ConfigLoader.php NEW phptramp.json -> seed Options (Task 4) +src/Console/ArgvParser.php MOD parse(args, Options $defaults) (Task 4) +src/Console/Options.php MOD + readonly list $exclude (Task 5) +src/Discovery/FileLocator.php MOD exclude glob filter (Task 5) +src/Report/GithubReporter.php NEW (Task 6) +src/Report/CheckstyleReporter.php NEW (Task 6) +src/Report/SarifReporter.php NEW (Task 6) +src/Ignore/TrampIgnore.php NEW the attribute class (Task 7) +src/Ignore/SuppressionIndex.php NEW (Task 7) +src/Index/IndexingVisitor.php MOD collect attributes + ignore comments (Task 7) +src/Index/MethodIndex.php MOD + suppressions(): SuppressionIndex (Task 7) +src/Chain/ChainTraversal.php MOD drop chains with a suppressed hop (Task 7) +src/Report/SummaryReporter.php NEW (Task 8) +tests/… mirrors all of the above +tests/fixtures/… format/config/suppression cases (Tasks 3, 9) +``` + +--- + +### Task 1: `Severity`, `Thresholds`, `Reporter` interface, warning rendering + +`--warn-limit` becomes real. Severity is computed at reporting time so `Finding` +stays a pure chain fact. + +**Files:** create `src/Report/{Severity,Thresholds,Reporter,ReporterFactory}.php`; +modify `src/Report/TextReporter.php`, `src/Console/Application.php`; +tests `tests/Report/ThresholdsTest.php`, extend `TextReporterTest`, +`ApplicationTest`. + +**Produces:** + +```php +enum Severity +{ + case Error; + case Warning; +} + +final class Thresholds +{ + /** @throws InvalidArgsException if warnLimit >= limit (a warn bar above the fail bar is a config error) */ + public function __construct( + public readonly int $limit, + public readonly ?int $warnLimit, + ) {} + + /** Null = below every threshold: not reported at all. */ + public function severityOf(Finding $finding): ?Severity; +} + +interface Reporter +{ + /** @param list $findings ALL findings, unfiltered — each reporter applies $thresholds itself */ + public function render(array $findings): string; +} + +final class ReporterFactory +{ + /** Constructed from Options; returns the Reporter for Options->format. */ + public function create(Options $options): Reporter; +} +``` + +Behavior: + +- `severityOf`: hops ≥ limit → `Error`; warnLimit set and hops ≥ warnLimit → + `Warning`; else null. +- `TextReporter` header for warnings is `WARNING` instead of `FINDING`; summary + line becomes `N findings (E errors, W warnings; limit: 3 hops, warn-limit: 2 hops).` + — with warnLimit unset, existing pinned output must NOT change (regression + guard: the current `TextReporterTest` expectations stay green untouched). +- `Application`: exit 1 iff any `Error`; reporters receive unfiltered findings. + The `format !== 'text'` guard stays for exactly the formats the factory cannot + build *yet* (it shrinks task by task and dies in Task 6). + +- [ ] **Step 1:** failing tests — `ThresholdsTest` (error at limit, warning in + `[warn, limit)`, null below, `warnLimit >= limit` throws); `TextReporterTest` + warning block + mixed summary line; `ApplicationTest`: warn-only run exits 0 + but prints `WARNING`. +- [ ] **Step 2:** red. **Step 3:** implement. **Step 4:** green + `composer check`. +- [ ] **Step 5:** commit `feat(#5): dual thresholds with warning severity`. + +### Task 2: `JsonReporter` + `Paths` + +**Files:** create `src/Report/{Paths,JsonReporter}.php`; +test `tests/Report/JsonReporterTest.php`. + +**Produces:** `Paths::relativize(string $absolute): string` — path relative to the +cwd passed at construction when the path lies under it, else unchanged. Every +machine reporter uses it (GitHub annotations and SARIF need workspace-relative +paths; JSON follows for consistency). + +**Output contract (pin byte-for-byte;** `JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES`, +trailing newline**):** + +```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}, + {"method": "Demo\\ServiceA::process", "role": "hop", "file": "src/Demo.php", "line": 18, "forwardLine": 20}, + {"method": "Demo\\ServiceB::run", "role": "hop", "file": "src/Demo.php", "line": 24, "forwardLine": 26}, + {"method": "Demo\\Mailer::__construct", "role": "terminal", "file": "src/Demo.php", "line": 32, "forwardLine": null} + ], + "notes": [] + } + ] +} +``` + +`warnLimit` renders as JSON `null` when unset. Roles: first chain entry `origin`, +last `terminal` only when the terminal node is in the chain (see `Finding` +contract), everything else `hop`. Findings below every threshold are omitted. +`severity` is `"error"` or `"warning"`. Empty run → `"findings": []`. + +- [ ] Steps: failing test (full document for the 3-hop case, warning case, empty + case) → red → implement → green + check → + commit `feat(#5): json reporter`. + +### Task 3: Fixture harness through the CLI + +**Files:** modify `tests/FixtureTest.php`; rewrite every +`tests/fixtures/*/expected-findings.json` to the Task 2 schema. + +The findings mode now invokes `Application` with +`['phptramp', '--folder', , '--format', 'json', '--limit', '1']` — real CLI, +real exit codes (`--limit 1` so every chain is an error and short chains stay +visible). Normalization stays: strip the case-dir prefix from `file` values. +Assert exit 1 when findings exist, 0 when not. + +- [ ] Steps: adapt the harness test-first (red on the first fixture), rewrite the + seven expected files by hand (do NOT generate them from the tool's own output — + a wrong tool would then pin its own bug; derive them from the fixture source), + green + check → commit `test(#5): fixture harness exercises the CLI json path`. + +### Task 4: `ConfigLoader` + precedence + +**Files:** create `src/Config/{ConfigException,ConfigLoader}.php`; modify +`src/Console/ArgvParser.php`, `src/Console/Application.php`; +tests `tests/Config/ConfigLoaderTest.php`, extend `ArgvParserTest`, +`ApplicationTest`. + +**Produces:** + +```php +final class ConfigLoader +{ + /** + * Reads phptramp.json, falling back to phptramp.dist.json, in $directory. + * Missing both -> Options defaults. Unknown key, non-JSON, or wrong value + * type -> ConfigException (exit 2). Never consults the environment. + */ + public function load(string $directory): Options; +} +``` + +- Recognized keys, mapped onto `Options`: `paths` (list of strings; entries + ending `.php` become `files`, all others `folders`), `exclude` (list of glob + strings — storage arrives in Task 5, the loader already accepts the key), + `limit`, `warnLimit`, `format`, `baseline`. Anything else → + `ConfigException("unknown config key: ")` — strict by design, typos must + not silently disable gating. +- **Precedence mechanic (CLI > config > defaults):** + `ArgvParser::parse(array $args, Options $defaults = new Options())` — the + parser's `reset()` seeds every field from `$defaults` instead of literals. Path + precedence is all-or-nothing: the first `--folder`/`--file`/`--files` flag + clears *both* seeded lists (config paths replaced entirely, never mixed). +- `Application` wires `ConfigLoader->load(getcwd())` as the parser's defaults; + a `ConfigException` prints to stderr and exits 2. + +- [ ] **Step 1:** failing tests — loader: reads `phptramp.json`; falls back to + `.dist`; prefers non-dist; missing → defaults; unknown key throws; `paths` + splits files/folders; wrong type (`"limit": "three"`) throws. Parser: seeded + defaults survive when flags absent; `--limit` overrides seed; first path flag + clears seeded paths. Application (chdir into a temp dir in try/finally): + config `limit: 1` makes a 1-hop chain fail without any CLI flag. +- [ ] **Step 2:** red. **Step 3:** implement. **Step 4:** green + check. +- [ ] **Step 5:** commit `feat(#5): phptramp.json config with CLI-over-config precedence`. + +### Task 5: `exclude` globs in `FileLocator` + +**Files:** modify `src/Console/Options.php` (add +`public readonly array $exclude = []`, `@param list`), +`src/Config/ConfigLoader.php` (store the key), `src/Discovery/FileLocator.php`; +tests extend `FileLocatorTest`, `ConfigLoaderTest`. + +Config-only (no CLI flag — YAGNI until asked; the master plan lists none). +Semantics, pinned: patterns are `fnmatch` globs with `FNM_PATHNAME` **not** set +(so `vendor/*` matches arbitrarily deep), matched against the path *relative to +the config directory* (= cwd), after realpath normalization. A file matching any +pattern is dropped after collection, before sorting. + +- [ ] Steps: failing tests (`vendor/*` drops nested file; `*Test.php` drops by + basename; explicit `--file` beats exclude — an explicitly named file is always + analyzed) → red → implement → green + check → + commit `feat(#5): exclude globs from config`. + +### Task 6: `GithubReporter`, `CheckstyleReporter`, `SarifReporter` + +**Files:** create the three reporters; register in `ReporterFactory` (the last +format guard in `Application` dies); tests one file per reporter. + +**GitHub contract (pin):** one annotation per finding at the origin, `::error` / +`::warning` by severity, plus one `::notice` per subsequent hop. Message data +escaping per Actions spec: `%` → `%25`, `\r` → `%0D`, `\n` → `%0A`. + +``` +::error file=src/Demo.php,line=12,title=phptramp::$config: 3 pass-through hops across 4 classes (terminal: Demo\Mailer::__construct [stored]) +::notice file=src/Demo.php,line=18,title=phptramp::hop 2 of $config chain from Demo\Controller::handle +``` + +Empty run → empty string, exit 0. + +**Checkstyle contract (pin):** findings grouped by origin file, one `` per +finding at the origin line, `source="phptramp.trampData"`, message XML-escaped +(`htmlspecialchars(..., ENT_XML1 | ENT_QUOTES)`): + +```xml + + + + + + +``` + +**SARIF contract (pin):** SARIF 2.1.0, one run, driver `name` `phptramp`, +`informationUri` `https://github.com/larspohlmann/phptramp`, **no `version` +property** (optional per spec; omitting keeps fixtures stable across releases), +one rule `phptramp.trampData`. One result per finding: `ruleId`, `level` +(`error`/`warning`), `message.text` (same message as checkstyle), one location at +the origin (`artifactLocation.uri` relative, `region.startLine`), and one +`relatedLocations` entry per further hop. Encoded +`JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES`. + +- [ ] Steps per reporter (three sub-cycles, ONE commit each): failing exact-string + test (error case, warning case, empty case) → red → implement → green + check → + commits `feat(#5): github actions reporter`, `feat(#5): checkstyle reporter`, + `feat(#5): sarif reporter`. + +### Task 7: Suppression + +**Files:** create `src/Ignore/{TrampIgnore,SuppressionIndex}.php`; modify +`src/Index/IndexingVisitor.php`, `src/Index/MethodIndex.php` (constructor gains +`SuppressionIndex $suppressions = new SuppressionIndex([], [], [])` — additive, +existing call sites keep working — plus `suppressions(): SuppressionIndex`), +`src/Chain/ChainTraversal.php`; tests `tests/Ignore/SuppressionIndexTest.php`, +extend `ChainBuilderTest`. + +**Produces:** + +```php +#[\Attribute(\Attribute::TARGET_CLASS | \Attribute::TARGET_METHOD | \Attribute::TARGET_FUNCTION | \Attribute::TARGET_PARAMETER)] +final class TrampIgnore +{ +} + +final class SuppressionIndex +{ + /** + * @param list $methods suppressed method FQMNs (method- or class-level attribute) + * @param list $params [fqmn, paramName] pairs + * @param array> $lines file -> lines carrying an ignore comment + */ + public function __construct(...); + + public function suppressesMethod(string $fqmn): bool; + public function suppressesParam(string $fqmn, string $param): bool; + public function suppressesLine(string $file, int $line): bool; +} +``` + +Semantics (frozen rule 11, made concrete): + +- **Attribute matching is by short name:** any attribute whose name's last + segment is `TrampIgnore` counts — analyzed codebases must not be forced to + autoload our exact class. Class-level attribute suppresses every method of the + class; method-/function-level every param of the method; param-level that param. +- **Comment form:** a `// phptramp-ignore` line comment (anywhere in the line, + e.g. after code) suppresses when it sits on the method's declaration line, the + line directly above it (doc-comment style), or on a forwarding call-site line + (suppressing chains through that specific edge). +- **Chain filter (in `ChainTraversal`):** a finding is dropped when ANY of its + hop nodes is suppressed — method-suppressed, param-suppressed for the chain's + param at that hop, or the hop's `forwardLine`/declaration line is + comment-suppressed. Terminal nodes never suppress (they are not hops). +- Stale-suppression *detection* is Phase 5 — do not build it here. + +- [ ] **Step 1:** failing tests — `SuppressionIndexTest` for the three lookups; + `ChainBuilderTest`: attribute on middle hop kills the chain; attribute on the + param only kills chains of that param; class-level kills all chains through the + class; ignore comment on the forwarding line kills the chain; comment on an + unrelated line does not; the README 3-hop chain without suppressions is still + found (regression). +- [ ] **Step 2:** red. **Step 3:** implement (visitor collects during the existing + traversal — attributes from `attrGroups`, comments via `getComments()` + + line scan). **Step 4:** green + check. +- [ ] **Step 5:** commit `feat(#5): TrampIgnore attribute and comment suppression`. + +### Task 8: `SummaryReporter` + +**Files:** create `src/Report/SummaryReporter.php`; register in factory; +test `tests/Report/SummaryReporterTest.php`. + +Summary is a whole-codebase overview: it renders **all** findings regardless of +thresholds (the point is triaging a legacy codebase before choosing a limit). +The exit code still follows the normal error rule. Contract (pin): + +``` +Chains by length: + 1 hop ######## 8 + 2 hops #### 4 + 3 hops ## 2 + +Top 10 longest chains: + 3 hops $config Demo\Controller::handle -> Demo\Mailer::__construct [stored] + 3 hops $request Demo\Kernel::boot -> external + +Most-forwarded parameters: + 6x $config + 3x $request + +14 chains total; 2 at or over the limit (limit: 3 hops). +``` + +Bars scale to the widest bucket = 40 `#` max; counts sorted descending, ties +alphabetical; top-10 ties broken by origin FQMN. + +- [ ] Steps: failing exact-string test (mixed case + empty case: + `No chains found.`) → red → implement → green + check → + commit `feat(#5): summary report`. + +### Task 9: End-to-end fixtures, docs, phase close-out + +**Files:** new fixture dirs; modify `README.md`, `docs/plan.md` (status), +`src/Console/Application.php` help text only if wording drifted. + +- **Fixtures:** `warn-vs-error` (chains of 2 and 3 hops; `expected-findings.json` + run with `--limit 3 --warn-limit 2` — extend the harness to read optional + per-fixture CLI args from a `phptramp-args.json` next to the expectation + file); `suppressed-chain` (attribute + comment variants, expects zero + findings); `config-driven` (a `phptramp.json` inside the fixture exercising + `paths` + `exclude` + `limit`). +- **README:** status → Phase 3; document config file, formats (one-line example + each), suppression, `--warn-limit`, and the `composer tramp` script again + against real behavior. +- **docs/plan.md:** check off Phase 3 bullets, mark the phase ✅. + +- [ ] Steps: harness extension + fixtures test-first → green + check → + commit `test(#5): format, config and suppression fixtures` → docs → + commit `docs(#5): phase 3 status and usage docs`. + +--- + +## Done when + +1. Every task committed on `feature/5-ci-config-surface`; `composer check` green + at every commit; `composer infection:diff` ≥ 80 MSI locally (`git fetch origin + main` first). +2. All Phase 3 bullets in `docs/plan.md` checked off; README/help text match + actual behavior; every reporter contract has an exact-string test AND a + fixture. +3. PR into `develop` with `Closes #5`; CI (matrix + static + mutation) green; + after merge, verify issue #5 auto-closed. From 2841cc5e0cadd21eb63a726a02e4c65def24087a Mon Sep 17 00:00:00 2001 From: Lars Pohlmann Date: Tue, 11 Aug 2026 10:45:38 +0200 Subject: [PATCH 02/26] feat(#5): dual thresholds with warning severity Severity is computed at reporting time (Thresholds::severityOf) so Finding stays a pure chain fact. Introduces the Reporter interface and ReporterFactory as the seam for later machine reporters; TextReporter now renders WARNING blocks and the combined errors/warnings summary when --warn-limit is set, and filters out findings below every threshold. Application builds the reporter through the factory and exits 1 only when a finding's severity is Error. --- src/Console/Application.php | 38 +++++++---- src/Report/Reporter.php | 16 +++++ src/Report/ReporterFactory.php | 21 ++++++ src/Report/Severity.php | 11 ++++ src/Report/TextReporter.php | 69 ++++++++++++++++---- src/Report/Thresholds.php | 45 +++++++++++++ tests/Console/ApplicationTest.php | 26 ++++++++ tests/Report/TextReporterTest.php | 104 +++++++++++++++++++++++++++--- tests/Report/ThresholdsTest.php | 69 ++++++++++++++++++++ 9 files changed, 365 insertions(+), 34 deletions(-) create mode 100644 src/Report/Reporter.php create mode 100644 src/Report/ReporterFactory.php create mode 100644 src/Report/Severity.php create mode 100644 src/Report/Thresholds.php create mode 100644 tests/Report/ThresholdsTest.php diff --git a/src/Console/Application.php b/src/Console/Application.php index b187004..6f16add 100644 --- a/src/Console/Application.php +++ b/src/Console/Application.php @@ -12,7 +12,9 @@ use PhpTramp\Index\MethodIndex; use PhpTramp\Index\ParamInfo; use PhpTramp\Index\ParseException; -use PhpTramp\Report\TextReporter; +use PhpTramp\Report\ReporterFactory; +use PhpTramp\Report\Severity; +use PhpTramp\Report\Thresholds; use PhpTramp\Resolve\CallResolver; use PhpTramp\Resolve\ClassHierarchy; @@ -73,24 +75,35 @@ public function run(array $argv): int private function analyze(Options $options): int { - if ($options->format !== 'text') { - fwrite($this->stderr, "phptramp: format '{$options->format}' is not implemented until Phase 3.\n"); - - return 2; - } - try { $index = $this->buildIndex($options); + $reporter = (new ReporterFactory())->create($options); } catch (InvalidArgsException | ParseException $e) { fwrite($this->stderr, 'phptramp: ' . $e->getMessage() . "\n"); return 2; } - $findings = $this->findChains($index, $options->limit); - fwrite($this->stdout, (new TextReporter($options->limit, $options->explain))->render($findings)); + $findings = $this->findChains($index); + fwrite($this->stdout, $reporter->render($findings)); + + $thresholds = new Thresholds($options->limit, $options->warnLimit); + + return $this->hasError($findings, $thresholds) ? 1 : 0; + } + + /** + * @param list $findings + */ + private function hasError(array $findings, Thresholds $thresholds): bool + { + foreach ($findings as $finding) { + if ($thresholds->severityOf($finding) === Severity::Error) { + return true; + } + } - return $findings === [] ? 0 : 1; + return false; } private function buildIndex(Options $options): MethodIndex @@ -103,12 +116,11 @@ private function buildIndex(Options $options): MethodIndex /** * @return list */ - private function findChains(MethodIndex $index, int $limit): array + private function findChains(MethodIndex $index): array { $resolver = new CallResolver($index, new ClassHierarchy($index)); - $findings = (new ChainBuilder($resolver))->build($index); - return array_values(array_filter($findings, static fn (Finding $finding): bool => $finding->hops >= $limit)); + return (new ChainBuilder($resolver))->build($index); } private function dumpIndex(Options $options): int diff --git a/src/Report/Reporter.php b/src/Report/Reporter.php new file mode 100644 index 0000000..e54b516 --- /dev/null +++ b/src/Report/Reporter.php @@ -0,0 +1,16 @@ + $findings ALL findings, unfiltered — each reporter + * applies its Thresholds itself + */ + public function render(array $findings): string; +} diff --git a/src/Report/ReporterFactory.php b/src/Report/ReporterFactory.php new file mode 100644 index 0000000..12a4a9f --- /dev/null +++ b/src/Report/ReporterFactory.php @@ -0,0 +1,21 @@ +limit, $options->warnLimit); + + return match ($options->format) { + 'text' => new TextReporter($thresholds, $options->explain), + default => throw new InvalidArgsException("format '{$options->format}' is not implemented yet."), + }; + } +} diff --git a/src/Report/Severity.php b/src/Report/Severity.php new file mode 100644 index 0000000..3eea41b --- /dev/null +++ b/src/Report/Severity.php @@ -0,0 +1,11 @@ + $findings already filtered to hops >= limit + * @param list $findings ALL findings, unfiltered */ public function render(array $findings): string { - if ($findings === []) { + $reportable = $this->reportableFindings($findings); + if ($reportable === []) { return 'No tramp data found (' . $this->limitClause() . ").\n"; } - $blocks = array_map($this->renderFinding(...), $findings); + $blocks = array_map( + fn (array $reportableFinding): string => $this->renderFinding(...$reportableFinding), + $reportable, + ); - return implode("\n\n", $blocks) . "\n\n" . $this->summary(count($findings)) . "\n"; + return implode("\n\n", $blocks) . "\n\n" . $this->summary($reportable) . "\n"; } - private function renderFinding(Finding $finding): string + /** + * @param list $findings + * @return list + */ + private function reportableFindings(array $findings): array + { + $reportable = []; + foreach ($findings as $finding) { + $severity = $this->thresholds->severityOf($finding); + if ($severity !== null) { + $reportable[] = [$finding, $severity]; + } + } + + return $reportable; + } + + private function renderFinding(Finding $finding, Severity $severity): string { $hopRows = $this->hopRows($finding); $labelWidth = $this->labelWidth($hopRows, $finding->notes); $methodWidth = $this->methodWidth($hopRows); - $lines = [$this->header($finding)]; + $lines = [$this->header($finding, $severity)]; foreach ($hopRows as $row) { $lines[] = $this->hopLine($row, $labelWidth, $methodWidth); } @@ -161,21 +182,43 @@ private function methodWidth(array $rows): int return max($widths); } - private function header(Finding $finding): string + private function header(Finding $finding, Severity $severity): string { - return 'FINDING $' . $finding->param . ': ' + $keyword = $severity === Severity::Warning ? 'WARNING' : 'FINDING'; + + return $keyword . ' $' . $finding->param . ': ' . $finding->hops . ' pass-through ' . $this->plural($finding->hops, 'hop') . ' across ' . $finding->classes . ' ' . $this->plural($finding->classes, 'class', 'classes'); } - private function summary(int $count): string + /** + * @param list $reportable + */ + private function summary(array $reportable): string { - return $count . ' ' . $this->plural($count, 'finding') . ' (' . $this->limitClause() . ').'; + $count = count($reportable); + if ($this->thresholds->warnLimit === null) { + return $count . ' ' . $this->plural($count, 'finding') . ' (' . $this->limitClause() . ').'; + } + + $errorCount = count(array_filter($reportable, static fn (array $entry): bool => $entry[1] === Severity::Error)); + $warningCount = $count - $errorCount; + + return $count . ' ' . $this->plural($count, 'finding') . ' (' + . $errorCount . ' ' . $this->plural($errorCount, 'error') . ', ' + . $warningCount . ' ' . $this->plural($warningCount, 'warning') . '; ' + . $this->limitClause() . ').'; } private function limitClause(): string { - return 'limit: ' . $this->limit . ' ' . $this->plural($this->limit, 'hop'); + $clause = 'limit: ' . $this->thresholds->limit . ' ' . $this->plural($this->thresholds->limit, 'hop'); + if ($this->thresholds->warnLimit !== null) { + $clause .= ', warn-limit: ' . $this->thresholds->warnLimit + . ' ' . $this->plural($this->thresholds->warnLimit, 'hop'); + } + + return $clause; } private function plural(int $count, string $singular, ?string $plural = null): string diff --git a/src/Report/Thresholds.php b/src/Report/Thresholds.php new file mode 100644 index 0000000..0afa440 --- /dev/null +++ b/src/Report/Thresholds.php @@ -0,0 +1,45 @@ +warnLimit !== null && $this->warnLimit >= $this->limit) { + throw new InvalidArgsException( + "warn-limit ({$this->warnLimit}) must be lower than limit ({$this->limit}).", + ); + } + } + + /** Null = below every threshold: not reported at all. */ + public function severityOf(Finding $finding): ?Severity + { + if ($finding->hops >= $this->limit) { + return Severity::Error; + } + if ($this->warnLimit !== null && $finding->hops >= $this->warnLimit) { + return Severity::Warning; + } + + return null; + } +} diff --git a/tests/Console/ApplicationTest.php b/tests/Console/ApplicationTest.php index 4f7ed30..721f3ab 100644 --- a/tests/Console/ApplicationTest.php +++ b/tests/Console/ApplicationTest.php @@ -106,6 +106,32 @@ public function testUnsupportedFormatExitsWithToolErrorCode(): void self::assertSame('', self::contents($this->stdout)); } + public function testWarnOnlyRunExitsZeroButPrintsWarning(): void + { + $folder = $this->fixtureWithTwoHopChain(); + + self::assertSame( + 0, + $this->app->run(['phptramp', '--folder', $folder, '--limit', '3', '--warn-limit', '2']), + ); + self::assertStringContainsString('WARNING', self::contents($this->stdout)); + } + + private function fixtureWithTwoHopChain(): string + { + $folder = sys_get_temp_dir() . '/phptramp-app-' . uniqid(); + mkdir($folder); + $this->folders[] = $folder; + + $code = 'process($config); } } ' + . 'class ServiceA { public function process(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', $code); + + return $folder; + } + private function fixtureWithThreeHopChain(): string { $folder = sys_get_temp_dir() . '/phptramp-app-' . uniqid(); diff --git a/tests/Report/TextReporterTest.php b/tests/Report/TextReporterTest.php index 5fa00f1..355419c 100644 --- a/tests/Report/TextReporterTest.php +++ b/tests/Report/TextReporterTest.php @@ -8,6 +8,7 @@ use PhpTramp\Chain\Hop; use PhpTramp\Chain\TerminalKind; use PhpTramp\Report\TextReporter; +use PhpTramp\Report\Thresholds; use PHPUnit\Framework\TestCase; final class TextReporterTest extends TestCase @@ -43,7 +44,7 @@ public function testRendersStoredChainWithAlignedColumns(): void TXT; - self::assertSame($expected, (new TextReporter(3))->render([$finding])); + self::assertSame($expected, (new TextReporter(new Thresholds(3, null)))->render([$finding])); } public function testRendersTruncatedChainWithNoteLineAndNoTerminal(): void @@ -70,11 +71,11 @@ public function testRendersTruncatedChainWithNoteLineAndNoTerminal(): void hop 2 Demo\B::step($cfg) src/B.php:9 note truncated: 2 implementations - 1 finding (limit: 3 hops). + 1 finding (limit: 2 hops). TXT; - self::assertSame($expected, (new TextReporter(3))->render([$finding])); + self::assertSame($expected, (new TextReporter(new Thresholds(2, null)))->render([$finding])); } public function testRendersSingularFormsForOneHopOneClassOneFinding(): void @@ -94,7 +95,7 @@ public function testRendersSingularFormsForOneHopOneClassOneFinding(): void TXT; - self::assertSame($expected, (new TextReporter(1))->render([$finding])); + self::assertSame($expected, (new TextReporter(new Thresholds(1, null)))->render([$finding])); } public function testExplainAppendsTraceLinesUnderHeader(): void @@ -113,11 +114,11 @@ public function testExplainAppendsTraceLinesUnderHeader(): void explain: Demo\A::go: method:step -> Demo\B::step - 1 finding (limit: 3 hops). + 1 finding (limit: 1 hop). TXT; - self::assertSame($expected, (new TextReporter(3, true))->render([$finding])); + self::assertSame($expected, (new TextReporter(new Thresholds(1, null), true))->render([$finding])); } public function testExplainDisabledOmitsTraceEvenWhenPresent(): void @@ -129,14 +130,101 @@ public function testExplainDisabledOmitsTraceEvenWhenPresent(): void $trace = ['Demo\A::go: method:step -> Demo\B::step']; $finding = new Finding('p', 'Demo\A::go', 'Demo\B::step', TerminalKind::Used, 1, $chain, 2, [], $trace); - self::assertStringNotContainsString('explain:', (new TextReporter(3))->render([$finding])); + $reporter = new TextReporter(new Thresholds(1, null)); + + self::assertStringNotContainsString('explain:', $reporter->render([$finding])); } public function testEmptyFindingsRendersReassuringLine(): void { self::assertSame( "No tramp data found (limit: 3 hops).\n", - (new TextReporter(3))->render([]), + (new TextReporter(new Thresholds(3, null)))->render([]), + ); + } + + public function testRendersSingleWarningFindingAndFiltersBelowWarnLimit(): void + { + $chain = [ + 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), + ]; + $warningFinding = new Finding('p', 'Demo\A::go', 'Demo\C::sink', TerminalKind::Used, 2, $chain, 2, [], []); + $belowWarnLimitChain = [ + new Hop('Demo\C::go', 'Demo\C', 'src/C.php', 5, null), + ]; + $belowWarnLimitFinding = new Finding( + 'q', + 'Demo\C::go', + 'Demo\C::go', + TerminalKind::Used, + 1, + $belowWarnLimitChain, + 1, + [], + [], + ); + + $expected = <<<'TXT' + WARNING $p: 2 pass-through hops across 2 classes + origin Demo\A::go($p) src/A.php:5 + hop 2 Demo\B::step($p) src/B.php:9 + terminal Demo\C::sink($p) src/C.php:13 (used) + + 1 finding (0 errors, 1 warning; limit: 3 hops, warn-limit: 2 hops). + + TXT; + + $reporter = new TextReporter(new Thresholds(3, 2)); + + self::assertSame($expected, $reporter->render([$warningFinding, $belowWarnLimitFinding])); + } + + public function testRendersMixedErrorAndWarningFindingsWithCombinedSummary(): void + { + $errorChain = [ + 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\Y::step', 'Demo\Y', 'src/Y.php', 15, 17), + new Hop('Demo\C::sink', 'Demo\C', 'src/C.php', 13, null), + ]; + $errorFinding = new Finding('p', 'Demo\A::go', 'Demo\C::sink', TerminalKind::Used, 3, $errorChain, 3, [], []); + $warningChain = [ + new Hop('Demo\D::go', 'Demo\D', 'src/D.php', 5, 7), + new Hop('Demo\E::step', 'Demo\E', 'src/E.php', 9, 11), + new Hop('Demo\F::sink', 'Demo\F', 'src/F.php', 13, null), + ]; + $warningFinding = new Finding( + 'q', + 'Demo\D::go', + 'Demo\F::sink', + TerminalKind::Used, + 2, + $warningChain, + 2, + [], + [], ); + + $expected = <<<'TXT' + FINDING $p: 3 pass-through hops across 3 classes + origin Demo\A::go($p) src/A.php:5 + hop 2 Demo\B::step($p) src/B.php:9 + hop 3 Demo\Y::step($p) src/Y.php:15 + terminal Demo\C::sink($p) src/C.php:13 (used) + + WARNING $q: 2 pass-through hops across 2 classes + origin Demo\D::go($q) src/D.php:5 + hop 2 Demo\E::step($q) src/E.php:9 + terminal Demo\F::sink($q) src/F.php:13 (used) + + 2 findings (1 error, 1 warning; limit: 3 hops, warn-limit: 2 hops). + + TXT; + + $reporter = new TextReporter(new Thresholds(3, 2)); + + self::assertSame($expected, $reporter->render([$errorFinding, $warningFinding])); } } diff --git a/tests/Report/ThresholdsTest.php b/tests/Report/ThresholdsTest.php new file mode 100644 index 0000000..800f76d --- /dev/null +++ b/tests/Report/ThresholdsTest.php @@ -0,0 +1,69 @@ +severityOf($this->findingWithHops(3))); + } + + public function testSeverityOfIsErrorAboveLimit(): void + { + $thresholds = new Thresholds(3, null); + + self::assertSame(Severity::Error, $thresholds->severityOf($this->findingWithHops(5))); + } + + public function testSeverityOfIsWarningBetweenWarnLimitAndLimit(): void + { + $thresholds = new Thresholds(3, 2); + + self::assertSame(Severity::Warning, $thresholds->severityOf($this->findingWithHops(2))); + } + + public function testSeverityOfIsNullBelowWarnLimit(): void + { + $thresholds = new Thresholds(3, 2); + + self::assertNull($thresholds->severityOf($this->findingWithHops(1))); + } + + public function testSeverityOfIsNullBelowLimitWhenWarnLimitIsNull(): void + { + $thresholds = new Thresholds(3, null); + + self::assertNull($thresholds->severityOf($this->findingWithHops(2))); + } + + public function testConstructorThrowsWhenWarnLimitEqualsLimit(): void + { + $this->expectException(InvalidArgsException::class); + + new Thresholds(3, 3); + } + + public function testConstructorThrowsWhenWarnLimitAboveLimit(): void + { + $this->expectException(InvalidArgsException::class); + + new Thresholds(2, 3); + } + + private function findingWithHops(int $hops): Finding + { + return new Finding('p', 'Demo\A::go', 'Demo\A::sink', TerminalKind::Used, $hops, [], 1, [], []); + } +} From 21ad2b733948479daf430c04da5f77b4e2882ae3 Mon Sep 17 00:00:00 2001 From: Lars Pohlmann Date: Tue, 11 Aug 2026 10:55:49 +0200 Subject: [PATCH 03/26] feat(#5): json reporter Adds JsonReporter and the Paths cwd-relativizing helper, producing the pinned {limit, warnLimit, findings} JSON document (JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES, trailing newline). Wires --format json through ReporterFactory (now constructed with a working directory) and Application, since a reporter unreachable from the CLI is dead code and Phase 3 Task 3 exercises it end-to-end. --- src/Console/Application.php | 2 +- src/Report/JsonEncodingException.php | 9 + src/Report/JsonReporter.php | 129 ++++++++++ src/Report/Paths.php | 34 +++ src/Report/ReporterFactory.php | 5 + tests/Console/ApplicationTest.php | 13 +- tests/Report/JsonReporterTest.php | 343 +++++++++++++++++++++++++++ tests/Report/PathsTest.php | 56 +++++ 8 files changed, 589 insertions(+), 2 deletions(-) create mode 100644 src/Report/JsonEncodingException.php create mode 100644 src/Report/JsonReporter.php create mode 100644 src/Report/Paths.php create mode 100644 tests/Report/JsonReporterTest.php create mode 100644 tests/Report/PathsTest.php diff --git a/src/Console/Application.php b/src/Console/Application.php index 6f16add..2197908 100644 --- a/src/Console/Application.php +++ b/src/Console/Application.php @@ -77,7 +77,7 @@ private function analyze(Options $options): int { try { $index = $this->buildIndex($options); - $reporter = (new ReporterFactory())->create($options); + $reporter = (new ReporterFactory(getcwd() ?: '.'))->create($options); } catch (InvalidArgsException | ParseException $e) { fwrite($this->stderr, 'phptramp: ' . $e->getMessage() . "\n"); diff --git a/src/Report/JsonEncodingException.php b/src/Report/JsonEncodingException.php new file mode 100644 index 0000000..82ef887 --- /dev/null +++ b/src/Report/JsonEncodingException.php @@ -0,0 +1,9 @@ + $findings ALL findings, unfiltered + */ + public function render(array $findings): string + { + $document = [ + 'limit' => $this->thresholds->limit, + 'warnLimit' => $this->thresholds->warnLimit, + 'findings' => $this->findingDocuments($findings), + ]; + + return $this->encode($document) . "\n"; + } + + /** + * @param list $findings + * @return list> + */ + private function findingDocuments(array $findings): array + { + $documents = []; + foreach ($findings as $finding) { + $severity = $this->thresholds->severityOf($finding); + if ($severity === null) { + continue; + } + $documents[] = $this->findingDocument($finding, $severity); + } + + return $documents; + } + + /** + * @return array + */ + private function findingDocument(Finding $finding, Severity $severity): array + { + return [ + 'param' => $finding->param, + 'severity' => $this->severityLabel($severity), + 'origin' => $finding->origin, + 'terminal' => $finding->terminal, + 'terminalKind' => $finding->terminalKind->value, + 'hops' => $finding->hops, + 'classes' => $finding->classes, + 'chain' => $this->chainDocument($finding), + 'notes' => $finding->notes, + ]; + } + + private function severityLabel(Severity $severity): string + { + return match ($severity) { + Severity::Error => 'error', + Severity::Warning => 'warning', + }; + } + + /** + * @return list> + */ + private function chainDocument(Finding $finding): array + { + $hasTerminalNode = count($finding->chain) > $finding->hops; + + $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, + ]; + } + + return $chain; + } + + private function role(int $index, bool $isTerminal): string + { + if ($isTerminal) { + return 'terminal'; + } + if ($index === 0) { + return 'origin'; + } + + return 'hop'; + } + + /** + * @param array $document + */ + private function encode(array $document): string + { + $json = json_encode($document, JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES); + if ($json === false) { + // Unreachable: $document is built entirely from scalars, strings, and + // arrays thereof — nothing here can produce a resource or invalid + // UTF-8 that would make json_encode fail. + throw new JsonEncodingException('Failed to encode findings as JSON.'); + } + + return $json; + } +} diff --git a/src/Report/Paths.php b/src/Report/Paths.php new file mode 100644 index 0000000..f134b57 --- /dev/null +++ b/src/Report/Paths.php @@ -0,0 +1,34 @@ +workingDirectory = rtrim($workingDirectory, '/'); + } + + public function relativize(string $absolutePath): string + { + if ($absolutePath === $this->workingDirectory) { + return ''; + } + + $prefix = $this->workingDirectory . '/'; + if (str_starts_with($absolutePath, $prefix)) { + return substr($absolutePath, strlen($prefix)); + } + + return $absolutePath; + } +} diff --git a/src/Report/ReporterFactory.php b/src/Report/ReporterFactory.php index 12a4a9f..9043b48 100644 --- a/src/Report/ReporterFactory.php +++ b/src/Report/ReporterFactory.php @@ -9,12 +9,17 @@ final class ReporterFactory { + public function __construct(private readonly string $workingDirectory) + { + } + public function create(Options $options): Reporter { $thresholds = new Thresholds($options->limit, $options->warnLimit); return match ($options->format) { 'text' => new TextReporter($thresholds, $options->explain), + 'json' => new JsonReporter($thresholds, new Paths($this->workingDirectory)), default => throw new InvalidArgsException("format '{$options->format}' is not implemented yet."), }; } diff --git a/tests/Console/ApplicationTest.php b/tests/Console/ApplicationTest.php index 721f3ab..b26a251 100644 --- a/tests/Console/ApplicationTest.php +++ b/tests/Console/ApplicationTest.php @@ -101,11 +101,22 @@ public function testUnsupportedFormatExitsWithToolErrorCode(): void { $folder = $this->fixtureWithThreeHopChain(); - self::assertSame(2, $this->app->run(['phptramp', '--folder', $folder, '--format', 'json'])); + self::assertSame(2, $this->app->run(['phptramp', '--folder', $folder, '--format', 'sarif'])); self::assertStringContainsString('not implemented', self::contents($this->stderr)); self::assertSame('', self::contents($this->stdout)); } + public function testJsonFormatReportsFindingsAndExitsOne(): void + { + $folder = $this->fixtureWithThreeHopChain(); + + self::assertSame(1, $this->app->run(['phptramp', '--folder', $folder, '--format', 'json'])); + + $output = self::contents($this->stdout); + self::assertStringContainsString('"findings"', $output); + self::assertStringContainsString('"severity": "error"', $output); + } + public function testWarnOnlyRunExitsZeroButPrintsWarning(): void { $folder = $this->fixtureWithTwoHopChain(); diff --git a/tests/Report/JsonReporterTest.php b/tests/Report/JsonReporterTest.php new file mode 100644 index 0000000..9a9cf3f --- /dev/null +++ b/tests/Report/JsonReporterTest.php @@ -0,0 +1,343 @@ +paths()); + + self::assertSame($expected, $reporter->render([$finding])); + } + + public function testRendersWarningSeverityAndOmitsBelowThresholdFinding(): void + { + $warningChain = [ + 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), + ]; + $warningFinding = new Finding( + 'p', + 'Demo\A::go', + 'Demo\C::sink', + TerminalKind::Used, + 2, + $warningChain, + 2, + [], + [], + ); + + $belowWarnLimitChain = [ + new Hop('Demo\C::go', 'Demo\C', 'src/C.php', 5, null), + ]; + $belowWarnLimitFinding = new Finding( + 'q', + 'Demo\C::go', + 'Demo\C::go', + TerminalKind::Used, + 1, + $belowWarnLimitChain, + 1, + [], + [], + ); + + $expected = <<<'JSON' + { + "limit": 3, + "warnLimit": 2, + "findings": [ + { + "param": "p", + "severity": "warning", + "origin": "Demo\\A::go", + "terminal": "Demo\\C::sink", + "terminalKind": "used", + "hops": 2, + "classes": 2, + "chain": [ + { + "method": "Demo\\A::go", + "role": "origin", + "file": "src/A.php", + "line": 5, + "forwardLine": 7 + }, + { + "method": "Demo\\B::step", + "role": "hop", + "file": "src/B.php", + "line": 9, + "forwardLine": 11 + }, + { + "method": "Demo\\C::sink", + "role": "terminal", + "file": "src/C.php", + "line": 13, + "forwardLine": null + } + ], + "notes": [] + } + ] + } + + JSON; + + $reporter = new JsonReporter(new Thresholds(3, 2), $this->paths()); + + // Below-threshold finding comes first so an off-by-one on the filter + // (e.g. stopping instead of skipping) would also fail this assertion. + self::assertSame($expected, $reporter->render([$belowWarnLimitFinding, $warningFinding])); + } + + public function testIncludesAllQualifyingFindingsWhenMultiplePresent(): void + { + $firstChain = [ + new Hop('Demo\A::go', 'Demo\A', 'src/A.php', 5, 7), + new Hop('Demo\A::sink', 'Demo\A', 'src/A.php', 9, null), + ]; + $firstFinding = new Finding('p', 'Demo\A::go', 'Demo\A::sink', TerminalKind::Used, 1, $firstChain, 1, [], []); + + $secondChain = [ + new Hop('Demo\B::go', 'Demo\B', 'src/B.php', 5, 7), + new Hop('Demo\B::sink', 'Demo\B', 'src/B.php', 9, null), + ]; + $secondFinding = new Finding('q', 'Demo\B::go', 'Demo\B::sink', TerminalKind::Used, 1, $secondChain, 1, [], []); + + $expected = <<<'JSON' + { + "limit": 1, + "warnLimit": null, + "findings": [ + { + "param": "p", + "severity": "error", + "origin": "Demo\\A::go", + "terminal": "Demo\\A::sink", + "terminalKind": "used", + "hops": 1, + "classes": 1, + "chain": [ + { + "method": "Demo\\A::go", + "role": "origin", + "file": "src/A.php", + "line": 5, + "forwardLine": 7 + }, + { + "method": "Demo\\A::sink", + "role": "terminal", + "file": "src/A.php", + "line": 9, + "forwardLine": null + } + ], + "notes": [] + }, + { + "param": "q", + "severity": "error", + "origin": "Demo\\B::go", + "terminal": "Demo\\B::sink", + "terminalKind": "used", + "hops": 1, + "classes": 1, + "chain": [ + { + "method": "Demo\\B::go", + "role": "origin", + "file": "src/B.php", + "line": 5, + "forwardLine": 7 + }, + { + "method": "Demo\\B::sink", + "role": "terminal", + "file": "src/B.php", + "line": 9, + "forwardLine": null + } + ], + "notes": [] + } + ] + } + + JSON; + + $reporter = new JsonReporter(new Thresholds(1, null), $this->paths()); + + self::assertSame($expected, $reporter->render([$firstFinding, $secondFinding])); + } + + public function testChainWithNoTerminalNodeGivesLastEntryHopRoleAndNullTerminal(): void + { + $chain = [ + new Hop('Demo\A::go', 'Demo\A', 'src/A.php', 5, 7), + new Hop('Demo\B::step', 'Demo\B', 'src/B.php', 9, 11), + ]; + $finding = new Finding( + 'cfg', + 'Demo\A::go', + null, + TerminalKind::Truncated, + 2, + $chain, + 2, + ['truncated: 2 implementations'], + [], + ); + + $expected = <<<'JSON' + { + "limit": 2, + "warnLimit": null, + "findings": [ + { + "param": "cfg", + "severity": "error", + "origin": "Demo\\A::go", + "terminal": null, + "terminalKind": "truncated", + "hops": 2, + "classes": 2, + "chain": [ + { + "method": "Demo\\A::go", + "role": "origin", + "file": "src/A.php", + "line": 5, + "forwardLine": 7 + }, + { + "method": "Demo\\B::step", + "role": "hop", + "file": "src/B.php", + "line": 9, + "forwardLine": 11 + } + ], + "notes": [ + "truncated: 2 implementations" + ] + } + ] + } + + JSON; + + $reporter = new JsonReporter(new Thresholds(2, null), $this->paths()); + + self::assertSame($expected, $reporter->render([$finding])); + } + + public function testRendersEmptyFindingsArrayForEmptyRun(): void + { + $expected = <<<'JSON' + { + "limit": 3, + "warnLimit": null, + "findings": [] + } + + JSON; + + $reporter = new JsonReporter(new Thresholds(3, null), $this->paths()); + + self::assertSame($expected, $reporter->render([])); + } +} diff --git a/tests/Report/PathsTest.php b/tests/Report/PathsTest.php new file mode 100644 index 0000000..2ed1973 --- /dev/null +++ b/tests/Report/PathsTest.php @@ -0,0 +1,56 @@ +relativize('/home/user/project/src/Demo.php')); + } + + public function testRelativizesNestedSubdirectoryPath(): void + { + $paths = new Paths('/home/user/project'); + + self::assertSame( + 'src/Chain/Deep/Nested.php', + $paths->relativize('/home/user/project/src/Chain/Deep/Nested.php'), + ); + } + + public function testLeavesPathOutsideWorkingDirectoryUnchanged(): void + { + $paths = new Paths('/home/user/project'); + + self::assertSame('/var/other/Demo.php', $paths->relativize('/var/other/Demo.php')); + } + + public function testPathEqualToWorkingDirectoryRelativizesToEmptyString(): void + { + $paths = new Paths('/home/user/project'); + + self::assertSame('', $paths->relativize('/home/user/project')); + } + + public function testNormalizesTrailingSlashOnWorkingDirectory(): void + { + $paths = new Paths('/home/user/project/'); + + self::assertSame('src/Demo.php', $paths->relativize('/home/user/project/src/Demo.php')); + } + + public function testDoesNotTreatDifferentDirectoryWithSamePrefixAsUnderIt(): void + { + $paths = new Paths('/home/user/project'); + + self::assertSame('/home/user/project-other/Demo.php', $paths->relativize('/home/user/project-other/Demo.php')); + } +} From c5d793f2c616f030279de5ba20e2724617f30be0 Mon Sep 17 00:00:00 2001 From: Lars Pohlmann Date: Tue, 11 Aug 2026 11:01:07 +0200 Subject: [PATCH 04/26] chore: ignore .superpowers SDD workspace --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index 32a5f86..84893ba 100644 --- a/.gitignore +++ b/.gitignore @@ -3,3 +3,4 @@ /.phpunit.cache/ /.idea/ /infection.log +/.superpowers/ From 1dde6cbf808fdf7d233c6d481321c01dbbbad543 Mon Sep 17 00:00:00 2001 From: Lars Pohlmann Date: Tue, 11 Aug 2026 11:10:23 +0200 Subject: [PATCH 05/26] test(#5): fixture harness exercises the CLI json path Route the findings fixture path through the real Application CLI (--format json --limit 1) instead of calling ChainBuilder directly, so the fixtures pin the actual JsonReporter output contract rather than an internal shortcut. Rewrite all 7 expected-findings.json to the Task 2 JSON schema (limit/warnLimit/severity/chain-of-objects with role/file/line/forwardLine), with line/forwardLine hand-derived from each fixture's source and cross-checked against the tool's own output. --- tests/FixtureTest.php | 106 +++++++++++++----- .../classifier-smoke/expected-findings.json | 10 +- .../expected-findings.json | 7 +- .../expected-findings.json | 8 +- .../fixtures/recursion/expected-findings.json | 7 +- .../expected-findings.json | 9 +- .../use-breaks-chain/expected-findings.json | 8 +- .../variadic-decorator/expected-findings.json | 9 +- 8 files changed, 127 insertions(+), 37 deletions(-) diff --git a/tests/FixtureTest.php b/tests/FixtureTest.php index 7c56e1d..7492486 100644 --- a/tests/FixtureTest.php +++ b/tests/FixtureTest.php @@ -4,24 +4,16 @@ namespace PhpTramp\Tests; -use PhpTramp\Chain\ChainBuilder; -use PhpTramp\Chain\Finding; -use PhpTramp\Chain\Hop; use PhpTramp\Console\Application; -use PhpTramp\Console\Options; -use PhpTramp\Discovery\FileLocator; -use PhpTramp\Index\Indexer; -use PhpTramp\Resolve\CallResolver; -use PhpTramp\Resolve\ClassHierarchy; use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\TestCase; /** * Fixture harness: runs the real pipeline over every tests/fixtures//src. * A case with expected-index.json pins the classifier's `--dump-index` output; a - * case with expected-findings.json pins the chain builder's findings (chain - * flattened to FQMNs — the CLI JSON format arrives in Phase 3). Adding a semantic - * rule means adding a fixture directory, not touching this test. + * case with expected-findings.json pins the real CLI's `--format json` output + * (via `Application::run`), byte-for-byte modulo `file` path normalization. + * Adding a semantic rule means adding a fixture directory, not touching this test. */ final class FixtureTest extends TestCase { @@ -70,38 +62,92 @@ public function testFindingsMatchExpectation(string $caseDir): void $expectedJson = file_get_contents($caseDir . '/expected-findings.json'); self::assertIsString($expectedJson); $expected = json_decode($expectedJson, true); + self::assertIsArray($expected); - self::assertEquals($expected, $this->findings($caseDir . '/src')); + [$exitCode, $decoded] = $this->runFindingsJson($caseDir); + + $expectedExitCode = $expected['findings'] === [] ? 0 : 1; + self::assertSame($expectedExitCode, $exitCode); + self::assertEquals($expected, $this->normalizeFindingsDocument($decoded, $caseDir)); } /** - * @return array{findings: list>} + * @return array{0: int, 1: array} */ - private function findings(string $srcDir): array + private function runFindingsJson(string $caseDir): array { - $files = (new FileLocator())->locate(new Options(folders: [$srcDir])); - $index = (new Indexer())->index($files); - $resolver = new CallResolver($index, new ClassHierarchy($index)); - $findings = (new ChainBuilder($resolver))->build($index); + $stdout = fopen('php://memory', 'w+'); + $stderr = fopen('php://memory', 'w+'); + self::assertIsResource($stdout); + self::assertIsResource($stderr); + + $argv = ['phptramp', '--folder', $caseDir . '/src', '--format', 'json', '--limit', '1']; + $exitCode = (new Application($stdout, $stderr))->run($argv); + + rewind($stdout); + $json = stream_get_contents($stdout); + self::assertIsString($json); + $decoded = json_decode($json, true); + self::assertIsArray($decoded); - return ['findings' => array_map($this->normalizeFinding(...), $findings)]; + return [$exitCode, $decoded]; + } + + /** + * The reporter relativizes `file` against getcwd() (the repo root at test + * time), so JSON `file` values arrive repo-relative, e.g. + * "tests/fixtures//src/Demo.php". Expected files use case-relative + * paths, e.g. "src/Demo.php" — strip the case-dir prefix, computed relative + * to getcwd(), from every chain entry's `file`. + * + * @param array $decoded + * @return array + */ + private function normalizeFindingsDocument(array $decoded, string $caseDir): array + { + $realCaseDir = realpath($caseDir); + $cwd = getcwd(); + self::assertIsString($realCaseDir); + self::assertIsString($cwd); + + $prefix = null; + if (str_starts_with($realCaseDir, $cwd . '/')) { + $prefix = substr($realCaseDir, strlen($cwd) + 1) . '/'; + } + + $findings = $decoded['findings'] ?? []; + self::assertIsArray($findings); + $decoded['findings'] = array_map( + fn (array $finding): array => $this->normalizeFindingDocument($finding, $prefix), + $findings, + ); + + return $decoded; } /** + * @param array $finding * @return array */ - private function normalizeFinding(Finding $finding): array + private function normalizeFindingDocument(array $finding, ?string $prefix): array { - return [ - 'param' => $finding->param, - 'origin' => $finding->origin, - 'terminal' => $finding->terminal, - 'terminalKind' => $finding->terminalKind->value, - 'hops' => $finding->hops, - 'chain' => array_map(static fn (Hop $hop): string => $hop->fqmn, $finding->chain), - 'classes' => $finding->classes, - 'notes' => $finding->notes, - ]; + $chain = $finding['chain'] ?? []; + self::assertIsArray($chain); + + $finding['chain'] = array_map( + function (array $hop) use ($prefix): array { + $file = $hop['file']; + self::assertIsString($file); + if ($prefix !== null && str_starts_with($file, $prefix)) { + $hop['file'] = substr($file, strlen($prefix)); + } + + return $hop; + }, + $chain, + ); + + return $finding; } /** diff --git a/tests/fixtures/classifier-smoke/expected-findings.json b/tests/fixtures/classifier-smoke/expected-findings.json index 5da1fc0..48a0ff5 100644 --- a/tests/fixtures/classifier-smoke/expected-findings.json +++ b/tests/fixtures/classifier-smoke/expected-findings.json @@ -1,13 +1,21 @@ { + "limit": 1, + "warnLimit": null, "findings": [ { "param": "config", + "severity": "error", "origin": "Demo\\Controller::handle", "terminal": "Demo\\Mailer::__construct", "terminalKind": "stored", "hops": 3, - "chain": ["Demo\\Controller::handle", "Demo\\ServiceA::process", "Demo\\ServiceB::run", "Demo\\Mailer::__construct"], "classes": 4, + "chain": [ + {"method": "Demo\\Controller::handle", "role": "origin", "file": "src/Demo.php", "line": 11, "forwardLine": 13}, + {"method": "Demo\\ServiceA::process", "role": "hop", "file": "src/Demo.php", "line": 19, "forwardLine": 21}, + {"method": "Demo\\ServiceB::run", "role": "hop", "file": "src/Demo.php", "line": 27, "forwardLine": 29}, + {"method": "Demo\\Mailer::__construct", "role": "terminal", "file": "src/Demo.php", "line": 37, "forwardLine": null} + ], "notes": [] } ] diff --git a/tests/fixtures/interface-multi-impl/expected-findings.json b/tests/fixtures/interface-multi-impl/expected-findings.json index 3ef9099..45bc8a0 100644 --- a/tests/fixtures/interface-multi-impl/expected-findings.json +++ b/tests/fixtures/interface-multi-impl/expected-findings.json @@ -1,13 +1,18 @@ { + "limit": 1, + "warnLimit": null, "findings": [ { "param": "config", + "severity": "error", "origin": "Demo\\Controller::run", "terminal": null, "terminalKind": "truncated", "hops": 1, - "chain": ["Demo\\Controller::run"], "classes": 1, + "chain": [ + {"method": "Demo\\Controller::run", "role": "origin", "file": "src/Demo.php", "line": 32, "forwardLine": 34} + ], "notes": ["truncated: 2 implementations"] } ] diff --git a/tests/fixtures/interface-single-impl/expected-findings.json b/tests/fixtures/interface-single-impl/expected-findings.json index a8dd48a..bc630f3 100644 --- a/tests/fixtures/interface-single-impl/expected-findings.json +++ b/tests/fixtures/interface-single-impl/expected-findings.json @@ -1,13 +1,19 @@ { + "limit": 1, + "warnLimit": null, "findings": [ { "param": "config", + "severity": "error", "origin": "Demo\\Controller::run", "terminal": "Demo\\RealHandler::handle", "terminalKind": "used", "hops": 1, - "chain": ["Demo\\Controller::run", "Demo\\RealHandler::handle"], "classes": 2, + "chain": [ + {"method": "Demo\\Controller::run", "role": "origin", "file": "src/Demo.php", "line": 24, "forwardLine": 26}, + {"method": "Demo\\RealHandler::handle", "role": "terminal", "file": "src/Demo.php", "line": 16, "forwardLine": null} + ], "notes": [] } ] diff --git a/tests/fixtures/recursion/expected-findings.json b/tests/fixtures/recursion/expected-findings.json index 3841b30..95d616f 100644 --- a/tests/fixtures/recursion/expected-findings.json +++ b/tests/fixtures/recursion/expected-findings.json @@ -1,13 +1,18 @@ { + "limit": 1, + "warnLimit": null, "findings": [ { "param": "config", + "severity": "error", "origin": "Demo\\Looper::go", "terminal": null, "terminalKind": "truncated", "hops": 1, - "chain": ["Demo\\Looper::go"], "classes": 1, + "chain": [ + {"method": "Demo\\Looper::go", "role": "origin", "file": "src/Demo.php", "line": 11, "forwardLine": 13} + ], "notes": ["(recursive)"] } ] diff --git a/tests/fixtures/static-function-mix/expected-findings.json b/tests/fixtures/static-function-mix/expected-findings.json index 5f8e923..758f236 100644 --- a/tests/fixtures/static-function-mix/expected-findings.json +++ b/tests/fixtures/static-function-mix/expected-findings.json @@ -1,13 +1,20 @@ { + "limit": 1, + "warnLimit": null, "findings": [ { "param": "config", + "severity": "error", "origin": "Demo\\entry", "terminal": "Demo\\Sink::__construct", "terminalKind": "stored", "hops": 2, - "chain": ["Demo\\entry", "Demo\\Middle::relay", "Demo\\Sink::__construct"], "classes": 2, + "chain": [ + {"method": "Demo\\entry", "role": "origin", "file": "src/Demo.php", "line": 9, "forwardLine": 11}, + {"method": "Demo\\Middle::relay", "role": "hop", "file": "src/Demo.php", "line": 16, "forwardLine": 18}, + {"method": "Demo\\Sink::__construct", "role": "terminal", "file": "src/Demo.php", "line": 26, "forwardLine": null} + ], "notes": [] } ] diff --git a/tests/fixtures/use-breaks-chain/expected-findings.json b/tests/fixtures/use-breaks-chain/expected-findings.json index 3fa5c02..cfb892a 100644 --- a/tests/fixtures/use-breaks-chain/expected-findings.json +++ b/tests/fixtures/use-breaks-chain/expected-findings.json @@ -1,13 +1,19 @@ { + "limit": 1, + "warnLimit": null, "findings": [ { "param": "config", + "severity": "error", "origin": "Demo\\Controller::handle", "terminal": "Demo\\Service::process", "terminalKind": "used", "hops": 1, - "chain": ["Demo\\Controller::handle", "Demo\\Service::process"], "classes": 2, + "chain": [ + {"method": "Demo\\Controller::handle", "role": "origin", "file": "src/Demo.php", "line": 11, "forwardLine": 13}, + {"method": "Demo\\Service::process", "role": "terminal", "file": "src/Demo.php", "line": 19, "forwardLine": null} + ], "notes": [] } ] diff --git a/tests/fixtures/variadic-decorator/expected-findings.json b/tests/fixtures/variadic-decorator/expected-findings.json index 5ea1462..3339d07 100644 --- a/tests/fixtures/variadic-decorator/expected-findings.json +++ b/tests/fixtures/variadic-decorator/expected-findings.json @@ -1,13 +1,20 @@ { + "limit": 1, + "warnLimit": null, "findings": [ { "param": "args", + "severity": "error", "origin": "Demo\\Outer::run", "terminal": "Demo\\Sink::consume", "terminalKind": "used", "hops": 2, - "chain": ["Demo\\Outer::run", "Demo\\Middle::run", "Demo\\Sink::consume"], "classes": 3, + "chain": [ + {"method": "Demo\\Outer::run", "role": "origin", "file": "src/Demo.php", "line": 11, "forwardLine": 13}, + {"method": "Demo\\Middle::run", "role": "hop", "file": "src/Demo.php", "line": 19, "forwardLine": 21}, + {"method": "Demo\\Sink::consume", "role": "terminal", "file": "src/Demo.php", "line": 27, "forwardLine": null} + ], "notes": [] } ] From b4fb5ea42fbb9d23e07f0be2bb738c46f0e733dc Mon Sep 17 00:00:00 2001 From: Lars Pohlmann Date: Tue, 11 Aug 2026 11:23:35 +0200 Subject: [PATCH 06/26] feat(#5): phptramp.json config with CLI-over-config precedence Adds a phptramp.json (falling back to phptramp.dist.json) config file so projects can set defaults for paths/limit/warnLimit/format/baseline without repeating CLI flags in every invocation. Precedence is CLI > config > tool defaults: ArgvParser::parse() now takes an Options $defaults seed, and the first --folder/--file/--files flag clears any config-seeded paths wholesale (config paths are replaced, never mixed with CLI paths). ConfigLoader is strict by design: an unrecognized key or a wrong-typed value throws ConfigException, because a typo in this file must not silently disable gating. The "exclude" key is accepted (so existing config files don't break) but intentionally not mapped/stored yet -- Options gains that field in a follow-up task. --- src/Config/ConfigException.php | 9 ++ src/Config/ConfigLoader.php | 156 +++++++++++++++++++++++++ src/Console/Application.php | 14 ++- src/Console/ArgvParser.php | 66 ++++++++--- tests/Config/ConfigLoaderTest.php | 188 ++++++++++++++++++++++++++++++ tests/Console/ApplicationTest.php | 27 +++++ tests/Console/ArgvParserTest.php | 70 +++++++++++ 7 files changed, 509 insertions(+), 21 deletions(-) create mode 100644 src/Config/ConfigException.php create mode 100644 src/Config/ConfigLoader.php create mode 100644 tests/Config/ConfigLoaderTest.php diff --git a/src/Config/ConfigException.php b/src/Config/ConfigException.php new file mode 100644 index 0000000..19e65ef --- /dev/null +++ b/src/Config/ConfigException.php @@ -0,0 +1,9 @@ +resolveFile($directory); + if ($file === null) { + return new Options(); + } + + return $this->toOptions($this->decode($file)); + } + + private function resolveFile(string $directory): ?string + { + $primary = $directory . '/phptramp.json'; + if (is_file($primary)) { + return $primary; + } + + $dist = $directory . '/phptramp.dist.json'; + + return is_file($dist) ? $dist : null; + } + + /** + * @return array + */ + private function decode(string $file): array + { + $contents = file_get_contents($file); + if ($contents === false) { + throw new ConfigException("unable to read config file: {$file}"); + } + + $decoded = json_decode($contents); + if (json_last_error() !== JSON_ERROR_NONE || ! $decoded instanceof \stdClass) { + throw new ConfigException("config file is not a JSON object: {$file}"); + } + + return $this->stringKeyed(get_object_vars($decoded)); + } + + /** + * Narrows get_object_vars()'s array to array: + * JSON object properties always decode to string keys, but PHPStan can't + * see that through stdClass alone. + * + * @param array $properties + * @return array + */ + private function stringKeyed(array $properties): array + { + $result = []; + foreach ($properties as $key => $value) { + $result[(string) $key] = $value; + } + + return $result; + } + + /** + * @param array $config + */ + private function toOptions(array $config): Options + { + $defaults = new Options(); + + $folders = []; + $files = []; + $limit = $defaults->limit; + $warnLimit = $defaults->warnLimit; + $format = $defaults->format; + $baseline = $defaults->baseline; + + foreach ($config as $key => $value) { + match ($key) { + 'paths' => $this->assignPaths($value, $folders, $files), + 'exclude' => null, // storage arrives in Task 5; allow-listed so it doesn't trip the unknown-key guard. + 'limit' => $limit = $this->requireInt('limit', $value), + 'warnLimit' => $warnLimit = $this->requireInt('warnLimit', $value), + 'format' => $format = $this->requireString('format', $value), + 'baseline' => $baseline = $this->requireString('baseline', $value), + default => throw new ConfigException("unknown config key: {$key}"), + }; + } + + return new Options( + folders: $folders, + files: $files, + limit: $limit, + warnLimit: $warnLimit, + format: $format, + baseline: $baseline, + ); + } + + /** + * @param list $folders + * @param list $files + * @param-out list $folders + * @param-out list $files + */ + private function assignPaths(mixed $value, array &$folders, array &$files): void + { + if (! is_array($value) || ! array_is_list($value)) { + throw new ConfigException('config key "paths" must be a list of strings'); + } + + foreach ($value as $path) { + if (! is_string($path)) { + throw new ConfigException('config key "paths" must be a list of strings'); + } + + if (str_ends_with($path, '.php')) { + $files[] = $path; + } else { + $folders[] = $path; + } + } + } + + private function requireInt(string $key, mixed $value): int + { + if (! is_int($value)) { + throw new ConfigException("config key \"{$key}\" must be an integer"); + } + + return $value; + } + + private function requireString(string $key, mixed $value): string + { + if (! is_string($value)) { + throw new ConfigException("config key \"{$key}\" must be a string"); + } + + return $value; + } +} diff --git a/src/Console/Application.php b/src/Console/Application.php index 2197908..3d9c3b2 100644 --- a/src/Console/Application.php +++ b/src/Console/Application.php @@ -6,6 +6,8 @@ use PhpTramp\Chain\ChainBuilder; use PhpTramp\Chain\Finding; +use PhpTramp\Config\ConfigException; +use PhpTramp\Config\ConfigLoader; use PhpTramp\Discovery\FileLocator; use PhpTramp\Index\ForwardSite; use PhpTramp\Index\Indexer; @@ -47,8 +49,9 @@ public function run(array $argv): int $args = array_slice($argv, 1); try { - $options = (new ArgvParser())->parse($args); - } catch (InvalidArgsException $e) { + $defaults = (new ConfigLoader())->load($this->workingDirectory()); + $options = (new ArgvParser())->parse($args, $defaults); + } catch (ConfigException | InvalidArgsException $e) { fwrite($this->stderr, 'phptramp: ' . $e->getMessage() . "\n"); return 2; @@ -73,11 +76,16 @@ public function run(array $argv): int return $this->analyze($options); } + private function workingDirectory(): string + { + return getcwd() ?: '.'; + } + private function analyze(Options $options): int { try { $index = $this->buildIndex($options); - $reporter = (new ReporterFactory(getcwd() ?: '.'))->create($options); + $reporter = (new ReporterFactory($this->workingDirectory()))->create($options); } catch (InvalidArgsException | ParseException $e) { fwrite($this->stderr, 'phptramp: ' . $e->getMessage() . "\n"); diff --git a/src/Console/ArgvParser.php b/src/Console/ArgvParser.php index 54d3af0..cc31e2f 100644 --- a/src/Console/ArgvParser.php +++ b/src/Console/ArgvParser.php @@ -50,13 +50,14 @@ final class ArgvParser private bool $dumpIndex = false; private bool $help = false; private bool $version = false; + private bool $clearedSeededPaths = false; /** * @param list $args */ - public function parse(array $args): Options + public function parse(array $args, Options $defaults = new Options()): Options { - $this->reset(); + $this->reset($defaults); $index = 0; $count = count($args); @@ -81,21 +82,22 @@ public function parse(array $args): Options ); } - private function reset(): void + private function reset(Options $defaults): void { - $this->folders = []; - $this->files = []; - $this->limit = 3; - $this->warnLimit = null; - $this->format = 'text'; - $this->explain = false; - $this->changedOnly = false; - $this->gitBase = 'origin/main'; - $this->baseline = null; - $this->generateBaseline = null; - $this->dumpIndex = false; - $this->help = false; - $this->version = false; + $this->folders = $defaults->folders; + $this->files = $defaults->files; + $this->limit = $defaults->limit; + $this->warnLimit = $defaults->warnLimit; + $this->format = $defaults->format; + $this->explain = $defaults->explain; + $this->changedOnly = $defaults->changedOnly; + $this->gitBase = $defaults->gitBase; + $this->baseline = $defaults->baseline; + $this->generateBaseline = $defaults->generateBaseline; + $this->dumpIndex = $defaults->dumpIndex; + $this->help = $defaults->help; + $this->version = $defaults->version; + $this->clearedSeededPaths = false; } /** @@ -134,8 +136,8 @@ private function consume(array $args, int $index, int $count): int private function applyValueFlag(string $name, string $value): void { match ($name) { - '--folder' => $this->folders[] = $value, - '--file' => $this->files[] = $value, + '--folder' => $this->appendFolder($value), + '--file' => $this->appendFile($value), '--files' => $this->appendFiles($value), '--limit' => $this->limit = $this->toInt($name, $value), '--warn-limit' => $this->warnLimit = $this->toInt($name, $value), @@ -159,13 +161,41 @@ private function applyBoolFlag(string $name): void }; } + private function appendFolder(string $value): void + { + $this->clearSeededPathsOnce(); + $this->folders[] = $value; + } + + private function appendFile(string $value): void + { + $this->clearSeededPathsOnce(); + $this->files[] = $value; + } + private function appendFiles(string $value): void { + $this->clearSeededPathsOnce(); foreach (explode(',', $value) as $file) { $this->files[] = $file; } } + /** + * The first path flag replaces config-seeded paths entirely (never mixed + * with CLI paths); later path flags append as usual. + */ + private function clearSeededPathsOnce(): void + { + if ($this->clearedSeededPaths) { + return; + } + + $this->folders = []; + $this->files = []; + $this->clearedSeededPaths = true; + } + private function toInt(string $flag, string $value): int { if ($value === '' || ! ctype_digit($value)) { diff --git a/tests/Config/ConfigLoaderTest.php b/tests/Config/ConfigLoaderTest.php new file mode 100644 index 0000000..af28aca --- /dev/null +++ b/tests/Config/ConfigLoaderTest.php @@ -0,0 +1,188 @@ +directory = $directory; + } + + protected function tearDown(): void + { + array_map('unlink', glob($this->directory . '/*') ?: []); + rmdir($this->directory); + } + + private function writeConfig(string $filename, string $contents): void + { + file_put_contents($this->directory . '/' . $filename, $contents); + } + + public function testMissingBothFilesReturnsPlainDefaults(): void + { + $options = (new ConfigLoader())->load($this->directory); + + self::assertEquals(new Options(), $options); + } + + public function testReadsPhptrampJson(): void + { + $this->writeConfig('phptramp.json', '{"limit": 5}'); + + $options = (new ConfigLoader())->load($this->directory); + + self::assertSame(5, $options->limit); + } + + public function testFallsBackToDistFileWhenPrimaryAbsent(): void + { + $this->writeConfig('phptramp.dist.json', '{"limit": 7}'); + + $options = (new ConfigLoader())->load($this->directory); + + self::assertSame(7, $options->limit); + } + + public function testPrefersNonDistFileWhenBothExist(): void + { + $this->writeConfig('phptramp.json', '{"limit": 5}'); + $this->writeConfig('phptramp.dist.json', '{"limit": 7}'); + + $options = (new ConfigLoader())->load($this->directory); + + self::assertSame(5, $options->limit); + } + + public function testUnknownKeyThrows(): void + { + $this->writeConfig('phptramp.json', '{"nope": true}'); + + $this->expectException(ConfigException::class); + $this->expectExceptionMessage('unknown config key: nope'); + + (new ConfigLoader())->load($this->directory); + } + + public function testPathsSplitsPhpEntriesIntoFilesAndRestIntoFolders(): void + { + $this->writeConfig('phptramp.json', '{"paths": ["src", "a.php", "tests", "b.php"]}'); + + $options = (new ConfigLoader())->load($this->directory); + + self::assertSame(['src', 'tests'], $options->folders); + self::assertSame(['a.php', 'b.php'], $options->files); + } + + public function testPathsMustBeAList(): void + { + $this->writeConfig('phptramp.json', '{"paths": "src"}'); + + $this->expectException(ConfigException::class); + + (new ConfigLoader())->load($this->directory); + } + + public function testPathsWithNonStringEntryThrows(): void + { + $this->writeConfig('phptramp.json', '{"paths": ["src", 3]}'); + + $this->expectException(ConfigException::class); + + (new ConfigLoader())->load($this->directory); + } + + public function testWrongTypeForLimitThrows(): void + { + $this->writeConfig('phptramp.json', '{"limit": "three"}'); + + $this->expectException(ConfigException::class); + + (new ConfigLoader())->load($this->directory); + } + + public function testWrongTypeForWarnLimitThrows(): void + { + $this->writeConfig('phptramp.json', '{"warnLimit": "two"}'); + + $this->expectException(ConfigException::class); + + (new ConfigLoader())->load($this->directory); + } + + public function testWrongTypeForFormatThrows(): void + { + $this->writeConfig('phptramp.json', '{"format": 3}'); + + $this->expectException(ConfigException::class); + + (new ConfigLoader())->load($this->directory); + } + + public function testWrongTypeForBaselineThrows(): void + { + $this->writeConfig('phptramp.json', '{"baseline": 3}'); + + $this->expectException(ConfigException::class); + + (new ConfigLoader())->load($this->directory); + } + + public function testMalformedJsonThrows(): void + { + $this->writeConfig('phptramp.json', '{not valid json'); + + $this->expectException(ConfigException::class); + + (new ConfigLoader())->load($this->directory); + } + + public function testTopLevelJsonArrayThrows(): void + { + $this->writeConfig('phptramp.json', '[1, 2, 3]'); + + $this->expectException(ConfigException::class); + + (new ConfigLoader())->load($this->directory); + } + + public function testExcludeKeyIsAllowedButNotYetMapped(): void + { + $this->writeConfig('phptramp.json', '{"exclude": ["vendor/*"]}'); + + $options = (new ConfigLoader())->load($this->directory); + + self::assertEquals(new Options(), $options); + } + + public function testFormatAndBaselineAreMapped(): void + { + $this->writeConfig('phptramp.json', '{"format": "json", "baseline": "baseline.json"}'); + + $options = (new ConfigLoader())->load($this->directory); + + self::assertSame('json', $options->format); + self::assertSame('baseline.json', $options->baseline); + } + + public function testWarnLimitIsMapped(): void + { + $this->writeConfig('phptramp.json', '{"warnLimit": 2, "limit": 3}'); + + $options = (new ConfigLoader())->load($this->directory); + + self::assertSame(2, $options->warnLimit); + } +} diff --git a/tests/Console/ApplicationTest.php b/tests/Console/ApplicationTest.php index b26a251..35f84c8 100644 --- a/tests/Console/ApplicationTest.php +++ b/tests/Console/ApplicationTest.php @@ -128,6 +128,33 @@ public function testWarnOnlyRunExitsZeroButPrintsWarning(): void self::assertStringContainsString('WARNING', self::contents($this->stdout)); } + public function testConfigFileLimitAppliesWithoutCliFlag(): 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]); + } finally { + chdir($previousCwd); + } + + self::assertSame(1, $exitCode); + self::assertStringContainsString('FINDING', self::contents($this->stdout)); + } + private function fixtureWithTwoHopChain(): string { $folder = sys_get_temp_dir() . '/phptramp-app-' . uniqid(); diff --git a/tests/Console/ArgvParserTest.php b/tests/Console/ArgvParserTest.php index 91f4f20..5e44c77 100644 --- a/tests/Console/ArgvParserTest.php +++ b/tests/Console/ArgvParserTest.php @@ -173,4 +173,74 @@ public function testMissingValueMessageNamesTheFlag(): void self::assertStringContainsString('--limit', $e->getMessage()); } } + + public function testSeededDefaultSurvivesWhenFlagAbsent(): void + { + $defaults = new Options(limit: 1); + + self::assertSame(1, (new ArgvParser())->parse([], $defaults)->limit); + } + + public function testExplicitFlagOverridesSeededDefault(): void + { + $defaults = new Options(limit: 1); + + self::assertSame(5, (new ArgvParser())->parse(['--limit', '5'], $defaults)->limit); + } + + public function testFirstPathFlagClearsSeededPaths(): void + { + $defaults = new Options(folders: ['configdir'], files: ['configfile.php']); + + $options = (new ArgvParser())->parse(['--folder', 'cli'], $defaults); + + self::assertSame(['cli'], $options->folders); + self::assertSame([], $options->files); + } + + public function testSecondPathFlagAppendsRatherThanClearing(): void + { + $defaults = new Options(folders: ['configdir']); + + $options = (new ArgvParser())->parse(['--folder', 'a', '--folder', 'b'], $defaults); + + self::assertSame(['a', 'b'], $options->folders); + } + + public function testFileFlagClearsSeededPaths(): void + { + $defaults = new Options(folders: ['configdir'], files: ['configfile.php']); + + $options = (new ArgvParser())->parse(['--file', 'cli.php'], $defaults); + + self::assertSame([], $options->folders); + self::assertSame(['cli.php'], $options->files); + } + + public function testFilesFlagClearsSeededPaths(): void + { + $defaults = new Options(folders: ['configdir'], files: ['configfile.php']); + + $options = (new ArgvParser())->parse(['--files', 'a.php,b.php'], $defaults); + + self::assertSame([], $options->folders); + self::assertSame(['a.php', 'b.php'], $options->files); + } + + public function testUnknownFlagWithEqualsSyntaxThrows(): void + { + $this->expectException(InvalidArgsException::class); + $this->parse('--nope=value'); + } + + public function testSeededNonPathDefaultsSurviveAlongsideExplicitFlags(): void + { + $defaults = new Options(format: 'json', gitBase: 'develop'); + + $options = (new ArgvParser())->parse(['--limit', '5'], $defaults); + + self::assertSame('json', $options->format); + self::assertSame('develop', $options->gitBase); + self::assertSame(5, $options->limit); + } } From 6570105e4fcdc894dd7d14b9705142de02f3f4ab Mon Sep 17 00:00:00 2001 From: Lars Pohlmann Date: Tue, 11 Aug 2026 11:38:34 +0200 Subject: [PATCH 07/26] feat(#5): exclude globs from config Wires the config-only `exclude` seam left as a no-op in Task 4: fnmatch globs (FNM_PATHNAME unset) drop folder-discovered files after collection, before sorting, matched against the path relative to the realpath'd working directory. Explicitly named --file/--files paths are never filtered. exclude has no CLI flag by design (YAGNI), but ArgvParser rebuilds Options from scratch on every parse, so it must carry the config-seeded value through untouched or it would be silently dropped. FileLocator now takes the working directory in its constructor (Application wires it at both call sites) so exclude patterns resolve against the same cwd the config was loaded from. --- src/Config/ConfigLoader.php | 24 +++++++++++- src/Console/Application.php | 4 +- src/Console/ArgvParser.php | 4 ++ src/Console/Options.php | 2 + src/Discovery/FileLocator.php | 60 +++++++++++++++++++++++++++-- tests/Config/ConfigLoaderTest.php | 24 ++++++++++-- tests/Console/ArgvParserTest.php | 7 ++++ tests/Discovery/FileLocatorTest.php | 52 ++++++++++++++++++++++++- 8 files changed, 167 insertions(+), 10 deletions(-) diff --git a/src/Config/ConfigLoader.php b/src/Config/ConfigLoader.php index 2897d08..1278ba1 100644 --- a/src/Config/ConfigLoader.php +++ b/src/Config/ConfigLoader.php @@ -84,6 +84,7 @@ private function toOptions(array $config): Options $folders = []; $files = []; + $exclude = $defaults->exclude; $limit = $defaults->limit; $warnLimit = $defaults->warnLimit; $format = $defaults->format; @@ -92,7 +93,7 @@ private function toOptions(array $config): Options foreach ($config as $key => $value) { match ($key) { 'paths' => $this->assignPaths($value, $folders, $files), - 'exclude' => null, // storage arrives in Task 5; allow-listed so it doesn't trip the unknown-key guard. + 'exclude' => $exclude = $this->requireStringList('exclude', $value), 'limit' => $limit = $this->requireInt('limit', $value), 'warnLimit' => $warnLimit = $this->requireInt('warnLimit', $value), 'format' => $format = $this->requireString('format', $value), @@ -108,6 +109,7 @@ private function toOptions(array $config): Options warnLimit: $warnLimit, format: $format, baseline: $baseline, + exclude: $exclude, ); } @@ -136,6 +138,26 @@ private function assignPaths(mixed $value, array &$folders, array &$files): void } } + /** + * @return list + */ + private function requireStringList(string $key, mixed $value): array + { + if (! is_array($value) || ! array_is_list($value)) { + throw new ConfigException("config key \"{$key}\" must be a list of strings"); + } + + $strings = []; + foreach ($value as $entry) { + if (! is_string($entry)) { + throw new ConfigException("config key \"{$key}\" must be a list of strings"); + } + $strings[] = $entry; + } + + return $strings; + } + private function requireInt(string $key, mixed $value): int { if (! is_int($value)) { diff --git a/src/Console/Application.php b/src/Console/Application.php index 3d9c3b2..2720063 100644 --- a/src/Console/Application.php +++ b/src/Console/Application.php @@ -116,7 +116,7 @@ private function hasError(array $findings, Thresholds $thresholds): bool private function buildIndex(Options $options): MethodIndex { - $files = (new FileLocator())->locate($options); + $files = (new FileLocator($this->workingDirectory()))->locate($options); return (new Indexer())->index($files); } @@ -134,7 +134,7 @@ private function findChains(MethodIndex $index): array private function dumpIndex(Options $options): int { try { - $files = (new FileLocator())->locate($options); + $files = (new FileLocator($this->workingDirectory()))->locate($options); $index = (new Indexer())->index($files); } catch (InvalidArgsException | ParseException $e) { fwrite($this->stderr, 'phptramp: ' . $e->getMessage() . "\n"); diff --git a/src/Console/ArgvParser.php b/src/Console/ArgvParser.php index cc31e2f..24d234c 100644 --- a/src/Console/ArgvParser.php +++ b/src/Console/ArgvParser.php @@ -51,6 +51,8 @@ final class ArgvParser private bool $help = false; private bool $version = false; private bool $clearedSeededPaths = false; + /** @var list */ + private array $exclude = []; /** * @param list $args @@ -79,6 +81,7 @@ public function parse(array $args, Options $defaults = new Options()): Options dumpIndex: $this->dumpIndex, help: $this->help, version: $this->version, + exclude: $this->exclude, ); } @@ -97,6 +100,7 @@ private function reset(Options $defaults): void $this->dumpIndex = $defaults->dumpIndex; $this->help = $defaults->help; $this->version = $defaults->version; + $this->exclude = $defaults->exclude; $this->clearedSeededPaths = false; } diff --git a/src/Console/Options.php b/src/Console/Options.php index a4b44c6..ece4ba9 100644 --- a/src/Console/Options.php +++ b/src/Console/Options.php @@ -17,6 +17,7 @@ final class Options /** * @param list $folders * @param list $files + * @param list $exclude */ public function __construct( public readonly array $folders = [], @@ -32,6 +33,7 @@ public function __construct( public readonly bool $dumpIndex = false, public readonly bool $help = false, public readonly bool $version = false, + public readonly array $exclude = [], ) { } } diff --git a/src/Discovery/FileLocator.php b/src/Discovery/FileLocator.php index 20dff75..1372447 100644 --- a/src/Discovery/FileLocator.php +++ b/src/Discovery/FileLocator.php @@ -13,18 +13,24 @@ */ final class FileLocator { + public function __construct(private readonly string $workingDirectory) + { + } + /** * @return list */ public function locate(Options $options): array { - /** @var array $found */ - $found = []; + /** @var array $discovered */ + $discovered = []; foreach ($options->folders as $folder) { - $this->collectFolder($folder, $found); + $this->collectFolder($folder, $discovered); } + $found = $this->withoutExcluded($discovered, $options->exclude); + foreach ($options->files as $file) { $real = realpath($file); if ($real === false || ! is_file($real)) { @@ -39,6 +45,54 @@ public function locate(Options $options): array return $paths; } + /** + * @param array $discovered + * @param list $exclude + * @return array + */ + private function withoutExcluded(array $discovered, array $exclude): array + { + $kept = []; + foreach ($discovered as $path => $true) { + if (! $this->isExcluded($path, $exclude)) { + $kept[$path] = $true; + } + } + + return $kept; + } + + /** + * @param list $exclude + */ + private function isExcluded(string $path, array $exclude): bool + { + $relative = $this->relativeToWorkingDirectory($path); + + foreach ($exclude as $pattern) { + if (fnmatch($pattern, $relative)) { + return true; + } + } + + return false; + } + + private function relativeToWorkingDirectory(string $path): string + { + $base = realpath($this->workingDirectory); + if ($base === false) { + return $path; + } + + $prefix = $base . '/'; + if (! str_starts_with($path, $prefix)) { + return $path; + } + + return substr($path, strlen($prefix)); + } + /** * @param array $found * @param-out array $found diff --git a/tests/Config/ConfigLoaderTest.php b/tests/Config/ConfigLoaderTest.php index af28aca..4d41ccd 100644 --- a/tests/Config/ConfigLoaderTest.php +++ b/tests/Config/ConfigLoaderTest.php @@ -158,13 +158,31 @@ public function testTopLevelJsonArrayThrows(): void (new ConfigLoader())->load($this->directory); } - public function testExcludeKeyIsAllowedButNotYetMapped(): void + public function testExcludeIsMapped(): void { - $this->writeConfig('phptramp.json', '{"exclude": ["vendor/*"]}'); + $this->writeConfig('phptramp.json', '{"exclude": ["vendor/*", "*Test.php"]}'); $options = (new ConfigLoader())->load($this->directory); - self::assertEquals(new Options(), $options); + self::assertSame(['vendor/*', '*Test.php'], $options->exclude); + } + + public function testExcludeMustBeAList(): void + { + $this->writeConfig('phptramp.json', '{"exclude": "nope"}'); + + $this->expectException(ConfigException::class); + + (new ConfigLoader())->load($this->directory); + } + + public function testExcludeWithNonStringEntryThrows(): void + { + $this->writeConfig('phptramp.json', '{"exclude": ["vendor/*", 3]}'); + + $this->expectException(ConfigException::class); + + (new ConfigLoader())->load($this->directory); } public function testFormatAndBaselineAreMapped(): void diff --git a/tests/Console/ArgvParserTest.php b/tests/Console/ArgvParserTest.php index 5e44c77..0ee8371 100644 --- a/tests/Console/ArgvParserTest.php +++ b/tests/Console/ArgvParserTest.php @@ -233,6 +233,13 @@ public function testUnknownFlagWithEqualsSyntaxThrows(): void $this->parse('--nope=value'); } + public function testSeededExcludeSurvivesWhenAbsentFromArgv(): void + { + $defaults = new Options(exclude: ['vendor/*']); + + self::assertSame(['vendor/*'], (new ArgvParser())->parse([], $defaults)->exclude); + } + public function testSeededNonPathDefaultsSurviveAlongsideExplicitFlags(): void { $defaults = new Options(format: 'json', gitBase: 'develop'); diff --git a/tests/Discovery/FileLocatorTest.php b/tests/Discovery/FileLocatorTest.php index ea67e6e..4afba1b 100644 --- a/tests/Discovery/FileLocatorTest.php +++ b/tests/Discovery/FileLocatorTest.php @@ -42,7 +42,7 @@ protected function tearDown(): void */ private function locate(Options $options): array { - return (new FileLocator())->locate($options); + return (new FileLocator($this->root))->locate($options); } private function real(string $relative): string @@ -112,4 +112,54 @@ public function testUppercasePhpExtensionIsMatchedCaseInsensitively(): void self::assertContains($this->real('Upper.PHP'), $this->locate(new Options(folders: [$this->root]))); } + + public function testExcludeGlobDropsNestedDiscoveredFile(): void + { + mkdir($this->root . '/vendor/pkg/src', 0777, true); + file_put_contents($this->root . '/vendor/pkg/src/X.php', "locate(new Options(folders: [$this->root], exclude: ['vendor/*'])); + + self::assertNotContains($this->real('vendor/pkg/src/X.php'), $found); + self::assertContains($this->real('a.php'), $found); + } + + public function testExcludeGlobDropsDiscoveredFileByBasenamePattern(): void + { + file_put_contents($this->root . '/sub/bThingTest.php', "locate(new Options(folders: [$this->root], exclude: ['*Test.php'])); + + self::assertNotContains($this->real('sub/bThingTest.php'), $found); + self::assertContains($this->real('a.php'), $found); + } + + public function testExcludeMatchesRawPathWhenFileIsOutsideWorkingDirectory(): void + { + $otherDirectory = sys_get_temp_dir() . '/phptramp-loc-other-' . uniqid('', true); + mkdir($otherDirectory); + + try { + $found = (new FileLocator($otherDirectory))->locate( + new Options(folders: [$this->root], exclude: [$this->real('a.php')]) + ); + + self::assertNotContains($this->real('a.php'), $found); + self::assertContains($this->real('sub/b.php'), $found); + } finally { + rmdir($otherDirectory); + } + } + + public function testExplicitFileBeatsExclude(): void + { + $found = $this->locate(new Options( + folders: [$this->root], + files: [$this->root . '/a.php'], + exclude: ['a.php', '*.php'], + )); + + self::assertContains($this->real('a.php'), $found); + self::assertNotContains($this->real('sub/b.php'), $found); + } } From 40a796094a0a837abbf5a4292c213c64f7b8e7e9 Mon Sep 17 00:00:00 2001 From: Lars Pohlmann Date: Tue, 11 Aug 2026 11:49:54 +0200 Subject: [PATCH 08/26] feat(#5): github actions reporter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds GithubReporter, emitting one ::error/::warning workflow-command annotation per reported finding at its origin, plus one ::notice per subsequent hop (the terminal node is never annotated). Message data is escaped per the Actions workflow-command spec (%, \r, \n only). Extracts FindingMessage as the shared "$param: N pass-through hops across M classes (terminal: ...)" formatter — this is its first of three planned reporter callers (Checkstyle and SARIF follow), so the duplication is removed at the point it would otherwise start. --- src/Report/FindingMessage.php | 40 ++++++++++ src/Report/GithubReporter.php | 114 +++++++++++++++++++++++++++ src/Report/ReporterFactory.php | 1 + tests/Report/FindingMessageTest.php | 78 +++++++++++++++++++ tests/Report/GithubReporterTest.php | 117 ++++++++++++++++++++++++++++ 5 files changed, 350 insertions(+) create mode 100644 src/Report/FindingMessage.php create mode 100644 src/Report/GithubReporter.php create mode 100644 tests/Report/FindingMessageTest.php create mode 100644 tests/Report/GithubReporterTest.php diff --git a/src/Report/FindingMessage.php b/src/Report/FindingMessage.php new file mode 100644 index 0000000..f555c73 --- /dev/null +++ b/src/Report/FindingMessage.php @@ -0,0 +1,40 @@ +param . ': ' . $finding->hops . ' pass-through ' . $this->plural($finding->hops, 'hop') + . ' across ' . $finding->classes . ' ' . $this->plural($finding->classes, 'class', 'classes') + . ' (terminal: ' . $this->terminalClause($finding) . ')'; + } + + private function terminalClause(Finding $finding): string + { + if ($finding->terminal === null) { + return $finding->terminalKind->value; + } + + return $finding->terminal . ' [' . $finding->terminalKind->value . ']'; + } + + private function plural(int $count, string $singular, ?string $plural = null): string + { + if ($count === 1) { + return $singular; + } + + return $plural ?? $singular . 's'; + } +} diff --git a/src/Report/GithubReporter.php b/src/Report/GithubReporter.php new file mode 100644 index 0000000..abf6378 --- /dev/null +++ b/src/Report/GithubReporter.php @@ -0,0 +1,114 @@ +message = new FindingMessage(); + } + + /** + * @param list $findings ALL findings, unfiltered + */ + public function render(array $findings): string + { + $lines = []; + foreach ($findings as $finding) { + $severity = $this->thresholds->severityOf($finding); + if ($severity === null) { + continue; + } + foreach ($this->annotationLines($finding, $severity) as $line) { + $lines[] = $line; + } + } + + if ($lines === []) { + return ''; + } + + return implode("\n", $lines) . "\n"; + } + + /** + * @return list + */ + private function annotationLines(Finding $finding, Severity $severity): array + { + $lines = [$this->originAnnotation($finding, $severity)]; + foreach ($this->subsequentHops($finding) as $index => $hop) { + $lines[] = $this->hopNotice($finding, $hop, $index); + } + + return $lines; + } + + private function originAnnotation(Finding $finding, Severity $severity): string + { + $origin = $finding->chain[0]; + $title = 'phptramp::' . $this->message->describe($finding); + + return '::' . $this->command($severity) + . ' file=' . $this->escape($this->paths->relativize($origin->file)) + . ',line=' . $origin->line + . ',title=' . $this->escape($title); + } + + private function hopNotice(Finding $finding, Hop $hop, int $index): string + { + $title = 'phptramp::hop ' . ($index + 1) . ' of $' . $finding->param . ' chain from ' . $finding->origin; + + return '::notice file=' . $this->escape($this->paths->relativize($hop->file)) + . ',line=' . $hop->line + . ',title=' . $this->escape($title); + } + + /** + * @return array subsequent-hop entries keyed by their chain index + * (1 .. hops-1) — the terminal node, at index + * hops, is never included + */ + private function subsequentHops(Finding $finding): array + { + $hops = []; + for ($index = 1; $index < $finding->hops; $index++) { + $hops[$index] = $finding->chain[$index]; + } + + return $hops; + } + + private function command(Severity $severity): string + { + return match ($severity) { + Severity::Error => 'error', + Severity::Warning => 'warning', + }; + } + + private function escape(string $value): string + { + $value = str_replace('%', '%25', $value); + $value = str_replace("\r", '%0D', $value); + + return str_replace("\n", '%0A', $value); + } +} diff --git a/src/Report/ReporterFactory.php b/src/Report/ReporterFactory.php index 9043b48..4890c1e 100644 --- a/src/Report/ReporterFactory.php +++ b/src/Report/ReporterFactory.php @@ -20,6 +20,7 @@ public function create(Options $options): Reporter return match ($options->format) { 'text' => new TextReporter($thresholds, $options->explain), 'json' => new JsonReporter($thresholds, new Paths($this->workingDirectory)), + 'github' => new GithubReporter($thresholds, new Paths($this->workingDirectory)), default => throw new InvalidArgsException("format '{$options->format}' is not implemented yet."), }; } diff --git a/tests/Report/FindingMessageTest.php b/tests/Report/FindingMessageTest.php new file mode 100644 index 0000000..034b010 --- /dev/null +++ b/tests/Report/FindingMessageTest.php @@ -0,0 +1,78 @@ +describe($finding), + ); + } + + public function testDescribesSingularHopAndClass(): void + { + $chain = [ + new Hop('Demo\A::go', 'Demo\A', 'src/A.php', 5, 7), + new Hop('Demo\A::sink', 'Demo\A', 'src/A.php', 9, null), + ]; + $finding = new Finding('p', 'Demo\A::go', 'Demo\A::sink', TerminalKind::Used, 1, $chain, 1, [], []); + + self::assertSame( + '$p: 1 pass-through hop across 1 class (terminal: Demo\A::sink [used])', + (new FindingMessage())->describe($finding), + ); + } + + public function testDescribesNullTerminalWithKindOnlyClause(): void + { + $chain = [ + new Hop('Demo\A::go', 'Demo\A', 'src/A.php', 5, 7), + new Hop('Demo\B::step', 'Demo\B', 'src/B.php', 9, 11), + ]; + $finding = new Finding( + 'cfg', + 'Demo\A::go', + null, + TerminalKind::Truncated, + 2, + $chain, + 2, + ['truncated: 2 implementations'], + [], + ); + + self::assertSame( + '$cfg: 2 pass-through hops across 2 classes (terminal: truncated)', + (new FindingMessage())->describe($finding), + ); + } +} diff --git a/tests/Report/GithubReporterTest.php b/tests/Report/GithubReporterTest.php new file mode 100644 index 0000000..451f6a9 --- /dev/null +++ b/tests/Report/GithubReporterTest.php @@ -0,0 +1,117 @@ +paths()); + + self::assertSame($expected, $reporter->render([$finding])); + } + + public function testEmitsWarningAnnotationAndOmitsNoticeWhenNoSubsequentHops(): void + { + $chain = [ + new Hop('Demo\A::go', 'Demo\A', 'src/A.php', 5, 7), + new Hop('Demo\A::sink', 'Demo\A', 'src/A.php', 9, null), + ]; + $finding = new Finding('p', 'Demo\A::go', 'Demo\A::sink', TerminalKind::Used, 1, $chain, 1, [], []); + + $expected = "::warning file=src/A.php,line=5,title=phptramp::\$p: 1 pass-through hop across 1 class " + . "(terminal: Demo\\A::sink [used])\n"; + + $reporter = new GithubReporter(new Thresholds(3, 1), $this->paths()); + + self::assertSame($expected, $reporter->render([$finding])); + } + + public function testOmitsBelowThresholdFindingEntirely(): void + { + $chain = [ + new Hop('Demo\C::go', 'Demo\C', 'src/C.php', 5, null), + ]; + $finding = new Finding('q', 'Demo\C::go', 'Demo\C::go', TerminalKind::Used, 1, $chain, 1, [], []); + + $reporter = new GithubReporter(new Thresholds(3, 2), $this->paths()); + + self::assertSame('', $reporter->render([$finding])); + } + + public function testEscapesPercentCarriageReturnAndNewlineInAnnotationData(): void + { + $chain = [ + new Hop("Demo\\A::go\r\nInjected", 'Demo\A', 'src/100%/A.php', 5, null), + ]; + $finding = new Finding( + 'p', + "Demo\\A::go\r\nInjected", + "Demo\\A::go\r\nInjected", + TerminalKind::Used, + 1, + $chain, + 1, + [], + [], + ); + + $expected = '::error file=src/100%25/A.php,line=5,title=phptramp::$p: 1 pass-through hop across 1 class ' + . "(terminal: Demo\\A::go%0D%0AInjected [used])\n"; + + $reporter = new GithubReporter(new Thresholds(1, null), $this->paths()); + + self::assertSame($expected, $reporter->render([$finding])); + } + + public function testEmptyRunRendersEmptyString(): void + { + $reporter = new GithubReporter(new Thresholds(3, null), $this->paths()); + + self::assertSame('', $reporter->render([])); + } +} From 249f328adf9ebcc2f60581c5f16e81ef8909c8e6 Mon Sep 17 00:00:00 2001 From: Lars Pohlmann Date: Tue, 11 Aug 2026 11:52:08 +0200 Subject: [PATCH 09/26] feat(#5): checkstyle reporter Adds CheckstyleReporter: findings grouped into elements by relativized origin file (first-seen file order, input order within a file), one per finding at its origin line, message rendered via the shared FindingMessage and XML-escaped with ENT_XML1 | ENT_QUOTES. Empty runs still produce the well-formed skeleton with zero children. --- src/Report/CheckstyleReporter.php | 100 ++++++++++++++ src/Report/ReporterFactory.php | 1 + tests/Report/CheckstyleReporterTest.php | 176 ++++++++++++++++++++++++ 3 files changed, 277 insertions(+) create mode 100644 src/Report/CheckstyleReporter.php create mode 100644 tests/Report/CheckstyleReporterTest.php diff --git a/src/Report/CheckstyleReporter.php b/src/Report/CheckstyleReporter.php new file mode 100644 index 0000000..b403499 --- /dev/null +++ b/src/Report/CheckstyleReporter.php @@ -0,0 +1,100 @@ +` per finding at + * its origin line, grouped into `` elements by origin file. Consumed by + * CI tooling that already understands the Checkstyle format (GitLab Code + * Quality, Jenkins Warnings NG, ...). + */ +final class CheckstyleReporter implements Reporter +{ + private readonly FindingMessage $message; + + public function __construct( + private readonly Thresholds $thresholds, + private readonly Paths $paths, + ) { + $this->message = new FindingMessage(); + } + + /** + * @param list $findings ALL findings, unfiltered + */ + public function render(array $findings): string + { + $lines = ['', '']; + foreach ($this->groupByOriginFile($findings) as $file => $fileFindings) { + foreach ($this->fileBlock($file, $fileFindings) as $line) { + $lines[] = $line; + } + } + $lines[] = ''; + + return implode("\n", $lines) . "\n"; + } + + /** + * @param list $fileFindings + * @return list + */ + private function fileBlock(string $file, array $fileFindings): array + { + $lines = [' ']; + foreach ($fileFindings as [$finding, $severity]) { + $lines[] = $this->errorLine($finding, $severity); + } + $lines[] = ' '; + + return $lines; + } + + private function errorLine(Finding $finding, Severity $severity): string + { + $origin = $finding->chain[0]; + + return ' '; + } + + /** + * Groups reportable findings by relativized origin file, in first-seen + * file order; findings within a file keep their input order. + * + * @param list $findings + * @return array> + */ + private function groupByOriginFile(array $findings): array + { + $groups = []; + foreach ($findings as $finding) { + $severity = $this->thresholds->severityOf($finding); + if ($severity === null) { + continue; + } + $file = $this->paths->relativize($finding->chain[0]->file); + $groups[$file][] = [$finding, $severity]; + } + + return $groups; + } + + private function severityLabel(Severity $severity): string + { + return match ($severity) { + Severity::Error => 'error', + Severity::Warning => 'warning', + }; + } + + private function escape(string $value): string + { + return htmlspecialchars($value, ENT_XML1 | ENT_QUOTES, 'UTF-8'); + } +} diff --git a/src/Report/ReporterFactory.php b/src/Report/ReporterFactory.php index 4890c1e..a6a2f86 100644 --- a/src/Report/ReporterFactory.php +++ b/src/Report/ReporterFactory.php @@ -21,6 +21,7 @@ public function create(Options $options): Reporter 'text' => new TextReporter($thresholds, $options->explain), 'json' => new JsonReporter($thresholds, new Paths($this->workingDirectory)), 'github' => new GithubReporter($thresholds, new Paths($this->workingDirectory)), + 'checkstyle' => new CheckstyleReporter($thresholds, new Paths($this->workingDirectory)), default => throw new InvalidArgsException("format '{$options->format}' is not implemented yet."), }; } diff --git a/tests/Report/CheckstyleReporterTest.php b/tests/Report/CheckstyleReporterTest.php new file mode 100644 index 0000000..60f6a28 --- /dev/null +++ b/tests/Report/CheckstyleReporterTest.php @@ -0,0 +1,176 @@ +' . "\n" + . '' . "\n" + . ' ' . "\n" + . ' ' . "\n" + . ' ' . "\n" + . '' . "\n"; + + $reporter = new CheckstyleReporter(new Thresholds(3, null), $this->paths()); + + self::assertSame($expected, $reporter->render([$finding])); + } + + public function testRendersWarningSeverityLabel(): void + { + $chain = [ + 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), + ]; + $finding = new Finding('p', 'Demo\A::go', 'Demo\C::sink', TerminalKind::Used, 2, $chain, 2, [], []); + + $expected = '' . "\n" + . '' . "\n" + . ' ' . "\n" + . ' ' . "\n" + . ' ' . "\n" + . '' . "\n"; + + $reporter = new CheckstyleReporter(new Thresholds(3, 2), $this->paths()); + + self::assertSame($expected, $reporter->render([$finding])); + } + + public function testGroupsFindingsByOriginFilePreservingFirstSeenAndInputOrder(): void + { + $aFinding = new Finding( + 'p', + 'Demo\A::go', + 'Demo\A::sink', + TerminalKind::Used, + 1, + [ + new Hop('Demo\A::go', 'Demo\A', 'src/A.php', 5, 7), + new Hop('Demo\A::sink', 'Demo\A', 'src/A.php', 9, null), + ], + 1, + [], + [], + ); + $bFinding = new Finding( + 'q', + 'Demo\B::go', + 'Demo\B::sink', + TerminalKind::Used, + 1, + [ + new Hop('Demo\B::go', 'Demo\B', 'src/B.php', 15, 17), + new Hop('Demo\B::sink', 'Demo\B', 'src/B.php', 19, null), + ], + 1, + [], + [], + ); + $secondAFinding = new Finding( + 'r', + 'Demo\A::again', + 'Demo\A::sink', + TerminalKind::Used, + 1, + [ + new Hop('Demo\A::again', 'Demo\A', 'src/A.php', 25, 27), + new Hop('Demo\A::sink', 'Demo\A', 'src/A.php', 29, null), + ], + 1, + [], + [], + ); + + $expected = '' . "\n" + . '' . "\n" + . ' ' . "\n" + . ' ' . "\n" + . ' ' . "\n" + . ' ' . "\n" + . ' ' . "\n" + . ' ' . "\n" + . ' ' . "\n" + . '' . "\n"; + + $reporter = new CheckstyleReporter(new Thresholds(1, null), $this->paths()); + + self::assertSame($expected, $reporter->render([$aFinding, $bFinding, $secondAFinding])); + } + + public function testEscapesMessageXmlEntities(): void + { + $chain = [ + new Hop('Demo\A::go', 'Demo\A', 'src/A.php', 5, 7), + new Hop('Demo\B::sink', 'Demo\B', 'src/A.php', 9, null), + ]; + $finding = new Finding('p', 'Demo\A::go', 'Demo\B::sink', TerminalKind::Used, 1, $chain, 1, [], []); + + $expected = '' . "\n" + . '' . "\n" + . ' ' . "\n" + . ' ' . "\n" + . ' ' . "\n" + . '' . "\n"; + + $reporter = new CheckstyleReporter(new Thresholds(1, null), $this->paths()); + + self::assertSame($expected, $reporter->render([$finding])); + } + + public function testEmptyRunRendersSkeletonWithNoFileElements(): void + { + $expected = '' . "\n" + . '' . "\n" + . '' . "\n"; + + $reporter = new CheckstyleReporter(new Thresholds(3, null), $this->paths()); + + self::assertSame($expected, $reporter->render([])); + } +} From 612ff29cca4837c9b3bb3e65285ccc3584012883 Mon Sep 17 00:00:00 2001 From: Lars Pohlmann Date: Tue, 11 Aug 2026 11:56:03 +0200 Subject: [PATCH 10/26] feat(#5): sarif reporter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds SarifReporter: SARIF 2.1.0 log with one run, one rule (phptramp.trampData), and one result per finding — ruleId/level/ message.text (via the shared FindingMessage), a location at the origin, and a relatedLocations entry per subsequent hop (omitted entirely when a finding has none). The driver deliberately carries no `version` property so fixtures stay stable across releases. Encoded with JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES. 'github', 'checkstyle', and 'sarif' are now all wired into ReporterFactory; only 'summary' (Task 8) remains unimplemented, so ApplicationTest's unsupported-format case switches to that. --- src/Report/ReporterFactory.php | 1 + src/Report/SarifReporter.php | 159 ++++++++++++++++++++ tests/Console/ApplicationTest.php | 2 +- tests/Report/SarifReporterTest.php | 233 +++++++++++++++++++++++++++++ 4 files changed, 394 insertions(+), 1 deletion(-) create mode 100644 src/Report/SarifReporter.php create mode 100644 tests/Report/SarifReporterTest.php diff --git a/src/Report/ReporterFactory.php b/src/Report/ReporterFactory.php index a6a2f86..679b25b 100644 --- a/src/Report/ReporterFactory.php +++ b/src/Report/ReporterFactory.php @@ -22,6 +22,7 @@ public function create(Options $options): Reporter 'json' => new JsonReporter($thresholds, new Paths($this->workingDirectory)), 'github' => new GithubReporter($thresholds, new Paths($this->workingDirectory)), 'checkstyle' => new CheckstyleReporter($thresholds, new Paths($this->workingDirectory)), + 'sarif' => new SarifReporter($thresholds, new Paths($this->workingDirectory)), default => throw new InvalidArgsException("format '{$options->format}' is not implemented yet."), }; } diff --git a/src/Report/SarifReporter.php b/src/Report/SarifReporter.php new file mode 100644 index 0000000..ae48c36 --- /dev/null +++ b/src/Report/SarifReporter.php @@ -0,0 +1,159 @@ +message = new FindingMessage(); + } + + /** + * @param list $findings ALL findings, unfiltered + */ + public function render(array $findings): string + { + $document = [ + 'version' => '2.1.0', + '$schema' => self::SCHEMA_URL, + 'runs' => [$this->run($findings)], + ]; + + return $this->encode($document) . "\n"; + } + + /** + * @param list $findings + * @return array + */ + private function run(array $findings): array + { + return [ + 'tool' => [ + 'driver' => [ + 'name' => 'phptramp', + 'informationUri' => 'https://github.com/larspohlmann/phptramp', + 'rules' => [ + ['id' => 'phptramp.trampData'], + ], + ], + ], + 'results' => $this->results($findings), + ]; + } + + /** + * @param list $findings + * @return list> + */ + private function results(array $findings): array + { + $results = []; + foreach ($findings as $finding) { + $severity = $this->thresholds->severityOf($finding); + if ($severity === null) { + continue; + } + $results[] = $this->result($finding, $severity); + } + + return $results; + } + + /** + * @return array + */ + private function result(Finding $finding, Severity $severity): array + { + $result = [ + 'ruleId' => 'phptramp.trampData', + 'level' => $this->levelLabel($severity), + 'message' => ['text' => $this->message->describe($finding)], + 'locations' => [$this->location($finding->chain[0])], + ]; + + $relatedLocations = $this->relatedLocations($finding); + if ($relatedLocations !== []) { + $result['relatedLocations'] = $relatedLocations; + } + + return $result; + } + + /** + * @return list> one entry per subsequent hop (chain + * indices 1 .. hops-1); the terminal + * node is never included + */ + private function relatedLocations(Finding $finding): array + { + $locations = []; + for ($index = 1; $index < $finding->hops; $index++) { + $hop = $finding->chain[$index]; + $location = $this->location($hop); + $location['message'] = [ + 'text' => 'hop ' . ($index + 1) . ' of $' . $finding->param . ' chain from ' . $finding->origin, + ]; + $locations[] = $location; + } + + return $locations; + } + + /** + * @return array + */ + private function location(Hop $hop): array + { + return [ + 'physicalLocation' => [ + 'artifactLocation' => ['uri' => $this->paths->relativize($hop->file)], + 'region' => ['startLine' => $hop->line], + ], + ]; + } + + private function levelLabel(Severity $severity): string + { + return match ($severity) { + Severity::Error => 'error', + Severity::Warning => 'warning', + }; + } + + /** + * @param array $document + */ + private function encode(array $document): string + { + $json = json_encode($document, JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES); + if ($json === false) { + // Unreachable: $document is built entirely from scalars, strings, and + // arrays thereof — nothing here can produce a resource or invalid + // UTF-8 that would make json_encode fail. + throw new JsonEncodingException('Failed to encode findings as SARIF.'); + } + + return $json; + } +} diff --git a/tests/Console/ApplicationTest.php b/tests/Console/ApplicationTest.php index 35f84c8..927a5d6 100644 --- a/tests/Console/ApplicationTest.php +++ b/tests/Console/ApplicationTest.php @@ -101,7 +101,7 @@ public function testUnsupportedFormatExitsWithToolErrorCode(): void { $folder = $this->fixtureWithThreeHopChain(); - self::assertSame(2, $this->app->run(['phptramp', '--folder', $folder, '--format', 'sarif'])); + self::assertSame(2, $this->app->run(['phptramp', '--folder', $folder, '--format', 'summary'])); self::assertStringContainsString('not implemented', self::contents($this->stderr)); self::assertSame('', self::contents($this->stdout)); } diff --git a/tests/Report/SarifReporterTest.php b/tests/Report/SarifReporterTest.php new file mode 100644 index 0000000..3f96cc1 --- /dev/null +++ b/tests/Report/SarifReporterTest.php @@ -0,0 +1,233 @@ +schemaUrl()}", + "runs": [ + { + "tool": { + "driver": { + "name": "phptramp", + "informationUri": "https://github.com/larspohlmann/phptramp", + "rules": [ + { + "id": "phptramp.trampData" + } + ] + } + }, + "results": [ + { + "ruleId": "phptramp.trampData", + "level": "error", + "message": { + JSON; + $expected .= "\n" . ' "text": "$config: 3 pass-through hops across 4 classes ' + . "(terminal: Demo\\\\Mailer::__construct [stored])\"\n"; + $expected .= <<paths()); + + self::assertSame($expected, $reporter->render([$finding])); + } + + public function testRendersWarningFindingAndOmitsRelatedLocationsWhenNoSubsequentHops(): void + { + $chain = [ + new Hop('Demo\A::go', 'Demo\A', 'src/A.php', 5, 7), + new Hop('Demo\A::sink', 'Demo\A', 'src/A.php', 9, null), + ]; + $finding = new Finding('p', 'Demo\A::go', 'Demo\A::sink', TerminalKind::Used, 1, $chain, 1, [], []); + + $expected = <<schemaUrl()}", + "runs": [ + { + "tool": { + "driver": { + "name": "phptramp", + "informationUri": "https://github.com/larspohlmann/phptramp", + "rules": [ + { + "id": "phptramp.trampData" + } + ] + } + }, + "results": [ + { + "ruleId": "phptramp.trampData", + "level": "warning", + "message": { + "text": "\$p: 1 pass-through hop across 1 class (terminal: Demo\\\\A::sink [used])" + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/A.php" + }, + "region": { + "startLine": 5 + } + } + } + ] + } + ] + } + ] + } + + JSON; + + $reporter = new SarifReporter(new Thresholds(3, 1), $this->paths()); + + self::assertSame($expected, $reporter->render([$finding])); + } + + public function testOmitsBelowThresholdFindingEntirely(): void + { + $chain = [ + new Hop('Demo\C::go', 'Demo\C', 'src/C.php', 5, null), + ]; + $finding = new Finding('q', 'Demo\C::go', 'Demo\C::go', TerminalKind::Used, 1, $chain, 1, [], []); + + $reporter = new SarifReporter(new Thresholds(3, 2), $this->paths()); + + self::assertStringNotContainsString('"ruleId"', $reporter->render([$finding])); + } + + public function testEmptyRunRendersValidDocumentWithEmptyResults(): void + { + $expected = <<schemaUrl()}", + "runs": [ + { + "tool": { + "driver": { + "name": "phptramp", + "informationUri": "https://github.com/larspohlmann/phptramp", + "rules": [ + { + "id": "phptramp.trampData" + } + ] + } + }, + "results": [] + } + ] + } + + JSON; + + $reporter = new SarifReporter(new Thresholds(3, null), $this->paths()); + + self::assertSame($expected, $reporter->render([])); + } + + private function schemaUrl(): string + { + return self::SCHEMA_URL; + } +} From 29c98d6d7abc55b3503de13632628ebef9582d9c Mon Sep 17 00:00:00 2001 From: Lars Pohlmann Date: Tue, 11 Aug 2026 12:08:11 +0200 Subject: [PATCH 11/26] test(#5): close mutation gaps in machine reporters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds coverage for four escaped-mutant gaps found in review: - ReporterFactoryTest: drives --format github/checkstyle/sarif through ReporterFactory::create(), asserting a format-appropriate marker in the rendered output. Kills MatchArmRemoval on those three arms (a removed/mistyped arm falls through to the default arm's exception instead of routing to the right reporter). - GithubReporterTest / SarifReporterTest: a below-threshold finding followed by an above-threshold one, asserting the second finding is still reported. Findings are not sorted by hop count, so the per-finding `continue` (skip, keep scanning) must not regress to a `break` (stop scanning, silently drop everything after the first skip) — verified this is already correct, then pinned it. - SarifReporterTest: two qualifying findings, asserting both appear in `results`. Kills ArrayOneItem truncation on the results-building loop. Also rides along, per CLAUDE.md's 3rd-occurrence DRY rule: extracts Severity::label() (the 'error'/'warning' match) and replaces the four now-duplicated local copies in JsonReporter, GithubReporter, CheckstyleReporter, and SarifReporter. New SeverityTest pins both cases. No reporter output changed — all pre-existing exact-string tests still pass byte-for-byte. --- src/Report/CheckstyleReporter.php | 10 +---- src/Report/GithubReporter.php | 10 +---- src/Report/JsonReporter.php | 10 +---- src/Report/SarifReporter.php | 10 +---- src/Report/Severity.php | 9 ++++ tests/Report/GithubReporterTest.php | 45 +++++++++++++++++++ tests/Report/ReporterFactoryTest.php | 61 +++++++++++++++++++++++++ tests/Report/SarifReporterTest.php | 67 ++++++++++++++++++++++++++++ tests/Report/SeverityTest.php | 21 +++++++++ 9 files changed, 207 insertions(+), 36 deletions(-) create mode 100644 tests/Report/ReporterFactoryTest.php create mode 100644 tests/Report/SeverityTest.php diff --git a/src/Report/CheckstyleReporter.php b/src/Report/CheckstyleReporter.php index b403499..1571a33 100644 --- a/src/Report/CheckstyleReporter.php +++ b/src/Report/CheckstyleReporter.php @@ -58,7 +58,7 @@ private function errorLine(Finding $finding, Severity $severity): string { $origin = $finding->chain[0]; - return ' line . '" severity="' . $severity->label() . '" message="' . $this->escape($this->message->describe($finding)) . '" source="phptramp.trampData"/>'; } @@ -85,14 +85,6 @@ private function groupByOriginFile(array $findings): array return $groups; } - private function severityLabel(Severity $severity): string - { - return match ($severity) { - Severity::Error => 'error', - Severity::Warning => 'warning', - }; - } - private function escape(string $value): string { return htmlspecialchars($value, ENT_XML1 | ENT_QUOTES, 'UTF-8'); diff --git a/src/Report/GithubReporter.php b/src/Report/GithubReporter.php index abf6378..ec1515b 100644 --- a/src/Report/GithubReporter.php +++ b/src/Report/GithubReporter.php @@ -66,7 +66,7 @@ private function originAnnotation(Finding $finding, Severity $severity): string $origin = $finding->chain[0]; $title = 'phptramp::' . $this->message->describe($finding); - return '::' . $this->command($severity) + return '::' . $severity->label() . ' file=' . $this->escape($this->paths->relativize($origin->file)) . ',line=' . $origin->line . ',title=' . $this->escape($title); @@ -96,14 +96,6 @@ private function subsequentHops(Finding $finding): array return $hops; } - private function command(Severity $severity): string - { - return match ($severity) { - Severity::Error => 'error', - Severity::Warning => 'warning', - }; - } - private function escape(string $value): string { $value = str_replace('%', '%25', $value); diff --git a/src/Report/JsonReporter.php b/src/Report/JsonReporter.php index 9834d75..e0aee9f 100644 --- a/src/Report/JsonReporter.php +++ b/src/Report/JsonReporter.php @@ -58,7 +58,7 @@ private function findingDocument(Finding $finding, Severity $severity): array { return [ 'param' => $finding->param, - 'severity' => $this->severityLabel($severity), + 'severity' => $severity->label(), 'origin' => $finding->origin, 'terminal' => $finding->terminal, 'terminalKind' => $finding->terminalKind->value, @@ -69,14 +69,6 @@ private function findingDocument(Finding $finding, Severity $severity): array ]; } - private function severityLabel(Severity $severity): string - { - return match ($severity) { - Severity::Error => 'error', - Severity::Warning => 'warning', - }; - } - /** * @return list> */ diff --git a/src/Report/SarifReporter.php b/src/Report/SarifReporter.php index ae48c36..e76d07b 100644 --- a/src/Report/SarifReporter.php +++ b/src/Report/SarifReporter.php @@ -87,7 +87,7 @@ private function result(Finding $finding, Severity $severity): array { $result = [ 'ruleId' => 'phptramp.trampData', - 'level' => $this->levelLabel($severity), + 'level' => $severity->label(), 'message' => ['text' => $this->message->describe($finding)], 'locations' => [$this->location($finding->chain[0])], ]; @@ -133,14 +133,6 @@ private function location(Hop $hop): array ]; } - private function levelLabel(Severity $severity): string - { - return match ($severity) { - Severity::Error => 'error', - Severity::Warning => 'warning', - }; - } - /** * @param array $document */ diff --git a/src/Report/Severity.php b/src/Report/Severity.php index 3eea41b..dc33ef4 100644 --- a/src/Report/Severity.php +++ b/src/Report/Severity.php @@ -8,4 +8,13 @@ enum Severity { case Error; case Warning; + + /** The lowercase token every machine reporter emits for this severity. */ + public function label(): string + { + return match ($this) { + self::Error => 'error', + self::Warning => 'warning', + }; + } } diff --git a/tests/Report/GithubReporterTest.php b/tests/Report/GithubReporterTest.php index 451f6a9..9857823 100644 --- a/tests/Report/GithubReporterTest.php +++ b/tests/Report/GithubReporterTest.php @@ -108,6 +108,51 @@ public function testEscapesPercentCarriageReturnAndNewlineInAnnotationData(): vo self::assertSame($expected, $reporter->render([$finding])); } + /** + * Findings are not sorted by hop count, so a below-threshold finding can + * precede an above-threshold one in real input order. This pins that the + * per-finding `continue` (skip this finding, keep scanning) is not a + * `break` in disguise (stop scanning entirely) — a `break` here would + * silently drop every qualifying finding that comes after the first + * skipped one. + */ + public function testStillReportsQualifyingFindingAfterAnEarlierBelowThresholdFindingIsSkipped(): void + { + $belowChain = [ + new Hop('Demo\Z::go', 'Demo\Z', 'src/Z.php', 1, null), + ]; + $belowFinding = new Finding('z', 'Demo\Z::go', 'Demo\Z::go', TerminalKind::Used, 1, $belowChain, 1, [], []); + + $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), + new Hop('Demo\ServiceB::run', 'Demo\ServiceB', 'src/Demo.php', 24, 26), + new Hop('Demo\Mailer::__construct', 'Demo\Mailer', 'src/Demo.php', 32, null), + ]; + $aboveFinding = new Finding( + 'config', + 'Demo\Controller::handle', + 'Demo\Mailer::__construct', + TerminalKind::Stored, + 3, + $chain, + 4, + [], + [], + ); + + $expected = "::error file=src/Demo.php,line=12,title=phptramp::\$config: 3 pass-through hops across 4 " + . "classes (terminal: Demo\\Mailer::__construct [stored])\n" + . "::notice file=src/Demo.php,line=18,title=phptramp::hop 2 of \$config chain from " + . "Demo\\Controller::handle\n" + . "::notice file=src/Demo.php,line=24,title=phptramp::hop 3 of \$config chain from " + . "Demo\\Controller::handle\n"; + + $reporter = new GithubReporter(new Thresholds(3, null), $this->paths()); + + self::assertSame($expected, $reporter->render([$belowFinding, $aboveFinding])); + } + public function testEmptyRunRendersEmptyString(): void { $reporter = new GithubReporter(new Thresholds(3, null), $this->paths()); diff --git a/tests/Report/ReporterFactoryTest.php b/tests/Report/ReporterFactoryTest.php new file mode 100644 index 0000000..c4cb894 --- /dev/null +++ b/tests/Report/ReporterFactoryTest.php @@ -0,0 +1,61 @@ + a single finding at or above every threshold used + * below (limit: 2) + */ + private function qualifyingFinding(): array + { + $chain = [ + 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), + ]; + + return [new Finding('p', 'Demo\A::go', 'Demo\C::sink', TerminalKind::Used, 2, $chain, 2, [], [])]; + } + + public function testGithubFormatRoutesToGithubReporter(): void + { + $factory = new ReporterFactory('/not/matching'); + $reporter = $factory->create(new Options(format: 'github', limit: 2)); + + self::assertStringContainsString('::error', $reporter->render($this->qualifyingFinding())); + } + + public function testCheckstyleFormatRoutesToCheckstyleReporter(): void + { + $factory = new ReporterFactory('/not/matching'); + $reporter = $factory->create(new Options(format: 'checkstyle', limit: 2)); + + self::assertStringContainsString('render($this->qualifyingFinding())); + } + + public function testSarifFormatRoutesToSarifReporter(): void + { + $factory = new ReporterFactory('/not/matching'); + $reporter = $factory->create(new Options(format: 'sarif', limit: 2)); + + self::assertStringContainsString('"phptramp.trampData"', $reporter->render($this->qualifyingFinding())); + } +} diff --git a/tests/Report/SarifReporterTest.php b/tests/Report/SarifReporterTest.php index 3f96cc1..2988b61 100644 --- a/tests/Report/SarifReporterTest.php +++ b/tests/Report/SarifReporterTest.php @@ -195,6 +195,73 @@ public function testOmitsBelowThresholdFindingEntirely(): void self::assertStringNotContainsString('"ruleId"', $reporter->render([$finding])); } + /** + * Findings are not sorted by hop count, so a below-threshold finding can + * precede an above-threshold one in real input order. This pins that the + * per-finding `continue` (skip this finding, keep scanning) is not a + * `break` in disguise (stop scanning entirely) — a `break` here would + * silently drop every qualifying finding that comes after the first + * skipped one. + */ + public function testStillIncludesQualifyingFindingAfterAnEarlierBelowThresholdFindingIsSkipped(): void + { + $belowChain = [ + new Hop('Demo\Z::go', 'Demo\Z', 'src/Z.php', 1, null), + ]; + $belowFinding = new Finding('z', 'Demo\Z::go', 'Demo\Z::go', TerminalKind::Used, 1, $belowChain, 1, [], []); + + $aboveChain = [ + 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), + ]; + $aboveFinding = new Finding('p', 'Demo\A::go', 'Demo\C::sink', TerminalKind::Used, 2, $aboveChain, 2, [], []); + + $reporter = new SarifReporter(new Thresholds(3, 2), $this->paths()); + + $document = json_decode($reporter->render([$belowFinding, $aboveFinding]), true); + $results = $document['runs'][0]['results']; + + self::assertCount(1, $results); + self::assertSame('warning', $results[0]['level']); + self::assertSame( + '$p: 2 pass-through hops across 2 classes (terminal: Demo\C::sink [used])', + $results[0]['message']['text'], + ); + } + + public function testIncludesBothResultsWhenMultipleFindingsQualify(): void + { + $firstChain = [ + new Hop('Demo\A::go', 'Demo\A', 'src/A.php', 5, 7), + new Hop('Demo\A::sink', 'Demo\A', 'src/A.php', 9, null), + ]; + $firstFinding = new Finding('p', 'Demo\A::go', 'Demo\A::sink', TerminalKind::Used, 1, $firstChain, 1, [], []); + + $secondChain = [ + new Hop('Demo\B::go', 'Demo\B', 'src/B.php', 5, 7), + new Hop('Demo\B::sink', 'Demo\B', 'src/B.php', 9, null), + ]; + $secondFinding = new Finding('q', 'Demo\B::go', 'Demo\B::sink', TerminalKind::Used, 1, $secondChain, 1, [], []); + + $reporter = new SarifReporter(new Thresholds(1, null), $this->paths()); + + $document = json_decode($reporter->render([$firstFinding, $secondFinding]), true); + $results = $document['runs'][0]['results']; + + self::assertCount(2, $results); + self::assertSame('phptramp.trampData', $results[0]['ruleId']); + self::assertSame('phptramp.trampData', $results[1]['ruleId']); + self::assertSame( + '$p: 1 pass-through hop across 1 class (terminal: Demo\A::sink [used])', + $results[0]['message']['text'], + ); + self::assertSame( + '$q: 1 pass-through hop across 1 class (terminal: Demo\B::sink [used])', + $results[1]['message']['text'], + ); + } + public function testEmptyRunRendersValidDocumentWithEmptyResults(): void { $expected = <<label()); + } + + public function testWarningLabelIsWarning(): void + { + self::assertSame('warning', Severity::Warning->label()); + } +} From 9a5a1b51d797a9370426b5436e119594baf5753f Mon Sep 17 00:00:00 2001 From: Lars Pohlmann Date: Tue, 11 Aug 2026 12:27:43 +0200 Subject: [PATCH 12/26] feat(#5): TrampIgnore attribute and comment suppression Adds two suppression mechanisms for tramp-data findings: a #[TrampIgnore] attribute (matched by short name, so analyzed codebases need not autoload the class) on classes, methods, functions, or individual parameters, and a `// phptramp-ignore` line comment on a method's declaration line, the line above it, or a forwarding call site. IndexingVisitor collects both during the existing traversal into a SuppressionIndex carried by MethodIndex; ChainTraversal drops a finding if any hop node (never the terminal) is suppressed. The SuppressionIndex default keeps every existing MethodIndex call site working unchanged. --- src/Chain/ChainTraversal.php | 36 ++++++++++ src/Ignore/SuppressionIndex.php | 81 ++++++++++++++++++++++ src/Ignore/TrampIgnore.php | 20 ++++++ src/Index/Indexer.php | 7 +- src/Index/IndexingVisitor.php | 86 +++++++++++++++++++++++- src/Index/MethodIndex.php | 11 ++- tests/Chain/ChainBuilderTest.php | 97 +++++++++++++++++++++++++++ tests/Ignore/SuppressionIndexTest.php | 47 +++++++++++++ 8 files changed, 382 insertions(+), 3 deletions(-) create mode 100644 src/Ignore/SuppressionIndex.php create mode 100644 src/Ignore/TrampIgnore.php create mode 100644 tests/Ignore/SuppressionIndexTest.php diff --git a/src/Chain/ChainTraversal.php b/src/Chain/ChainTraversal.php index 934d60c..f96f121 100644 --- a/src/Chain/ChainTraversal.php +++ b/src/Chain/ChainTraversal.php @@ -4,6 +4,7 @@ namespace PhpTramp\Chain; +use PhpTramp\Ignore\SuppressionIndex; use PhpTramp\Index\ForwardSite; use PhpTramp\Index\MethodIndex; use PhpTramp\Index\MethodInfo; @@ -35,10 +36,13 @@ final class ChainTraversal /** @var array */ private array $visited = []; + private readonly SuppressionIndex $suppressions; + public function __construct( private readonly MethodIndex $index, private readonly CallResolver $resolver, ) { + $this->suppressions = $index->suppressions(); } /** @@ -182,6 +186,10 @@ private function terminalKind(ParamInfo $param): TerminalKind private function record(PartialChain $chain, Terminal $terminal): void { + if ($this->isSuppressedChain($chain)) { + return; + } + $fullChain = $terminal->hop === null ? $chain->hops : [...$chain->hops, $terminal->hop]; $this->findings[] = new Finding( @@ -197,6 +205,34 @@ private function record(PartialChain $chain, Terminal $terminal): void ); } + private function isSuppressedChain(PartialChain $chain): bool + { + foreach ($chain->hops as $index => $hop) { + [$fqmn, $param] = explode(self::KEY_SEPARATOR, $chain->keys[$index], 2); + if ($this->isSuppressedHop($hop, $fqmn, $param)) { + return true; + } + } + + return false; + } + + private function isSuppressedHop(Hop $hop, string $fqmn, string $param): bool + { + if ($this->suppressions->suppressesMethod($fqmn) || $this->suppressions->suppressesParam($fqmn, $param)) { + return true; + } + + if ( + $this->suppressions->suppressesLine($hop->file, $hop->line) + || $this->suppressions->suppressesLine($hop->file, $hop->line - 1) + ) { + return true; + } + + return $hop->forwardLine !== null && $this->suppressions->suppressesLine($hop->file, $hop->forwardLine); + } + /** * @param list $chain */ diff --git a/src/Ignore/SuppressionIndex.php b/src/Ignore/SuppressionIndex.php new file mode 100644 index 0000000..1283b89 --- /dev/null +++ b/src/Ignore/SuppressionIndex.php @@ -0,0 +1,81 @@ + */ + private readonly array $methods; + + /** @var array */ + private readonly array $params; + + /** @var array> */ + private readonly array $lines; + + /** + * @param list $methods suppressed method FQMNs (method- or class-level attribute) + * @param list $params [fqmn, paramName] pairs + * @param array> $lines file -> lines carrying an ignore comment + */ + public function __construct(array $methods, array $params, array $lines) + { + $this->methods = array_fill_keys($methods, true); + $this->params = $this->indexParams($params); + $this->lines = $this->indexLines($lines); + } + + public function suppressesMethod(string $fqmn): bool + { + return isset($this->methods[$fqmn]); + } + + public function suppressesParam(string $fqmn, string $param): bool + { + return isset($this->params[$fqmn . self::KEY_SEPARATOR . $param]); + } + + public function suppressesLine(string $file, int $line): bool + { + return isset($this->lines[$file][$line]); + } + + /** + * @param list $params + * @return array + */ + private function indexParams(array $params): array + { + $index = []; + foreach ($params as [$fqmn, $param]) { + $index[$fqmn . self::KEY_SEPARATOR . $param] = true; + } + + return $index; + } + + /** + * @param array> $lines + * @return array> + */ + private function indexLines(array $lines): array + { + $index = []; + foreach ($lines as $file => $lineNumbers) { + $index[$file] = array_fill_keys($lineNumbers, true); + } + + return $index; + } +} diff --git a/src/Ignore/TrampIgnore.php b/src/Ignore/TrampIgnore.php new file mode 100644 index 0000000..6d8af69 --- /dev/null +++ b/src/Ignore/TrampIgnore.php @@ -0,0 +1,20 @@ +classifyPending($visitor->pending()), $visitor->classes()); + return new MethodIndex( + $this->classifyPending($visitor->pending()), + $visitor->classes(), + $visitor->suppressions(), + ); } /** @@ -95,6 +99,7 @@ private function indexFile( } $visitor->setFile($file); + $visitor->recordIgnoreComments($code); $traverser->traverse($ast); return null; diff --git a/src/Index/IndexingVisitor.php b/src/Index/IndexingVisitor.php index 98b8e1a..93b4125 100644 --- a/src/Index/IndexingVisitor.php +++ b/src/Index/IndexingVisitor.php @@ -5,8 +5,11 @@ namespace PhpTramp\Index; use PhpParser\Node; +use PhpParser\Node\AttributeGroup; +use PhpParser\Node\Expr\Variable; use PhpParser\Node\FunctionLike; use PhpParser\Node\Name; +use PhpParser\Node\Param; use PhpParser\Node\Stmt\Class_; use PhpParser\Node\Stmt\ClassLike; use PhpParser\Node\Stmt\ClassMethod; @@ -16,6 +19,7 @@ use PhpParser\Node\Stmt\Trait_; use PhpParser\Node\Stmt\TraitUse; use PhpParser\NodeVisitorAbstract; +use PhpTramp\Ignore\SuppressionIndex; /** * Accumulates class-hierarchy facts and a list of pending function/method @@ -37,6 +41,18 @@ final class IndexingVisitor extends NodeVisitorAbstract /** @var array */ private array $classes = []; + /** @var list */ + private array $suppressedMethods = []; + + /** @var list */ + private array $suppressedClasses = []; + + /** @var list */ + private array $suppressedParams = []; + + /** @var array> */ + private array $ignoreLines = []; + private string $file = ''; public function setFile(string $file): void @@ -67,6 +83,27 @@ public function pending(): array return $this->pending; } + public function recordIgnoreComments(string $code): void + { + foreach (explode("\n", $code) as $index => $lineText) { + if (str_contains($lineText, '// phptramp-ignore')) { + $this->ignoreLines[$this->file][] = $index + 1; + } + } + } + + public function suppressions(): SuppressionIndex + { + $methods = $this->suppressedMethods; + foreach ($this->pending as $entry) { + if ($entry['class'] !== null && in_array($entry['class'], $this->suppressedClasses, true)) { + $methods[] = $entry['fqmn']; + } + } + + return new SuppressionIndex($methods, $this->suppressedParams, $this->ignoreLines); + } + /** * @return array */ @@ -111,6 +148,10 @@ private function recordClass(ClassLike $node): void 'interfaces' => $interfaces, 'traits' => [], ]; + + if ($this->hasTrampIgnore($node->attrGroups)) { + $this->suppressedClasses[] = $fqcn; + } } private function kindOf(ClassLike $node): ClassKind @@ -143,13 +184,16 @@ private function recordMethod(ClassMethod $node): void return; } + $fqmn = $fqcn . '::' . $node->name->toString(); $this->pending[] = [ - 'fqmn' => $fqcn . '::' . $node->name->toString(), + 'fqmn' => $fqmn, 'file' => $this->file, 'line' => $node->getStartLine(), 'class' => $fqcn, 'node' => $node, ]; + + $this->recordFunctionLikeSuppressions($fqmn, $node); } private function recordFunction(Function_ $node): void @@ -166,6 +210,46 @@ private function recordFunction(Function_ $node): void 'class' => null, 'node' => $node, ]; + + $this->recordFunctionLikeSuppressions($fqmn, $node); + } + + private function recordFunctionLikeSuppressions(string $fqmn, FunctionLike $node): void + { + if ($this->hasTrampIgnore($node->getAttrGroups())) { + $this->suppressedMethods[] = $fqmn; + } + + foreach ($node->getParams() as $param) { + if ($this->hasTrampIgnore($param->attrGroups)) { + $this->recordSuppressedParam($fqmn, $param); + } + } + } + + private function recordSuppressedParam(string $fqmn, Param $param): void + { + if (! $param->var instanceof Variable || ! is_string($param->var->name)) { + return; + } + + $this->suppressedParams[] = [$fqmn, $param->var->name]; + } + + /** + * @param array $attrGroups + */ + private function hasTrampIgnore(array $attrGroups): bool + { + foreach ($attrGroups as $group) { + foreach ($group->attrs as $attribute) { + if ($attribute->name->getLast() === 'TrampIgnore') { + return true; + } + } + } + + return false; } private function enclosingClassName(Node $node): ?string diff --git a/src/Index/MethodIndex.php b/src/Index/MethodIndex.php index 49086ab..44bb392 100644 --- a/src/Index/MethodIndex.php +++ b/src/Index/MethodIndex.php @@ -4,9 +4,12 @@ namespace PhpTramp\Index; +use PhpTramp\Ignore\SuppressionIndex; + /** * The whole-project index: fully-qualified method name -> MethodInfo, plus - * per-class hierarchy facts. Immutable; produced by {@see Indexer}. + * per-class hierarchy facts and collected suppressions. Immutable; produced by + * {@see Indexer}. */ final class MethodIndex { @@ -17,9 +20,15 @@ final class MethodIndex public function __construct( private readonly array $methods, private readonly array $classes = [], + private readonly SuppressionIndex $suppressions = new SuppressionIndex([], [], []), ) { } + public function suppressions(): SuppressionIndex + { + return $this->suppressions; + } + public function get(string $fqmn): ?MethodInfo { return $this->methods[$fqmn] ?? null; diff --git a/tests/Chain/ChainBuilderTest.php b/tests/Chain/ChainBuilderTest.php index 8ccd9a8..50bc2e3 100644 --- a/tests/Chain/ChainBuilderTest.php +++ b/tests/Chain/ChainBuilderTest.php @@ -245,4 +245,101 @@ public function testFindingsFromOneOriginAreSortedByParamName(): void self::assertSame('aaa', $findings[0]->param); self::assertSame('bbb', $findings[1]->param); } + + public function testMethodLevelAttributeOnMiddleHopKillsTheChain(): void + { + $code = 'process($config); } } ' + . 'class ServiceA { #[TrampIgnore] public function process(Cfg $config): void { ' + . '(new ServiceB())->run($config); } } ' + . 'class ServiceB { public function run(Cfg $config): void { $config->x(); } }'; + + self::assertCount(0, $this->build($code)); + } + + public function testParamLevelAttributeOnlyKillsChainsOfThatParam(): void + { + $code = 'take($ignored); (new B())->take($kept); } } ' + . 'class B { public function take(Cfg $p): void { $p->x(); } }'; + + $findings = $this->build($code); + self::assertCount(1, $findings); + self::assertSame('kept', $findings[0]->param); + } + + public function testAttributeOnAPlainFunctionMiddleHopKillsTheChain(): void + { + // Function-level (not method-level) attribute suppression: recordFunction + // must collect it, exactly like recordMethod does for class methods. + $code = 'take($p); } ' + . 'class B { public function take(Cfg $p): void { $p->x(); } }'; + + self::assertCount(0, $this->build($code)); + } + + public function testClassLevelAttributeKillsAllChainsThroughAnyMethodOfTheClass(): void + { + // Two independent origins, each forwarding through a different method of + // the suppressed class ServiceA (process, alt) into Sink. Both chains + // must be dropped: class-level suppression covers every method. + $code = 'take($config); } ' + . 'public function alt(Cfg $config): void { (new Sink())->take($config); } } ' + . 'class Controller { public function handle(Cfg $config): void { (new ServiceA())->process($config); } ' + . 'public function handleAlt(Cfg $config): void { (new ServiceA())->alt($config); } } ' + . 'class Sink { public function take(Cfg $config): void { $config->x(); } }'; + + self::assertCount(0, $this->build($code)); + } + + public function testIgnoreCommentOnForwardingCallSiteKillsTheChain(): void + { + $code = "step(\$p); // phptramp-ignore\n } }\n" + . 'class B { public function step(Cfg $p): void { $p->x(); } }'; + + self::assertCount(0, $this->build($code)); + } + + public function testIgnoreCommentOnDeclarationLineKillsTheChain(): void + { + // Comment suppression targets a hop node's own declaration line. B::step + // must be a mid-chain hop (forwarding on to C), not the chain's terminal, + // since terminal nodes never suppress. + $code = "step(\$p); } }\n" + . "class B { public function step(Cfg \$p): void { (new C())->use(\$p); } } // phptramp-ignore\n" + . 'class C { public function use(Cfg $p): void { $p->x(); } }'; + + self::assertCount(0, $this->build($code)); + } + + public function testIgnoreCommentOnLineAboveDeclarationKillsTheChain(): void + { + $code = "step(\$p); } }\n" + . "class B {\n" + . "// phptramp-ignore\n" + . "public function step(Cfg \$p): void { (new C())->use(\$p); }\n" + . "}\n" + . 'class C { public function use(Cfg $p): void { $p->x(); } }'; + + self::assertCount(0, $this->build($code)); + } + + public function testIgnoreCommentOnUnrelatedLineDoesNotDropTheChain(): void + { + $code = "step(\$p); } }\n" + . "class B { public function step(Cfg \$p): void { \$p->x(); } }"; + + self::assertCount(1, $this->build($code)); + } } diff --git a/tests/Ignore/SuppressionIndexTest.php b/tests/Ignore/SuppressionIndexTest.php new file mode 100644 index 0000000..a556d5a --- /dev/null +++ b/tests/Ignore/SuppressionIndexTest.php @@ -0,0 +1,47 @@ +suppressesMethod('Demo\A::go')); + self::assertFalse($index->suppressesMethod('Demo\A::other')); + } + + public function testSuppressesParamForExactFqmnAndParamPair(): void + { + $index = new SuppressionIndex([], [['Demo\A::go', 'config']], []); + + self::assertTrue($index->suppressesParam('Demo\A::go', 'config')); + self::assertFalse($index->suppressesParam('Demo\A::go', 'other')); + self::assertFalse($index->suppressesParam('Demo\A::other', 'config')); + } + + public function testSuppressesLineForExactFileAndLine(): void + { + $index = new SuppressionIndex([], [], ['/path/to/File.php' => [10, 12]]); + + self::assertTrue($index->suppressesLine('/path/to/File.php', 10)); + self::assertTrue($index->suppressesLine('/path/to/File.php', 12)); + self::assertFalse($index->suppressesLine('/path/to/File.php', 11)); + self::assertFalse($index->suppressesLine('/other/File.php', 10)); + } + + public function testEmptyIndexSuppressesNothing(): void + { + $index = new SuppressionIndex([], [], []); + + self::assertFalse($index->suppressesMethod('Demo\A::go')); + self::assertFalse($index->suppressesParam('Demo\A::go', 'config')); + self::assertFalse($index->suppressesLine('/path/to/File.php', 1)); + } +} From d87eb2dde8974da281360f33e114da0c652efe77 Mon Sep 17 00:00:00 2001 From: Lars Pohlmann Date: Tue, 11 Aug 2026 12:42:43 +0200 Subject: [PATCH 13/26] fix(#5): suppress only on real phptramp-ignore comments recordIgnoreComments() scanned raw physical lines with str_contains, so the marker inside a string literal or heredoc on a forwarding line silently suppressed a genuine finding. Tokenize with token_get_all() and record only T_COMMENT/T_DOC_COMMENT tokens containing the marker instead. Extracted all suppression bookkeeping into a new SuppressionCollector collaborator so IndexingVisitor's complexity stays under PHPMD's threshold after the tokenizer change. Also locks in two frozen-spec points that were correct but untested: a renamed param at a non-origin mid-chain hop still suppresses only its own chains (proving the per-hop lookup key comes from PartialChain, not the origin param name), and short-name attribute matching works without importing PhpTramp\Ignore\TrampIgnore. --- src/Index/IndexingVisitor.php | 81 ++------------- src/Index/SuppressionCollector.php | 137 ++++++++++++++++++++++++++ tests/Chain/ChainBuilderTest.php | 48 +++++++++ tests/Ignore/SuppressionIndexTest.php | 24 +++++ 4 files changed, 219 insertions(+), 71 deletions(-) create mode 100644 src/Index/SuppressionCollector.php diff --git a/src/Index/IndexingVisitor.php b/src/Index/IndexingVisitor.php index 93b4125..4dcb866 100644 --- a/src/Index/IndexingVisitor.php +++ b/src/Index/IndexingVisitor.php @@ -5,11 +5,8 @@ namespace PhpTramp\Index; use PhpParser\Node; -use PhpParser\Node\AttributeGroup; -use PhpParser\Node\Expr\Variable; use PhpParser\Node\FunctionLike; use PhpParser\Node\Name; -use PhpParser\Node\Param; use PhpParser\Node\Stmt\Class_; use PhpParser\Node\Stmt\ClassLike; use PhpParser\Node\Stmt\ClassMethod; @@ -41,20 +38,13 @@ final class IndexingVisitor extends NodeVisitorAbstract /** @var array */ private array $classes = []; - /** @var list */ - private array $suppressedMethods = []; - - /** @var list */ - private array $suppressedClasses = []; - - /** @var list */ - private array $suppressedParams = []; - - /** @var array> */ - private array $ignoreLines = []; - private string $file = ''; + public function __construct( + private readonly SuppressionCollector $suppressionCollector = new SuppressionCollector(), + ) { + } + public function setFile(string $file): void { $this->file = $file; @@ -85,23 +75,12 @@ public function pending(): array public function recordIgnoreComments(string $code): void { - foreach (explode("\n", $code) as $index => $lineText) { - if (str_contains($lineText, '// phptramp-ignore')) { - $this->ignoreLines[$this->file][] = $index + 1; - } - } + $this->suppressionCollector->recordIgnoreComments($this->file, $code); } public function suppressions(): SuppressionIndex { - $methods = $this->suppressedMethods; - foreach ($this->pending as $entry) { - if ($entry['class'] !== null && in_array($entry['class'], $this->suppressedClasses, true)) { - $methods[] = $entry['fqmn']; - } - } - - return new SuppressionIndex($methods, $this->suppressedParams, $this->ignoreLines); + return $this->suppressionCollector->suppressions($this->pending); } /** @@ -149,9 +128,7 @@ private function recordClass(ClassLike $node): void 'traits' => [], ]; - if ($this->hasTrampIgnore($node->attrGroups)) { - $this->suppressedClasses[] = $fqcn; - } + $this->suppressionCollector->recordClassAttributes($fqcn, $node->attrGroups); } private function kindOf(ClassLike $node): ClassKind @@ -193,7 +170,7 @@ private function recordMethod(ClassMethod $node): void 'node' => $node, ]; - $this->recordFunctionLikeSuppressions($fqmn, $node); + $this->suppressionCollector->recordFunctionLikeAttributes($fqmn, $node); } private function recordFunction(Function_ $node): void @@ -211,45 +188,7 @@ private function recordFunction(Function_ $node): void 'node' => $node, ]; - $this->recordFunctionLikeSuppressions($fqmn, $node); - } - - private function recordFunctionLikeSuppressions(string $fqmn, FunctionLike $node): void - { - if ($this->hasTrampIgnore($node->getAttrGroups())) { - $this->suppressedMethods[] = $fqmn; - } - - foreach ($node->getParams() as $param) { - if ($this->hasTrampIgnore($param->attrGroups)) { - $this->recordSuppressedParam($fqmn, $param); - } - } - } - - private function recordSuppressedParam(string $fqmn, Param $param): void - { - if (! $param->var instanceof Variable || ! is_string($param->var->name)) { - return; - } - - $this->suppressedParams[] = [$fqmn, $param->var->name]; - } - - /** - * @param array $attrGroups - */ - private function hasTrampIgnore(array $attrGroups): bool - { - foreach ($attrGroups as $group) { - foreach ($group->attrs as $attribute) { - if ($attribute->name->getLast() === 'TrampIgnore') { - return true; - } - } - } - - return false; + $this->suppressionCollector->recordFunctionLikeAttributes($fqmn, $node); } private function enclosingClassName(Node $node): ?string diff --git a/src/Index/SuppressionCollector.php b/src/Index/SuppressionCollector.php new file mode 100644 index 0000000..6e38872 --- /dev/null +++ b/src/Index/SuppressionCollector.php @@ -0,0 +1,137 @@ + */ + private array $suppressedMethods = []; + + /** @var list */ + private array $suppressedClasses = []; + + /** @var list */ + private array $suppressedParams = []; + + /** @var array> */ + private array $ignoreLines = []; + + /** + * @param array $attrGroups + */ + public function recordClassAttributes(string $fqcn, array $attrGroups): void + { + if ($this->hasIgnoreAttribute($attrGroups)) { + $this->suppressedClasses[] = $fqcn; + } + } + + public function recordFunctionLikeAttributes(string $fqmn, FunctionLike $node): void + { + if ($this->hasIgnoreAttribute($node->getAttrGroups())) { + $this->suppressedMethods[] = $fqmn; + } + + foreach ($node->getParams() as $param) { + if ($this->hasIgnoreAttribute($param->attrGroups)) { + $this->recordSuppressedParam($fqmn, $param); + } + } + } + + public function recordIgnoreComments(string $file, string $code): void + { + foreach (token_get_all($code) as $token) { + if (is_array($token) && $this->isIgnoreCommentToken($token)) { + $this->ignoreLines[$file][] = $token[2]; + } + } + } + + /** + * @param list $pending + */ + public function suppressions(array $pending): SuppressionIndex + { + return new SuppressionIndex( + $this->expandClassSuppressions($pending), + $this->suppressedParams, + $this->ignoreLines, + ); + } + + /** + * @param list $pending + * @return list + */ + private function expandClassSuppressions(array $pending): array + { + $methods = $this->suppressedMethods; + foreach ($pending as $entry) { + if ($entry['class'] !== null && in_array($entry['class'], $this->suppressedClasses, true)) { + $methods[] = $entry['fqmn']; + } + } + + return $methods; + } + + private function recordSuppressedParam(string $fqmn, Param $param): void + { + if (! $param->var instanceof Variable || ! is_string($param->var->name)) { + return; + } + + $this->suppressedParams[] = [$fqmn, $param->var->name]; + } + + /** + * @param array $attrGroups + */ + private function hasIgnoreAttribute(array $attrGroups): bool + { + foreach ($attrGroups as $group) { + foreach ($group->attrs as $attribute) { + if ($attribute->name->getLast() === self::IGNORE_ATTRIBUTE_NAME) { + return true; + } + } + } + + return false; + } + + /** + * @param array{0: int, 1: string, 2: int} $token + */ + private function isIgnoreCommentToken(array $token): bool + { + [$id, $text] = $token; + + return ($id === T_COMMENT || $id === T_DOC_COMMENT) && str_contains($text, self::IGNORE_COMMENT_MARKER); + } +} diff --git a/tests/Chain/ChainBuilderTest.php b/tests/Chain/ChainBuilderTest.php index 50bc2e3..0a2aee5 100644 --- a/tests/Chain/ChainBuilderTest.php +++ b/tests/Chain/ChainBuilderTest.php @@ -342,4 +342,52 @@ public function testIgnoreCommentOnUnrelatedLineDoesNotDropTheChain(): void self::assertCount(1, $this->build($code)); } + + public function testIgnoreMarkerInsideAStringLiteralDoesNotSuppress(): void + { + // The marker text appears inside a string literal on the forwarding + // line, not in an actual PHP comment. It must not be mistaken for a + // suppression - the finding must still be reported. + $code = "step($p); } } ' + . 'class B { public function step(Cfg $p): void { $p->x(); } }'; + + self::assertCount(1, $this->build($code)); + } + + public function testRenamedParamAtNonOriginHopSuppressesOnlyThatParamsChains(): void + { + // The attribute sits on a mid-chain hop's own (differently-named) + // parameter, not the origin's. This proves the per-hop param name used + // for suppression lookup comes from PartialChain::keys (built from + // B::mid's own param name) rather than the origin's param name. The + // sibling param "other", bound positionally to B::mid's un-suppressed + // second param, must still be found. + $code = 'mid($ignored, $kept); } } ' + . 'class B { public function mid(#[TrampIgnore] Cfg $renamed, Cfg $other): void { ' + . '(new C())->takeRenamed($renamed); (new C())->takeOther($other); } } ' + . 'class C { public function takeRenamed(Cfg $p): void { $p->x(); } ' + . 'public function takeOther(Cfg $p): void { $p->x(); } }'; + + $findings = $this->build($code); + self::assertCount(1, $findings); + self::assertSame('kept', $findings[0]->param); + self::assertSame('Demo\C::takeOther', $findings[0]->terminal); + } + + public function testShortNameAttributeMatchesWithoutImportingOurTrampIgnoreClass(): void + { + // No `use PhpTramp\Ignore\TrampIgnore;` here: matching is purely by the + // attribute name's short name, so analyzed codebases are not required + // to autoload our class. + $code = 'step($p); } } ' + . 'class B { #[TrampIgnore] public function step(Cfg $p): void { (new C())->use($p); } } ' + . 'class C { public function use(Cfg $p): void { $p->x(); } }'; + + self::assertCount(0, $this->build($code)); + } } diff --git a/tests/Ignore/SuppressionIndexTest.php b/tests/Ignore/SuppressionIndexTest.php index a556d5a..a548c23 100644 --- a/tests/Ignore/SuppressionIndexTest.php +++ b/tests/Ignore/SuppressionIndexTest.php @@ -36,6 +36,30 @@ public function testSuppressesLineForExactFileAndLine(): void self::assertFalse($index->suppressesLine('/other/File.php', 10)); } + public function testSuppressesParamForMultipleDistinctPairsIndependently(): void + { + $index = new SuppressionIndex([], [ + ['Demo\A::go', 'config'], + ['Demo\B::step', 'other'], + ], []); + + self::assertTrue($index->suppressesParam('Demo\A::go', 'config')); + self::assertTrue($index->suppressesParam('Demo\B::step', 'other')); + self::assertFalse($index->suppressesParam('Demo\B::step', 'config')); + } + + public function testSuppressesLineAcrossMultipleFilesIndependently(): void + { + $index = new SuppressionIndex([], [], [ + '/path/to/A.php' => [10], + '/path/to/B.php' => [20], + ]); + + self::assertTrue($index->suppressesLine('/path/to/A.php', 10)); + self::assertTrue($index->suppressesLine('/path/to/B.php', 20)); + self::assertFalse($index->suppressesLine('/path/to/B.php', 10)); + } + public function testEmptyIndexSuppressesNothing(): void { $index = new SuppressionIndex([], [], []); From dc0c1aa3a94f89874aa4c7367138ce9246bd4757 Mon Sep 17 00:00:00 2001 From: Lars Pohlmann Date: Tue, 11 Aug 2026 13:06:45 +0200 Subject: [PATCH 14/26] feat(#5): summary report SummaryReporter renders all findings unfiltered, as a whole-codebase overview for triaging a legacy codebase before a --limit is chosen: a histogram of chain lengths, the ten longest chains, the most-forwarded parameters, and a totals footer. Wires 'summary' into ReporterFactory as the last of the six formats, so its match arm is now exhaustive over every value ArgvParser::validateFormat accepts; the default arm becomes defensive-only, covered directly since ArgvParser no longer lets an invalid format reach it end to end. FindingMessage::terminalClause() is made public and reused for the "-> X [kind]" / "-> kind" column instead of duplicating that logic a third time. --- src/Report/FindingMessage.php | 3 +- src/Report/ReporterFactory.php | 3 + src/Report/SummaryReporter.php | 225 +++++++++++++++++++++++++++ tests/Console/ApplicationTest.php | 14 +- tests/Report/ReporterFactoryTest.php | 24 +++ tests/Report/SummaryReporterTest.php | 130 ++++++++++++++++ 6 files changed, 395 insertions(+), 4 deletions(-) create mode 100644 src/Report/SummaryReporter.php create mode 100644 tests/Report/SummaryReporterTest.php diff --git a/src/Report/FindingMessage.php b/src/Report/FindingMessage.php index f555c73..36206e7 100644 --- a/src/Report/FindingMessage.php +++ b/src/Report/FindingMessage.php @@ -20,7 +20,8 @@ public function describe(Finding $finding): string . ' (terminal: ' . $this->terminalClause($finding) . ')'; } - private function terminalClause(Finding $finding): string + /** Shared with {@see SummaryReporter}'s "Top 10 longest chains" column. */ + public function terminalClause(Finding $finding): string { if ($finding->terminal === null) { return $finding->terminalKind->value; diff --git a/src/Report/ReporterFactory.php b/src/Report/ReporterFactory.php index 679b25b..08843a1 100644 --- a/src/Report/ReporterFactory.php +++ b/src/Report/ReporterFactory.php @@ -23,6 +23,9 @@ public function create(Options $options): Reporter 'github' => new GithubReporter($thresholds, new Paths($this->workingDirectory)), 'checkstyle' => new CheckstyleReporter($thresholds, new Paths($this->workingDirectory)), 'sarif' => new SarifReporter($thresholds, new Paths($this->workingDirectory)), + 'summary' => new SummaryReporter($thresholds), + // Unreachable: ArgvParser::validateFormat rejects any format outside + // VALID_FORMATS at parse time, so every case above is exhaustive. default => throw new InvalidArgsException("format '{$options->format}' is not implemented yet."), }; } diff --git a/src/Report/SummaryReporter.php b/src/Report/SummaryReporter.php new file mode 100644 index 0000000..9c82bd3 --- /dev/null +++ b/src/Report/SummaryReporter.php @@ -0,0 +1,225 @@ +message = new FindingMessage(); + } + + /** + * @param list $findings ALL findings, unfiltered + */ + public function render(array $findings): string + { + if ($findings === []) { + return "No chains found.\n"; + } + + $blocks = [ + $this->chainsByLengthBlock($findings), + $this->topLongestChainsBlock($findings), + $this->mostForwardedParametersBlock($findings), + $this->footer($findings), + ]; + + return implode("\n\n", $blocks) . "\n"; + } + + /** + * @param list $findings + */ + private function chainsByLengthBlock(array $findings): string + { + $counts = $this->countsByHopLength($findings); + $labels = $this->hopLengthLabels(array_keys($counts)); + $labelWidth = $this->maxLength($labels); + $maxCount = $this->nonEmptyMax($counts); + + $rows = []; + foreach ($counts as $hops => $count) { + $rows[] = self::INDENT . str_pad($labels[$hops], $labelWidth) . self::COLUMN_GAP + . str_repeat('#', $this->barLength($count, $maxCount)) . ' ' . $count; + } + + return "Chains by length:\n" . implode("\n", $rows); + } + + /** + * @param list $findings + * @return array hop count => number of findings, ascending by hop count + */ + private function countsByHopLength(array $findings): array + { + $counts = []; + foreach ($findings as $finding) { + $counts[$finding->hops] = ($counts[$finding->hops] ?? 0) + 1; + } + ksort($counts); + + return $counts; + } + + /** + * @param list $hopCounts + * @return array hop count => rendered label, e.g. "1 hop"/"2 hops" + */ + private function hopLengthLabels(array $hopCounts): array + { + $labels = []; + foreach ($hopCounts as $hops) { + $labels[$hops] = $hops . ' ' . $this->plural($hops, 'hop'); + } + + return $labels; + } + + private function barLength(int $count, int $maxCount): int + { + if ($maxCount <= self::MAX_BAR_LENGTH) { + return $count; + } + + return max(1, (int) round($count * self::MAX_BAR_LENGTH / $maxCount)); + } + + /** + * @param list $findings + */ + private function topLongestChainsBlock(array $findings): string + { + $top = $this->longestFindings($findings); + $hopLabels = array_map( + fn (Finding $finding): string => $finding->hops . ' ' . $this->plural($finding->hops, 'hop'), + $top, + ); + $params = array_map(static fn (Finding $finding): string => '$' . $finding->param, $top); + + $hopLabelWidth = $this->maxLength($hopLabels); + $paramWidth = $this->maxLength($params); + + $rows = []; + foreach ($top as $index => $finding) { + $rows[] = self::INDENT . str_pad($hopLabels[$index], $hopLabelWidth) . self::COLUMN_GAP + . str_pad($params[$index], $paramWidth) . self::COLUMN_GAP + . $finding->origin . ' -> ' . $this->message->terminalClause($finding); + } + + return "Top 10 longest chains:\n" . implode("\n", $rows); + } + + /** + * @param list $findings + * @return list sorted by hops desc, then origin asc, then param asc; capped at 10 + */ + private function longestFindings(array $findings): array + { + $sorted = $findings; + usort($sorted, static function (Finding $left, Finding $right): int { + return [-$left->hops, $left->origin, $left->param] + <=> [-$right->hops, $right->origin, $right->param]; + }); + + return array_slice($sorted, 0, self::TOP_CHAINS_LIMIT); + } + + /** + * @param list $findings + */ + private function mostForwardedParametersBlock(array $findings): string + { + $rows = []; + foreach ($this->paramCounts($findings) as $paramCount) { + $rows[] = self::INDENT . $paramCount['count'] . 'x' . self::COLUMN_GAP . '$' . $paramCount['param']; + } + + return "Most-forwarded parameters:\n" . implode("\n", $rows); + } + + /** + * @param list $findings + * @return list sorted by count desc, then param asc + */ + private function paramCounts(array $findings): array + { + $counts = []; + foreach ($findings as $finding) { + $counts[$finding->param] = ($counts[$finding->param] ?? 0) + 1; + } + + $pairs = []; + foreach ($counts as $param => $count) { + $pairs[] = ['param' => $param, 'count' => $count]; + } + + usort($pairs, static function (array $left, array $right): int { + return [-$left['count'], $left['param']] <=> [-$right['count'], $right['param']]; + }); + + return $pairs; + } + + /** + * @param list $findings + */ + private function footer(array $findings): string + { + $total = count($findings); + $atOrOverLimit = count( + array_filter($findings, fn (Finding $finding): bool => $finding->hops >= $this->thresholds->limit), + ); + + return $total . ' ' . $this->plural($total, 'chain') . ' total; ' + . $atOrOverLimit . ' at or over the limit (limit: ' + . $this->thresholds->limit . ' ' . $this->plural($this->thresholds->limit, 'hop') . ').'; + } + + /** + * @param array $strings key order and identity are + * irrelevant — only lengths matter + */ + private function maxLength(array $strings): int + { + return $this->nonEmptyMax(array_map('strlen', $strings)); + } + + /** + * @param array $values key order and identity are + * irrelevant — only the max value matters + */ + private function nonEmptyMax(array $values): int + { + // render() already guards against an empty $findings, so every caller + // reaches this with at least one value; the check just proves that to + // PHPStan without a magic fallback number. + if ($values === []) { + throw new \LogicException('nonEmptyMax() called with an empty array.'); + } + + return max($values); + } + + private function plural(int $count, string $singular): string + { + return $count === 1 ? $singular : $singular . 's'; + } +} diff --git a/tests/Console/ApplicationTest.php b/tests/Console/ApplicationTest.php index 927a5d6..9a25f23 100644 --- a/tests/Console/ApplicationTest.php +++ b/tests/Console/ApplicationTest.php @@ -97,15 +97,23 @@ public function testLimitAboveChainLengthReportsNothingAndExitsZero(): void self::assertStringContainsString('No tramp data found', self::contents($this->stdout)); } - public function testUnsupportedFormatExitsWithToolErrorCode(): void + public function testUnknownFormatExitsWithToolErrorCode(): void { $folder = $this->fixtureWithThreeHopChain(); - self::assertSame(2, $this->app->run(['phptramp', '--folder', $folder, '--format', 'summary'])); - self::assertStringContainsString('not implemented', self::contents($this->stderr)); + self::assertSame(2, $this->app->run(['phptramp', '--folder', $folder, '--format', 'xml'])); + self::assertStringContainsString('unknown format: xml', self::contents($this->stderr)); self::assertSame('', self::contents($this->stdout)); } + public function testSummaryFormatReportsAllChainsAndExitsOne(): void + { + $folder = $this->fixtureWithThreeHopChain(); + + self::assertSame(1, $this->app->run(['phptramp', '--folder', $folder, '--format', 'summary'])); + self::assertStringContainsString('at or over the limit', self::contents($this->stdout)); + } + public function testJsonFormatReportsFindingsAndExitsOne(): void { $folder = $this->fixtureWithThreeHopChain(); diff --git a/tests/Report/ReporterFactoryTest.php b/tests/Report/ReporterFactoryTest.php index c4cb894..da5aebd 100644 --- a/tests/Report/ReporterFactoryTest.php +++ b/tests/Report/ReporterFactoryTest.php @@ -7,6 +7,7 @@ use PhpTramp\Chain\Finding; use PhpTramp\Chain\Hop; use PhpTramp\Chain\TerminalKind; +use PhpTramp\Console\InvalidArgsException; use PhpTramp\Console\Options; use PhpTramp\Report\ReporterFactory; use PHPUnit\Framework\TestCase; @@ -58,4 +59,27 @@ public function testSarifFormatRoutesToSarifReporter(): void self::assertStringContainsString('"phptramp.trampData"', $reporter->render($this->qualifyingFinding())); } + + public function testSummaryFormatRoutesToSummaryReporter(): void + { + $factory = new ReporterFactory('/not/matching'); + $reporter = $factory->create(new Options(format: 'summary', limit: 2)); + + self::assertStringContainsString('at or over the limit', $reporter->render($this->qualifyingFinding())); + } + + /** + * ArgvParser::validateFormat already rejects any format outside the six + * valid values before Options ever reaches the factory, so this exercises + * the `default` arm directly — the only way to reach it at all. + */ + public function testUnroutableFormatThrows(): void + { + $factory = new ReporterFactory('/not/matching'); + + $this->expectException(InvalidArgsException::class); + $this->expectExceptionMessage("format 'xml' is not implemented yet."); + + $factory->create(new Options(format: 'xml')); + } } diff --git a/tests/Report/SummaryReporterTest.php b/tests/Report/SummaryReporterTest.php new file mode 100644 index 0000000..cc4044c --- /dev/null +++ b/tests/Report/SummaryReporterTest.php @@ -0,0 +1,130 @@ +render([])); + } + + /** + * Twelve findings across four hop lengths (1..4), exercising: ascending + * hop-length bars, the Top-10 cap (one of three 1-hop findings is + * excluded), the hops-desc/origin-asc/param-asc tiebreak (the two 4-hop + * findings share an origin and are ordered by param), both terminal-clause + * forms (`-> external`/`-> truncated` vs `-> X [kind]`), a most-forwarded + * count tie broken alphabetically ($alpha and $beta both appear 4x), and + * findings both under and at/over the limit of 3 hops. + */ + public function testRendersMixedHopLengthsWithAllFourBlocks(): void + { + $findings = [ + $this->finding('beta', 4, 'Demo\Kernel::boot', null, TerminalKind::External), + $this->finding('alpha', 4, 'Demo\Kernel::boot', null, TerminalKind::External), + $this->finding('gamma', 3, 'Demo\ServiceA::run', 'Demo\Mailer::__construct', TerminalKind::Stored), + $this->finding('gamma', 3, 'Demo\ServiceB::run', 'Demo\Logger::log', TerminalKind::Used), + $this->finding('gamma', 3, 'Demo\ServiceC::run', null, TerminalKind::Truncated), + $this->finding('alpha', 2, 'Demo\Two::a', 'Demo\Sink::a', TerminalKind::Used), + $this->finding('beta', 2, 'Demo\Two::b', 'Demo\Sink::b', TerminalKind::Stored), + $this->finding('alpha', 2, 'Demo\Two::c', null, TerminalKind::External), + $this->finding('delta', 2, 'Demo\Two::d', null, TerminalKind::Truncated), + $this->finding('alpha', 1, 'Demo\One::x', 'Demo\Sink::x', TerminalKind::Used), + $this->finding('beta', 1, 'Demo\One::y', 'Demo\Sink::y', TerminalKind::Used), + $this->finding('beta', 1, 'Demo\One::z', 'Demo\Sink::z', TerminalKind::Used), + ]; + + $expected = <<<'TXT' + Chains by length: + 1 hop ### 3 + 2 hops #### 4 + 3 hops ### 3 + 4 hops ## 2 + + Top 10 longest chains: + 4 hops $alpha Demo\Kernel::boot -> external + 4 hops $beta Demo\Kernel::boot -> external + 3 hops $gamma Demo\ServiceA::run -> Demo\Mailer::__construct [stored] + 3 hops $gamma Demo\ServiceB::run -> Demo\Logger::log [used] + 3 hops $gamma Demo\ServiceC::run -> truncated + 2 hops $alpha Demo\Two::a -> Demo\Sink::a [used] + 2 hops $beta Demo\Two::b -> Demo\Sink::b [stored] + 2 hops $alpha Demo\Two::c -> external + 2 hops $delta Demo\Two::d -> truncated + 1 hop $alpha Demo\One::x -> Demo\Sink::x [used] + + Most-forwarded parameters: + 4x $alpha + 4x $beta + 3x $gamma + 1x $delta + + 12 chains total; 5 at or over the limit (limit: 3 hops). + + TXT; + + self::assertSame($expected, (new SummaryReporter(new Thresholds(3, null)))->render($findings)); + } + + /** + * Largest bucket (100) forces scaling; the other three buckets each pin + * one otherwise-ambiguous edge of `round()`: 1 exercises the `max(1, …)` + * floor (its raw scaled value rounds down to 0), 13 has a scaled value + * (5.2) where `ceil()` would disagree with `round()`, and 17 has one + * (6.8) where `floor()` would disagree with `round()`. + */ + public function testScalesBarsWhenLargestBucketExceedsFortyHashes(): void + { + $findings = [ + ...$this->findingsWithHops(1, 100), + ...$this->findingsWithHops(2, 1), + ...$this->findingsWithHops(3, 13), + ...$this->findingsWithHops(4, 17), + ]; + + $expected = 'Chains by length:' . "\n" + . ' 1 hop ' . str_repeat('#', 40) . ' 100' . "\n" + . ' 2 hops # 1' . "\n" + . ' 3 hops ##### 13' . "\n" + . ' 4 hops ####### 17'; + + $output = (new SummaryReporter(new Thresholds(3, null)))->render($findings); + $chainsByLengthBlock = explode("\n\n", $output)[0]; + + self::assertSame($expected, $chainsByLengthBlock); + } + + /** + * @return list + */ + private function findingsWithHops(int $hops, int $count): array + { + $findings = []; + for ($index = 0; $index < $count; $index++) { + $findings[] = $this->finding('p', $hops, "Demo\\Scale::m{$index}", null, TerminalKind::External); + } + + return $findings; + } + + private function finding( + string $param, + int $hops, + string $origin, + ?string $terminal, + TerminalKind $terminalKind, + ): Finding { + return new Finding($param, $origin, $terminal, $terminalKind, $hops, [], $hops + 1, [], []); + } +} From 3f4c55b0e84257f84b268399ee6cd8c5cf382ff0 Mon Sep 17 00:00:00 2001 From: Lars Pohlmann Date: Tue, 11 Aug 2026 13:24:17 +0200 Subject: [PATCH 15/26] test(#5): format, config and suppression fixtures Extends the fixture harness's findings path with two per-fixture behaviors needed to exercise Phase 3 features end-to-end: an optional phptramp-args.json for custom CLI args (default stays --limit 1, so the existing 7 fixtures run byte-identically), and a phptramp.json-routed config mode that chdirs into the fixture instead of passing --folder. The exit-code assertion now checks for any error-severity expected finding rather than just non-empty findings, so an all-warnings fixture can assert exit 0. Adds three fixtures with hand-derived expectations: warn-vs-error (2-hop and 3-hop chains under --limit 3 --warn-limit 2, asserting warning vs error severity), suppressed-chain (TrampIgnore attribute and phptramp-ignore comment each independently dropping a chain), and config-driven (paths + exclude + limit read from phptramp.json, proving the exclude glob actually removes a file from analysis). --- tests/FixtureTest.php | 115 +++++++++++++++++- .../config-driven/expected-findings.json | 20 +++ tests/fixtures/config-driven/phptramp.json | 5 + tests/fixtures/config-driven/src/Excluded.php | 23 ++++ tests/fixtures/config-driven/src/Included.php | 23 ++++ .../suppressed-chain/expected-findings.json | 5 + tests/fixtures/suppressed-chain/src/Demo.php | 42 +++++++ .../warn-vs-error/expected-findings.json | 37 ++++++ .../fixtures/warn-vs-error/phptramp-args.json | 1 + tests/fixtures/warn-vs-error/src/Demo.php | 63 ++++++++++ 10 files changed, 330 insertions(+), 4 deletions(-) create mode 100644 tests/fixtures/config-driven/expected-findings.json create mode 100644 tests/fixtures/config-driven/phptramp.json create mode 100644 tests/fixtures/config-driven/src/Excluded.php create mode 100644 tests/fixtures/config-driven/src/Included.php create mode 100644 tests/fixtures/suppressed-chain/expected-findings.json create mode 100644 tests/fixtures/suppressed-chain/src/Demo.php create mode 100644 tests/fixtures/warn-vs-error/expected-findings.json create mode 100644 tests/fixtures/warn-vs-error/phptramp-args.json create mode 100644 tests/fixtures/warn-vs-error/src/Demo.php diff --git a/tests/FixtureTest.php b/tests/FixtureTest.php index 7492486..7bc64c0 100644 --- a/tests/FixtureTest.php +++ b/tests/FixtureTest.php @@ -14,6 +14,12 @@ * case with expected-findings.json pins the real CLI's `--format json` output * (via `Application::run`), byte-for-byte modulo `file` path normalization. * Adding a semantic rule means adding a fixture directory, not touching this test. + * + * A findings case runs in one of two modes, chosen by what sits next to its + * expected-findings.json: a `phptramp.json` routes it through config mode + * (chdir into the fixture, no `--folder`); otherwise it runs in folder mode + * (`--folder /src`), with CLI args from an optional `phptramp-args.json` + * or the default `--limit 1`. */ final class FixtureTest extends TestCase { @@ -66,22 +72,123 @@ public function testFindingsMatchExpectation(string $caseDir): void [$exitCode, $decoded] = $this->runFindingsJson($caseDir); - $expectedExitCode = $expected['findings'] === [] ? 0 : 1; - self::assertSame($expectedExitCode, $exitCode); - self::assertEquals($expected, $this->normalizeFindingsDocument($decoded, $caseDir)); + self::assertSame($this->expectedExitCode($expected), $exitCode); + self::assertEquals($expected, $decoded); } /** + * A finding at or over `--limit` reports `severity: "error"` and must fail + * the run; anything else (all-warnings, or no findings) must not. + * + * @param array $expected + */ + private function expectedExitCode(array $expected): int + { + $findings = $expected['findings'] ?? []; + self::assertIsArray($findings); + + foreach ($findings as $finding) { + self::assertIsArray($finding); + if ($finding['severity'] === 'error') { + return 1; + } + } + + return 0; + } + + /** + * Routes to config mode (a `phptramp.json` sits in the fixture directory) or + * folder mode (plain `--folder /src`), each producing an + * already-normalized findings document ready to compare against + * expected-findings.json. + * * @return array{0: int, 1: array} */ private function runFindingsJson(string $caseDir): array + { + if (is_file($caseDir . '/phptramp.json')) { + return $this->runConfigMode($caseDir); + } + + return $this->runFolderMode($caseDir); + } + + /** + * Folder mode: `--folder /src` plus either the fixture's + * `phptramp-args.json` extra CLI args or the default `--limit 1`. The + * reporter relativizes `file` against getcwd() (the repo root at test + * time), so JSON `file` values arrive repo-relative, e.g. + * "tests/fixtures//src/Demo.php" — normalize back to case-relative. + * + * @return array{0: int, 1: array} + */ + private function runFolderMode(string $caseDir): array + { + $argv = ['phptramp', '--folder', $caseDir . '/src', '--format', 'json', ...$this->extraCliArgs($caseDir)]; + [$exitCode, $decoded] = $this->runApplication($argv); + + return [$exitCode, $this->normalizeFindingsDocument($decoded, $caseDir)]; + } + + /** + * Config mode: chdir into the fixture directory so its `phptramp.json` + * supplies paths/exclude/limit, then run with no `--folder` at all. Always + * restores the previous cwd — other tests depend on it. Findings' `file` + * values come out already fixture-relative (cwd = fixture dir at render + * time), so no prefix-strip is needed. + * + * @return array{0: int, 1: array} + */ + private function runConfigMode(string $caseDir): array + { + $previousCwd = getcwd(); + self::assertIsString($previousCwd); + + try { + self::assertTrue(chdir($caseDir)); + + return $this->runApplication(['phptramp', '--format', 'json']); + } finally { + chdir($previousCwd); + } + } + + /** + * @return list + */ + private function extraCliArgs(string $caseDir): array + { + $argsFile = $caseDir . '/phptramp-args.json'; + if (! is_file($argsFile)) { + return ['--limit', '1']; + } + + $json = file_get_contents($argsFile); + self::assertIsString($json); + $args = json_decode($json, true); + self::assertIsArray($args); + + $cliArgs = []; + foreach ($args as $arg) { + self::assertIsString($arg); + $cliArgs[] = $arg; + } + + return $cliArgs; + } + + /** + * @param list $argv + * @return array{0: int, 1: array} + */ + private function runApplication(array $argv): array { $stdout = fopen('php://memory', 'w+'); $stderr = fopen('php://memory', 'w+'); self::assertIsResource($stdout); self::assertIsResource($stderr); - $argv = ['phptramp', '--folder', $caseDir . '/src', '--format', 'json', '--limit', '1']; $exitCode = (new Application($stdout, $stderr))->run($argv); rewind($stdout); diff --git a/tests/fixtures/config-driven/expected-findings.json b/tests/fixtures/config-driven/expected-findings.json new file mode 100644 index 0000000..f9c62fa --- /dev/null +++ b/tests/fixtures/config-driven/expected-findings.json @@ -0,0 +1,20 @@ +{ + "limit": 1, + "warnLimit": null, + "findings": [ + { + "param": "config", + "severity": "error", + "origin": "Demo\\IncludedOrigin::run", + "terminal": "Demo\\IncludedTerminal::consume", + "terminalKind": "used", + "hops": 1, + "classes": 2, + "chain": [ + {"method": "Demo\\IncludedOrigin::run", "role": "origin", "file": "src/Included.php", "line": 11, "forwardLine": 13}, + {"method": "Demo\\IncludedTerminal::consume", "role": "terminal", "file": "src/Included.php", "line": 19, "forwardLine": null} + ], + "notes": [] + } + ] +} diff --git a/tests/fixtures/config-driven/phptramp.json b/tests/fixtures/config-driven/phptramp.json new file mode 100644 index 0000000..39c6f8d --- /dev/null +++ b/tests/fixtures/config-driven/phptramp.json @@ -0,0 +1,5 @@ +{ + "paths": ["src"], + "exclude": ["src/Excl*.php"], + "limit": 1 +} diff --git a/tests/fixtures/config-driven/src/Excluded.php b/tests/fixtures/config-driven/src/Excluded.php new file mode 100644 index 0000000..7aa2130 --- /dev/null +++ b/tests/fixtures/config-driven/src/Excluded.php @@ -0,0 +1,23 @@ +consume($config); + } +} + +class ExcludedTerminal +{ + public function consume(ExcludedCfg $config): void + { + $config->go(); + } +} diff --git a/tests/fixtures/config-driven/src/Included.php b/tests/fixtures/config-driven/src/Included.php new file mode 100644 index 0000000..ca304c6 --- /dev/null +++ b/tests/fixtures/config-driven/src/Included.php @@ -0,0 +1,23 @@ +consume($config); + } +} + +class IncludedTerminal +{ + public function consume(Cfg $config): void + { + $config->go(); + } +} diff --git a/tests/fixtures/suppressed-chain/expected-findings.json b/tests/fixtures/suppressed-chain/expected-findings.json new file mode 100644 index 0000000..7bec990 --- /dev/null +++ b/tests/fixtures/suppressed-chain/expected-findings.json @@ -0,0 +1,5 @@ +{ + "limit": 1, + "warnLimit": null, + "findings": [] +} diff --git a/tests/fixtures/suppressed-chain/src/Demo.php b/tests/fixtures/suppressed-chain/src/Demo.php new file mode 100644 index 0000000..5fb66da --- /dev/null +++ b/tests/fixtures/suppressed-chain/src/Demo.php @@ -0,0 +1,42 @@ +consume($config); + } +} + +class AttributeSuppressedTerminal +{ + public function consume(Cfg $config): void + { + $config->go(); + } +} + +class CommentSuppressedOrigin +{ + public function run(Cfg $config): void + { + (new CommentSuppressedTerminal())->consume($config); // phptramp-ignore + } +} + +class CommentSuppressedTerminal +{ + public function consume(Cfg $config): void + { + $config->go(); + } +} diff --git a/tests/fixtures/warn-vs-error/expected-findings.json b/tests/fixtures/warn-vs-error/expected-findings.json new file mode 100644 index 0000000..2994760 --- /dev/null +++ b/tests/fixtures/warn-vs-error/expected-findings.json @@ -0,0 +1,37 @@ +{ + "limit": 3, + "warnLimit": 2, + "findings": [ + { + "param": "config", + "severity": "warning", + "origin": "Demo\\TwoHopOrigin::run", + "terminal": "Demo\\TwoHopTerminal::consume", + "terminalKind": "used", + "hops": 2, + "classes": 3, + "chain": [ + {"method": "Demo\\TwoHopOrigin::run", "role": "origin", "file": "src/Demo.php", "line": 11, "forwardLine": 13}, + {"method": "Demo\\TwoHopMiddle::relay", "role": "hop", "file": "src/Demo.php", "line": 19, "forwardLine": 21}, + {"method": "Demo\\TwoHopTerminal::consume", "role": "terminal", "file": "src/Demo.php", "line": 27, "forwardLine": null} + ], + "notes": [] + }, + { + "param": "config", + "severity": "error", + "origin": "Demo\\ThreeHopOrigin::run", + "terminal": "Demo\\ThreeHopTerminal::consume", + "terminalKind": "used", + "hops": 3, + "classes": 4, + "chain": [ + {"method": "Demo\\ThreeHopOrigin::run", "role": "origin", "file": "src/Demo.php", "line": 35, "forwardLine": 37}, + {"method": "Demo\\ThreeHopMiddleA::relay", "role": "hop", "file": "src/Demo.php", "line": 43, "forwardLine": 45}, + {"method": "Demo\\ThreeHopMiddleB::relay", "role": "hop", "file": "src/Demo.php", "line": 51, "forwardLine": 53}, + {"method": "Demo\\ThreeHopTerminal::consume", "role": "terminal", "file": "src/Demo.php", "line": 59, "forwardLine": null} + ], + "notes": [] + } + ] +} diff --git a/tests/fixtures/warn-vs-error/phptramp-args.json b/tests/fixtures/warn-vs-error/phptramp-args.json new file mode 100644 index 0000000..f13906c --- /dev/null +++ b/tests/fixtures/warn-vs-error/phptramp-args.json @@ -0,0 +1 @@ +["--limit", "3", "--warn-limit", "2"] diff --git a/tests/fixtures/warn-vs-error/src/Demo.php b/tests/fixtures/warn-vs-error/src/Demo.php new file mode 100644 index 0000000..028beb7 --- /dev/null +++ b/tests/fixtures/warn-vs-error/src/Demo.php @@ -0,0 +1,63 @@ +relay($config); + } +} + +class TwoHopMiddle +{ + public function relay(Cfg $config): void + { + (new TwoHopTerminal())->consume($config); + } +} + +class TwoHopTerminal +{ + public function consume(Cfg $config): void + { + $config->go(); + } +} + +class ThreeHopOrigin +{ + public function run(Cfg $config): void + { + (new ThreeHopMiddleA())->relay($config); + } +} + +class ThreeHopMiddleA +{ + public function relay(Cfg $config): void + { + (new ThreeHopMiddleB())->relay($config); + } +} + +class ThreeHopMiddleB +{ + public function relay(Cfg $config): void + { + (new ThreeHopTerminal())->consume($config); + } +} + +class ThreeHopTerminal +{ + public function consume(Cfg $config): void + { + $config->go(); + } +} From b12014bdbd47a93cbe1a323e17d1fef9935162dd Mon Sep 17 00:00:00 2001 From: Lars Pohlmann Date: Tue, 11 Aug 2026 13:27:10 +0200 Subject: [PATCH 16/26] docs(#5): phase 3 status and usage docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit README: status Phase 2 -> Phase 3; documents phptramp.json (recognized keys, CLI-over-config precedence, exclude globs), all six --format values with a one-line example each, #[TrampIgnore]/phptramp-ignore suppression, and --warn-limit. Corrects the composer-tramp recipe: this repo has no "tramp" composer script (only test/stan/cs/cs:fix/md/check/infection/ infection:diff exist) — the section now documents the real invocation and flags that the recipe is something a consuming project adds itself. Marks --changed-only/--git-base/--baseline/--generate-baseline as accepted but not yet wired up, since they remain Phase 4/5 work. docs/plan.md: checks off the Phase 3 bullets (3.1-3.5) and marks the phase done, matching what actually shipped on this branch. Application::helpText: Status line now reflects that all six formats, config, suppression, and --warn-limit work, replacing the stale "Phase 2 / other formats land in Phase 3" wording. --- README.md | 93 +++++++++++++++++++++++++++++++------ docs/plan.md | 16 ++++--- src/Console/Application.php | 6 ++- 3 files changed, 93 insertions(+), 22 deletions(-) diff --git a/README.md b/README.md index b6449ef..bd06ba2 100644 --- a/README.md +++ b/README.md @@ -4,10 +4,11 @@ > methods that never use them — "this parameter was passed through 4 classes / 5 methods > before being used." -**Status: Phase 2.** Cross-file chain reporting works: `phptramp --folder src` -stitches forwarding chains across files and prints text findings, exiting `0`/`1`/`2`. -The classifier and whole-project index remain inspectable via `--dump-index`; other -output formats (`json`, `github`, …) land in Phase 3. +**Status: Phase 3.** 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`. See [docs/plan.md](docs/plan.md) for the full implementation plan and current phase. ## What it does @@ -55,10 +56,14 @@ Add a script to your project's `composer.json` to invoke it the short way: ```bash composer tramp -composer tramp -- --changed-only --git-base origin/main +composer tramp -- --format json --warn-limit 2 ``` -## Planned CLI +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. + +## CLI ```text phptramp [options] @@ -68,17 +73,77 @@ phptramp [options] --files Comma-separated list of files --limit Fail on chains with >= n pass-through hops (default: 3) --warn-limit Warn (do not fail CI) on chains with >= n hops - --format text|json|github|checkstyle|sarif (default: text) + --format text|json|github|checkstyle|sarif|summary (default: text) --explain Show why chains ended (call resolution trace) - --changed-only Only report chains touching changed lines - --git-base Diff base for --changed-only (default: origin/main) - --baseline Ignore findings recorded in the baseline - --generate-baseline Write current findings to a baseline file + --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 + --baseline Reserved for baselining (Phase 5) — accepted, not yet wired up + --generate-baseline Reserved for baselining (Phase 5) — accepted, not yet wired up +``` + +Exit codes: `0` no finding at error severity, `1` at least one finding at/over +`--limit`, `2` tool error — so it drops straight into any CI pipeline. + +## Configuration + +phptramp reads `phptramp.json` (falling back to `phptramp.dist.json`) from the current +working directory. CLI flags always take precedence over the config file. An unknown +key, or a value of the wrong type, is a config error rather than a silently ignored typo. + +```json +{ + "paths": ["src"], + "exclude": ["src/Legacy/*.php"], + "limit": 3, + "warnLimit": 2, + "format": "json" +} ``` -Exit codes: `0` no findings, `1` findings at/over `--limit`, `2` tool error — -so it drops straight into any CI pipeline. Configuration lives in `phptramp.json` -(or `phptramp.dist.json`); CLI flags override config. +| Key | Type | Same as | +|---|---|---| +| `paths` | list of strings | entries ending in `.php` become `--file`s, everything else a `--folder` | +| `exclude` | list of strings | `fnmatch` globs matched against each folder-discovered file's path, relative to the config file's directory; explicit files (`paths` entries ending in `.php`) are never excluded | +| `limit` | integer | `--limit` | +| `warnLimit` | integer | `--warn-limit` | +| `format` | string | `--format` | +| `baseline` | string | `--baseline` (reserved for Phase 5) | + +## Output formats + +`--format` selects the renderer; all six are implemented and covered by an exact-string +test plus a fixture. + +| Format | One-line example | +|---|---| +| `text` | `FINDING $config: 3 pass-through hops across 4 classes` | +| `json` | `{"limit":3,"warnLimit":null,"findings":[{"param":"config","severity":"error","hops":3,...}]}` | +| `github` | `::error file=src/Demo.php,line=12,title=phptramp::$config: 3 pass-through hops across 4 classes (terminal: Demo\Mailer::__construct [stored])` | +| `checkstyle` | `` | +| `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).` | + +## Suppression + +Mark a specific false positive as intentional instead of raising `--limit` project-wide: + +- **`#[TrampIgnore]`** on a class, method, function, or parameter. Matching is by the + attribute name's *short name*, so an analyzed codebase never has to `require` or + autoload phptramp's own `PhpTramp\Ignore\TrampIgnore` class to use it. +- **`// phptramp-ignore`** as a real PHP comment (a marker inside a string literal does + not count) on a hop's declaration line, the line directly above it, or a forwarding + call-site line. + +Either drops the *entire chain* passing through that hop, not just the one parameter +reaching the flagged declaration. + +## `--warn-limit` + +`--warn-limit ` adds a second, lower threshold below `--limit`. Chains whose hop +count falls in `[warn-limit, limit)` render at `severity: "warning"` (`WARNING` in text, +`::warning` in the GitHub format, `"level": "warning"` in SARIF, …) but never fail the +run — only chains at or over `--limit` set exit code `1`. Useful for tightening a hop +budget gradually: warn today, promote to a hard failure once the codebase is clean. ## IDE integration diff --git a/docs/plan.md b/docs/plan.md index de091cf..adaf828 100644 --- a/docs/plan.md +++ b/docs/plan.md @@ -680,7 +680,7 @@ Tasks (expanded in `docs/plans/phase-2.md`): --- -## Phase 3 — CI & config surface (in progress — issue #5) +## Phase 3 — CI & config surface ✅ (issue #5) **Deliverable:** all output formats, config file, suppression, dual thresholds, summary. @@ -690,22 +690,26 @@ against the actual Phase 1–2 interfaces; where it refines the sketches below ( parser-default seeding, suppression exposed through `MethodIndex::suppressions()`), the detailed plan wins. -- **3.1 ConfigLoader:** `phptramp.json` then `phptramp.dist.json`; keys `paths`, +- [x] **3.1 ConfigLoader:** `phptramp.json` then `phptramp.dist.json`; keys `paths`, `exclude`, `limit`, `warnLimit`, `format`, `baseline`; strict unknown-key error; precedence CLI > config > defaults (merge into `Options`). `exclude` glob patterns feed FileLocator. -- **3.2 Formats:** `JsonReporter` (the `expected.json` schema — one schema everywhere), +- [x] **3.2 Formats:** `JsonReporter` (the `expected.json` schema — one schema everywhere), `GithubReporter` (`::error file=...,line=...::...` per origin, `::notice` per hop), `CheckstyleReporter`, `SarifReporter` (rule id `phptramp.trampData`). Fixtures assert each format byte-for-byte. -- **3.3 Suppression:** `#[TrampIgnore]` attribute class + `// phptramp-ignore` comments +- [x] **3.3 Suppression:** `#[TrampIgnore]` attribute class + `// phptramp-ignore` comments scanned during indexing into a `SuppressionIndex`; ChainBuilder drops chains with any suppressed hop (frozen rule 11). -- **3.4 Dual thresholds:** `--warn-limit` — findings in `[warn, limit)` render as +- [x] **3.4 Dual thresholds:** `--warn-limit` — findings in `[warn, limit)` render as warnings, never affect exit code. GitHub format uses `::warning`. -- **3.5 SummaryReporter:** `--format=summary` — histogram of chain lengths, top 10 +- [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. + --- ## Phase 4 — Diff-aware mode (flagship) diff --git a/src/Console/Application.php b/src/Console/Application.php index 2720063..9914a32 100644 --- a/src/Console/Application.php +++ b/src/Console/Application.php @@ -221,8 +221,10 @@ private static function helpText(): string 1 at least one finding at or over --limit 2 tool error (bad arguments, parse failure, ...) - Status: Phase 2 - cross-file chain reporting works in the text format; - other formats land in Phase 3. See docs/plan.md. + 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. TXT; } From 02292ad5e8a3762b2c15093103720d754dceaa3d Mon Sep 17 00:00:00 2001 From: Lars Pohlmann Date: Tue, 11 Aug 2026 13:48:30 +0200 Subject: [PATCH 17/26] refactor(#5): extract shared pluralizer TextReporter and FindingMessage each carried an identical 3-arg plural() helper; SummaryReporter had a 2-arg variant. Extract one Pluralizer::of() used by all three so the pluralization rule lives in exactly one place. --- src/Report/FindingMessage.php | 21 ++++++++++----------- src/Report/Pluralizer.php | 22 ++++++++++++++++++++++ src/Report/SummaryReporter.php | 15 ++++++--------- src/Report/TextReporter.php | 28 +++++++++++----------------- tests/Report/PluralizerTest.php | 32 ++++++++++++++++++++++++++++++++ 5 files changed, 81 insertions(+), 37 deletions(-) create mode 100644 src/Report/Pluralizer.php create mode 100644 tests/Report/PluralizerTest.php diff --git a/src/Report/FindingMessage.php b/src/Report/FindingMessage.php index 36206e7..df4bbbe 100644 --- a/src/Report/FindingMessage.php +++ b/src/Report/FindingMessage.php @@ -13,10 +13,18 @@ */ final class FindingMessage { + private readonly Pluralizer $pluralizer; + + public function __construct() + { + $this->pluralizer = new Pluralizer(); + } + public function describe(Finding $finding): string { - return '$' . $finding->param . ': ' . $finding->hops . ' pass-through ' . $this->plural($finding->hops, 'hop') - . ' across ' . $finding->classes . ' ' . $this->plural($finding->classes, 'class', 'classes') + return '$' . $finding->param . ': ' . $finding->hops . ' pass-through ' + . $this->pluralizer->of($finding->hops, 'hop') + . ' across ' . $finding->classes . ' ' . $this->pluralizer->of($finding->classes, 'class', 'classes') . ' (terminal: ' . $this->terminalClause($finding) . ')'; } @@ -29,13 +37,4 @@ public function terminalClause(Finding $finding): string return $finding->terminal . ' [' . $finding->terminalKind->value . ']'; } - - private function plural(int $count, string $singular, ?string $plural = null): string - { - if ($count === 1) { - return $singular; - } - - return $plural ?? $singular . 's'; - } } diff --git a/src/Report/Pluralizer.php b/src/Report/Pluralizer.php new file mode 100644 index 0000000..0ade64a --- /dev/null +++ b/src/Report/Pluralizer.php @@ -0,0 +1,22 @@ +message = new FindingMessage(); + $this->pluralizer = new Pluralizer(); } /** @@ -87,7 +89,7 @@ private function hopLengthLabels(array $hopCounts): array { $labels = []; foreach ($hopCounts as $hops) { - $labels[$hops] = $hops . ' ' . $this->plural($hops, 'hop'); + $labels[$hops] = $hops . ' ' . $this->pluralizer->of($hops, 'hop'); } return $labels; @@ -109,7 +111,7 @@ private function topLongestChainsBlock(array $findings): string { $top = $this->longestFindings($findings); $hopLabels = array_map( - fn (Finding $finding): string => $finding->hops . ' ' . $this->plural($finding->hops, 'hop'), + fn (Finding $finding): string => $finding->hops . ' ' . $this->pluralizer->of($finding->hops, 'hop'), $top, ); $params = array_map(static fn (Finding $finding): string => '$' . $finding->param, $top); @@ -188,9 +190,9 @@ private function footer(array $findings): string array_filter($findings, fn (Finding $finding): bool => $finding->hops >= $this->thresholds->limit), ); - return $total . ' ' . $this->plural($total, 'chain') . ' total; ' + return $total . ' ' . $this->pluralizer->of($total, 'chain') . ' total; ' . $atOrOverLimit . ' at or over the limit (limit: ' - . $this->thresholds->limit . ' ' . $this->plural($this->thresholds->limit, 'hop') . ').'; + . $this->thresholds->limit . ' ' . $this->pluralizer->of($this->thresholds->limit, 'hop') . ').'; } /** @@ -217,9 +219,4 @@ private function nonEmptyMax(array $values): int return max($values); } - - private function plural(int $count, string $singular): string - { - return $count === 1 ? $singular : $singular . 's'; - } } diff --git a/src/Report/TextReporter.php b/src/Report/TextReporter.php index 6f24d68..6eba0a9 100644 --- a/src/Report/TextReporter.php +++ b/src/Report/TextReporter.php @@ -18,10 +18,13 @@ final class TextReporter implements Reporter private const COLUMN_GAP = ' '; private const MIN_LABEL_WIDTH = 8; + private readonly Pluralizer $pluralizer; + public function __construct( private readonly Thresholds $thresholds, private readonly bool $explain = false, ) { + $this->pluralizer = new Pluralizer(); } /** @@ -187,8 +190,8 @@ private function header(Finding $finding, Severity $severity): string $keyword = $severity === Severity::Warning ? 'WARNING' : 'FINDING'; return $keyword . ' $' . $finding->param . ': ' - . $finding->hops . ' pass-through ' . $this->plural($finding->hops, 'hop') - . ' across ' . $finding->classes . ' ' . $this->plural($finding->classes, 'class', 'classes'); + . $finding->hops . ' pass-through ' . $this->pluralizer->of($finding->hops, 'hop') + . ' across ' . $finding->classes . ' ' . $this->pluralizer->of($finding->classes, 'class', 'classes'); } /** @@ -198,35 +201,26 @@ private function summary(array $reportable): string { $count = count($reportable); if ($this->thresholds->warnLimit === null) { - return $count . ' ' . $this->plural($count, 'finding') . ' (' . $this->limitClause() . ').'; + return $count . ' ' . $this->pluralizer->of($count, 'finding') . ' (' . $this->limitClause() . ').'; } $errorCount = count(array_filter($reportable, static fn (array $entry): bool => $entry[1] === Severity::Error)); $warningCount = $count - $errorCount; - return $count . ' ' . $this->plural($count, 'finding') . ' (' - . $errorCount . ' ' . $this->plural($errorCount, 'error') . ', ' - . $warningCount . ' ' . $this->plural($warningCount, 'warning') . '; ' + return $count . ' ' . $this->pluralizer->of($count, 'finding') . ' (' + . $errorCount . ' ' . $this->pluralizer->of($errorCount, 'error') . ', ' + . $warningCount . ' ' . $this->pluralizer->of($warningCount, 'warning') . '; ' . $this->limitClause() . ').'; } private function limitClause(): string { - $clause = 'limit: ' . $this->thresholds->limit . ' ' . $this->plural($this->thresholds->limit, 'hop'); + $clause = 'limit: ' . $this->thresholds->limit . ' ' . $this->pluralizer->of($this->thresholds->limit, 'hop'); if ($this->thresholds->warnLimit !== null) { $clause .= ', warn-limit: ' . $this->thresholds->warnLimit - . ' ' . $this->plural($this->thresholds->warnLimit, 'hop'); + . ' ' . $this->pluralizer->of($this->thresholds->warnLimit, 'hop'); } return $clause; } - - private function plural(int $count, string $singular, ?string $plural = null): string - { - if ($count === 1) { - return $singular; - } - - return $plural ?? $singular . 's'; - } } diff --git a/tests/Report/PluralizerTest.php b/tests/Report/PluralizerTest.php new file mode 100644 index 0000000..7e51eeb --- /dev/null +++ b/tests/Report/PluralizerTest.php @@ -0,0 +1,32 @@ +of(1, 'hop')); + } + + public function testAppendsDefaultSSuffixWhenCountIsNotOne(): void + { + self::assertSame('hops', (new Pluralizer())->of(2, 'hop')); + self::assertSame('hops', (new Pluralizer())->of(0, 'hop')); + } + + public function testUsesExplicitIrregularPluralWhenCountIsNotOne(): void + { + self::assertSame('classes', (new Pluralizer())->of(4, 'class', 'classes')); + } + + public function testExplicitPluralIsIgnoredWhenCountIsOne(): void + { + self::assertSame('class', (new Pluralizer())->of(1, 'class', 'classes')); + } +} From 7aff4045876d7cbbfc3891430692c6d7d90639d5 Mon Sep 17 00:00:00 2001 From: Lars Pohlmann Date: Tue, 11 Aug 2026 13:50:24 +0200 Subject: [PATCH 18/26] fix(#5): relativize file paths in text reporter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TextReporter never received a Paths, so location() printed the raw absolute Hop::file path from FileLocator — contradicting the README's src/... example and diverging from every machine reporter. Wire a Paths through the constructor and ReporterFactory, and relativize in location() like the other reporters already do. --- src/Report/ReporterFactory.php | 2 +- src/Report/TextReporter.php | 3 ++- tests/Report/TextReporterTest.php | 37 ++++++++++++++++++++++++------- 3 files changed, 32 insertions(+), 10 deletions(-) diff --git a/src/Report/ReporterFactory.php b/src/Report/ReporterFactory.php index 08843a1..05dc2fc 100644 --- a/src/Report/ReporterFactory.php +++ b/src/Report/ReporterFactory.php @@ -18,7 +18,7 @@ public function create(Options $options): Reporter $thresholds = new Thresholds($options->limit, $options->warnLimit); return match ($options->format) { - 'text' => new TextReporter($thresholds, $options->explain), + 'text' => new TextReporter($thresholds, new Paths($this->workingDirectory), $options->explain), 'json' => new JsonReporter($thresholds, new Paths($this->workingDirectory)), 'github' => new GithubReporter($thresholds, new Paths($this->workingDirectory)), 'checkstyle' => new CheckstyleReporter($thresholds, new Paths($this->workingDirectory)), diff --git a/src/Report/TextReporter.php b/src/Report/TextReporter.php index 6eba0a9..95268c3 100644 --- a/src/Report/TextReporter.php +++ b/src/Report/TextReporter.php @@ -22,6 +22,7 @@ final class TextReporter implements Reporter public function __construct( private readonly Thresholds $thresholds, + private readonly Paths $paths, private readonly bool $explain = false, ) { $this->pluralizer = new Pluralizer(); @@ -134,7 +135,7 @@ private function label(int $index, bool $isTerminal): string private function location(Hop $hop): string { - return $hop->file . ':' . $hop->line; + return $this->paths->relativize($hop->file) . ':' . $hop->line; } /** diff --git a/tests/Report/TextReporterTest.php b/tests/Report/TextReporterTest.php index 355419c..45cf86a 100644 --- a/tests/Report/TextReporterTest.php +++ b/tests/Report/TextReporterTest.php @@ -7,6 +7,7 @@ use PhpTramp\Chain\Finding; use PhpTramp\Chain\Hop; use PhpTramp\Chain\TerminalKind; +use PhpTramp\Report\Paths; use PhpTramp\Report\TextReporter; use PhpTramp\Report\Thresholds; use PHPUnit\Framework\TestCase; @@ -44,7 +45,9 @@ public function testRendersStoredChainWithAlignedColumns(): void TXT; - self::assertSame($expected, (new TextReporter(new Thresholds(3, null)))->render([$finding])); + $reporter = new TextReporter(new Thresholds(3, null), new Paths('/nonexistent-root')); + + self::assertSame($expected, $reporter->render([$finding])); } public function testRendersTruncatedChainWithNoteLineAndNoTerminal(): void @@ -75,7 +78,9 @@ public function testRendersTruncatedChainWithNoteLineAndNoTerminal(): void TXT; - self::assertSame($expected, (new TextReporter(new Thresholds(2, null)))->render([$finding])); + $reporter = new TextReporter(new Thresholds(2, null), new Paths('/nonexistent-root')); + + self::assertSame($expected, $reporter->render([$finding])); } public function testRendersSingularFormsForOneHopOneClassOneFinding(): void @@ -95,7 +100,9 @@ public function testRendersSingularFormsForOneHopOneClassOneFinding(): void TXT; - self::assertSame($expected, (new TextReporter(new Thresholds(1, null)))->render([$finding])); + $reporter = new TextReporter(new Thresholds(1, null), new Paths('/nonexistent-root')); + + self::assertSame($expected, $reporter->render([$finding])); } public function testExplainAppendsTraceLinesUnderHeader(): void @@ -118,7 +125,9 @@ public function testExplainAppendsTraceLinesUnderHeader(): void TXT; - self::assertSame($expected, (new TextReporter(new Thresholds(1, null), true))->render([$finding])); + $reporter = new TextReporter(new Thresholds(1, null), new Paths('/nonexistent-root'), true); + + self::assertSame($expected, $reporter->render([$finding])); } public function testExplainDisabledOmitsTraceEvenWhenPresent(): void @@ -130,7 +139,7 @@ public function testExplainDisabledOmitsTraceEvenWhenPresent(): void $trace = ['Demo\A::go: method:step -> Demo\B::step']; $finding = new Finding('p', 'Demo\A::go', 'Demo\B::step', TerminalKind::Used, 1, $chain, 2, [], $trace); - $reporter = new TextReporter(new Thresholds(1, null)); + $reporter = new TextReporter(new Thresholds(1, null), new Paths('/nonexistent-root')); self::assertStringNotContainsString('explain:', $reporter->render([$finding])); } @@ -139,7 +148,7 @@ public function testEmptyFindingsRendersReassuringLine(): void { self::assertSame( "No tramp data found (limit: 3 hops).\n", - (new TextReporter(new Thresholds(3, null)))->render([]), + (new TextReporter(new Thresholds(3, null), new Paths('/nonexistent-root')))->render([]), ); } @@ -176,11 +185,23 @@ public function testRendersSingleWarningFindingAndFiltersBelowWarnLimit(): void TXT; - $reporter = new TextReporter(new Thresholds(3, 2)); + $reporter = new TextReporter(new Thresholds(3, 2), new Paths('/nonexistent-root')); self::assertSame($expected, $reporter->render([$warningFinding, $belowWarnLimitFinding])); } + public function testRelativizesAbsoluteFilePathUnderWorkingDirectory(): void + { + $chain = [ + new Hop('Demo\A::go', 'Demo\A', '/tmp/project/src/Demo.php', 5, null), + ]; + $finding = new Finding('p', 'Demo\A::go', 'Demo\A::go', TerminalKind::Used, 1, $chain, 1, [], []); + + $reporter = new TextReporter(new Thresholds(1, null), new Paths('/tmp/project')); + + self::assertStringContainsString('src/Demo.php:5', $reporter->render([$finding])); + } + public function testRendersMixedErrorAndWarningFindingsWithCombinedSummary(): void { $errorChain = [ @@ -223,7 +244,7 @@ public function testRendersMixedErrorAndWarningFindingsWithCombinedSummary(): vo TXT; - $reporter = new TextReporter(new Thresholds(3, 2)); + $reporter = new TextReporter(new Thresholds(3, 2), new Paths('/nonexistent-root')); self::assertSame($expected, $reporter->render([$errorFinding, $warningFinding])); } From 41cc10e741971c20913d4c6c2f0094f1b75ce530 Mon Sep 17 00:00:00 2001 From: Lars Pohlmann Date: Tue, 11 Aug 2026 13:53:02 +0200 Subject: [PATCH 19/26] fix(#5): escape github annotation property values GithubReporter only applied message-data escaping (%, \r, \n) to the file= and title= PROPERTY values of its workflow commands. Per @actions/core's escapeProperty, property values additionally need : and , escaped, since those characters are property separators. Add escapeProperty() (escaping % first so it never re-escapes the %3A/%2C/%0D/%0A it introduces) and apply it to both file= and title= on every annotation line. Updates the previously-frozen Task 6 github output and the README example to the corrected form. --- README.md | 2 +- src/Report/GithubReporter.php | 23 ++++++++--- tests/Report/GithubReporterTest.php | 62 +++++++++++++++++++++-------- 3 files changed, 64 insertions(+), 23 deletions(-) diff --git a/README.md b/README.md index bd06ba2..5e6d3c6 100644 --- a/README.md +++ b/README.md @@ -118,7 +118,7 @@ test plus a fixture. |---|---| | `text` | `FINDING $config: 3 pass-through hops across 4 classes` | | `json` | `{"limit":3,"warnLimit":null,"findings":[{"param":"config","severity":"error","hops":3,...}]}` | -| `github` | `::error file=src/Demo.php,line=12,title=phptramp::$config: 3 pass-through hops across 4 classes (terminal: Demo\Mailer::__construct [stored])` | +| `github` | `::error file=src/Demo.php,line=12,title=phptramp%3A%3A$config%3A 3 pass-through hops across 4 classes (terminal%3A Demo\Mailer%3A%3A__construct [stored])` | | `checkstyle` | `` | | `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).` | diff --git a/src/Report/GithubReporter.php b/src/Report/GithubReporter.php index ec1515b..85286a6 100644 --- a/src/Report/GithubReporter.php +++ b/src/Report/GithubReporter.php @@ -12,7 +12,8 @@ * `::warning` annotation at the origin per finding, plus one `::notice` per * subsequent hop (the terminal node, when present, is not annotated). See * https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions - * for the message-data escaping this reporter applies. + * for the property-value escaping this reporter applies to every `file=` and + * `title=` value. */ final class GithubReporter implements Reporter { @@ -67,18 +68,18 @@ private function originAnnotation(Finding $finding, Severity $severity): string $title = 'phptramp::' . $this->message->describe($finding); return '::' . $severity->label() - . ' file=' . $this->escape($this->paths->relativize($origin->file)) + . ' file=' . $this->escapeProperty($this->paths->relativize($origin->file)) . ',line=' . $origin->line - . ',title=' . $this->escape($title); + . ',title=' . $this->escapeProperty($title); } private function hopNotice(Finding $finding, Hop $hop, int $index): string { $title = 'phptramp::hop ' . ($index + 1) . ' of $' . $finding->param . ' chain from ' . $finding->origin; - return '::notice file=' . $this->escape($this->paths->relativize($hop->file)) + return '::notice file=' . $this->escapeProperty($this->paths->relativize($hop->file)) . ',line=' . $hop->line - . ',title=' . $this->escape($title); + . ',title=' . $this->escapeProperty($title); } /** @@ -96,9 +97,19 @@ private function subsequentHops(Finding $finding): array return $hops; } - private function escape(string $value): string + /** + * Escapes a workflow-command PROPERTY value (a `file=`/`title=` component), + * per `@actions/core`'s `escapeProperty`: the same %/\r/\n data-escaping as + * a command's message body, plus `:` and `,` — the two characters that + * would otherwise be misread as property separators. `%` must be escaped + * first, or the `%3A`/`%2C`/`%0D`/`%0A` sequences introduced below would + * themselves get re-escaped. + */ + private function escapeProperty(string $value): string { $value = str_replace('%', '%25', $value); + $value = str_replace(':', '%3A', $value); + $value = str_replace(',', '%2C', $value); $value = str_replace("\r", '%0D', $value); return str_replace("\n", '%0A', $value); diff --git a/tests/Report/GithubReporterTest.php b/tests/Report/GithubReporterTest.php index 9857823..37d6c62 100644 --- a/tests/Report/GithubReporterTest.php +++ b/tests/Report/GithubReporterTest.php @@ -43,12 +43,12 @@ public function testEmitsErrorAnnotationAtOriginAndNoticePerSubsequentHop(): voi [], ); - $expected = "::error file=src/Demo.php,line=12,title=phptramp::\$config: 3 pass-through hops across 4 " - . "classes (terminal: Demo\\Mailer::__construct [stored])\n" - . "::notice file=src/Demo.php,line=18,title=phptramp::hop 2 of \$config chain from " - . "Demo\\Controller::handle\n" - . "::notice file=src/Demo.php,line=24,title=phptramp::hop 3 of \$config chain from " - . "Demo\\Controller::handle\n"; + $expected = "::error file=src/Demo.php,line=12,title=phptramp%3A%3A\$config%3A 3 pass-through hops " + . "across 4 classes (terminal%3A Demo\\Mailer%3A%3A__construct [stored])\n" + . "::notice file=src/Demo.php,line=18,title=phptramp%3A%3Ahop 2 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()); @@ -63,8 +63,8 @@ public function testEmitsWarningAnnotationAndOmitsNoticeWhenNoSubsequentHops(): ]; $finding = new Finding('p', 'Demo\A::go', 'Demo\A::sink', TerminalKind::Used, 1, $chain, 1, [], []); - $expected = "::warning file=src/A.php,line=5,title=phptramp::\$p: 1 pass-through hop across 1 class " - . "(terminal: Demo\\A::sink [used])\n"; + $expected = "::warning file=src/A.php,line=5,title=phptramp%3A%3A\$p%3A 1 pass-through hop across 1 class " + . "(terminal%3A Demo\\A%3A%3Asink [used])\n"; $reporter = new GithubReporter(new Thresholds(3, 1), $this->paths()); @@ -100,8 +100,38 @@ public function testEscapesPercentCarriageReturnAndNewlineInAnnotationData(): vo [], ); - $expected = '::error file=src/100%25/A.php,line=5,title=phptramp::$p: 1 pass-through hop across 1 class ' - . "(terminal: Demo\\A::go%0D%0AInjected [used])\n"; + $expected = '::error file=src/100%25/A.php,line=5,title=phptramp%3A%3A$p%3A 1 pass-through hop across 1 ' + . "class (terminal%3A Demo\\A%3A%3Ago%0D%0AInjected [used])\n"; + + $reporter = new GithubReporter(new Thresholds(1, null), $this->paths()); + + self::assertSame($expected, $reporter->render([$finding])); + } + + /** + * A `,` in a property value would otherwise be misread as the separator + * between it and the next `key=value` pair; pins the escapeProperty() + * branch that the percent/CR/LF test above never exercises. + */ + public function testEscapesCommaInAnnotationPropertyValue(): void + { + $chain = [ + new Hop('Demo\A::go', 'Demo\A', 'src/A.php', 5, null), + ]; + $finding = new Finding( + 'p', + 'Demo\A::go', + 'Demo\A, Inc::sink', + TerminalKind::Used, + 1, + $chain, + 1, + [], + [], + ); + + $expected = '::error file=src/A.php,line=5,title=phptramp%3A%3A$p%3A 1 pass-through hop across 1 class ' + . "(terminal%3A Demo\\A%2C Inc%3A%3Asink [used])\n"; $reporter = new GithubReporter(new Thresholds(1, null), $this->paths()); @@ -141,12 +171,12 @@ public function testStillReportsQualifyingFindingAfterAnEarlierBelowThresholdFin [], ); - $expected = "::error file=src/Demo.php,line=12,title=phptramp::\$config: 3 pass-through hops across 4 " - . "classes (terminal: Demo\\Mailer::__construct [stored])\n" - . "::notice file=src/Demo.php,line=18,title=phptramp::hop 2 of \$config chain from " - . "Demo\\Controller::handle\n" - . "::notice file=src/Demo.php,line=24,title=phptramp::hop 3 of \$config chain from " - . "Demo\\Controller::handle\n"; + $expected = "::error file=src/Demo.php,line=12,title=phptramp%3A%3A\$config%3A 3 pass-through hops " + . "across 4 classes (terminal%3A Demo\\Mailer%3A%3A__construct [stored])\n" + . "::notice file=src/Demo.php,line=18,title=phptramp%3A%3Ahop 2 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()); From aaf2ed52b7525468bcf7ac1fa53ebd7ecf581650 Mon Sep 17 00:00:00 2001 From: Lars Pohlmann Date: Tue, 11 Aug 2026 13:55:08 +0200 Subject: [PATCH 20/26] refactor(#5): validate thresholds before indexing Thresholds (and thus warnLimit < limit validation) was first constructed inside ReporterFactory::create(), which runs after buildIndex() has already parsed every file, and a second time for the exit-code check. Construct it once, before buildIndex(), inside the existing try that already returns exit 2 on InvalidArgsException, and reuse it for the exit-code computation. An invalid --warn-limit/--limit combination now fails fast, before the codebase is parsed. --- src/Console/Application.php | 3 +-- tests/Console/ApplicationTest.php | 34 +++++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 2 deletions(-) diff --git a/src/Console/Application.php b/src/Console/Application.php index 9914a32..ef34ea5 100644 --- a/src/Console/Application.php +++ b/src/Console/Application.php @@ -84,6 +84,7 @@ private function workingDirectory(): string private function analyze(Options $options): int { try { + $thresholds = new Thresholds($options->limit, $options->warnLimit); $index = $this->buildIndex($options); $reporter = (new ReporterFactory($this->workingDirectory()))->create($options); } catch (InvalidArgsException | ParseException $e) { @@ -95,8 +96,6 @@ private function analyze(Options $options): int $findings = $this->findChains($index); fwrite($this->stdout, $reporter->render($findings)); - $thresholds = new Thresholds($options->limit, $options->warnLimit); - return $this->hasError($findings, $thresholds) ? 1 : 0; } diff --git a/tests/Console/ApplicationTest.php b/tests/Console/ApplicationTest.php index 9a25f23..6963e5e 100644 --- a/tests/Console/ApplicationTest.php +++ b/tests/Console/ApplicationTest.php @@ -136,6 +136,26 @@ public function testWarnOnlyRunExitsZeroButPrintsWarning(): void self::assertStringContainsString('WARNING', self::contents($this->stdout)); } + /** + * Thresholds validation must happen before the codebase is indexed, so an + * invalid --warn-limit/--limit combination fails fast: the fixture here is + * a normal, successfully-parsing 1-hop chain, since fail-fast only changes + * *when* the error surfaces, not whether this particular folder would + * parse. + */ + public function testInvalidWarnLimitFailsFastWithThresholdError(): void + { + $folder = $this->fixtureWithOneHopChain(); + + $exitCode = $this->app->run(['phptramp', '--folder', $folder, '--limit', '3', '--warn-limit', '5']); + + self::assertSame(2, $exitCode); + self::assertStringContainsString( + 'warn-limit (5) must be lower than limit (3)', + self::contents($this->stderr), + ); + } + public function testConfigFileLimitAppliesWithoutCliFlag(): void { $directory = sys_get_temp_dir() . '/phptramp-cwd-' . uniqid(); @@ -163,6 +183,20 @@ public function testConfigFileLimitAppliesWithoutCliFlag(): void self::assertStringContainsString('FINDING', self::contents($this->stdout)); } + private function fixtureWithOneHopChain(): string + { + $folder = sys_get_temp_dir() . '/phptramp-app-' . uniqid(); + mkdir($folder); + $this->folders[] = $folder; + + $code = 'c = $config; } }'; + file_put_contents($folder . '/Demo.php', $code); + + return $folder; + } + private function fixtureWithTwoHopChain(): string { $folder = sys_get_temp_dir() . '/phptramp-app-' . uniqid(); From f3f5ba0a9c203f95817204a8a406c10f555eada1 Mon Sep 17 00:00:00 2001 From: Lars Pohlmann Date: Tue, 11 Aug 2026 13:55:26 +0200 Subject: [PATCH 21/26] docs(#5): correct fixture coverage wording The output-formats section claimed all six formats are "covered by an exact-string test plus a fixture", but tests/fixtures/ only ever runs --format json. The other five formats have exact-string unit tests, not fixture coverage. Reword to say what's actually true. --- README.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 5e6d3c6..67a1af9 100644 --- a/README.md +++ b/README.md @@ -111,8 +111,9 @@ key, or a value of the wrong type, is a config error rather than a silently igno ## Output formats -`--format` selects the renderer; all six are implemented and covered by an exact-string -test plus a fixture. +`--format` selects the renderer; all six are implemented and each has an exact-string +unit test. `json` is additionally exercised end-to-end by the `tests/fixtures/` harness, +which always runs with `--format json`. | Format | One-line example | |---|---| From b89de5a6b3ab23dce6b6d4fa4def37b8448c1f17 Mon Sep 17 00:00:00 2001 From: Lars Pohlmann Date: Tue, 11 Aug 2026 14:05:48 +0200 Subject: [PATCH 22/26] test(#5): tighten text reporter relativization assertion testRelativizesAbsoluteFilePathUnderWorkingDirectory only asserted assertStringContainsString('src/Demo.php:5', ...), a substring that the raw un-relativized '/tmp/project/src/Demo.php:5' also contains -- so the test would still pass if location() stopped relativizing. Add assertStringNotContainsString('/tmp/project', ...) so the test actually discriminates between the two behaviors. --- tests/Report/TextReporterTest.php | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/Report/TextReporterTest.php b/tests/Report/TextReporterTest.php index 45cf86a..9c230dc 100644 --- a/tests/Report/TextReporterTest.php +++ b/tests/Report/TextReporterTest.php @@ -199,7 +199,10 @@ public function testRelativizesAbsoluteFilePathUnderWorkingDirectory(): void $reporter = new TextReporter(new Thresholds(1, null), new Paths('/tmp/project')); - self::assertStringContainsString('src/Demo.php:5', $reporter->render([$finding])); + $output = $reporter->render([$finding]); + + self::assertStringContainsString('src/Demo.php:5', $output); + self::assertStringNotContainsString('/tmp/project', $output); } public function testRendersMixedErrorAndWarningFindingsWithCombinedSummary(): void From f24df21915219605c6077fcbfde7655adf56512c Mon Sep 17 00:00:00 2001 From: Lars Pohlmann Date: Tue, 11 Aug 2026 14:22:16 +0200 Subject: [PATCH 23/26] refactor(#5): share json encoder across reporters JsonReporter and SarifReporter carried a byte-identical json_encode wrapper (same flags, same JsonEncodingException guard, same comment). Extract it to a shared JsonEncoder so the encoding policy lives in one place. --- src/Report/JsonEncoder.php | 30 ++++++++++++++++++++++++++++++ src/Report/JsonReporter.php | 21 ++++----------------- src/Report/SarifReporter.php | 20 +++----------------- 3 files changed, 37 insertions(+), 34 deletions(-) create mode 100644 src/Report/JsonEncoder.php diff --git a/src/Report/JsonEncoder.php b/src/Report/JsonEncoder.php new file mode 100644 index 0000000..0592f0c --- /dev/null +++ b/src/Report/JsonEncoder.php @@ -0,0 +1,30 @@ + $document + * @throws JsonEncodingException + */ + public function encode(array $document, string $subject): string + { + $json = json_encode($document, JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES); + if ($json === false) { + // Unreachable: $document is built entirely from scalars, strings, and + // arrays thereof — nothing here can produce a resource or invalid + // UTF-8 that would make json_encode fail. + throw new JsonEncodingException("Failed to encode findings as {$subject}."); + } + + return $json; + } +} diff --git a/src/Report/JsonReporter.php b/src/Report/JsonReporter.php index e0aee9f..abb868d 100644 --- a/src/Report/JsonReporter.php +++ b/src/Report/JsonReporter.php @@ -13,10 +13,13 @@ */ final class JsonReporter implements Reporter { + private readonly JsonEncoder $encoder; + public function __construct( private readonly Thresholds $thresholds, private readonly Paths $paths, ) { + $this->encoder = new JsonEncoder(); } /** @@ -30,7 +33,7 @@ public function render(array $findings): string 'findings' => $this->findingDocuments($findings), ]; - return $this->encode($document) . "\n"; + return $this->encoder->encode($document, 'JSON') . "\n"; } /** @@ -102,20 +105,4 @@ private function role(int $index, bool $isTerminal): string return 'hop'; } - - /** - * @param array $document - */ - private function encode(array $document): string - { - $json = json_encode($document, JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES); - if ($json === false) { - // Unreachable: $document is built entirely from scalars, strings, and - // arrays thereof — nothing here can produce a resource or invalid - // UTF-8 that would make json_encode fail. - throw new JsonEncodingException('Failed to encode findings as JSON.'); - } - - return $json; - } } diff --git a/src/Report/SarifReporter.php b/src/Report/SarifReporter.php index e76d07b..97a448e 100644 --- a/src/Report/SarifReporter.php +++ b/src/Report/SarifReporter.php @@ -20,12 +20,14 @@ final class SarifReporter implements Reporter . 'sarif-schema-2.1.0.json'; private readonly FindingMessage $message; + private readonly JsonEncoder $encoder; public function __construct( private readonly Thresholds $thresholds, private readonly Paths $paths, ) { $this->message = new FindingMessage(); + $this->encoder = new JsonEncoder(); } /** @@ -39,7 +41,7 @@ public function render(array $findings): string 'runs' => [$this->run($findings)], ]; - return $this->encode($document) . "\n"; + return $this->encoder->encode($document, 'SARIF') . "\n"; } /** @@ -132,20 +134,4 @@ private function location(Hop $hop): array ], ]; } - - /** - * @param array $document - */ - private function encode(array $document): string - { - $json = json_encode($document, JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES); - if ($json === false) { - // Unreachable: $document is built entirely from scalars, strings, and - // arrays thereof — nothing here can produce a resource or invalid - // UTF-8 that would make json_encode fail. - throw new JsonEncodingException('Failed to encode findings as SARIF.'); - } - - return $json; - } } From 91492ec78804683b576c70b55966507f96ff57c6 Mon Sep 17 00:00:00 2001 From: Lars Pohlmann Date: Tue, 11 Aug 2026 14:22:16 +0200 Subject: [PATCH 24/26] refactor(#5): reuse string-list validation in config loader assignPaths() re-implemented the same list-of-strings check requireStringList() already provides; delegate to it and keep only the files/folders split. --- src/Config/ConfigLoader.php | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/src/Config/ConfigLoader.php b/src/Config/ConfigLoader.php index 1278ba1..ab93896 100644 --- a/src/Config/ConfigLoader.php +++ b/src/Config/ConfigLoader.php @@ -121,15 +121,7 @@ private function toOptions(array $config): Options */ private function assignPaths(mixed $value, array &$folders, array &$files): void { - if (! is_array($value) || ! array_is_list($value)) { - throw new ConfigException('config key "paths" must be a list of strings'); - } - - foreach ($value as $path) { - if (! is_string($path)) { - throw new ConfigException('config key "paths" must be a list of strings'); - } - + foreach ($this->requireStringList('paths', $value) as $path) { if (str_ends_with($path, '.php')) { $files[] = $path; } else { From 293f50cef44945ecf0e2835f1e1f33675adce4f0 Mon Sep 17 00:00:00 2001 From: Lars Pohlmann Date: Tue, 11 Aug 2026 14:22:16 +0200 Subject: [PATCH 25/26] refactor(#5): simplify github escaping and hop slicing escapeProperty()'s five ordered str_replace calls (with a comment warning % must go first) collapse to one strtr(), which is single-pass and removes the ordering hazard. subsequentHops()'s manual loop becomes array_slice(). --- src/Report/GithubReporter.php | 26 +++++++++++--------------- 1 file changed, 11 insertions(+), 15 deletions(-) diff --git a/src/Report/GithubReporter.php b/src/Report/GithubReporter.php index 85286a6..d7f8d74 100644 --- a/src/Report/GithubReporter.php +++ b/src/Report/GithubReporter.php @@ -89,29 +89,25 @@ private function hopNotice(Finding $finding, Hop $hop, int $index): string */ private function subsequentHops(Finding $finding): array { - $hops = []; - for ($index = 1; $index < $finding->hops; $index++) { - $hops[$index] = $finding->chain[$index]; - } - - return $hops; + return array_slice($finding->chain, 1, $finding->hops - 1, true); } /** * Escapes a workflow-command PROPERTY value (a `file=`/`title=` component), * per `@actions/core`'s `escapeProperty`: the same %/\r/\n data-escaping as * a command's message body, plus `:` and `,` — the two characters that - * would otherwise be misread as property separators. `%` must be escaped - * first, or the `%3A`/`%2C`/`%0D`/`%0A` sequences introduced below would - * themselves get re-escaped. + * would otherwise be misread as property separators. `strtr` replaces every + * character in a single left-to-right pass, so the `%NN` sequences it + * introduces are never themselves re-escaped. */ private function escapeProperty(string $value): string { - $value = str_replace('%', '%25', $value); - $value = str_replace(':', '%3A', $value); - $value = str_replace(',', '%2C', $value); - $value = str_replace("\r", '%0D', $value); - - return str_replace("\n", '%0A', $value); + return strtr($value, [ + '%' => '%25', + ':' => '%3A', + ',' => '%2C', + "\r" => '%0D', + "\n" => '%0A', + ]); } } From 6964783a85cd0416435dec79a0ee276fe91d0fc5 Mon Sep 17 00:00:00 2001 From: Lars Pohlmann Date: Tue, 11 Aug 2026 14:22:16 +0200 Subject: [PATCH 26/26] refactor(#5): skip exclude relativization when unused The exclude path called realpath(workingDirectory) once per discovered file even when no exclude patterns exist (the common case). Short-circuit when exclude is empty and resolve the base realpath once per run. --- src/Discovery/FileLocator.php | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/src/Discovery/FileLocator.php b/src/Discovery/FileLocator.php index 1372447..8bd6764 100644 --- a/src/Discovery/FileLocator.php +++ b/src/Discovery/FileLocator.php @@ -52,10 +52,16 @@ public function locate(Options $options): array */ private function withoutExcluded(array $discovered, array $exclude): array { + if ($exclude === []) { + return $discovered; + } + + $base = realpath($this->workingDirectory); + $kept = []; - foreach ($discovered as $path => $true) { - if (! $this->isExcluded($path, $exclude)) { - $kept[$path] = $true; + foreach ($discovered as $path => $present) { + if (! $this->matchesAny($this->relativeTo($base, $path), $exclude)) { + $kept[$path] = $present; } } @@ -65,10 +71,8 @@ private function withoutExcluded(array $discovered, array $exclude): array /** * @param list $exclude */ - private function isExcluded(string $path, array $exclude): bool + private function matchesAny(string $relative, array $exclude): bool { - $relative = $this->relativeToWorkingDirectory($path); - foreach ($exclude as $pattern) { if (fnmatch($pattern, $relative)) { return true; @@ -78,9 +82,8 @@ private function isExcluded(string $path, array $exclude): bool return false; } - private function relativeToWorkingDirectory(string $path): string + private function relativeTo(string|false $base, string $path): string { - $base = realpath($this->workingDirectory); if ($base === false) { return $path; }