Repository navigation
feat(#7): diff-aware mode (flagship) + self-gating dogfood - #8
Merged
Merged
Conversation
Diff-aware mode against the shipped Phase 3 interfaces, plus two items pulled forward at the maintainer's request: a composer tramp script backed by the repo's own phptramp.dist.json, and the CI dogfood step becoming a real gate. Thresholds are measured, not guessed: the current tree has one 4-hop finding (CallResolver's $caller chain, to be fixed by refactoring, not suppressed) and six 2-hop chains, so limit 3 / warn-limit 2 bites without noise. The --no-config escape hatch lands first because the fixture harness's folder mode runs from the repo root and must not inherit the repo's own config.
Task 1 of Phase 4 (diff-aware mode): DiffParser turns unified diff text into a ChangedLines set of added/modified line numbers keyed by the diff's new-file paths. ChangedLines exposes containsLine/isEmpty plus resolveAgainst() to re-key paths to realpath-absolute form, matching Hop->file, for the intersection filter landing in a later task. The parser recognizes "+++ "/"@@ " structure only outside a hunk body, tracking the old-side/new-side line counts a hunk header declares and consuming exactly that many body lines before looking for structure again. Otherwise a hunk-body ADDED line whose own content starts with "++ " renders as "+++ ..." once the diff's leading marker is prepended, indistinguishable from a real file header, and would silently misattribute or drop every subsequent hunk until the next real header.
GitDiffRunner shells out to `git diff --unified=0 <base>...HEAD` (three-dot merge-base semantics) via proc_open with an argv array, so the user-supplied base ref is never shell-interpolated. A $base starting with '-' is rejected up front, closing the remaining argument-injection hole: `git` itself would otherwise parse a leading-dash argv element as an option (e.g. `--output=<path>`) rather than a revision, even though the shell never gets a chance to interpret it. Non-zero exit raises DiffException carrying git's stderr, falling back to a message naming the exit code when git wrote nothing to stderr (e.g. killed by a signal). Stdout is returned on success. Reuses the existing PhpTramp\Diff\DiffException from Task 1. Tests build a real throwaway git repo per test in a temp dir (init, commit, branch, commit) rather than mocking git, covering the success path, an unknown base ref, a non-repository working directory, and a leading-dash base (asserting git is never invoked with the injected option). Skips itself via `git --version` when git is unavailable.
Implements frozen rule 9: diff-aware mode keeps a finding iff at least one of its hops intersects the diff, marking which hops are the user's. Hop->changed is a purely additive, default-false constructor parameter so every existing call site is untouched. ChangedChainFilter is a pure post-processor over the finished Finding list; the terminal node (forwardLine === null) never matches and is never marked.
Wires the Task 1-3 diff-aware building blocks into the CLI: a new --diff <path|-> flag reads a unified diff from a file or stdin instead of shelling out to git, and Application's analyze() pipeline now runs the changed-only filter (when active) between chain-building and reporting. --diff implies --changed-only, since naming a diff source with no filter to apply it to is contradictory. An empty diff is a success (no findings, exit 0), not a failure, so a doc-only PR never fails the gate. Also introduces DiffAwareFlags, grouping ArgvParser's changedOnly/gitBase/diff fields behind one holder: adding $diff as a fourth flat field would have pushed ArgvParser's field count from 15 to 16, tripping PHPMD's TooManyFields ceiling. The three flags already form a conceptual unit (the CLI help text's own "Diff-aware mode:" section), so grouping them is a real fix rather than a threshold tweak.
Surface Hop::changed (set by ChangedChainFilter in changed-only runs) in three reporters: - TextReporter: a changed non-terminal hop gets " *YOURS*" appended after its location column, via the existing per-row `annotation` slot the terminal already uses for "(kind)". - JsonReporter: chain entries gain a trailing "changed": true|false key, but only when the new constructor flag `changedOnly` is true (wired from Options::$changedOnly via ReporterFactory). Outside changed-only runs the key is omitted entirely, so every Phase 3 JSON fixture stays byte-identical. - GithubReporter: the ::error/::warning annotation now anchors at the first changed non-terminal hop instead of unconditionally at the origin, falling back to the origin when nothing is marked (every normal run). The title gains a " (hop N of the chain, changed by this diff)" suffix only when anchored off-origin. Every other non-terminal hop, including the origin when it's not the anchor, becomes a ::notice in chain order. When nothing is changed this reproduces today's output exactly. CheckstyleReporter, SarifReporter and SummaryReporter are deliberately unchanged this phase: their consumers (CI annotation ingestion, code scanning) key on stable locations, and revisiting that is left for real usage feedback.
The task-5 brief required documenting the new JsonReporter "changed" key in the README's output-formats section; it was omitted from the original commit. Add a two-sentence note after the format table: each chain entry gains "changed": true|false only in a --changed-only run, absent otherwise.
Adds a --no-config bool flag that bypasses ConfigLoader entirely. Because config seeds the parser's defaults, run() detects it from the raw args via a small loadDefaults() helper before parsing exists. The fixture harness's folder mode and --dump-index helper, plus every repo-root ApplicationTest analysis run, now pass --no-config so the repo's own root config (next commit) can never leak into the tests.
Adds phptramp.dist.json (paths: src, limit 3, warn-limit 2 — thresholds measured against the actual tree) and a "tramp": "@php bin/phptramp" composer script so the repo gates itself the config-driven way, args still passing through (composer tramp -- --format summary). Semantics refinement (flagged per CLAUDE.md): a no-argument invocation now analyzes when config supplies paths; help is shown only when there is nothing to analyze (no folders and no files, whether from args or config). run()'s dispatch is reordered so --help/--version/--dump-index are unaffected and the old fragile $args === [] guard is replaced by a nothingToAnalyze() predicate. composer tramp is deliberately red here (exit 1, the single 4-hop $caller chain through CallResolver); the next commit refactors that finding away. The gate does not enter CI until a later commit, so the suite stays green.
functionTarget/functionCandidates only ever needed the caller's namespace, yet the whole MethodInfo $caller was threaded four levels deep (resolve -> resolveFunction -> functionTarget -> functionCandidates) just to reach namespaceOf() at the bottom — phptramp's own flagship tramp-data finding. Derive the namespace once in resolveFunction via namespaceOf($caller) and pass the resulting string down. The 4-hop error collapses to a 2-hop chain (resolve -> resolveFunction -> namespaceOf), below the limit-3 threshold, so composer tramp now exits 0. Resolution semantics are unchanged: the existing tests/Resolve suite stays byte-green and no fixtures move.
The dogfood step was report-only (--limit 99, never failing). Now that the tree is deliberately chain-clean it runs composer tramp, which gates on phptramp's own phptramp.dist.json (limit 3, warn-limit 2) and fails the build on any finding at or over the limit. Updates the stale 'Phase 6 lowers it' comment. README: documents that this repo gates itself with composer tramp — the same script a consuming project adds, here driven by config paths rather than an inline --folder.
Three FixtureTest cases exercise --diff end-to-end through the real CLI: a changed middle hop keeps the finding and marks it, a two-chain source proves --changed-only drops the untouched chain entirely, and a diff that only touches the terminal's declaration line yields zero findings (the terminal is never a hop and never matches).
Add docs/ci.md with copy-paste GitHub Actions / GitLab MR / arbitrary-diff / full-scan recipes. Bring README, the CLI help text, and docs/plan.md up to date with diff-aware mode being shipped rather than reserved, including a present-tense *YOURS* example in the README's flagship section.
Quality-only cleanups from the /simplify pass; no behavior change (all 321 tests stay green, reporter output byte-identical): - GithubReporter: compute nonTerminalHops once per finding instead of slicing the chain twice (anchorIndex + noticeHops). - ChangedChainFilter: single-pass rebuild via new immutable withers Hop::withChanged() and Finding::withChain(), so the filter no longer hand-reconstructs those value objects positionally (decouples it from their full constructor signatures) and walks each chain once. - ArgvParser: share one ArgvParser::NO_CONFIG_FLAG constant with Application's pre-parse raw-args detection instead of duplicating the literal across files.
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 #7
Phase 4 — the flagship diff-aware feature, plus two items pulled forward from Phase 6 (the repo's own
composer trampgate).What ships
phptramp --changed-only --git-base origin/mainreports only chains intersecting the diff and marks which hops are yours (frozen rule 9: any-hop intersection at changed-line granularity; the terminal is never a hop and never qualifies a chain).src/Diff/layer:DiffParser(unified diff →ChangedLines),GitDiffRunner(git diff --unified=0 base...HEADviaproc_open, argv-array — shell and leading-dash argument injection both guarded),ChangedChainFilter(drops non-intersecting chains, rebuilds kept ones immutably withHop->changed). The chain walk never learns about diffs.--diff <path|->reads a unified diff from a file or stdin (implies--changed-only); an empty diff is success (exit 0), so doc-only PRs never fail.*YOURS*, json conditional"changed"key (only in changed-only runs, so every Phase 3 fixture stays byte-stable), github anchors the::error/::warningat the first changed hop. checkstyle/sarif/summary unchanged by design.--no-configescape hatch so the repo's own config can't leak into tests.phptramp.dist.json(limit 3, warn-limit 2 — measured), acomposer trampscript, and CI's dogfood step is now a real gate. phptramp's own former 4-hop$callerself-finding was refactored away inCallResolver(not suppressed).docs/ci.mdcookbook (GitHub Actions + GitLab MR + piping + the self-gate pattern), README flagship section, help text,docs/plan.mdPhase 4 ✅.Verification
composer checkgreen (321 tests, 1039 assertions) at every commit.composer infection:diff= 94% MSI (gate is minMsi 80).changed-hop-reported,changed-only-filters-rest,changed-terminal-not-enough— each verified to pass for the right reason.Built task-by-task with a fresh implementer + independent spec/quality review per task, a final whole-branch review (no blocking findings), and a
/simplifyquality pass.