♻️ refactor(repo): cut two dependencies, one triplicated delegate, and the seams with no production side - #73
Merged
Merged
Conversation
… the standard library gofrs/flock wrapped one non-blocking flock(2) attempt, and the polling that makes waiting cancellable was already written here rather than taken from it. renameio wrapped a temporary file, a chmod and a rename, and its one documented gap -- it fsyncs the file but not the directory -- was already compensated for at the call site. Both are now what they wrap. The lock keeps its own descriptor and releases it explicitly before the close, so a forked descriptor cannot outlive the release. The atomic write syncs before the rename rather than after, which is the ordering renameio was providing. Also: slices.Sorted for a hand-written insertion sort, slog.DiscardHandler for a hand-written one, and ui.Truncate for the tty renderer's own copy -- which was not ANSI-aware, so it would have cut a styled cell mid-escape. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ts had written out identically Every store-backed backup target implemented Push, List, Fetch, FetchFile, Verify, Remove, PutObject, ObjectKeys and GetObject the same way: open the store, call the function in blob that knows what a backup is. Three copies of twenty-seven methods, identical right up until one of them would not have been. What actually differs is how the store is opened, so that is what an adapter supplies now. Two openers rather than one, because the halves differ for a reason that is not reading versus writing: S3 probes the bucket for the backup half and deliberately skips the probe for the object half, where a missing prefix is the ordinary state before the first publish. Embedded rather than called, so the three methods with a reason to wrap a call keep it: localdir refuses to push a directory onto itself before its root is created, localdir and sftp each tidy an emptied directory afterwards. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Scripted is a Runner whose replies are written in advance, and Collector is a Sink that records what an operation announced. Neither is reachable from any main; both sat in internal/infra/exec and internal/events, where they read as production code. Collector's own comment claimed the JSON presenter as a user, which stopped being true without the comment noticing. They now live in test/fakes with the other doubles. Nothing shipped changes -- the linker had already dropped both -- but the packages that hold the real Runner and the real Bus no longer also hold a fake one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Five options and two helpers were exported API that no production path reached. An option only a test passes is an option no test exercises as production leaves it, which is the shape a defect hides in. Unexported behind an export_test.go, where the test lives in the same directory: sopsage's sops path and clock, systemd's systemctl path, the health waiter's clock. WithUnitDir stays exported -- the suites in test/ install real units into a temporary directory and cannot reach an export_test.go -- and so do the https, oci and imagepack seams, for the same reason. Deleted outright: compose.WithDockerBinary, whose two callers wanted the default anyway, and Registry.Available, which no caller ever wanted -- every one of them wanted the error Lookup returns. atomicfs.ReadFileIn goes too, and its test moves to WriteFileIn. That test was the only place the root-containment refusal was asserted, and it was asserting it through a function nothing calls, while the function that renders a secret into a release directory had no such test. Same rule, now guarded where it runs. events.StepOutput was a constructor only tests called, while the one place that publishes the event built it as a struct literal. The literal now calls the constructor, so the shape the tests assert on is the shape production emits. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
docs-check caught the rename: the filesystem-containment table pointed at TestReadFileInReadsAndRefuses, which went with the function it was testing. The claim is the same one, asserted through WriteFileIn. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
🤖 CodeAnt AI — Review Status
|
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
Bot
added
the
size:XL
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change centralizes blob target operations, narrows production APIs to test-only seams, migrates tests to shared fakes, replaces filesystem and lock implementations, and simplifies CLI, event, logging, and TTY helpers. ChangesAdapter and infrastructure refactor
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to This refactor replaces two dependencies with standard-library implementations and consolidates backup delegation without a demonstrated behavior change; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description is detailed and covers the required summary, type, design, lifecycle, compatibility, safety, verification, risks, and rollback. It also records known verification limits and explains why they are environmental.
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
The dependency this replaced checked the context before its first attempt, in tryCtx, and the replacement only checked it in the poll. With the lock held that is invisible -- the waiter reaches the select and is answered there, which is why the existing cancellation test passed either way. With the lock free the first attempt succeeds, so a command the operator had already interrupted took the deployment lock and went on to mutate the installation. The waiting path only, which is the parity that was lost. A non-waiting acquisition never consulted the context and still does not: one syscall that either succeeds or names the holder, with nothing to interrupt. The test leaves the lock free, which is the arrangement no existing test had. It fails on this branch before this commit and passes on main, which is what makes it a regression rather than a gap. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
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.
User description
Summary
A repo-wide audit for over-engineering, applied. Two dependencies go back to the
standard library, twenty-seven identical forwarding methods across three backup
transports become one delegate, and the exported seams that no production path
ever reached are closed. No behaviour changes.
Closes #
RFC: none — no design decision changes. RFC 0005 names
renameio/v2in a dated"what is present" snapshot as one of three reasons the language version is
1.25.0;
x/termandx/sysstill require it, so the conclusion holds. Thatrecord is left as written rather than edited after the fact.
Type
What changed
gofrs/flock→syscall.Flockrenameio/v2→os.CreateTemp+ renameblob.DelegateScripted,Collector→test/fakesexport_test.gocompose.WithDockerBinary,Registry.Available,atomicfs.ReadFileIndeletedslices.Sorted,slog.DiscardHandler,ui.Truncate−280 lines from
internal/+cmd/, −2 direct dependencies.Two of these are worth a reviewer's eye rather than a skim:
atomicfs.ReadFileInwas the only thing asserting root containment. Itstest was the sole place
cleanRel's escape refusal was checked — through afunction nothing outside that test called — while
WriteFileIn, which renders asecret into a release directory, had no such test. The test now drives
WriteFileIn. SabotagedcleanRelto confirm it fails, and it fails namingErrPathEscaperather than a syscall error, which is the distinctioncleanRelexists to make.
events.StepOutputwas a constructor only tests called whileroot.gobuiltthe same event as a struct literal. The literal now calls the constructor, so
the shape the tests assert on is the shape production emits.
Design
Lifecycle invariants
No steps added or changed; the plan, journal and recovery paths are untouched.
--dry-runplan output — n/a, no new stepsCompatibility
schemas/) unchangedSecrets & safety
Both rewritten primitives are on secret-handling paths, so specifically:
WriteFilestill chmods before the rename (os.CreateTempopens at 0600, so thewindow is narrower than the final mode until this widens it) and now fsyncs
before the rename rather than after — a rename landing ahead of its contents
would leave the old name pointing at zeros. The lock releases with an explicit
LOCK_UNbefore the close, so a forked descriptor cannot outlive the release.Verification
just cigreenjust demo,just demo-plan,just demo-recoverystill passWhat I ran and what it proved:
Every gate in the
cirecipe, run individually:fmt-check,vet,darwin-check(amd64 + arm64 build and vet),lint(golangci-lint, 0 issues),shellcheck,runtime-check(17 known mentions, 0 runtime branches),docs-check(41 pages, 56 checks, no drift),log-check, the strict contractrun (no suite skipped),
test-race, andcoverage-gate— 87.0% against an 84%floor.
go vet -tags docker ./...too, since a deletion that only breaks atagged file passes an untagged gate. The acceptance scenario passes against real
Docker, which exercises the rewritten
atomicfs.WriteFileand the lock on everystep; it does not exercise the backup-target delegate.
The container lane does not pass on my machine, and does not pass on
maineither. Nine failures, all 120s timeouts, two environmental causes:
test/installerneeds a daemon sharing the host network namespace and says soitself; the five minisign suites
apk add minisigninsidealpine:3.20, andthis host's containers cannot reach the Alpine CDN (
Permission deniedfetchingthe index, reproduced with a bare
docker run).apkfails,&&short-circuits, and
require.NoErrorthen prints "the real minisign rejected oursignature" for what is a failed package install. I confirmed by running two of
them on
main: identical failures, identical timings. So the S3, SFTP andvolume-capture suites that would exercise the new delegate against real
containers are unverified locally — CI is the first place they run.
Risk & rollback
The two rewritten primitives are the ones a bad merge would hurt most: every
atomic write and the lock that stops two concurrent deployments. A defect in
WriteFilecorrupts state files on a crash rather than immediately, and adefect in the lock allows two mutating operations at once. Both are covered by
existing unit tests and the acceptance run, but neither failure mode is one a
green suite proves absent.
Rollback is
git revertof the five commits andgo mod tidy; nothing on diskchanges format, so an operator on the old binary reads everything this one wrote.
CodeAnt-AI Description
Reduce runtime dependencies while preserving backup, locking, and file-safety behavior
What Changed
Impact
✅ Fewer production dependencies✅ Cancellable deployment lock waits✅ Safer atomic writes and root-contained file writes💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by cubic
Refactors the repo to cut two dependencies and ~280 lines:
gofrs/flockandrenameio/v2go back to the standard library, three backup transports share one delegate, and test-only seams are closed. It also fixes a regression the lock rewrite introduced — with a free lock, a cancelled--waitacquisition took the lock and went on to mutate the installation.Attention areas
LOCK_UNbefore close; both sit on secret-handling paths.ReadFileIntoWriteFileIn, which is what production actually calls.main, so S3, SFTP, and volume-capture suites are unverified until CI runs them.Written for commit bb2927a. Summary will update on new commits.