Files
orca/.ciagent/GRILL_v0.3.md
T
Jon Chery df58bc25a3 docs(milestone): complete scheduling-streaming (v0.3)
---ci---
project: orca
phase: 3
milestone: v0.3
status: complete
requirements:
  covered: [REQ-022, REQ-030, REQ-032]
  partial: []
---/ci---

v0.3 milestone merged to main. Includes all v0.2 work (P08-P10) that
was previously on the milestone branch but not yet merged to main, plus
the v0.3 completion work (iter.Seq streaming + doctor network/db).

v0.2 phases included: P08 (mTLS), P09 (scheduling), P10 (security scan).
v0.3 phases: P0 (pre-execution), P1 (iter.Seq streaming), P2 (doctor),
P3 (final review+ship).

Total: 40 requirements, all complete. No new go.mod dependencies.
Full test suite passes under -race. gofmt + go vet clean.
2026-08-01 20:06:47 +00:00

27 KiB
Raw Blame History

Grill Report: Orca v0.3 — scheduling-streaming

Date: 2026-08-01 Reviewer: ci-griller (red-team, adversarial) Plan under review: .ciagent/PLAN_v0.3.md (commit 89fa172) Branch: phase/00-pre-execution Mode: Full autonomy


Methodology

Every claim in PLAN_v0.3.md and RESEARCH_v0.3.md was cross-checked against the actual codebase (the 7 source files listed in the task, plus migrate.go, store.go, doctor_test.go, security/integration_test.go, security/ca.go, security/csr.go, model/node.go, model/job.go, and cli/doctor.go). Findings are scored on 9 axes. Binding verdicts are ACCEPT (plan must change), REJECT (concern noted, plan stands), or DEFER (address during execution).


Summary Verdict

Severity Count
CRITICAL 2
HIGH 2
MEDIUM 5
LOW 3
Total 12

Overall verdict: PROCEED WITH CHANGES

The plan is fundamentally sound — the scope is right-sized, the requirements coverage is complete, the persona territories are respected, and the no-new-dependencies promise holds. However, two CRITICAL findings require plan changes before execution begins. Neither is a scope expansion; both are correctness fixes to the design as written. With the 2 ACCEPT changes applied, this plan is ready to execute.


Per-Axis Findings

Axis 1 — Feasibility (can each task actually be implemented?)

F-01 [CRITICAL] — Watch yields per-row but CLI table mode requires full-snapshot-per-tick

Severity: CRITICAL Axis: Feasibility / Vertical slice integrity Binding verdict: ACCEPT (plan must change)

Finding: The plan is internally contradictory about what Watch yields.

  • D-028 (RESEARCH:88) says Watch "yields the full current snapshot (one element per row)."
  • Task 01-01-01 (PLAN:30) says Watch "yields one *model.Job per row via scanJob" — i.e., iter.Seq[*model.Job], one element per row per tick.
  • Task 01-02-02 (PLAN:42) says the CLI table render "collect the full snapshot from seq into a []*model.Job" then compares against the previous snapshot's rendered table.

These are incompatible. iter.Seq[*model.Job] yields individual jobs with no tick-boundary signal. The CLI ranging for job := range seq receives a flat stream of jobs and cannot know when a tick's snapshot is complete. It cannot collect "the full snapshot" because it cannot detect the end of a tick.

The JSON mode (01-02-03) can work without tick boundaries (per-element dedup via map[string][]byte), but the table mode cannot. Table mode needs the complete snapshot to render the table, clear the screen, and compare against the previous frame.

Evidence:

  • RESEARCH_v0.3.md:106-142 — implementation yields yield(j) per row inside for rows.Next(), not yield(allJobs) per tick.
  • PLAN_v0.3.md:30 — "yields one *model.Job per row"
  • PLAN_v0.3.md:42 — "collect the full snapshot from seq into a []*model.Job"
  • PLAN_v0.3.md:44 (01-02-04) — nodeListCmd watch bypasses registry, same per-row yield.
  • D-028 says "full current snapshot" but the code yields per-row.

Required change: Change the Watch element type from iter.Seq[*model.Job] to iter.Seq[[]*model.Job] (and iter.Seq[[]*model.Node] analogously). Each tick yields the full snapshot as a single slice. This:

  1. Makes D-028 ("yields the full current snapshot") literally true.
  2. Makes table mode trivial: for snapshot := range seq { render(snapshot) }.
  3. Makes JSON mode cleaner: per-tick, diff the snapshot against the previous one, emit one JSON line per changed element. This also enables a natural "delete" event for elements that disappeared (not possible with per-row yield).
  4. Simplifies the test contract: TestWatch_YieldsSnapshots ranges over iter.Seq[[]*model.Job] and each yield is a complete tick — no timing ambiguity about "did I get all rows for this tick?"

Impact on plan:

  • Tasks 01-01-01, 01-01-02: signature changes to iter.Seq[[]*model.Job] / iter.Seq[[]*model.Node]. Implementation collects all rows into a slice per tick, then yield(slice).
  • Task 01-02-02 (table): for snapshot := range seq { ... } — direct, no collection needed.
  • Task 01-02-03 (JSON): per-tick diff against previous snapshot's map[string][]byte. Emit "init"/"update"/"delete" events.
  • Task 01-01-04 (tests): assert each yield is a complete snapshot slice.
  • D-026, D-028, D-046: update to reflect slice-per-tick semantics.
  • Must-have criteria for 01-01-01/01-01-02: update signature assertions.

This is a mechanical change to the plan, not a scope change. The implementation is simpler (no tick-boundary detection needed).

Confidence: 0.92


F-02 [CRITICAL] — First-tick delay: Watch waits a full interval before first yield

Severity: CRITICAL Axis: Feasibility / UX correctness Binding verdict: ACCEPT (plan must change)

Finding: The Watch implementation (RESEARCH:110-116) has this structure:

ticker := time.NewTicker(1 * time.Second)
defer ticker.Stop()
for {
    select {
    case <-ctx.Done(): return
    case <-ticker.C:   // <-- waits 1s BEFORE first query
    }
    // query + yield
}

The select waits for the first ticker pulse before running the first query. With a 1s default interval, orca job list --watch shows nothing for 1 full second, then the first snapshot appears. For a CLI tool, a 1s blank screen is a poor UX and looks broken. The user expects immediate output, then refreshes every 1s.

The tests (01-01-04) use watchInterval=10ms, so the delay is only 10ms and the test passes — but the test does NOT catch this UX bug because the interval is tiny. In production (1s), the bug is visible.

Evidence:

  • RESEARCH_v0.3.md:110-116select before first query.
  • PLAN_v0.3.md:30 — "pull-based inline polling loop on a 1s ticker" — no mention of immediate first yield.
  • Standard top-like tools yield immediately, then tick.

Required change: Add to tasks 01-01-01 and 01-01-02: the polling loop must query and yield immediately on the first iteration, then select on the ticker for subsequent ticks. Implementation shape:

for {
    // query + yield (runs immediately on first iteration)
    rows, err := r.db.QueryContext(ctx, ...)
    // ... yield snapshot ...
    select {
    case <-ctx.Done(): return
    case <-ticker.C:
    }
}

Or equivalently, query once before the loop, then loop with select-first. The must-have criteria should add: "first yield occurs immediately (no watchInterval delay before first snapshot)."

Impact on plan:

  • Tasks 01-01-01, 01-01-02: add "immediate first yield" to description + must-have.
  • Task 01-01-04 (tests): add assertion that the first snapshot appears within a short deadline (e.g., <50ms) even with watchInterval=10ms — proving the first yield is not tick-gated.

Confidence: 0.95


F-03 [HIGH] — P01 and P02 both modify internal/cli/node.go (file-disjoint claim is false)

Severity: HIGH Axis: Feasibility / Timeline (parallelism) Binding verdict: ACCEPT (plan must change)

Finding: D-042 (PLAN:133, RESEARCH:489) claims "P01 and P02 are file-disjoint — no file is modified by both." This is false.

  • P01 task 01-02-04 (PLAN:44) modifies internal/cli/node.go — adds --watch flag + render modes to nodeListCmd.
  • P02 task 02-01-01 (PLAN:76) modifies internal/cli/node.go — removes the dbPath function and updates openDB to call certpaths.DBPath().

Both phases touch internal/cli/node.go. If developed in parallel (as D-042 permits), this causes merge conflicts.

Evidence:

  • PLAN_v0.3.md:44 — 01-02-04 files: internal/cli/node.go
  • PLAN_v0.3.md:76 — 02-01-01 files: internal/cli/node.go (remove old dbPath)
  • PLAN_v0.3.md:133 — "P01 and P02 are file-disjoint"
  • Actual code: internal/cli/node.go:22-28 defines dbPath; :30-36 defines openDB which calls dbPath(). openDB is used by 14 call sites across job.go, daemon.go, node_capacity.go, audit.go, node.go.

Required change: Update D-042 and the cross-phase notes (PLAN:131-133) to acknowledge the overlap. Two options (pick one):

  1. Serialize: P02 Wave 1 (02-01-01) runs before P01 Wave 2 (01-02-04). P02 Wave 1 is a prerequisite for P01 Wave 2 on the node.go file. P01 Wave 1 (store layer) and P02 Wave 1 can still run in parallel.
  2. Merge the changes: task 02-01-01 is folded into P01 Wave 2's node.go modification (the cli-engineer updates openDB to use certpaths.DBPath() while also adding --watch).

Recommended: Option 1 (serialize P02 Wave 1 before P01 Wave 2). It preserves the wave structure and persona assignments. Update the cross-phase note to say: "P02 Wave 1 (02-01-01) must complete before P01 Wave 2 (01-02-04) due to shared internal/cli/node.go modification. P01 Wave 1 and P02 Wave 1 may run in parallel."

Confidence: 0.90


F-04 [HIGH] — D-037 ServerName = node.Name assumption is fragile and unverified against real join flow

Severity: HIGH Axis: Feasibility / Security Binding verdict: DEFER (address in execution, with documentation)

Finding: D-037 (RESEARCH:261, confidence 0.80) assumes serverName = node.Name for the mTLS health probe. The TLS client's ServerName must match a SAN entry on the peer's server cert. But GenerateCSR(commonName, sans) (csr.go:24) takes the commonName and SANs as separate arguments. The commonName becomes the cert Subject CN, but ServerName in tls.Config is matched against SANs (DNSNames/IPAddresses), not the CN (per Go's crypto/tls behavior since Go 1.15).

If a node joined with --name node-b but its cert SAN is localhost (or an IP), serverName = "node-b" will fail the TLS handshake with a "certificate is valid for localhost, not node-b" error — even though the peer is perfectly healthy.

The research (RESEARCH:261) says "confirmed in integration_test.go:41 GenerateCSR("test-server", ...)" — but that test uses serverName = "localhost" (integration_test.go:83), which matches the SAN localhost, not the commonName test-server. The test proves SAN-matching, not CN-matching.

Evidence:

  • internal/security/csr.go:24GenerateCSR(commonName, sans) — CN and SANs are separate.
  • internal/security/integration_test.go:41GenerateCSR("test-server", []string{"localhost", "127.0.0.1"}) — CN is "test-server", SANs are localhost/127.0.0.1.
  • internal/security/integration_test.go:83ClientTLSConfig(..., "localhost", ...) — serverName = "localhost" (a SAN), NOT "test-server" (the CN).
  • internal/transport/mtls.go:49-51serverName is required and set as tls.Config.ServerName (matched against SANs).
  • PLAN_v0.3.md:88 — 02-02-03: serverName = n.Name.

Mitigation (DEFER to execution):

  1. Document the assumption in the Network() check message: "probing at (assuming cert SAN = node name)".
  2. If the handshake fails with a SAN mismatch error, the FAIL message should include the cert's actual SANs (parsed from the error) so the operator can diagnose. This is a refinement, not a plan blocker.
  3. The test 02-02-05(e) uses Name = "localhost" which matches the SAN — so the test passes, but it doesn't prove the general case. Add a test comment noting this assumption.

Why DEFER not ACCEPT: The assumption is documented (D-037, 0.80 confidence), the failure mode is graceful (FAIL with handshake error, not a crash), and fixing it properly (storing SANs in the nodes table) is a scope expansion beyond v0.3. The plan should note the limitation; execution should add diagnostic context to the error message.

Confidence: 0.78


F-05 [MEDIUM] — store.Open runs migrations before integrity_check can run

Severity: MEDIUM Axis: Feasibility / Testing Binding verdict: REJECT (concern noted, plan stands)

Finding: The DB check (02-02-01) calls store.Open(path) which runs migrate(db) (store .go:39) before the integrity_check executes. On a truly corrupt DB, store.Open fails at Ping() or migrate() — the integrity_check never runs. The check returns FAIL with the open/migrate error, which is the correct outcome (a DB that can't be opened is broken), but the message says "open : " not "integrity_check failed."

The plan's TestDBCheck_Corrupt (RESEARCH:469) is explicitly called "brittle" and made optional. The plan accepts that integrity_check is somewhat redundant with store.Open's own validation.

Evidence:

  • internal/store/store.go:37-41db.Ping() then migrate(db) inside Open.
  • PLAN_v0.3.md:86 — 02-02-01: db, err := store.Open(path).
  • RESEARCH_v0.3.md:455-456 — pitfall table acknowledges this.

Why REJECT: The failure surfaces correctly (FAIL with error message). The integrity_check adds value for the case where the DB opens but has logical corruption (e.g., foreign key violations, orphaned pages) that Ping/migrate don't catch. The plan's approach is acceptable for v0.3. The optional corrupt test is correctly deferred.

Confidence: 0.85


Axis 2 — Scope

F-06 [MEDIUM] — No "delete" event in JSON watch mode (with per-row yield)

Severity: MEDIUM Axis: Scope / Completeness Binding verdict: DEFER (address in execution)

Finding: With the current per-row iter.Seq[*model.Job] design (F-01), the JSON watch mode (01-02-03) emits "init" and "update" events but has no way to emit "delete" events — a job that disappears from the snapshot simply stops being yielded, and the CLI has no tick boundary to detect "this ID was in the previous tick but not this one."

With the F-01 fix (iter.Seq[[]*model.Job], full snapshot per tick), "delete" events become trivially possible: diff the previous snapshot's ID set against the current snapshot's ID set. The plan should add "delete" event semantics to D-046.

Evidence:

  • PLAN_v0.3.md:43 — 01-02-03: only "init" and "update" events.
  • PLAN_v0.3.md:139 — D-046: only "init" and "update".
  • Neither jobs nor nodes are hard-deleted in the current CLI (node leave sets state to left, doesn't delete the row), so "delete" events are not strictly needed for v0.3. But the NodeRepo.Delete method exists and could be used by future code.

Mitigation (DEFER): If F-01 is accepted (slice-per-tick), add "delete" event to D-046 as a natural extension. If F-01 is not accepted, document the no-delete-event limitation explicitly.

Confidence: 0.70


Axis 3 — Testing

F-07 [MEDIUM] — Test timing fragility: 10ms tick + 30ms insert + 80ms cancel

Severity: MEDIUM Axis: Testing Binding verdict: DEFER (address in execution)

Finding: The store-layer tests (01-01-04) use watchInterval=10ms with timing-based assertions: "insert a 2nd job from a goroutine after ~30ms, cancel ctx after ~80ms." Under CI load (especially with -race overhead), 10ms ticks can be missed or delayed. A 10ms ticker pulse is not guaranteed to fire within 10ms under load — the Go runtime scheduler may delay it. If the 2nd job is inserted at 30ms but the 2nd tick fires at 45ms, the test might see the 2nd job in the 3rd tick (at ~55ms) which is still before the 80ms cancel — so it likely passes, but it's fragile.

Evidence:

  • PLAN_v0.3.md:33 — 01-01-04: "after ~30ms", "after ~80ms".
  • time.NewTicker does not guarantee exact timing under load.

Mitigation (DEFER): Use more generous margins (e.g., 50ms insert, 200ms cancel) or a synchronization mechanism (e.g., insert the 2nd job, then poll the collected slice with a 500ms timeout). The test hook (watchInterval) already enables fast tests; the margins just need to be wider. Execution should validate the tests pass reliably under -race in CI before marking Wave 1 complete.

Confidence: 0.75


F-08 [LOW] — -race does not detect goroutine leaks; the plan claims it does

Severity: LOW Axis: Testing Binding verdict: REJECT (concern noted, plan stands)

Finding: The plan (01-01-04 must-have, PLAN:33) says "-race reports no leaks/data races." go test -race detects data races, not goroutine leaks. Goroutine leak detection requires goleak or explicit goroutine-count assertions. The claim is technically incorrect.

However, the actual risk is negligible: Watch does not spawn a goroutine (D-032, inline pull loop). time.NewTicker spawns an internal goroutine, but defer ticker.Stop() terminates it. There is nothing to leak. The no-goroutine-leak constraint (data-engineer persona) is satisfied by design, not by testing.

Evidence:

  • PLAN_v0.3.md:33 — "-race reports no leaks/data races"
  • RESEARCH_v0.3.md:92 — D-032: "No goroutine is spawned by Watch."
  • Go -race detector documentation: detects concurrent access, not leaks.

Why REJECT: The claim is imprecise but the risk is zero by design. Execution may optionally add runtime.NumGoroutine() before/after assertions for belt-and-suspenders, but it's not required.

Confidence: 0.90


Axis 4 — Security

F-09 [MEDIUM] — Doctor network check probes peers using the local server cert as client cert (confirmed valid, but undocumented)

Severity: MEDIUM Axis: Security Binding verdict: DEFER (document in execution)

Finding: The plan (02-02-03, PLAN:88) uses certpaths.ServerCertPath()/ServerKeyPath() as the client cert for the mTLS health probe. I verified this is valid: security/ca.go:255 signs server certs with ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth, x509.ExtKeyUsageClientAuth} — the server cert has both ServerAuth and ClientAuth EKUs, so it can be presented as a client cert. The daemon's RequireAndVerifyClientCert (security/tls_config.go:92) will accept it.

This is correct and feasible. The finding is that this cross-use (server cert as client cert) is not documented in the plan or the security architecture. A security auditor might flag it as "server cert used for client auth — is this intended?"

Evidence:

  • internal/security/ca.go:255ExtKeyUsage: ServerAuth, ClientAuth.
  • internal/security/tls_config.go:92ClientAuth: RequireAndVerifyClientCert.
  • PLAN_v0.3.md:88 — 02-02-03 uses ServerCertPath()/ServerKeyPath().
  • RESEARCH_v0.3.md:261 — D-037: "presents the local node's client cert."

Mitigation (DEFER): Add a code comment in probeHealthz and a note in ARCHITECTURE.md §5 explaining that the local server cert doubles as the client cert for doctor probes (justified by the dual EKU). This is documentation, not a code change.

Confidence: 0.88


Axis 5 — Performance

F-10 [LOW] — 1s poll ticker re-runs full List query every second; no concern but worth noting

Severity: LOW Axis: Performance Binding verdict: REJECT (concern noted, plan stands)

Finding: The 1s ticker (D-019) re-runs SELECT ... FROM jobs ORDER BY created_at DESC every second. For a CLI tool run by a human watching a terminal, this is fine — the query is cheap (single table, no joins, indexed by created_at if an index exists). For an AI agent tailing --watch --json for hours, this is 1 query/second × 3600 = 3600 queries/hour. SQLite handles this trivially in WAL mode (store.go:36).

The cadence is correct for a "top-like" refresh. Faster (e.g., 100ms) would waste CPU; slower (e.g., 5s) would feel sluggish. 1s is the right default.

Evidence:

  • PROJECT.md:116 — D-019: "Poll-based, 1s ticker" (confidence 0.90).
  • internal/store/store.go:36 — WAL mode enabled.

Why REJECT: The cadence is justified. No change needed.

Confidence: 0.92


Axis 6 — Maintainability

F-11 [LOW] — watchInterval package var is mutable global state (test hook)

Severity: LOW Axis: Maintainability Binding verdict: REJECT (concern noted, plan stands)

Finding: D-043 (PLAN:136) uses an unexported package var watchInterval = 1 * time.Second in internal/store, overridable from _test.go. This is mutable global state — if tests run in parallel within the internal/store package and one test sets watchInterval=10ms while another expects 1s, they interfere.

However, Go tests within a single package run sequentially by default unless t.Parallel() is called. I verified no test in internal/store calls t.Parallel() (grep found 0 matches). So the global var is safe as long as no Watch test calls t.Parallel(). The plan should note this constraint.

Evidence:

  • PLAN_v0.3.md:32 — 01-01-03: "unexported package var watchInterval"
  • PLAN_v0.3.md:136 — D-043.
  • grep for t.Parallel() in internal/: 0 matches.

Why REJECT: The approach is pragmatic and safe given sequential test execution. The alternative (a WatchWithInterval constructor or an option pattern) would leak test-only API into production, which D-043 explicitly avoids. Execution should add a comment: "do not call t.Parallel() in Watch tests — they share the watchInterval package var."

Confidence: 0.85


Axis 7 — Completeness

F-12 [MEDIUM] — Plan does not address openDB() being the single chokepoint for dbPath relocation

Severity: MEDIUM Axis: Completeness / Feasibility Binding verdict: DEFER (clarify in execution)

Finding: Task 02-01-01 (PLAN:76) says "Update internal/cli/node.go (and any other internal/cli caller of the old unexported dbPath) to call certpaths.DBPath()." This is imprecise. dbPath() is defined in node.go:22 and called only by openDB() in node.go:31. openDB() is then called by 14 sites across job.go, daemon.go, node_capacity.go, audit.go, node.go. The correct change is:

  1. Add certpaths.DBPath().
  2. Change openDB() body from store.Open(dbPath()) to store.Open(certpaths.DBPath()).
  3. Delete the dbPath() function from node.go.

No other caller needs changing — they all go through openDB(). The plan's "any other internal/cli caller" language suggests a broader scan that isn't needed. This is a clarity issue, not a correctness issue.

Evidence:

  • internal/cli/node.go:22-28dbPath() definition.
  • internal/cli/node.go:30-36openDB() calls dbPath().
  • grep openDB(): 14 call sites, all in internal/cli/.
  • grep dbPath(): only in node.go:31 (inside openDB).

Mitigation (DEFER): Execution should note that openDB() is the single chokepoint — update its body and delete dbPath(). No other file needs changes. The plan's must-have ("internal/cli no longer defines dbPath") is correct.

Confidence: 0.88


Axis 8 — Vertical Slice Integrity

Covered by F-01 (the tick-boundary problem breaks the Wave 1 → Wave 2 vertical slice: Wave 1 produces iter.Seq[*model.Job] which Wave 2's table mode cannot consume correctly). With F-01's fix (iter.Seq[[]*model.Job]), the vertical slice is clean: Wave 1 yields full snapshots, Wave 2 renders them.

Axis 9 — Risk

Highest-risk task: 02-02-05(e) TestNetworkCheck_PeerReachable — integration test requiring CA bootstrap, server cert signing with correct SAN, httptest TLS server with RequireAndVerifyClientCert, node row insert, and mTLS probe. Has the most moving parts and the most assumptions (D-037 ServerName, dual-EKU client cert, httptest HTTP/1.1 vs h2c quirks per integration_test.go:100-126). If D-037 is wrong in production (not in test, since the test uses Name = "localhost" matching the SAN), the network check fails for real deployments but the test passes — a false-positive.

What could go catastrophically wrong: The F-01 tick-boundary issue, if not caught, would cause orca job list --watch (table mode) to either hang (trying to collect a "full snapshot" that never completes) or render incomplete tables (rendering after each row instead of after a full tick). This is a user-visible broken feature shipped as "complete."


Binding Decisions (G-series)

ID Decision Rationale Confidence Verdict
G-001 Change Watch to iter.Seq[[]*model.Job] / iter.Seq[[]*model.Node] (full snapshot per tick) F-01: per-row yield has no tick boundary; table mode needs full snapshot. Slice-per-tick makes D-028 literally true and simplifies both render modes. 0.92 ACCEPT
G-002 Watch must yield immediately on first iteration, then tick F-02: current design waits 1s before first output. Unacceptable UX. 0.95 ACCEPT
G-003 P02 Wave 1 (02-01-01) must complete before P01 Wave 2 (01-02-04) — shared internal/cli/node.go F-03: D-042 file-disjoint claim is false for node.go. 0.90 ACCEPT
G-004 D-037 ServerName = node.Name assumption is deferred; execution must add diagnostic context to handshake-fail errors F-04: assumption is documented (0.80), failure is graceful, proper fix is out of v0.3 scope. 0.78 DEFER
G-005 store.Open runs migrations before integrity_check — acceptable F-05: failure surfaces correctly as FAIL. 0.85 REJECT
G-006 Add "delete" event to JSON watch mode if G-001 is accepted F-06: slice-per-tick makes delete events trivial. 0.70 DEFER
G-007 Widen test timing margins (10ms tick → generous insert/cancel margins) F-07: 10ms ticker under CI load is fragile. 0.75 DEFER
G-008 -race does not detect goroutine leaks — claim is imprecise but risk is zero by design F-08: no goroutine spawned. 0.90 REJECT
G-009 Document dual-EKU (server cert as client cert) in probeHealthz + ARCHITECTURE.md F-09: valid but undocumented. 0.88 DEFER
G-010 1s poll ticker cadence is correct F-10: justified by D-019. 0.92 REJECT
G-011 watchInterval package var is safe (no t.Parallel in store tests) F-11: pragmatic, avoids leaking test API. 0.85 REJECT
G-012 openDB() is the single chokepoint for dbPath relocation — clarify in execution F-12: plan is imprecise but correct. 0.88 DEFER

Escalations

None. All 12 findings are resolved with confidence ≥ 0.60 (either ACCEPT, REJECT, or DEFER). No axis requires human escalation.


Overall Verdict

PROCEED WITH CHANGES

The plan is approved for execution after the 3 ACCEPT binding verdicts (G-001, G-002, G-003) are applied to PLAN_v0.3.md:

  1. G-001: Change Watch element type to iter.Seq[[]*model.Job] / iter.Seq[[]*model.Node] (full snapshot per tick). Update tasks 01-01-01, 01-01-02, 01-02-02, 01-02-03, 01-02-04, 01-01-04, and decisions D-026, D-028, D-046.
  2. G-002: Add "immediate first yield" to tasks 01-01-01, 01-01-02 and must-have criteria + test assertion in 01-01-04.
  3. G-003: Update D-042 and cross-phase notes: P02 Wave 1 (02-01-01) precedes P01 Wave 2 (01-02-04) due to shared internal/cli/node.go.

The 5 DEFER items (G-004, G-006, G-007, G-009, G-012) are execution-time refinements that do not block the plan.

The scope is right-sized (21 tasks across 5 waves, 2 execution phases + 1 review phase). No requirements gaps exist between REQ-022/030/032 and the plan tasks. The no-new-dependencies promise holds. The persona territories are respected. The test strategy is adequate (with the timing-margin note in G-007). The plan does not violate the minimalist pillar.

Confidence in verdict: 0.88