Add prune-baseline command with shrink-only semantics#657
Draft
fain182 wants to merge 8 commits into
Draft
Conversation
Closes #649. Regenerating the baseline snapshots the entire current state, so it silently legitimizes any violation introduced since the baseline was created. prune-baseline is the safe counterpart: phparkitect prune-baseline [filename] It runs the analysis and keeps only the intersection between the existing baseline and the current violations — the baseline can only shrink. Matching ignores line numbers and the kept entries are the current violations, so pruning also refreshes line numbers gone stale after refactorings. Unlike generation, pruning is safe to automate. The check hint introduced in #638 now points to the new command instead of suggesting a (risky) full regeneration. This change also splits the two responsibilities Baseline used to mix, since pruning needs the full load -> transform -> save lifecycle: - Baseline is now a pure domain object: applyTo(), stale counting, the new prune() (built on the new Violations::intersection(), the complementary operation of Violations::remove(), reusing the same stable-key matching) and withoutLineNumbers(). - BaselineFileRepository owns every filesystem concern: load(), save(), resolveFilePath() and the default filename. Check, generate-baseline and prune-baseline all take it via constructor injection. The command follows the same structure as its siblings, composing CommonOptions and CommandRuntime. --ignore-baseline-linenumbers on prune-baseline strips line numbers from the saved file (matching already ignores them by design). Covered by unit tests (Baseline, BaselineFileRepository, PruneBaselineHandler, Violations::intersection) and E2E tests; README and CLAUDE.md updated. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015K1TKiwtwkW7twBCTz758P
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #657 +/- ##
============================================
- Coverage 98.28% 97.84% -0.45%
- Complexity 737 762 +25
============================================
Files 94 98 +4
Lines 2100 2181 +81
============================================
+ Hits 2064 2134 +70
- Misses 36 47 +11
🚀 New features to boost your workflow:
|
…stored format instead On prune-baseline the flag did not control matching (pruning always matches ignoring line numbers — that is how it refreshes stale ones): it only stripped line numbers from the saved file. That made it a footgun for the line-numberless workflow (generate-baseline -i + check -i): forgetting it on a prune would silently convert the baseline back to a line-numbered file, reintroducing exactly the drift the user opted out of. Instead of an output-format flag, Baseline::prune() now preserves the stored format: a baseline whose entries have no line numbers stays line-numberless after pruning. No flag to remember, and pruning can never make the baseline worse. CommonOptions::addTo() loses the -i option, which moves to a separate addIgnoreBaselineLinenumbers() composed only by check and generate-baseline, where the option actually means something. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015K1TKiwtwkW7twBCTz758P
The stderr redirect exists in check because stdout is reserved for the violations payload (json/gitlab formats piped to files). The baseline commands have no stdout payload — their console output is all diagnostics — so redirecting it to stderr was cargo cult that made 'phparkitect generate-baseline > log.txt' capture nothing. They now write to the output they receive, like any Symfony command; progress bars still go to stderr via Symfony's own ConsoleOutputInterface handling. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015K1TKiwtwkW7twBCTz758P
The old name hid both decisions the method took: 'resolve' did not say it probes the filesystem, and the nullable-in/nullable-out signature mixed two different concerns — the explicit-path-wins precedence (command policy) and the default-file auto-detection (persistence knowledge). The precedence now reads inline in Check::parseOptions, and the repository keeps only its own piece with a name that says what it does: findDefaultFilePath() returns the default baseline file or null when none exists. The RiskyTruthyFalsyComparison suppression dies with the falsy-parameter check. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015K1TKiwtwkW7twBCTz758P
findDefaultFilePath() returned ?string but carried a single bit: the non-null value was always DEFAULT_FILENAME, never a different path. So it was a yes/no question dressed as a finder. It becomes hasDefaultBaseline(): bool, a pure persistence predicate, and Check maps 'default exists' to the DEFAULT_FILENAME path — keeping the path-selection policy in the command layer, consistent with where the explicit-wins precedence already lives. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015K1TKiwtwkW7twBCTz758P
…alue 'Which baseline to subtract' was represented twice — as ?string baselineFilePath and bool skipBaseline — and resolved in two layers: parseOptions handled --use-baseline and the default file, while CheckHandler handled skip (skip ? null : path). skip is just another route to 'empty baseline', i.e. baselineFilePath = null, so the two fields encoded the same decision. The decision now lives in one place, Check::resolveBaselineFilePath(), which reads top-to-bottom as the precedence it implements: --skip-baseline forces empty; an explicit --use-baseline wins over the auto-detected default; a missing default just means empty. Its output is the single ?string baselineFilePath, where null means 'empty baseline'. CheckOptions loses the redundant skipBaseline field, and CheckHandler drops the baselineFilePath/skipBaseline pass-through it wrote into Config and immediately read back (nothing else reads them from Config) — it now loads-or-empties straight from the resolved value. No behavior change: the CLI still resolves exactly as before. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015K1TKiwtwkW7twBCTz758P
baselineFilePath is a field — a value — but resolveBaselineFilePath was stuffing filesystem logic into it: 'the default path, but only if the file exists'. That is why hasDefaultBaseline() had to probe disk at parse time; a path field secretly carried an existence fact. Now the field holds a plain path: --use-baseline, or the default phparkitect-baseline.json (null only for --skip-baseline). Existence is decided where loading happens: CheckHandler loads the baseline if the file is there and falls back to an empty one otherwise — the baseline is optional for check, so an absent file just means nothing to ignore. hasDefaultBaseline() is replaced by a general BaselineFileRepository:: exists(), used by the handler for both the fallback and the 'found' message. Behavior change: check --use-baseline=missing.json no longer errors, it runs with an empty baseline — consistent with a missing default, and it fails in the safe direction (all violations shown). prune-baseline keeps its strict load: there a missing baseline is still an error. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015K1TKiwtwkW7twBCTz758P
exists() was a thin file_exists() wrapper with a single caller, which then did check-then-load: exists() followed by load(). Fold both into one repository capability — loadIfPresent() returns the baseline, or null when the file is absent — so CheckHandler makes one call, there is no TOCTOU gap, and the method name states the optional-load intent. load() stays strict for prune-baseline; loadIfPresent() is the optional variant for check, where a missing baseline just means nothing to ignore. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015K1TKiwtwkW7twBCTz758P
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
2 participants
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.
New Feature
Summary
Completes the
*-baselinecommand family started with #655/#656. Regenerating the baseline snapshots the entire current state, so it silently legitimizes any violation introduced since the baseline was created — the classic baseline trap.prune-baselineis the safe counterpart:Violations::intersection(), the complementary operation ofViolations::remove(), reusing the same stable-key matching) and the kept entries are the current violations, so pruning also repairs a baseline whose line numbers drifted after refactorings.💡 1 violation in the baseline looks fixed — runphparkitect prune-baselineto remove it(it used to suggest a risky full regeneration).--ignore-baseline-linenumbersflag: on pruning it would not control matching (which always ignores line numbers) but only the saved format — a footgun for the line-numberless workflow. Instead,Baseline::prune()preserves the stored format: a baseline generated with-i(no line numbers) stays line-numberless after pruning, with nothing to remember. The-ioption definition moved out ofCommonOptions::addTo()into a separate method composed only bycheckandgenerate-baseline, where it actually means something.Pruning needs the full load → transform → save lifecycle, so this PR also splits the two responsibilities
Baselineused to mix:Baselineis now a pure domain object:applyTo(), stale counting, the newprune()andwithoutLineNumbers().BaselineFileRepositoryowns every filesystem concern:load(),save(),resolveFilePath()and the default filename. All three handlers (check,generate-baseline,prune-baseline) take it via constructor injection.The command mirrors its siblings, composing
CommonOptionsandCommandRuntime.Covered by unit tests (
BaselineTest— including format preservation,BaselineFileRepositoryTest,PruneBaselineHandlerTest,Violations::intersectioncases) and E2E tests (PruneBaselineCommandTest); the full generate → hint → prune cycle was also exercised against the real CLI. README and CLAUDE.md updated.🤖 Generated with Claude Code
https://claude.ai/code/session_015K1TKiwtwkW7twBCTz758P