Repository navigation
refactor(lore): decompose runOnboard and generateManifest, with characterization tests - #371
Merged
Merged
Conversation
Phase 1b-i of the issue #365 audit remediation, applying the #312 discipline: behavior-preserving extract-method into same-file unexported helpers, no public signature changes, no semantic changes, and the existing test suite passing unmodified. Both //nolint:gocognit,gocyclo directives are deleted rather than given reasons, per ruling 4. runOnboard read as six sequential concerns and now reads as its steps: read config, build the provider, get a synced registry, analyze, report, write. The helpers are parseLoreOnboardConfig (with a loreOnboardConfig carrier matching the file's existing loreDeployConfig convention), newOnboardProvider, syncedRegistry, reportOnboardResult, and writeOnboardManifest. The onboard.Options literal stays inline in runOnboard — it is a data assembly with no branching, and hiding it behind a method would add indirection for nothing. The unused args parameter becomes _. generateManifest is a document builder and now appends through writeProductHeader, writeComplexityWarning, and writeInstallCommands. Two mechanical simplifications ride along, both behavior-identical: the redundant len(slot.Annotations) > 0 guard is dropped, since ranging an empty slice is already a no-op, and the slot filter is inverted into a continue guard. Measured with gocyclo and gocognit: runOnboard 18/21 -> 4/3 generateManifest 16/26 -> 3/2 No helper exceeds a threshold; the largest is reportOnboardResult at 10/12. Neither file has any function over golangci's gocyclo 15 or gocognit 20. Bare nolint directives across the tree: 9 -> 7. parseLoreOnboardConfig deliberately does not carry the unused error return that parseLoreDeployConfig has. That return is dead because a manifest.Load failure is swallowed there — issue #368 — and copying the shape would copy the bug. Helpers are placed immediately after runOnboard, following this file's existing organization: each command's helpers sit with the command they serve, as the deploy group already does. Verified: make vet, make build, and the full make test suite green — zero FAIL or panic lines by count. gofmt clean. No test was modified. Plan: docs/plans/audit-remediation.md (phase 1b-i, flipped to completed here). Issue: #365
David-Noble-at-work
changed the title
David-Noble-at-work
added a commit
that referenced
this pull request
Aug 12, 2026
PR #371 merged at d75cb86, its first commit. The test commit f06cbc9 was pushed to the branch but GitHub's record of the pull request head never advanced past d75cb86, so the squash merge took the decomposition without its tests. f06cbc9 carries zero check runs — CI never executed these tests, and the five green checks reported on #371 all belong to the pre-test commit. Nothing was lost: f06cbc9 remained intact on GitHub and these three files are restored from it verbatim, byte for byte, rather than rewritten. That leaves develop currently holding the runOnboard and generateManifest decomposition with no test covering either. This commit closes that gap. It is also the first time these tests run on Linux and Windows; they assert exact byte sequences including newlines, which has only been exercised on macOS. The tests themselves are unchanged from the reviewed commit. Expectations were hand-derived from the pre-decomposition implementations rather than captured from the refactored code, and both suites were mutation-checked: a one-space change to the annotation indent and a "." -> "./" change to the output directory default each failed exactly the tests that should have failed. Coverage restored: cmd/lore/lore/onboard 0% -> 25.8%, cmd/lore/lore 12% -> 19.8%. Per-helper blocks: parseLoreOnboardConfig 3/3, newOnboardProvider 5/5, reportOnboardResult 13/13, writeOnboardManifest 3/3. syncedRegistry (0/6) and runOnboard itself (0/7) stay uncovered — both are bound to the registry and the network. Verified: make test green, zero FAIL or panic lines by count; gofmt clean. Plan: docs/plans/audit-remediation.md (phase 1b-i). Issue: #365 Co-authored-by: David Noble <[email protected]>
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.
Phase 1b-i of the issue #365 audit remediation: decompose
runOnboardandgenerateManifest, with characterization tests that land in the same PR.Why the tests are here
The first version of this PR offered "no test was modified" as behavior-preservation evidence. That was worthless —
cmd/lore/lore/onboardhad 0% coverage and both target functions measured zero covered blocks, so there were no tests to modify and a greenmake testsaid nothing about them.The tests now in this PR are written against the pre-decomposition behavior: every expected value was hand-derived by reading the original implementations, not captured from the refactored code. A decomposition that changed behavior fails them rather than being absorbed by them.
Both suites are mutation-checked:
" # "→" # ""."→"./"TestParseLoreOnboardConfig_DefaultsOutputDirToWorkingDirectoryBoth mutations were reverted and the tree re-verified.
Complexity result
runOnboardgenerateManifestBoth
//nolint:gocognit,gocyclodirectives are deleted, not given reasons, per ruling 4. Bare directives across the tree: 9 → 7. No helper exceeds a threshold — the largest isreportOnboardResultat 10/12.Coverage
cmd/lore/lore/onboardcmd/lore/lorePer-helper blocks:
parseLoreOnboardConfig3/3,newOnboardProvider5/5,reportOnboardResult13/13,writeOnboardManifest3/3.syncedRegistry(0/6) andrunOnboarditself (0/7) remain uncovered — both are bound to the registry and the network, and neither was testable before this change either. The extraction is what made the other four reachable.Two judgment calls
onboard.Optionsliteral stays inline inrunOnboard. It is a data assembly with no branching; hiding it behind a method would add indirection and reduce complexity by zero.parseLoreOnboardConfighas no error return, deliberately breaking symmetry with this file'sparseLoreDeployConfig. That function's error return is dead because amanifest.Loadfailure is swallowed — issue lore deploy silently skips an unloadable manifest and exits 0 #368 — and copying the shape would copy the bug.Verified:
make vet,make build, fullmake testgreen (zero FAIL/panic by count),gofmtclean.