Jon Chery
7c4b603811
Codebase audit: cert command unreachable, no config parser, low coverage (engine 8.3%, transport 26.3%, proxmox 5.1%, audit 0%), pprof deferred. 5 architectural decisions (AD-022..AD-026). Zero new deps. ---ci--- project: orca phase: 0 milestone: v0.7 status: research ---/ci---
161 lines
8.3 KiB
Markdown
161 lines
8.3 KiB
Markdown
# Research: Orca v0.7 — Hardening & Completion
|
||
|
||
## 1. Codebase audit findings (RESEARCH stage)
|
||
|
||
A full codebase audit surfaced the gaps that define the v0.7 scope.
|
||
Each finding is grounded in a specific file/coverage measurement.
|
||
|
||
### 1.1 `orca cert` command tree is unreachable (critical)
|
||
|
||
- `internal/cli/cert.go:44` exports `NewCommand(log *slog.Logger)
|
||
*cobra.Command` which builds the full `cert ca-init | gen | show |
|
||
renew | fingerprint` tree (5 subcommands, all implemented, all
|
||
spec-compliant per REQ-033/035/036).
|
||
- **No file in the repo calls `NewCommand` or registers it on
|
||
`rootCmd`.** `grep -rn "rootCmd.AddCommand" internal/cli/` lists
|
||
daemon, init, audit, version, job, node, doctor, status — `cert` is
|
||
absent. `./bin/orca cert` returns `error: unknown command "cert"`.
|
||
- The function is named `NewCommand` (not `newCertCmd`), so it is not
|
||
picked up by any init-based registration convention.
|
||
- **Impact**: every cert operation the spec promises (REQ-023, REQ-025,
|
||
REQ-033, REQ-035, REQ-036) is unreachable from the CLI. Operators
|
||
cannot bootstrap a CA, issue a server cert, or rotate one without
|
||
hand-crafting calls into the `security` package. This is the single
|
||
highest-impact bug in the v0.1–v0.6 line.
|
||
- **Fix**: one-line `rootCmd.AddCommand(NewCommand(log))` in
|
||
`internal/cli/cert.go` (or a new `init()`), plus a regression test
|
||
that asserts `rootCmd.Commands()` contains a child whose `Use ==
|
||
"cert"`.
|
||
|
||
### 1.2 `internal/store/cert_repo.go` has no test file
|
||
|
||
- `internal/store/cert_repo.go` exists (the `certs` table from
|
||
migration 0004) but `internal/store/cert_repo_test.go` does not.
|
||
- Every other repo in `internal/store/` has a `_test.go`:
|
||
`node_repo_test.go`, `job_task_repo_test.go`, `capacity_repo_test.go`,
|
||
`audit_repo_test.go`, `migrate_test.go`.
|
||
- **Fix**: add `cert_repo_test.go` covering Insert/Get/List/rotation
|
||
history (N=3 per REQ-025) + serial_hex uniqueness.
|
||
|
||
### 1.3 Low test coverage in core packages
|
||
|
||
| Package | Coverage | Missing tests for |
|
||
|---------|----------|-------------------|
|
||
| `internal/engine` | 8.3% | `executor.go`, `dispatcher.go`, `peer.go` (only `scheduler_test.go` exists) |
|
||
| `internal/transport` | 26.3% | `mtls.go`, `dispatch.go`, `handshake_log.go` (only `idempotency_test.go` exists) |
|
||
| `internal/proxmox` | 5.1% | `bootstrap.go` SSH path (only `bootstrap_test.go` exists, exercises the no-op dry-run) |
|
||
| `internal/audit` | no test files | `audit.go` (Emit, EmitWithErr, LogHandshake*) |
|
||
|
||
- Target per D-042: 50% floor per package, 70% for new code in P02/P04.
|
||
- Strategy: table-driven tests + `httptest.NewTLSServer` for transport;
|
||
interface-based mocks for the SSH dialer (already an interface in
|
||
`proxmox/bootstrap.go:211` `defaultSSHDialer` with `DialContext`).
|
||
|
||
### 1.4 No HCL config file parser
|
||
|
||
- D-009 specified `~/.orca/config.hcl` and `/etc/orca/orca.hcl` as
|
||
config locations. `find . -name "*.hcl"` returns only testdata
|
||
(`testdata/hello.hcl`, `testdata/fail.hcl`) used by jobspec tests.
|
||
- The CLI relies entirely on flags + env vars (`ORCA_HOME`,
|
||
`ORCA_DB`, `ORCA_PROXMOX_PASSWORD`). There is no `internal/config`
|
||
package.
|
||
- `internal/jobspec/spec.go:40` already uses
|
||
`hclsimple.Decode(filename, data, nil, &spec)` — the exact same
|
||
pattern works for a `Config` struct. No new dep required (hashicorp/hcl/v2
|
||
is already a direct dep).
|
||
- **Fix**: new `internal/config` package with a `Config` struct (HCL
|
||
tags: `db_path`, `listen_addr`, `ca_path`, `server_cert_path`,
|
||
`server_key_path`, `node_capacity`), a `Load(paths ...string)`
|
||
function, and a `--config` flag on the root command. Precedence per
|
||
D-039: flag > env > file > default.
|
||
|
||
### 1.5 pprof endpoint (I-308, deferred since v0.2)
|
||
|
||
- I-308 was deferred in v0.2 IDEATE ("keep v0.2 lean") and never
|
||
revisited. The daemon (`internal/daemon/server.go`) has no pprof
|
||
surface today.
|
||
- `net/http/pprof` is stdlib — zero new deps. Mount on a separate
|
||
`*http.ServeMux` so it never touches the mTLS daemon listener.
|
||
- **Fix**: `--pprof <addr>` flag on `orca daemon` (default disabled).
|
||
If set, start a second `http.Server` on `<addr>` with
|
||
`pprof.Index`/`pprof.Cmdline`/etc. registered. Log a WARN that the
|
||
endpoint is unauthenticated + operator-only.
|
||
|
||
## 2. Prior art & patterns
|
||
|
||
### 2.1 HCL config in HashiCorp tools
|
||
|
||
Nomad, Consul, and Terraform all use HCL for config with the same
|
||
`hclsimple.Decode` + struct-tag pattern. The precedence model (flag >
|
||
env > file > default) is the de-facto standard; Viper implements it but
|
||
adds a large dep. Orca's `internal/config` will implement the 4-layer
|
||
merge by hand (~80 LOC) to stay minimal-deps.
|
||
|
||
### 2.2 pprof in Go daemons
|
||
|
||
Standard pattern: `import _ "net/http/pprof"` registers handlers on
|
||
`http.DefaultServeMux`. Best practice for production daemons is a
|
||
**separate listener** (not DefaultServeMux) so pprof is never exposed
|
||
on the public port. Orca will use a dedicated `*http.ServeMux` +
|
||
`http.Server` on the `--pprof` addr, default disabled.
|
||
|
||
### 2.3 Test coverage for concurrent Go
|
||
|
||
`internal/engine` (executor, dispatcher) and `internal/transport`
|
||
(mtls, dispatch) are concurrent. Coverage strategy:
|
||
- `httptest.NewTLSServer` for transport — exercise real TLS handshakes
|
||
against an in-process server.
|
||
- Interface-based mocks for the SSH dialer (proxmox) and the peer
|
||
client (transport) — both already have interface seams.
|
||
- `sync.WaitGroup` + channel assertions for executor/dispatcher
|
||
lifecycle.
|
||
- `-race` is already on in CI (REQ-031) — new tests inherit it.
|
||
|
||
## 3. v0.7 Architectural Decisions (AD-022..AD-026)
|
||
|
||
| ID | Decision | Rationale |
|
||
|----|----------|-----------|
|
||
| AD-022 | `orca cert` registered via `init()` in `cert.go` calling `rootCmd.AddCommand(NewCommand(slog.Default()))` | Keeps registration co-located with the command definition; matches the pattern in `daemon.go`/`audit.go` where each command file self-registers. Avoids a central registration function that would drift. |
|
||
| AD-023 | `internal/config` package: `Config` struct + `Load(paths ...string) (*Config, error)`; no global singleton | Config is passed explicitly to `daemon.NewServer`, `cli` commands, etc. No package-level state — testable, no init-order surprises. |
|
||
| AD-024 | pprof on a separate `*http.Server` + `*http.ServeMux`, default disabled | Never co-mingles with the mTLS daemon listener. Operator opts in via `--pprof :6060`. Matches Go daemon best practice. |
|
||
| AD-025 | Coverage floor measured per-package via `go test -cover ./<pkg>` | No aggregate threshold (aggregates hide low-coverage packages). CI gate added in P03: `go test -cover ./internal/engine ./internal/transport ./internal/proxmox ./internal/audit` and assert each ≥ 50%. |
|
||
| AD-026 | No new direct dependencies in v0.7 | `net/http/pprof` (stdlib), `hashicorp/hcl/v2` (already direct). v0.7 preserves the minimal-deps ethos. |
|
||
|
||
## 4. PERSONAS assessment
|
||
|
||
v0.7 is an NFR milestone touching CLI, config, tests, and daemon. The
|
||
default 3-persona roster (lead-developer, backend-engineer,
|
||
data-engineer) is sufficient:
|
||
|
||
- **lead-developer**: owns P01 (cert registration) + P04 (pprof) — CLI/
|
||
daemon territory.
|
||
- **backend-engineer**: owns P02 (config package) — internal/config +
|
||
CLI integration.
|
||
- **data-engineer**: owns P01 cert_repo tests + P03 store coverage —
|
||
`internal/store` territory.
|
||
- **lead-developer** also owns P03 engine/transport/proxmox/audit
|
||
coverage (test-only phase, no schema changes).
|
||
|
||
No new personas needed. No phase-specific personas. Territory
|
||
enforcement stays `warn`. See `.ciagent/PERSONAS.md` (updated).
|
||
|
||
## 5. Dependencies
|
||
|
||
v0.7 adds **zero** new direct dependencies:
|
||
- HCL parsing: `hashicorp/hcl/v2` (already direct, used by jobspec).
|
||
- pprof: `net/http/pprof` (stdlib).
|
||
- Tests: `net/http/httptest` (stdlib), existing interfaces.
|
||
|
||
`go.mod` is unchanged by v0.7.
|
||
|
||
## 6. Risks
|
||
|
||
- **P01 cert registration** may surface latent bugs in the cert
|
||
subcommands (they've never been exercised end-to-end). Mitigation:
|
||
P01 includes a smoke test that runs `cert ca-init` + `cert gen` +
|
||
`cert show` + `cert fingerprint` against a temp `ORCA_HOME`.
|
||
- **P02 config precedence** is easy to get wrong (flag/env/file/default
|
||
merge order). Mitigation: table-driven test covering all 4 layers.
|
||
- **P03 coverage** on concurrent packages may reveal race conditions
|
||
(already hidden by the 8.3% coverage). Mitigation: `-race` is on; P03
|
||
fixes any races it uncovers as part of the same phase. |