runLaunch holds the launch lifecycle in locals; the strict-audit abort path skips four cleanups including secret zeroing

[nhuelstng]

Duplicate check

• I have searched the existing issues

Context / Summary

Found during an architecture review of main (6257211). internal/cli/start.go is the highest-churn file in the repo (20 of the last 120 commits).

runLaunch is a 769-line single function that holds an entire process lifecycle in local variables. Because no object owns the acquired resources, teardown is written twice — and the two versions disagree. The code's own comment asserts they agree.

Refs #174 (the 123-line arming phase inside runLaunch), #173 (the facade-wiring phase). Both shrink this function; doing them first makes this extraction much smaller.

Problem / What

internal/cli/runLaunch — internal/cli/start.go:180-948:

• 769 lines, 17 unnamed phases (comment markers at :205, 224, 231, 247, 278, 316, 351, 592, 603, 673, 707, 729, 758, 775, 848, 896, 910)
• 26 return Exit* sites, 8 defers (:376, 637, 649, 668, 675, 700, 751, 770), 18 rebinds of one shared err
• ~65 locals still live at the sandbox.ExecWithReady call (:936)
• begins by re-unpacking opts into 9 shadow locals (:181-189), so both opts.noSandbox and noSandbox stay live for 750 lines
• internal/cli/runServe (serve.go:48-530, 483 lines) re-derives much of the same sequence

The concrete defect: the strict-audit abort skips four cleanups

fatalTeardown (start.go:900-908) ends in os.Exit, which skips every defer:

fatalTeardown = func(ferr error) {
	fmt.Fprintln(env.Stderr, prefix+": audit (strict) write failed, aborting:", ferr)
	if !keepRunning { sup.ShutdownAll(5 * time.Second) }
	_ = f.Close()
	closeControl()
	os.Exit(ExitIOError)
}

Diffed against the deferred stack, it omits:

skipped cleanup	defined at
allSecrets[i].Zero() — plaintext secrets are not wiped	start.go:376-380
os.RemoveAll(sandboxTmp) — temp dir leaked	:649
cacheScope.Close() — releases a flock (toolcache/cache.go:131)	:668
auditor.Close(), cancel()	:637, :700

The comment at start.go:606-607 states the opposite of what the code does:

"Under --audit-strict a failure to open/write is fatal: fatalTeardown runs the same cleanup the deferred teardowns do and exits non-zero."

That is exactly the drift an owning object prevents.

Related: the secret-zeroing discipline is already defeated

start.go:611-616 copies every resolved secret into secretValues []string via sec.ExposeString() to seed the audit redactor, and keeps it for the whole run. Go strings are immutable, so Secret.Zero() (internal/secrets/secret.go:45-50) can never reach those copies — the redactor's seed permanently defeats the zeroing the rest of the file maintains. Feeding the redactor Secret values (or hashes/lengths) fixes it.

Two more symptoms of the same cause

• fatalTeardown is forward-declared (start.go:620, body assigned 280 lines later at :900) purely because a closure must reference locals that do not exist yet.
• createRuntimeDir (start.go:1288-1303) does os.RemoveAll(dir) on a path derived only from sha256(workdir) (:1291-1295) and is untested. Two concurrent omac start in one workdir: the second deletes the first's rtDir, unlinking the live bridge.sock (:601). createRuntimeDirServe (serve.go:1790-1803) is the same 14 lines with a different salt.

Suggested fix / Ask

Give the launch an object that owns what it acquires.

• Add a launch.Session (own package or internal/cli) owning rtDir, auditor, cache scope, supervisor, facade, control plane, sandbox tmp dir and the resolved secrets:
  go func Open(env Env, plan Plan) (*Session, error) // phases 4–7 func (s *Session) Argv(plan Plan) ([]string, error) func (s *Session) Run(argv []string, extra map[string]string) (int, error) func (s *Session) Close() error // the ONLY teardown
• Replace fatalTeardown with s.Close(); return ExitIOError — no os.Exit mid-pipeline, so secret zeroing, tmp removal and the flock release cannot be skipped.
• Seed the audit redactor from secrets.Secret values (or hashes) rather than []string, so Zero() is meaningful.
• Reduce runLaunch to flag parsing + plan building + Open/Argv/Run (target ≲ 100 lines).
• Reuse the same Session from runServe so the two entry points stop re-deriving the sequence.
• Make createRuntimeDir concurrency-safe (or fail loudly on an in-use rtDir) and unify it with createRuntimeDirServe; add a test for the concurrent-launch case.

Acceptance criteria

• Exactly one teardown path; a strict-audit write failure runs the same cleanup as a normal exit (regression test asserting secrets zeroed, sandbox tmp removed, cache flock released).
• No os.Exit inside the launch pipeline.
• Audit redaction no longer requires immutable copies of secret values.
• Phases 4–7 exercisable in tests without execing a harness.
• A second omac start in the same workdir cannot delete the first's runtime dir.
• runLaunch and runServe share the sequence rather than duplicating it.

Non-goals

• Changing CLI flags, exit codes or any user-visible launch behaviour.
• The skill-arming rewrite (Skill readiness (secrets + config resolution) is implemented five times and the copies have diverged #174) or the sandbox-plan value (Facade sandbox wiring resolves the launcher profile name against the sandbox-policy namespace, disabling GET /sandbox/denied on every default launch #173) — land those first; this is the remaining lifecycle extraction.
• Reworking the control-plane endpoint surface (see Reduce control plane surface area in omac start mode #70).
• Changing the audit event schema.

Evidence / Environment

main @ 6257211, Linux.

awk '/^func runLaunch/,/^}/' internal/cli/start.go | wc -l   # 769
awk '/^func runServe/,/^}/'  internal/cli/serve.go | wc -l   # 483
sed -n '374,382p' internal/cli/start.go     # deferred secret zeroing
sed -n '896,912p' internal/cli/start.go     # fatalTeardown -> os.Exit
sed -n '602,620p' internal/cli/start.go     # the comment claiming parity + secretValues
grep -n "defer " internal/cli/start.go | sed -n '1,12p'

🤖 Filed by an agent during an architecture review of main.