refactor: decompose main.go into internal/ packages (RM-9) - #26
Merged
Conversation
Move the tiny TOML lexer shared by the registry parser and the lockfile parser into its own leaf package: stripComment, parseQuoted, parseBool, parseStringArray, tomlQuote, tomlArray. Both parsers reused these helpers, so leaving them in the registry package would have forced internal/lockfile to import internal/registry solely for two lexer functions, permanently coupling the lock format's parser to the registry package's exported surface. Pure move: function bodies are byte-identical, only the package clause, imports, and exported casing differ. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Move atomicWriteFile and recoverFromBackup into a leaf package. validateRegistryBackup deliberately does NOT come along: its body is a parseRegistryTOML call, so moving it here would make atomicio import the registry package while registry already imports atomicio for its own writes — an import cycle. recoverFromBackup already takes a validate func(string) error callback precisely so the format-specific validation policy stays with the caller; that split is now enforced by the package boundary. Pure move: function bodies are byte-identical, only the package clause, imports, and exported casing differ. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…wrapper in main
Split withMutationLock along the line that RM-8's "exit-code policy lives in
exactly one place" invariant actually draws:
internal/mutationlock Acquire/PathFor/readHolder + Wait/retryInterval.
Knows nothing about signals or exit codes.
main withMutationLock — signal.Notify, the interrupt
goroutine, and osExit(exitInterrupted) — unchanged.
This preserves the invariant literally rather than by plumbing an
onInterrupt callback through the lock package, and needs no signature
change to the wrapper.
Tests split the same way: the nine lock-primitive tests move to the new
package; TestMutationLockWithReleasesOnError stays in main because it tests
the wrapper. TestMutationLockPath stays in main for now since it asserts the
mutation-lock path differs from the digest-lock path and lockPathForRegistry
has not moved yet.
Pure move: function bodies are byte-identical, only the package clause,
imports, and exported casing differ.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
current *Tool pointed at a block-local Tool, not at the map entry: the section branch did reg.Tools[name] = t (a copy) and then current = &t, so *current was never the map's value. Every field-assignment branch already re-read t := reg.Tools[current.Name] from the map and wrote back via reg.Tools[current.Name] = t, and the only field ever read through current was .Name — which no TOML key can change. The trailing *current = t was therefore unobservable. Replace the pointer with currentName string. "" is a safe "no section yet" marker because validToolName rejects the empty name, so it cannot collide with a real section. This is the single sanctioned non-mechanical change in RM-9, landed as its own commit so its diff is reviewable as a logic fix rather than buried in a package move. No behavior change; the registry parser tests are unmodified and still pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…(4a) Move the Tool/Registry types, the TOML-subset parser, the built-in default registry, tool-name validation and listTools into internal/registry. parseVolumeBinding comes here rather than to pathmap (where the original design brief had put it): parseRegistryTOML calls it to validate stateful volume specs, so leaving it above registry would have made the parser depend on a higher layer. Exported: Tool, Registry, DefaultTOML, Default, DefaultToolSections, ParseTOML, ValidToolName, ReservedToolName, ParseVolumeBinding, ListTools. validEnvAssignment stays unexported — its only caller is the parser. Registry-profile tests move with the code, including goprofile_test.go's seven profile-shape assertions; goprofile_test.go keeps only its two Windows path-mapping tests. The moved profile tests carry a local containsString helper rather than inverting the layering for six lines. Pure move: function bodies are byte-identical, only the package clause, imports, and exported casing differ. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…nal/registry (4b) Move registryPath, ensureRegistryFile, appendMissingDefaultTools, loadRegistry, validateRegistryBackup, rewriteRegistryWithoutTools, installShims, copyFile and removeShim. Two consequences worth calling out: - appendMissingDefaultTools gains a version parameter. It stamps "# Added by container-bin <version>" into the upgraded registry, and version is the -ldflags "-X main.version=..." target, which must stay in package main under that exact symbol path or every future release build silently reports "dev". Threading it as a parameter matches the existing buildSelfTestReport(cbVersion string, ...) precedent. - validateRegistryBackup stays here as unexported validateBackup rather than travelling to atomicio, because its body is a ParseTOML call. copyFile stays unexported (sole caller: InstallShims). exitcode_test.go's buildTestCb gets its own copyTestFile fallback instead, rather than widening the registry package's exported surface for one test consumer. Pure move: function bodies are byte-identical apart from the version parameter noted above. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Move Windows path classification, argv path mapping, project-root discovery and deterministic volume naming into internal/pathmap: CanonicalPath, MapToolArgs, NormalizeToolArgs, PathMount, FindProjectRoot, ProjectMarkersFor, WorkspaceRootFor, StatefulWorkspaceDestination, VolumeHash, StatefulProjectVolumeID, StatefulSharedVolumeID, PythonEnvID, plus the unexported classifiers (isWindowsAbsPath, pathWithin, externalMountRoot, resolveWindowsPathArgMode, pathMapper) that only MapToolArgs uses. mountSpec deliberately stays behind for now; it moves to internal/dockerrun with the rest of the docker run argument assembly. resolveWindowsPathArg travels as-is, doc comment intact, even though it is dead code whose comment documents resolveWindowsPathArgMode instead — see the findings section of the handoff report. All three path test files move wholesale, plus goprofile_test.go's two Windows path-mapping tests (its seven profile-shape tests went to internal/registry in 4a). Verified additionally with GOOS=windows go vet ./... so the Windows-gated test bodies type-check; those tests still cannot RUN until Windows CI. Pure move: function bodies are byte-identical, only the package clause, imports, and exported casing differ. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Move LockFile/LockEntry, the lockfile parser and renderer, digest resolution and RuntimeImageForTool into internal/lockfile. Exported: LockFile, LockEntry, PathFor, Load, LoadForRegistry, Write, ConfiguredImages, ResolveImage, RuntimeImageForTool. entryID, render, imageRepository, canonicalRepository and matchRepoDigest stay unexported — their only callers are inside this package. The package depends on internal/toml for StripComment/ParseQuoted/Quote, which the lockfile parser shares with the registry parser, and on internal/registry for Tool/Registry and Path(). recover_test.go is gone: its four atomicio tests left in step 2, its registry-backup test in 4b, and its three lockfile tests land here. TestMutationLockPath becomes an external test (package mutationlock_test) beside internal/mutationlock, now that it can import both packages without mutationlock depending on lockfile. Pure move: function bodies are byte-identical, only the package clause, imports, and exported casing differ. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Move every `docker volume` shell-out into a leaf package: Names, ExistsSet, Remove, RemoveQuiet, Managed, LabeledManaged, Labels, EnsureManaged. This package is what makes the layer above it acyclic. runTool must create labelled volumes (EnsureManaged), while cb state / cb gc / cb doctor must list, inspect and remove them. Leaving those primitives in the state or diagnostic layer would have put them above dockerrun, which needs them — dockerrun -> state -> dockerrun. As a leaf with no internal dependencies, all three callers can reach it independently. It carries no tests because it had none: every function here is a thin exec.Command wrapper, matching this project's established pattern of not unit-testing bare I/O call sites (see the coverage note on runSelfTestChecks). Pure move: function bodies are byte-identical, only the package clause, imports, and exported casing differ. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Move RunTool, MountSpec, EnsureImageLocalForTool and the unexported interactiveTerminal/selectedHostEnv helpers into internal/dockerrun. mountSpec lands here rather than in pathmap. My phase-1 review argued for pathmap (it is a pure string function, its second caller is cb expose, and mountspec_test.go pairs it with WorkspaceRootFor); the finalized plan chose dockerrun, to keep docker-argument construction in one package. Following the plan: both placements are cycle-free, the preference was mild, and mountspec_test.go imports pathmap cleanly from here since dockerrun already depends on it. Pure move: function bodies are byte-identical, only the package clause, imports, and exported casing differ. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Move `cb state` and `cb gc` — Volume (was stateVolume), currentProjectState, Show, GC — into internal/state. It sits above dockervol (which owns the docker volume primitives) and alongside dockerrun rather than inside diag, even though cb doctor also reads volume state: doctor's need is LabeledManaged, which is a dockervol call, not a state call. diag therefore depends on dockervol directly and does not depend on state at all. currentProjectState stays unexported; its only caller is Show. Pure move: function bodies are byte-identical, only the package clause, imports, and exported casing differ. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Move cb doctor, cb self-test and cb bugreport into internal/diag, together with the pure verdict functions (dockerOSTypeVerdict, shimDirACLVerdict, reparsePointVerdict, networkStorageVerdict), captureStdout, redactSecrets, windowsDriveType and windowsHostVersionInfo. Exported surface is just the three command entry points plus the flag parser main's dispatch needs: Doctor, Bugreport, SelfTest, ParseSelfTestArgs. Everything else — including the whole self-test report model and its versioned JSON schema — stays unexported inside the package, so the schema's field tags are untouched. Bugreport and SelfTest take version as a parameter; SelfTest threads it through runSelfTestChecksAndCleanup and runSelfTestChecks to buildSelfTestReport, which already took a cbVersion argument. diag depends on lockfile and dockervol — two edges the original design table omitted (doctor calls LoadForRegistry/ConfiguredImages and LabeledManaged). It does not depend on state. Pure move apart from the version parameters noted above. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…steps 11-12) Move the composite subcommands into internal/cli: Setup, Install, Trace, Env, Expose, Unexpose, Uninstall, Inspect, Backup, Restore, Lock, Update. These are the commands the original design brief had placed in internal/registry, which was the source of three of the import cycles found in review: Inspect needs lockfile, Expose needs dockerrun and pathmap, and Setup ends by calling doctor. They are composites that sit above every other package, so they get their own layer directly beneath main. Install is newly extracted from main's dispatch switch, where it was an inline closure. The closure assigned to main's outer reg/err; neither is read after the switch (main returns immediately), so hoisting them to locals inside cli.Install is behaviour-identical. Backup and Setup/Install take version as a parameter, for the same -ldflags reason as 4b. main.go is now 244 lines: argv[0] dispatch, the subcommand switch, version, usage, invokedName, the exit-code constants, osExit/fatalf, and withMutationLock's signal wrapper. Nothing else. Pure move apart from the version parameters and the Install extraction noted above. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
docs/shell-contract.md cited ~25 main.go:NNNN line numbers that the decomposition invalidated. Each now names the file and function that actually holds the code (internal/dockerrun/dockerrun.go RunTool, internal/pathmap/pathmap.go NormalizeToolArgs/CanonicalPath, internal/diag/diag.go captureStdout); the citations that still say main.go do so because invokedName, withMutationLock, the exit constants and main() itself are genuinely still there. Two prose claims were re-verified against the split tree rather than carried over: signal.Notify still has exactly one call site (main.go), and SysProcAttr still has none anywhere. docs/architecture.md's "Known structural debt" section, which described main.go as "a single ~2,700-line file", is replaced by a Package layout section giving the real dependency graph plus the two boundaries that are load-bearing (mutationlock's ignorance of exit codes, dockervol's leaf-ness) and the reason version stays in package main. README's roadmap no longer lists the decomposition as pending. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…t diagram accuracy Devin Review found two real, low-risk documentation defects on PR #26: - internal/pathmap/goprofile_test.go: TestGoProfilesDeclareNoForcedPathSemantics moved to internal/registry/registry_test.go during the decomposition, but its explanatory comment was left behind and now sits above (and misleadingly documents) TestGofmtPathMappingWindows instead, which asserts something different. Removed the orphaned copy; the correctly-attached comment already exists at registry_test.go:361. - docs/architecture.md's Package layout diagram implied a linear chain (dockervol/lockfile -> pathmap -> registry) that does not match the real import edges: lockfile depends on registry/atomicio/toml, never pathmap; pathmap depends only on registry; dockervol is a true leaf with no internal dependencies at all, not something pathmap/lockfile sit 'above'; and mutationlock is reached only from main, unrelated to that chain. Replaced the implied-linear framing with an explicit note that the tiers are grouped by rough depth (not per-edge claims) and added the exact edge list from github.com/AviBackToBlack/container-bin, independently reproduced three times this session (implementer, SWE-1.7 Max reviewer, orchestrator) and now verified a fourth time against this specific diagram before the fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
58 tasks
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.
What
Decomposes
main.go(3,876 lines) into 11internal/packages plus a 249-line thinmain:atomicio,toml,mutationlock(leaves) →registry→pathmap→lockfile→dockervol(leaf) →
dockerrun/state→diag→cli→main. Strict dependency-acyclic-graph,independently reproduced via
go listby two different reviewers (not just asserted).All 130 test functions preserved, redistributed to the package whose behavior they test. One
sanctioned non-mechanical change, isolated to its own commit: the dead
current = &taliasing inparseRegistryTOML(current *Toolpointed at a block-local copy, never the map entry; everyreal mutation already re-read/wrote through the map directly, making the pointer provably
decorative) is replaced with
currentName string. Everything else is a pure move — same logic,same error text, same test assertions, just a different package.
14 commits, each independently green (
gofmt/go vet/go test -raceafter every step, plus aWindows cross-compile +
go vetcheckpoint after the path-mapper extraction specifically, sincemost of that package's tests are Windows-gated and skip on Linux CI).
Why
Tracked as RM-9 in issue #2 since the open-source bootstrap: a single ~3,800-line file was
coherent but its size was already flagged as structural debt, and several other roadmap items
(RM-6b, test-file consolidation) were explicitly deferred until this landed. Package boundaries
make dependency direction explicit and enforceable by the compiler rather than by convention.
Process
This is the one task in the roadmap series run through a deliberately different pipeline than
every other PR here, agreed with the operator up front because of its risk shape — one large,
interconnected, mechanical-but-error-prone diff rather than a small scoped change:
package-boundary proposal against the real code before anything moved, and found it had five
genuine import cycles — commands like
expose/inspectwere assigned to the same package asthe
Registrytype they mutate, but those commands needpathmap/lockfile/dockerrun,which would need to depend back on
registry. It also caught thatversion(injected via-ldflags "-X main.version=…"in both CI and the release workflow) was read by four functionsthe original plan silently moved out of
main— every one would have shipped reporting"dev"forever. Both were fixed in the plan before implementation started.
after every step — three real mid-step errors were caught and fixed before being committed,
never landing in a red commit.
repo access, reproduced the dependency graph itself, spot-checked functions and tests the
implementer's own report hadn't emphasized, and ran a full user-facing-string diff across every
non-test file (old vs. new) to rule out a changed error message anywhere in the diff. GLM-5.2,
kept deliberately blind (an isolated scratch environment with no repo access, working only from
pasted diffs and the other reviewer's report), was told explicitly not to just re-approve that
verdict — to interrogate whether the report's own evidence actually backed its claims. It found
two genuinely unverifiable-from-the-paste gaps (whether an empty TOML section name could reach
the new aliasing-fix code path; whether the mutation-lock split had silently added the
sync.Once/token-check sophistication rather than preserving it) — both closed cleanly oncechecked against the real source, but they were real gaps in what a blind reviewer could confirm
on its own, exactly the class of thing that pipeline shape exists to surface.
relying on any single report — same discipline as every other PR in this series.
No blockers surfaced at any stage. Full records:
.handoff/RM-9/{design-brief,impl-prompt,r1-handoff,swe-max-review-r1}.md.🤖 Generated with Claude Code