mirror of
https://github.com/shankar0123/certctl.git
synced 2026-06-07 12:21:31 +00:00
shankar0123
3f1344e806
Phase 9 Sprint 8: shape change from the config.go cuts.
cmd/server/main.go is the second-largest hotspot (2966 LOC at audit
time, 2351 LOC pre-this-commit). The Phase 9 prompt asks for THREE
files: main.go (entrypoint) + wire.go (DI assembly) + migrations.go
(boot-time migration handling). This sprint ships TWO of those three;
migrations.go is deferred with explicit rationale. Decision logged
inline in wire.go's doc-comment + tasks-deferred row in the audit doc.
What moved
==========
cmd/server/wire.go (new, 758 lines incl. BSL header + Phase 9
doc-comment + imports + 12 declarations)
Seven preflight + DI helper functions extracted from the bottom of
main.go (lines 2353-2966 pre-edit):
- preflightSCEPChallengePassword (H-2 fix: SCEP needs non-empty
shared secret)
- preflightSCEPMTLSTrustBundle (SCEP Phase 6.5: mTLS CA bundle)
- preflightESTMTLSClientCATrustBundle (EST Phase 2.5: SIGHUP-reloadable
*trustanchor.Holder)
- preflightSCEPIntuneTrustAnchor (SCEP Phase 8.2: Intune Connector
signing-cert bundle)
- loadSCEPRAPair (post-preflight RA cert+key load)
- preflightSCEPRACertKey (RA pair validation: mode 0600,
cert/key match, NotAfter, RSA-
or-ECDSA alg)
- preflightEnrollmentIssuer (L-005: EST/SCEP issuer can
serve GetCACertPEM)
- buildFinalHandler (M-001 option D: HTTP dispatch
wrapper routing auth vs no-auth
chains by URL prefix)
Five adapter types bridging package boundaries to avoid import cycles:
- authPermissionCheckerAdapter (typed-string Authorizer →
plain-string PermissionChecker)
- authCheckResolverAdapter (postgres ActorRoleRepository →
handler.AuthCheckResolver)
- sessionMinterAdapter (session.Service → OIDC
SessionMinter port)
- breakglassSessionMinterAdapter (session.Service → breakglass
SessionMinter + HIGH-1 revoke-all)
- oidcProvidersListAdapter (postgres OIDCProviderRepository
→ handler.OIDCProvidersListResolver
with MED-9 enabled-filter)
Plus the silenceUnusedImports var-block (`_ = oidcdomain.OIDCProvider{}`)
that pins the oidcdomain import as load-bearing.
Why this shape rather than the full 3-file split
=================================================
The Phase 9 prompt names migrations.go as the third file. The
migration code in main.go is INLINE inside the 2300-line main()
function — Phase 4's DEPL-M1 --migrate-only flag handling (lines
~59-77) + the RunMigrations + RunSeed + early-exit branch (lines
~199-264). It is NOT a standalone helper function ready to relocate.
Extracting it into migrations.go would require:
1. Creating a new runMigrations(ctx, cfg, db, logger) error
function that consolidates the inline blocks.
2. Replacing the inline code in main() with a single call site.
3. Reshaping the os.Exit(0) early-exit semantics (used at line 247
when --migrate-only is set) into a return-and-exit-from-main
pattern.
That's BEHAVIOR-CHANGE territory — a new function call frame, a
new defer scope, error-handling pattern shift. Different shape of
risk from the pure-data type relocations Sprints 1-7 did. The
Phase 9 prompt explicitly says:
"Do NOT change exported type signatures during the split. The
refactor is mechanical relocation; behavior change is a separate
concern."
Creating runMigrations() doesn't change exported signatures (it'd
be unexported), but the SPIRIT of the rule — "no behavior change" —
is what extracting a chunk of inline code from main() into a new
function pushes against (defer ordering, panic recovery, stack
shape).
Deferring with explicit rationale to a follow-up that the operator
can review specifically for the new function-extraction risk.
Estimated impact: another ~80-120 LOC out of main.go into a new
migrations.go file. Recommended path: smaller standalone PR with
its own review focus on the runMigrations function shape +
early-exit semantics + unit tests for the new function via the
existing main_test.go fixture.
Imports rebalanced after the move
==================================
The build surfaced 5 unused imports in main.go after the cut.
Removed:
- "crypto" (used only by loadSCEPRAPair return type)
- "crypto/tls" (used only by preflight* X509KeyPair)
- oidcdomain (used only by silenceUnusedImports;
moved along with the var-block)
- userdomain (used only by sessionMinterAdapter)
- "github.com/certctl-io/certctl/internal/repository"
(used only by adapters'
EffectivePermission + OIDCProviderRepository)
All five now live in wire.go's import block. Same `crypto/x509` +
`encoding/pem` + `net/http` + `strings` + `time` imports that
wire.go needs are STILL needed by other code in main.go, so they
stay in both.
Public-surface invariant
========================
All moved declarations are in package `main` (unexported by Go
rules — package main cannot expose to importers). No exported
surface changes. Reorganization is invisible outside cmd/server/.
Same-package callers in main.go (preflight* invocations, adapter
instantiation) resolve via the package symbol table.
Verification (all clean):
go build ./cmd/server/... → clean
gofmt -l cmd/server/ → clean (after -w)
staticcheck ./cmd/server/... → clean
go test ./cmd/server/... -count=1 -short → ok (0.39s; existing
main_test.go +
preflight_*_test.go +
finalhandler_test.go
+ auth_*_test.go +
tls_test.go all pass)
grep -nE '^func (preflightSCEP|preflightEST|loadSCEP|preflightEnroll|buildFinalHandler)|^type (authPermissionCheckerAdapter|authCheckResolverAdapter|sessionMinterAdapter|breakglassSessionMinterAdapter|oidcProvidersListAdapter)'
cmd/server/main.go → empty (none remain in main.go)
cmd/server/wire.go → 8 funcs + 5 types (correct)
LOC delta:
main.go: 2966 → 2347 (-619 lines: -614 from moved declarations,
-5 from removed unused imports)
wire.go: new, 758 lines (incl. 152-line Phase 9 doc-comment +
BSL header + package decl + 16-line
import block)
main.go is now under 2400 LOC for the first time post-audit
(audit baseline was 2966).
Cumulative Phase 9 progress (all 8 sprints):
config.go: 3403 → 1342 LOC (-2,061, -60.6%) across 7 sprints
cmd/server/main.go: 2966 → 2347 LOC (-619, -20.9%) this sprint
Pattern lesson — behavior-change boundary
==========================================
Sprints 1-7 (config.go cuts) were purely mechanical relocation —
data type definitions moved between sibling files in the same
package. Zero risk of changing runtime semantics; the
broader-importer build was the only verification needed.
Sprint 8 first encountered the boundary where mechanical relocation
ends. The helpers + adapter types in this sprint are still
pure-mechanical (no function-call-frame change), so the bound was
respected. The migrations.go extraction would cross the bound,
which is why it's deferred to a dedicated review.
Future sprints touching main() (Sprint 9-12 for the non-config
hotspots) will face the same boundary question. The right pattern
is the one this sprint demonstrated: ship the safe mechanical
relocation now, defer the behavior-shift extraction with explicit
rationale for the operator to review when they have time.
Next queued (Sprint 9): internal/service/acme.go (1965 LOC) split
into a subpackage internal/service/acme/{orders,authz,challenges,
nonces,gc}.go. The current acme.go is a single-file service with
related but separable concerns; the split shape here will be a NEW
SUBPACKAGE rather than a sibling file, which is a third pattern
(after type-family-in-sibling-file from config.go and
helper-functions-in-sibling-file from this sprint). Will be the
trickiest cut of Phase 9 because the import path changes from
`service` (consumers do `service.ACMEService`) to `service/acme`
(consumers would do `acme.Service`). Detailed planning + external-
caller audit needed before any code moves.
Closes: cowork/certctl-architecture-diligence-audit.html#fix-ARCH-M2
(partial — 8 of 12 — wire.go shipped; migrations.go deferred
with rationale)