fix(P05): final review fixes — PERSONAS.md body, config --addr precedence, migration 0007 dedup
Audit fix: PERSONAS.md body roster updated for v0.7 (was stale v0.6 content). Review P1-004: daemon --addr now uses cmd.Flags().Changed() to detect explicit flag, so config listen_addr only applies when --addr was not explicitly passed (correct flag>env>file>default precedence). Review P1-001: migration 0007 now dedups existing duplicate serial_hex rows before creating the UNIQUE index (backward-compat with v0.6 DBs that accumulated duplicates before the constraint existed). ---ci--- project: orca phase: 5 milestone: v0.7 status: execute requirements: covered: [REQ-053, REQ-054, REQ-055, REQ-056] partial: [] ---/ci---
This commit is contained in:
Jon Chery
+27
-34
@@ -38,67 +38,60 @@ reason: |
|
||||
- **Constraints**: `boundary-enforcement`, `offline-first`, `no-redundant-implementations`
|
||||
- **Territory**: `**/*.go`, `cmd/**`, `internal/**`
|
||||
- **Active**: true
|
||||
- **Reason**: Coordination across P01/P02/P03. SSH/bootstrap touches security + cli + store + doctor — territory overlaps need adjudication (proxmox package boundary, doctor Proxmox check scaffolding).
|
||||
- **Reason**: Coordination across P01/P03/P04. Owns cert command registration (P01), engine/transport/audit test coverage (P03), and pprof daemon integration (P04). Adjudicates territory overlaps between config (backend) and CLI wiring (lead).
|
||||
|
||||
### backend-engineer
|
||||
- **Domain**: backend
|
||||
- **Frameworks**: `cobra`, `net/http`, `golang.org/x/crypto/ssh`
|
||||
- **Constraints**: `API-first`, `error-handling`, `minimal-dependencies`, `security-first`, `idempotent-bootstrap`
|
||||
- **Territory**: `**/api/**`, `**/*_handler*`, `**/*_handler.go`, `internal/daemon/**`, `internal/proxmox/**`, `internal/cli/init.go`
|
||||
- **Frameworks**: `cobra`, `hashicorp/hcl/v2`
|
||||
- **Constraints**: `API-first`, `error-handling`, `minimal-dependencies`, `no-package-level-state`
|
||||
- **Territory**: `**/config/**`, `**/api/**`, `**/*_handler*`, `internal/daemon/**` (non-pprof), `internal/cli/root.go` (config flag wiring)
|
||||
- **Active**: true
|
||||
- **Reason**: Owns the `orca init` full-bootstrap orchestration (CA + cert + db + localhost node, idempotent) and the `internal/proxmox/bootstrap.go` SSH session sequence (dial, deploy pubkey, useradd, pveum, sudoers, visudo validate). Added `idempotent-bootstrap` constraint (D-036 — re-run must be skip-and-refresh) and `golang.org/x/crypto/ssh` to frameworks.
|
||||
- **Reason**: Owns `internal/config` package (P02 — HCL config file parsing, Load + MergeOverrides with flag>env>file>default precedence per D-039). No package-level state (AD-023). Config is a pure function passed explicitly to consumers.
|
||||
|
||||
### data-engineer
|
||||
- **Domain**: data
|
||||
- **Frameworks**: `modernc/sqlite`, `iter`
|
||||
- **Constraints**: `schema-first`, `migration-safe`, `local-storage-only`, `no-goroutine-leak`, `nullable-column-handling`
|
||||
- **Territory**: `**/store/**`, `**/model.go`, `**/migration*`, `migrations/**`, `internal/store/migrations/**`, `internal/model/node.go`
|
||||
- **Constraints**: `schema-first`, `migration-safe`, `local-storage-only`
|
||||
- **Territory**: `**/store/**`, `**/model.go`, `**/migration*`, `migrations/**`, `internal/store/migrations/**`
|
||||
- **Active**: true
|
||||
- **Reason**: Reactivated for v0.6. Owns migration `0006_node_kind_os.sql` (REQ-049 — nullable `kind`/`os` columns, backward-compatible) and `NodeRepo` schema extension (Insert/Get/List/Watch/scanNode column additions + new `GetByName`/`UpdateLastSeenAndOS` helpers). Added `nullable-column-handling` constraint (NULL → `""` in Go struct, not nil-deref).
|
||||
- **Reason**: Owns `cert_repo_test.go` (P01 companion — 11 tests covering Insert/Get/List/ListByNode/LatestForKind/PruneOlderThan/Delete + duplicate serial) and migration 0007 (UNIQUE index on `certs.serial_hex`). Co-owns `internal/proxmox/ssh_session_test.go` + `bootstrap_test.go` extension (P03 — transport/proxmox coverage).
|
||||
|
||||
### cli-engineer
|
||||
- **Domain**: CLI/UX
|
||||
- **Frameworks**: `cobra`, `pflag`
|
||||
- **Constraints**: `discoverable-help`, `consistent-flag-naming`, `human-readable-output`, `machine-readable-json-flag`, `signal-handling`, `password-flag-redaction`
|
||||
- **Territory**: `cmd/**`, `internal/cli/**`, `internal/commands/**`
|
||||
- **Active**: true
|
||||
- **Reason**: Owns `orca init` multi-step bootstrap output UX (progress lines per step), `orca node join --type/--host/--user/--password/--proxmox-user/--proxmox-role` flag wiring, and `doctor os`/`doctor proxmox` subcommand wiring. Added `password-flag-redaction` constraint (D-031 — `--password` never echoed, prefer `$ORCA_PROXMOX_PASSWORD`, zero after use).
|
||||
- **Active**: false (v0.7)
|
||||
- **Reason**: Deactivated — merged into lead-developer for v0.7. The cert registration is a 1-line AddCommand; the `--config` flag is root-command wiring; pprof is a daemon flag. No new CLI subsystem requiring a dedicated CLI persona.
|
||||
|
||||
### security-engineer
|
||||
- **Domain**: security
|
||||
- **Frameworks**: `crypto/tls`, `crypto/x509`, `crypto/ed25519`, `golang.org/x/crypto/ssh`, `slog`
|
||||
- **Constraints**: `no-panic-in-production`, `structured-audit-logging`, `no-secret-in-logs`, `input-validation`, `least-privilege`, `tofu-host-key-pinning`, `noexec-sudoers`
|
||||
- **Territory**: `**/auth/**`, `**/audit/**`, `internal/security/**`, `internal/transport/**` (TLS config only), `internal/proxmox/**` (SSH + sudoers + PVE role)
|
||||
- **Active**: true
|
||||
- **Reason**: Reactivated for v0.6. Owns `internal/security/sshkey.go` (Ed25519 keygen, 0600/0644 mode enforcement per REQ-033 spirit), TOFU host-key pinning via `knownhosts.New`, sudoers least-privilege design (NOEXEC on pct/qm, exclude pvesh, no NOEXEC on apt-get/dpkg), password redaction (D-031), and audit logging of all bootstrap/join actions (REQ-052). Added `tofu-host-key-pinning` and `noexec-sudoers` constraints. Co-owns `internal/proxmox/**` with backend-engineer (security owns SSH auth + sudoers content; backend owns the session orchestration).
|
||||
- **Active**: false (v0.7)
|
||||
- **Reason**: Deactivated — v0.7 adds no new security surface. pprof is operator-only, addr-gated (AD-024); cert registration exposes existing security code, does not add new. The config package handles paths only (no secrets). Existing security constraints (file modes, redaction) are exercised by P01 smoke tests but not extended.
|
||||
|
||||
### devops-engineer
|
||||
- **Active**: false (v0.6)
|
||||
- **Reason**: Deactivated — v0.6 has no install.sh, Dockerfile, .coreci.yml, or release-pipeline surface. The Proxmox SSH bootstrap is backend + security work, not devops. Was active in v0.5 (distribution milestone).
|
||||
- **Active**: false (v0.7)
|
||||
- **Reason**: Deactivated — v0.7 has no packaging/distribution/release surface. Was active in v0.5 (distribution milestone).
|
||||
|
||||
### network-engineer
|
||||
- **Active**: false (v0.6)
|
||||
- **Reason**: v0.6 has no transport/mTLS surface. SSH is point-to-point bootstrap, not the mTLS mesh network-engineer owns.
|
||||
- **Active**: false (v0.7)
|
||||
- **Reason**: Deactivated — v0.7 has no transport/mTLS surface changes. P03 adds tests for existing transport code but no new network surface.
|
||||
|
||||
### frontend-engineer
|
||||
- **Active**: false (v0.6)
|
||||
- **Active**: false (v0.7)
|
||||
- **Reason**: No web UI in Orca (unchanged from v0.1 onward).
|
||||
|
||||
## Territory Enforcement
|
||||
|
||||
- **Mode**: `warn` (per `config.json`)
|
||||
- **Behavior**: Out-of-territory file changes log a warning but do not block.
|
||||
- **Key overlaps in v0.6** (lead-developer adjudicates):
|
||||
- `internal/proxmox/bootstrap.go` — security-engineer (SSH auth, sudoers, PVE role) + backend-engineer (session orchestration, error handling). Boundary: security package exposes `BootstrapProxmox(ctx, opts) error`; the function lives in `internal/proxmox` but imports `internal/security` for SSH key handling.
|
||||
- `internal/doctor/doctor.go` `Proxmox()` — reuses `internal/proxmox` SSH client (security) but check scaffolding clones `doctor.Network()` pattern. Backend-engineer adjudicates (network-engineer deactivated).
|
||||
- `internal/store/node_repo.go` — data-engineer territory, but the `UpdateLastSeenAndOS` caller is `internal/cli/init.go` (backend). Standard repo-consumer boundary.
|
||||
- **Key overlaps in v0.7** (lead-developer adjudicates):
|
||||
- `internal/cli/root.go` — backend-engineer (config flag + context wiring) + lead-developer (existing root command). Boundary: backend owns `--config` flag + `configFromCtx`; lead owns all other root command behavior.
|
||||
- `internal/cli/daemon.go` — lead-developer (pprof flag + config listen_addr wiring) + backend-engineer (config consumption). Boundary: lead owns the daemon command; backend's config package is consumed, not modified.
|
||||
- `internal/store/migrations/` — data-engineer owns all migrations. No overlap in v0.7.
|
||||
|
||||
## v0.6 vs v0.5 Persona Diff
|
||||
## v0.7 vs v0.6 Persona Diff
|
||||
|
||||
| Change | Rationale |
|
||||
|--------|-----------|
|
||||
| `data-engineer` reactivated | Owns migration 0006 + NodeRepo schema extension (kind/os columns). |
|
||||
| `security-engineer` reactivated | Owns SSH keygen, TOFU host-key, sudoers, PVE role — first-class security surface. |
|
||||
| `devops-engineer` deactivated | v0.6 has no packaging/distribution surface. |
|
||||
| `network-engineer` remains deactivated | No transport/mTLS surface. |
|
||||
| `data-engineer` retained | Owns cert_repo tests + migration 0007 + proxmox/transport test coverage. |
|
||||
| `security-engineer` deactivated | v0.7 adds no new security surface (pprof is operator-only, cert registration exposes existing code). |
|
||||
| `cli-engineer` deactivated | Merged into lead-developer (cert registration is 1-line; config flag is root wiring). |
|
||||
| `devops-engineer` remains deactivated | No packaging/distribution in v0.7. |
|
||||
| `network-engineer` remains deactivated | No transport/mTLS surface changes. |
|
||||
| `frontend-engineer` remains deactivated | No web UI. |
|
||||
@@ -36,7 +36,7 @@ var daemonCmd = &cobra.Command{
|
||||
|
||||
log := newLogger()
|
||||
addr := daemonAddr
|
||||
if cfg := configFromCtx(cmd.Context()); cfg != nil && cfg.ListenAddr != "" && addr == ":8080" {
|
||||
if cfg := configFromCtx(cmd.Context()); cfg != nil && cfg.ListenAddr != "" && !cmd.Flags().Changed("addr") {
|
||||
addr = cfg.ListenAddr
|
||||
}
|
||||
srv := daemon.NewServer(daemon.Options{
|
||||
|
||||
@@ -2,4 +2,16 @@
|
||||
-- issued by orca may share the same serial. Implemented as a UNIQUE
|
||||
-- INDEX so existing 0004_certs.sql need not be re-run on deployed
|
||||
-- databases. v0.7 P01 (REQ-053 companion).
|
||||
--
|
||||
-- P1-001 fix (final review): before creating the UNIQUE index, dedup
|
||||
-- any existing rows that share a serial_hex. Keep the newest row
|
||||
-- (MAX(created_at)) per serial_hex and delete older duplicates. This
|
||||
-- makes the migration backward-compatible with v0.6 deployments that
|
||||
-- may have accumulated duplicate serials before the constraint existed.
|
||||
DELETE FROM certs WHERE id NOT IN (
|
||||
SELECT id FROM (
|
||||
SELECT id, ROW_NUMBER() OVER (PARTITION BY serial_hex ORDER BY created_at DESC) AS rn
|
||||
FROM certs
|
||||
) WHERE rn = 1
|
||||
);
|
||||
CREATE UNIQUE INDEX IF NOT EXISTS idx_certs_serial_unique ON certs(serial_hex);
|
||||
Reference in New Issue
Block a user