Skip to content

chore: remove dead code and enable the unused linter - #51

Merged
vishr merged 2 commits into
mainfrom
chore/remove-dead-code
Aug 17, 2026
Merged

chore: remove dead code and enable the unused linter#51
vishr merged 2 commits into
mainfrom
chore/remove-dead-code

Conversation

@vishr

@vishr vishr commented Aug 17, 2026

Copy link
Copy Markdown
Member

Removes every callerless symbol found by the audit in #50 (items 7 and 8), and turns on the linter that would have caught them, so the next one fails the build it arrives in.

868 deletions, 38 insertions.

Removed

internal/engine/protection_units.go and its test, 527 lines. ProtectionUnitTarget had no implementation outside the in-memory test double, and nothing in cmd/ or internal/ reached GenerateProtectionUnits, ReconcileProtectionUnits, or InspectProtectionUnits. Live systemd unit management stays with internal/engine/schedule.go, which is reachable and covered.

Worth recording before it leaves the tree: the two files encoded different ownership rules for the same host resource, and the deleted one had the stronger scheme — X-Onebox-* unit metadata plus an explicit refusal to overwrite a foreign-owned unit, against schedule.go matching on filename prefix alone. That difference is #50's item 5, not this change's; the code is recoverable from history if that work resumes.

Eleven exported symbols with zero references anywhere in internal/, cmd/, or e2e/: release.Activate (superseded by the manifest activation path), app.Spec.ComposeRefsOf, app.Rendered.Runnable, app.SortedRouteKeys, app.Names.ProtectionTimer (the ForEnvironment variant is the live one), transport.NewSSH (superseded by NewSSHContext), onebox.LoadMigrationBackupOverride, onebox.SaveScheduledOperationEnvelope, onebox.ScheduledArtifactsAsProtectionResources, onebox.ActiveVolumeRecord.Selection, and InspectProtectionUnits with the file above.

The thirteen unexported symbols unused reported, including the status-output sanitisation layer in internal/onebox/status.go.

Enforcement

.golangci.yml held unused back with a note saying the findings were staged work and that deciding their fate "is not this gate's call". That call is now made, so the note and the exemption both go.

The config change was verified rather than assumed: adding func zzDeadProbe() int { return 1 } to internal/engine/plan.go and running golangci-lint run ./internal/engine/... reports plan.go:529:6: func zzDeadProbe is unused (unused).

unused does not see exported identifiers, which is the gap that let all eleven accumulate. just dead-exports covers that half and reports checked 362 exported identifiers, 0 unreferenced. It is deliberately outside check and ci: an exported helper may legitimately land before its caller, and the match is on identifier name alone, so two types with a same-named method count as one. A report that under-counts is a good prompt to go and look and a bad rule to fail a build on.

Left in place on purpose

DeploymentProposal.Preconditions (internal/onebox/types.go:134) is still declared. proposalPreconditions was the only thing that could populate it and it was dead, so the field has always serialised as its zero value — but it is part of the published proposal schema, which makes removing it a schema change rather than a cleanup.

statusIssueCodes, canonicalStatus and statusDigest survive because tests reference them. With sanitizeStatus and proposalPreconditions gone they have no production caller, so status.go is now a test-only island of 95 lines. unused counts them used and will not flag it.

What I ran

  • just check — fmt, vet, test, docs-generate-check, site-build: all checks passed
  • golangci-lint config verify and golangci-lint run ./... — 0 issues
  • go test -race -count=1 ./... — pass
  • just vuln — 0 vulnerabilities in reachable code
  • just dead-exports — 0 unreferenced

What I did not run

  • just e2e. The Docker end-to-end suite was not run locally; CI runs it as its own job.

Refs #50

🤖 Generated with Claude Code

vishr and others added 2 commits August 16, 2026 23:02
Removes every callerless symbol the audit in #50 identified, and turns the
`unused` linter on so the next one fails the build it arrives in.

`.golangci.yml` held `unused` back with a note saying the findings were
staged work and that deciding their fate "is not this gate's call". That
decision has now been made, so the note and the exemption both go.

Removed wholesale:

  - internal/engine/protection_units.go and its test. `ProtectionUnitTarget`
    had no implementation outside the in-memory test double, and nothing in
    cmd/ or internal/ called `GenerateProtectionUnits`,
    `ReconcileProtectionUnits`, or `InspectProtectionUnits`. Live systemd
    unit management goes through internal/engine/schedule.go, which is
    reachable and covered. Recoverable from history if the work resumes.

Removed individually — exported, zero references anywhere in internal/,
cmd/, or e2e/:

  - release.Activate, superseded by the manifest activation path
  - app.Spec.ComposeRefsOf, app.Rendered.Runnable, app.SortedRouteKeys
  - app.Names.ProtectionTimer, whose ForEnvironment variant is the live one
  - transport.NewSSH, superseded by NewSSHContext
  - onebox.LoadMigrationBackupOverride, onebox.SaveScheduledOperationEnvelope,
    onebox.ScheduledArtifactsAsProtectionResources,
    onebox.ActiveVolumeRecord.Selection

Plus the unexported set `unused` reported, including the status-output
sanitisation layer in internal/onebox/status.go. Note that
`DeploymentProposal.Preconditions` is still declared and still never
populated: the field is part of the published proposal schema, so removing
it is a schema change rather than a cleanup, and it is left alone here.

`unused` does not see exported identifiers, so `just dead-exports` covers
that half. It stays out of `check` and `ci` deliberately — an exported
helper may land before its caller, and a gate cannot tell that apart from
something left behind.

`just check` and `golangci-lint run ./...` are green; `go test -race ./...`
passes.

Refs #50

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two problems in the previous commit, both found on re-reading its own diff.

internal/compose/compose.go: goimports, run to drop the `regexp` import that
the deleted `ident` var was the last user of, also split `strings` into a
group of its own. The file's import block was already unusual — `strings` sat
in the third-party group — and the automated fix made it worse rather than
better. Grouped with the rest of the standard library, where it belongs.

Justfile: `dead-exports` matches on the identifier alone, so two types with a
same-named method count as one and the report can under-count. That is fine
for a recipe you read and act on, and it is exactly why this must not become
a gate — now said so in the comment rather than left for someone to discover.
The recipe also now fails loudly if the definition pattern matches nothing,
instead of reporting a clean zero when it has stopped working.

`just check`, `just lint`, `just vuln`, `just dead-exports` and
`go test -race ./...` all pass.

Refs #50

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@vishr
vishr merged commit f9ef6bc into main Aug 17, 2026
5 checks passed
@vishr
vishr deleted the chore/remove-dead-code branch August 17, 2026 06:36
vishr added a commit that referenced this pull request Aug 17, 2026
… as staged

Two items from a cleanup pass. The second is a correction, not a cleanup.

`nonGraduatingTestLifecycleCapability` defined a fake lifecycle driver in
service_lifecycle_records.go, beside the real ones, and only the tests used it.
A fake driver defined in production code is a fake driver compiled into the
shipped binary, so it moves to service_lifecycle_test.go. Verified as a pure
move: the code lines removed and added are identical. The helpers it uses —
lifecycleRecord, artifact, helperArtifact, seededDigest — stay where they are;
each is used seven to fourteen times by the real records.

The `_test_lifecycle` exemption in lifecycleCapabilityRecord.validate stays,
because it is what lets that fixture validate without being a real driver, and
validating is the thing the test exists to check. It now records why it is not
a hole: a record is only ever constructed in this repository, and validate has
exactly two callers — the fixture, and validateLifecycleCatalogue over the
fixed catalogue. An authored `driver` is a map key looked up by
lifecycleCapabilityFor and never builds a record, so it cannot arrive there
whatever it is named.

status.go was going to be deleted in this commit. It is not, and the earlier
recommendation to delete it was wrong.

Every caller in that file is a test, which is exactly the signal #51 acted on
when it removed protection_units.go — and #63 has since established that the
signal was misleading there. The same two reasons apply here. `statusIssueCodes`
is one half of a contract whose other half is the issue prose in
proxystatus.go, which states outright that every sentence leads with its
component *because* statusIssueCodes matches on that prose, and that leading
with the path instead collapsed every message to the generic
<component>_diverged. Delete one side and the other silently loses its reason
to stay stable. The rest was written against a structured status shape the
engine does not yet emit, which the lint config used to say in as many words
before #51 removed the note along with the exemption:

    Most are status-output sanitisation in internal/onebox/status.go — written
    against a status shape the engine does not yet emit [...] Deleting them is a
    judgement about whether that work is still coming, which is not this gate's
    call.

That judgement is still not made, so the file keeps a header saying what it is
and pointing at #63. The note it lost is now in the code it describes, where a
dead-code pass will actually encounter it.

Worth recording: the same #51 pass also removed sortedRecoveryKinds and
joinedRecoveryKinds from internal/app, which that note covered too. They are
recoverable at f9ef6bc^ if the status work resumes.

`just check`, `golangci-lint run ./...` and `go test -race ./...` pass.

Refs #63

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
vishr added a commit that referenced this pull request Aug 18, 2026
chore: remove dead code and enable the unused linter
vishr added a commit that referenced this pull request Aug 18, 2026
… as staged

Two items from a cleanup pass. The second is a correction, not a cleanup.

`nonGraduatingTestLifecycleCapability` defined a fake lifecycle driver in
service_lifecycle_records.go, beside the real ones, and only the tests used it.
A fake driver defined in production code is a fake driver compiled into the
shipped binary, so it moves to service_lifecycle_test.go. Verified as a pure
move: the code lines removed and added are identical. The helpers it uses —
lifecycleRecord, artifact, helperArtifact, seededDigest — stay where they are;
each is used seven to fourteen times by the real records.

The `_test_lifecycle` exemption in lifecycleCapabilityRecord.validate stays,
because it is what lets that fixture validate without being a real driver, and
validating is the thing the test exists to check. It now records why it is not
a hole: a record is only ever constructed in this repository, and validate has
exactly two callers — the fixture, and validateLifecycleCatalogue over the
fixed catalogue. An authored `driver` is a map key looked up by
lifecycleCapabilityFor and never builds a record, so it cannot arrive there
whatever it is named.

status.go was going to be deleted in this commit. It is not, and the earlier
recommendation to delete it was wrong.

Every caller in that file is a test, which is exactly the signal #51 acted on
when it removed protection_units.go — and #63 has since established that the
signal was misleading there. The same two reasons apply here. `statusIssueCodes`
is one half of a contract whose other half is the issue prose in
proxystatus.go, which states outright that every sentence leads with its
component *because* statusIssueCodes matches on that prose, and that leading
with the path instead collapsed every message to the generic
<component>_diverged. Delete one side and the other silently loses its reason
to stay stable. The rest was written against a structured status shape the
engine does not yet emit, which the lint config used to say in as many words
before #51 removed the note along with the exemption:

    Most are status-output sanitisation in internal/onebox/status.go — written
    against a status shape the engine does not yet emit [...] Deleting them is a
    judgement about whether that work is still coming, which is not this gate's
    call.

That judgement is still not made, so the file keeps a header saying what it is
and pointing at #63. The note it lost is now in the code it describes, where a
dead-code pass will actually encounter it.

Worth recording: the same #51 pass also removed sortedRecoveryKinds and
joinedRecoveryKinds from internal/app, which that note covered too. They are
recoverable at 74b4578^ if the status work resumes.

`just check`, `golangci-lint run ./...` and `go test -race ./...` pass.

Refs #63

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant