Repository navigation
Phase 3: CI & config surface (formats, config, suppression, warn-limit) - #6
Merged
Merged
Conversation
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).
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.
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.
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.
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.
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.
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.
Adds CheckstyleReporter: findings grouped into <file> elements by relativized origin file (first-seen file order, input order within a file), one <error> 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 <checkstyle> skeleton with zero <file> children.
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.
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.
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.
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.
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.
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).
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.
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.
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.
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.
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.
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.
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.
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.
assignPaths() re-implemented the same list-of-strings check requireStringList() already provides; delegate to it and keep only the files/folders split.
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().
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #5
Phase 3 makes phptramp CI-shaped: machine output formats, a config file, suppression, and warning severity.
What's new
--warn-limit— severity is computed at reporting time (Thresholds/Severity);Findingstays threshold-agnostic. Warnings never affect the exit code.ReporterFactory:json,github(Actions annotations),checkstyle,sarif, andsummary(whole-codebase overview). Each has a byte-for-byte exact-string test.phptramp.jsonconfig with CLI-over-config precedence (the parser is seeded with config-derived defaults) andexcludefnmatch globs.#[TrampIgnore](matched by short name; class/method/function/param level) and// phptramp-ignorecomments (declaration line, line above, or forwarding call-site line), detected only in real comment tokens.docs/plan.md, and CLI help updated to match actual behavior; new end-to-end fixtures for warn/error, suppression, and config-driven runs.Notable decision
The GitHub reporter now escapes annotation property values per GitHub's spec (
:→%3A,,→%2C), a deliberate change from the plan's originally-frozen contract — without it, a title likephptramp::$config: …misparses on the runner.Quality
composer checkgreen (267 tests, 761 assertions); diff-scoped Infection at ~96% MSI (gate 80)./simplifycleanup pass (sharedJsonEncoder, reused config validation,strtr-based GitHub escaping, cheaperFileLocatorexclude path).Exit-code contract (unchanged)
0no errors ·1≥1 error-severity finding ·2tool error.