test(packman,config): pin that gitignored artifacts do not dirty a cached import - #11
Open
benw5483 wants to merge 3 commits into
Open
test(packman,config): pin that gitignored artifacts do not dirty a cached import#11benw5483 wants to merge 3 commits into
benw5483 wants to merge 3 commits into
Conversation
…ched import Both cache-dirtiness checks deliberately run `git status --porcelain` without --ignored, so a pack's own gitignored artifacts (__pycache__/*.pyc, .DS_Store, a runtime state directory recreated after install) are not treated as local edits. Neither call site had a test, so re-adding the flag would pass CI while wedging every city behind a perpetual "run gc import install" gate that no .gitignore can escape -- --ignored prints a `!! <path>` line for exactly the files the ignore rule was meant to neutralize, and the city loses every pack-provided subcommand. Adds coverage at both call sites: - internal/packman cachedRepoDirty, against a real git repo. One test asserts a gitignored .runtime/ artifact leaves the cache clean, guarded by a positive control that fails the fixture if `--ignored` would not have seen it. A table test asserts untracked, edited, and deleted files still report dirty. - internal/config validateLockedRemoteCache, using the package's existing runRepoCacheGit stub, asserting the status invocation carries no --ignored and that a modified worktree is still rejected. Verified by injection: re-adding --ignored at either call site turns that site's test red, and reverting turns it green again. Generated by the operator's software factory. City: factory-main · Agent: local-core.builder-5 On behalf of: @benw5483 Co-Authored-By: <operator-factory-bot> <factory-bot@actual.invalid>
…ng out The first version of this test drove a real git repo. That tripped the checked resource ledger in internal/testpolicy/resourcecensus: it is an anti-growth ratchet on subprocess use in test source, and the file pushed the untagged Small baseline from 391 calls / 110 files to 392 / 111. Clearing that would have meant declaring a Medium owner and bumping audit baselines that belong to another tracking owner, which is a policy decision this change has no reason to make. Stubbing runGit is also the established pattern in this package -- every other test in internal/packman already does it, and the real-git version was the lone exception. The assertion does not weaken. The stub models git's own behavior, returning a `!! <path>` line when --ignored is present and nothing when it is not, so the test still pins the outcome: an ignored artifact leaves the cache clean. It also now asserts directly that the status invocation carries no --ignored, which is the defect itself. The companion table gains a staged-addition case. Re-verified by injection after the rewrite: adding --ignored back to cachedRepoDirty turns this test red, and reverting turns it green. Generated by the operator's software factory. City: factory-main · Agent: local-core.builder-5 On behalf of: @benw5483 Co-Authored-By: <operator-factory-bot> <factory-bot@actual.invalid>
Review caught a real hole. The guards matched the flag with an exact string comparison against "--ignored", but git accepts several spellings of the same behaviour, and they are not interchangeable to an equality check. Verified against git 2.50.1 on a repo with a gitignored .runtime/: --ignored, --ignored=traditional and --ignored=matching each print the identical "!! .runtime/" line, and only --ignored=no suppresses it. So --ignored=matching reintroduced the exact defect these tests exist to ratchet against while all four of them stayed green. The packman stub made it worse by modelling git with the same too-narrow predicate, so the outcome assertion agreed with the flag assertion instead of checking it independently. Both files now share a statusShowsIgnored helper that matches on the "--ignored" prefix, exempts only --ignored=no, and fails closed on unrecognized modes. The packman stub and the assertion both route through it. Two follow-ups from the same review: - TestValidateLockedRemoteCacheRejectsModifiedWorktree asserted only that an error came back. validateLockedRemoteCache fails earlier when rev-parse HEAD disagrees with the locked commit, so a reorder could have kept the test green with the status check rejecting nothing. It now asserts that status actually ran and that the error is the worktree one. Confirmed by forcing a HEAD mismatch: the test fails with "never ran git status" rather than passing. - gitStatusStub returns a closure accessor instead of a pointer to a slice. Injection matrix re-run at this head, one spelling at a time, both call sites: --ignored, --ignored=traditional and --ignored=matching all fail; --ignored=no passes, which is the negative control showing the guard is not simply refusing every argument. Production files reverted after each run. Generated by the operator's software factory. City: factory-main · Agent: local-core.builder-5 On behalf of: @benw5483 Co-Authored-By: <operator-factory-bot> <factory-bot@actual.invalid>
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
1 participant
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.
Summary
internal/packmancachedRepoDirtyandinternal/configvalidateLockedRemoteCache. Both deliberately rungit status --porcelainwithout--ignored, and neither had a test.--ignoredtoday is a green build.Why this matters
--ignoredprints a!! <path>line for every gitignored file. A pack's own artifacts land in the cache clone in place:__pycache__/*.pycfrom running a cached pack's scripts, a stray.DS_Store, a runtime state directory recreated seconds after install. With the flag on, those count as local worktree changes, so the city fails validation and drops every pack-provided subcommand until someone runsgc import install. Then the artifact comes back and it all happens again.The part worth pinning is that no
.gitignoregets you out of this. Ignoring the path is exactly what turns a??line into a!!line, and--ignoredreports both, so a pack can't defend itself by ignoring its own artifacts. That leaves chasing down each individual writer as the only remedy. It's a lot of work to have to re-learn from scratch.What the tests assert
Both files stub the package's git hook, which is what every other test in these packages already does.
internal/packman/cache_dirty_test.gocoverscachedRepoDirty:!! <path>line when--ignoredis present and nothing when it isn't. So the test pins the outcome, an ignored artifact leaves the cache clean, and not just the call shape.--ignored, which is the defect itself.internal/config/pack_include_cache_dirty_test.gocoversvalidateLockedRemoteCachethrough the existingrunRepoCacheGitstub, asserting the same absent flag and that a modified tracked file is still rejected.An earlier revision drove a real git repo instead, which tripped the checked resource ledger in
internal/testpolicy/resourcecensus. That ledger ratchets subprocess use in test source, and clearing it would've meant declaring a Medium owner and bumping audit baselines that belong to another tracking owner. That's a policy call this change has no business making, so the tests went hermetic instead.Test plan
go test ./internal/packman/ ./internal/config/ ./internal/testpolicy/resourcecensus/passes.internal/packman/cache.goandinternal/config/pack_include.go), reverting production files after each run.--ignoredis not one flag. Against git 2.50.1, on a repo with a gitignored.runtime/, three spellings print the identical!! .runtime/line and one does not:--ignored--ignored=traditional--ignored=matching--ignored=noThat last row is the negative control, showing the guards aren't just refusing every argument they see. An earlier revision matched the flag by exact string equality, so
--ignored=matchingreintroduced the defect with every test still green. Review caught it. The sharedstatusShowsIgnoredhelper now matches on the prefix, exempts only--ignored=no, and fails closed on modes it doesn't recognize, so it's worth a second thought before anyone narrows it back to an equality check.TestValidateLockedRemoteCacheRejectsModifiedWorktreealso asserts that status actually ran, since the function has an earlier failure path on a HEAD mismatch and would otherwise stay green with the status check rejecting nothing. Force that mismatch and the test fails, as it should.Note for reviewers: this changes no production code. It is coverage for behavior that already ships.