test: cover the destructive paths, and fix two gaps they exposed - #27
Merged
Merged
Conversation
The delete/write paths had the weakest verification in the codebase, which is where a bug is unrecoverable for a user: #22 found two live data-loss bugs there, and the areas around them were still thin. 66 new tests, plus the two fixes they turned up. Fixes: - readManagedManifest crashed with "Cannot read properties of null" when a manifest body was null, a bare string, a number, or an array. JSON.parse accepts all four, and the field checks then dereferenced them, so a truncated or hand-edited manifest produced a TypeError instead of the intended "Invalid managed manifest". - clean() now refuses to delete a path another configured target manifest also claims (CHK-035). build() rejects overlapping output paths and prune() inherits that check by running build({dryRun: true}) first, but clean() deliberately never builds, so it could not inherit it. --force still wins so a repo can always be torn down. Coverage on the destructive modules, branch: fs 32 -> 82, cleanup 50 -> 83, managed 67 -> 78, diff 55 -> 68. Two of the new tests were mutation-verified rather than trusted: interleaving build() write/prune loops turns the ordering test red (closing CHK-034, which had no regression test), and disabling the new clean guard turns its refusal tests red. Two branches were deliberately left untested because they are unreachable rather than uncovered: removeManagedPath escape check (normalizeManagedPath rejects everything that could reach it) and the engine rootFiles destination check (config validation rejects the key first). The rootFiles test asserts the layer that actually enforces it, with a comment saying why. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
The file-deleting half of pluginpack had the weakest verification in the codebase. That is where a bug is unrecoverable for a user, and it is where #22 already found two live data-loss bugs. This fills it in: 66 new tests, plus the two fixes they turned up.
Fixes
readManagedManifest crashed on a manifest body it should have rejected cleanly.
JSON.parseacceptsnull, a bare string, a number, and an array; the field checks then dereferenced them, so a truncated or hand-edited.pluginpack/<target>.jsonproducedTypeError: Cannot read properties of null (reading version)instead of the intendedInvalid managed manifest.clean() now refuses to delete a path another configured target manifest also claims (closes CHK-035).
build()rejects two targets writing overlapping output paths, andprune()inherits that check for free by runningbuild({dryRun: true})first.clean()deliberately never builds — teardown has to keep working when the source tree or config no longer does, which is often exactly why someone is cleaning — so it could not inherit the check the same way. This is the standalone equivalent, working purely from the manifests on disk.--forcestill wins, so a repo can always be torn down.Coverage
Branch coverage on the destructive modules:
fs.ts32 to 82,cleanup.ts50 to 83,managed.ts67 to 78,diff.ts55 to 68. Total 74 to 78.Worth noting for anyone reading those numbers: v8 line attribution is unreliable in this repo (it reported
adapters.tsline 40 with 111 hits while lines 25-33, which you must execute to reach it, showed 0). This work was driven by enumerating behaviors and grepping for error messages with no test, not by chasing line data.What is covered
tests/destructive.test.ts(62 tests): every escape shapenormalizeManagedPathmust reject and every normalization it must accept;writeArtifactrefusing relative, absolute, and post-normalization escapes; all six invalid manifest bodies; the delete guard applied to clean, which previously had none — source-tree paths, the config file, the default unsetsource.pluginsroot,--forcedeleting, every blocked path named; clean against an unconfigured target, an unbuilt target, and all targets at once; empty-parent cleanup that stops at a directory holding an unmanaged file; symlinks pointing inside the output dir; already-missing files; and diff added/changed/removed/ignored classification.tests/core.test.ts(4 tests): root-skills id collision, missingsource.skillsdirectory, unreadablerootFilessource, escapingrootFilesdestination.Verification
Two tests were mutation-verified rather than trusted:
build()write/prune loops makes the ordering test fail with ENOENT on the stale file. That closes CHK-034, which the previous review left open specifically because nothing would fail red on a reorder.Two branches were deliberately left untested because they are unreachable rather than uncovered:
removeManagedPathescape check (normalizeManagedPathrejects everything that could reach it) and the enginerootFilesdestination check (zod rejects the key first). TherootFilestest asserts the layer that actually enforces it, with a comment explaining why.🤖 Generated with Claude Code