Convoy Manager Specification

Daemon-resident event-driven completion and stranded convoy recovery.

Status: Implementation complete (all stories DONE) Owner: Daemon subsystem Related: convoy-lifecycle.md | convoy-manager.md

1. Problem Statement

Convoys group work but don't drive it. Completion depends on a single poll-based Deacon patrol cycle running gt convoy check. When Deacon is down or slow, convoys stall. Work finishes but the loop never lands:

Create -> Track -> Execute -> Issue closes -> ??? -> Convoy closes

The gap needs three capabilities:

1. Event-driven completion -- react to issue closes, not poll for them.
2. Stranded recovery -- catch convoys missed by event-driven path (crash, restart, stale state).
3. Redundant observation -- multiple agents detect completion so no single failure blocks the loop.

2. Architecture

2.1 ConvoyManager (daemon-resident)

Two goroutines inside gt daemon:

Both goroutines are context-cancellable and coordinate shutdown via sync.WaitGroup.

The event poll opens beads stores for all known rigs (via routes.jsonl) plus the town-level hq store. Parked/docked rigs are skipped during polling. Convoy lookups always use the hq store since convoys are hq-* prefixed. Each store has an independent high-water mark for event IDs.

2.2 Shared Observer (convoy.CheckConvoysForIssue)

Shared function called by the daemon's event poll:

The shared function:

1. Finds convoys tracking the closed issue (SDK GetDependentsWithMetadata on hq store, filtered by tracks type)
2. Skips already-closed convoys
3. Runs gt convoy check <id> for open convoys
4. If convoy remains open after check, feeds next ready issue via gt sling
5. Idempotent -- safe to call multiple times for the same event

2.3 Key Design Decisions

3. Stories

Legend

Quality Gates (for all implementation stories)

These commands must pass for every implementation story in this spec:

• go test ./...
• golangci-lint run

S-01: Event-driven convoy completion detection [DONE]

Description: When an issue closes, the daemon detects the close event via SDK polling and triggers convoy completion checks.

Implementation: ConvoyManager.runEventPoll + pollEvents in convoy_manager.go

Acceptance criteria:

• Polls GetAllEventsSince on a 5-second interval
• Detects EventClosed events
• Detects EventStatusChanged where new_value == "closed"
• Skips non-close events (close path not triggered)
• Skips events with empty issue_id
• Calls convoy.CheckConvoysForIssue for each detected close
• High-water mark advances monotonically (no duplicate processing)
• Error on GetAllEventsSince logs and retries next interval
• Nil store disables event polling (returns immediately)
• Context cancellation exits cleanly

Tests:

• TestEventPoll_DetectsCloseEvents -- real beads store, creates+closes issue, verifies log
• TestEventPoll_SkipsNonCloseEvents -- create-only, no close detection

Corrective note: "Zero side effects" negative assertions have been added via TestEventPoll_SkipsNonCloseEvents_NegativeAssertion (verifies no subprocess calls, no close detection, and no convoy activity for non-close events). Originally tracked in S-11; now resolved.

S-02: Periodic stranded convoy scan [DONE]

Description: Every 30 seconds, scan for stranded convoys (unassigned work or empty). Feed ready work or auto-close empties.

Implementation: ConvoyManager.runStrandedScan + scan + findStranded + feedFirstReady + closeEmptyConvoy in convoy_manager.go

Acceptance criteria:

• Runs immediately on start, then every scanInterval
• Calls gt convoy stranded --json and parses output
• For convoys with ready_count > 0: dispatches first ready issue via gt sling <id> <rig> --no-boot
• For convoys with ready_count == 0: runs gt convoy check <id> to auto-close
• Resolves issue prefix to rig name via beads.ExtractPrefix + beads.GetRigNameForPrefix
• Skips issues with unknown prefix (logged)
• Skips issues with unknown rig (logged)
• Continues to next convoy after dispatch failure
• Context cancellation exits mid-iteration
• Scan interval defaults to 30s when 0 or negative

Tests:

• TestScanStranded_FeedsReadyIssues -- mock gt, verify sling log file
• TestScanStranded_ClosesEmptyConvoys -- mock gt, verify check log file
• TestScanStranded_NoStrandedConvoys -- empty list: asserts sling log absent, check log absent, no convoy activity in logs
• TestScanStranded_DispatchFailure -- first sling fails, scan continues
• TestConvoyManager_ScanInterval_Configurable -- 0 -> default, custom preserved
• TestStrandedConvoyInfo_JSONParsing -- JSON round-trip

S-03: Shared convoy observer function [DONE]

Description: A shared function for checking convoy completion and feeding the next ready issue, callable from any observer.

Implementation: CheckConvoysForIssue + feedNextReadyIssue in convoy/operations.go

Acceptance criteria:

• Finds tracking convoys via GetDependentsWithMetadata filtered by tracks type
• Filters out blocks dependencies
• Skips already-closed convoys
• Runs gt convoy check <id> for open convoys
• After check, if still open: feeds next ready issue via gt sling
• Ready = open status + no assignee
• Feeds one issue at a time (first match)
• Handles external:prefix:id wrapper format via extractIssueID
• Refreshes issue status via GetIssuesByIDs for cross-rig accuracy
• Falls back to dependency metadata if fresh status unavailable
• Nil store returns immediately
• Nil logger replaced with no-op (no panic)
• Idempotent (calling multiple times for same issue is safe)
• Returns list of checked convoy IDs

Tests:

• TestGetTrackingConvoys_FiltersByTracksType -- real store, blocks filtered
• TestIsConvoyClosed_ReturnsCorrectStatus -- real store, open vs closed
• TestExtractIssueID -- all wrapper variants
• TestFeedNextReadyIssue_SkipsNonOpenIssues -- filtering logic
• TestFeedNextReadyIssue_FindsReadyIssue -- first match
• TestCheckConvoysForIssue_NilStore -- returns nil
• TestCheckConvoysForIssue_NilLogger -- no panic
• TestCheckConvoysForIssueWithAutoStore_NoStore -- non-existent path, nil

S-04: Witness integration [REMOVED]

Description: Witness convoy observer removed. The daemon's multi-rig event poll (watching all rig databases + hq) provides event-driven coverage for close events from any rig. The stranded scan (30s) provides backup. The witness's core job is polecat lifecycle management -- convoy tracking is orthogonal.

History: Originally had 6 CheckConvoysForIssueWithAutoStore call sites in handlers.go (1 post-merge, 5 zombie cleanup paths). All were pure side-effect notification hooks. Removed when daemon gained multi-rig event polling.

S-05: Refinery integration [REMOVED]

Description: Refinery convoy observer removed. The daemon event poll (5s) and witness observer provide sufficient coverage. The refinery observer was silently broken (S-17: wrong root path) for the entire feature lifetime with no visible impact, confirming the other two observers are sufficient. Since beads unavailability disables the entire town (not just convoy checks), the "degraded mode" justification for a third observer does not hold.

History: Originally called CheckConvoysForIssueWithAutoStore after merge. S-17 found it passed rig path instead of town root. S-18 fixed it. Subsequently removed as unnecessary redundancy.

S-06: Daemon lifecycle integration [DONE]

Description: ConvoyManager starts and stops cleanly with the daemon.

Implementation: Integrated in daemon.go Run() and shutdown() methods.

Acceptance criteria:

• Opens beads store at daemon startup (nil if unavailable)
• Passes resolved gtPath/bdPath to ConvoyManager
• Passes logger.Printf for daemon log integration
• Starts after feed curator
• Stops before beads store is closed (correct shutdown order)
• Stop completes within bounded time (no hang)

Tests:

• TestDaemon_StartsManagerAndScanner -- start + stop with mock binaries
• TestDaemon_StopsManagerAndScanner -- stop completes within 5s

S-07: Convoy fields in MR beads [DONE]

Description: Merge-request beads carry convoy tracking fields for priority scoring and starvation prevention.

Implementation: ConvoyID and ConvoyCreatedAt in MRFields struct in beads/fields.go

Acceptance criteria:

• convoy_id field parsed and formatted
• convoy_created_at field parsed and formatted
• Supports underscore, hyphen, and camelCase key variants
• Used by refinery for merge queue priority scoring

S-08: ConvoyManager lifecycle safety [DONE]

Description: Start/Stop are safe under edge conditions.

Acceptance criteria:

• Stop() is idempotent (double-call does not deadlock)
• Stop() before Start() returns immediately
• Start() is guarded against double-call (atomic.Bool with CompareAndSwap at convoy_manager.go:50-51,80-83)

Tests:

• TestManagerLifecycle_StartStop -- basic start + stop
• TestConvoyManager_DoubleStop_Idempotent -- double stop
• TestStart_DoubleCall_Guarded -- second Start() is no-op, warning logged

S-09: Subprocess context cancellation [DONE]

Description: All subprocess calls in ConvoyManager and observer propagate context cancellation so daemon shutdown is not blocked by hanging subprocesses.

Implementation: All exec.Command calls replaced with exec.CommandContext. Process group killing via setProcessGroup + syscall.Kill(-pid, SIGKILL) prevents orphaned child processes.

Acceptance criteria:

• All exec.Command calls in ConvoyManager use exec.CommandContext(m.ctx, ...) (convoy_manager.go:200,241,257)
• All exec.Command calls in operations.go accept and use a context parameter
• Daemon shutdown completes within bounded time even if gt subprocess hangs (convoy_manager_integration_test.go:154-206)
• Killed subprocesses do not leave orphaned child processes (convoy_manager.go, operations.go)

S-10: Resolved binary paths in operations.go [DONE]

Description: Observer subprocess calls use resolved binary paths instead of bare "gt" to avoid PATH-dependent behavior drift.

Implementation: CheckConvoysForIssue resolves via exec.LookPath("gt") with fallback to bare "gt". Threads gtPath parameter to runConvoyCheck and dispatchIssue in operations.go.

Acceptance criteria:

• runConvoyCheck and dispatchIssue accept a gtPath parameter
• CheckConvoysForIssue threads resolved path
• All callers updated: daemon (resolved m.gtPath)
• Fallback to bare "gt" if resolution fails

S-11: Test gap -- priority 1 (high blast-radius invariants) [DONE]

Description: Filled testing gaps for core invariants identified in the test plan analysis.

Tests added:

Acceptance criteria:

• All 5 tests passing
• Each test has explicit assertions (no assertion-free "no panic" tests)

S-12: Test gap -- priority 2 (error paths) [DONE]

Description: Covered error paths that previously had no test coverage.

Tests added:

Acceptance criteria:

• All 4 tests passing
• Error messages are verified in log assertions

S-13: Test gap -- priority 3 (lifecycle edge cases) [DONE]

Description: Covered lifecycle edge cases identified in the test plan.

Tests added:

Acceptance criteria:

• All 3 tests passing

S-14: Test infrastructure improvements [DONE]

Description: Improved test harness quality and reduced duplication.

Items:

Acceptance criteria:

• Shared mock builder exists and is used by >= 3 scan tests (5 tests use it)
• All mock scripts write to call log files (negative tests can assert empty)
• No assertion-free tests remain in convoy_manager_test.go

S-15: Documentation update [DONE]

Description: Update stale documentation to reflect current implementation.

Items:

Acceptance criteria:

• Mermaid diagram shows SDK polling architecture
• Text accurately describes poll-retry semantics
• Testing.md reflects current test inventory
• Lifecycle observer and manual-close sections match implementation
• Broken links in lifecycle doc are fixed
• Spec file-map counts and command list match current source

Completion note: Completed in this review pass; remaining ambiguity about refinery root-path semantics is tracked separately in S-17.

S-16: Corrective follow-up for DONE stories [DONE]

Description: Add explicit corrective tasks for inaccuracies discovered in stories marked DONE, without changing the implementation status itself.

Rationale: DONE stories can still contain stale supporting narrative or inventory details after nearby refactors. Corrections are tracked explicitly to avoid silently editing historical delivery claims.

Scope:

• S-01: clarify that non-close event "zero side effects" is currently partial until negative subprocess assertions are added (see S-11)
• S-04: replace brittle line-number call-site references with symbol/section anchors in handlers.go
• S-05: validate/clarify refinery townRoot vs rig-path argument assumptions for CheckConvoysForIssueWithAutoStore

Acceptance criteria:

• Corrective notes are added to affected DONE stories without downgrading status
• S-04 call-site references no longer depend on fixed line numbers
• S-05 includes an explicit note on root-path assumptions and validation status

Status note: All corrective notes updated. S-01 negative assertion test now exists (resolved). S-04 call sites already use semantic descriptions. S-05 note updated to reflect S-17 verification findings (incorrect path, fix in S-18).

S-17: Refinery observer root-path verification [DONE]

Description: Verify whether refinery passing e.rig.Path into CheckConvoysForIssueWithAutoStore is correct for convoy visibility.

Context:

• Observer helper opens beads store under <townRoot>/.beads/dolt
• Refinery currently passes rig path, not explicitly town root

Findings:

The current behavior is incorrect. e.rig.Path is a rig-level path (<townRoot>/<rigName>), set in rig/manager.go as filepath.Join(m.townRoot, name). OpenStoreForTown constructs <path>/.beads/dolt, so the refinery opens <townRoot>/<rigName>/.beads/dolt instead of <townRoot>/.beads/dolt.

The rig-level .beads/ directory typically contains either a redirect file (pointing to mayor/rig/.beads) or rig-scoped metadata -- not the town-level Dolt database that holds convoy data. As a result, beadsdk.Open either fails (no dolt/ directory) or opens a rig-scoped store that does not contain convoy tracking dependencies. In both cases CheckConvoysForIssueWithAutoStore silently returns nil, effectively disabling convoy checks from the refinery observer.

Other observers handle this correctly:

• Witness: resolves town root via workspace.Find(workDir) before calling
• Daemon: passes d.config.TownRoot directly

Fix required: Resolve town root from e.rig.Path using workspace.Find before passing to CheckConvoysForIssueWithAutoStore, matching the witness pattern. See S-18 for implementation.

Acceptance criteria:

• Behavioral expectation is documented (town root vs rig root)
• If current behavior is correct, add code comment/spec note explaining why
• If incorrect, create implementation follow-up story and cross-link here -> S-18

S-18: Fix refinery convoy observer town-root path [DONE]

Description: Fixed the refinery's CheckConvoysForIssueWithAutoStore call to pass the town root instead of the rig path, so convoy checks actually open the correct beads store.

Context: Identified by S-17 verification. The refinery was passing e.rig.Path (<townRoot>/<rigName>) but the function expects the town root. This silently disabled convoy observation from the refinery.

Implementation: engineer.go now resolves town root via workspace.Find(e.rig.Path) before calling CheckConvoysForIssueWithAutoStore, matching the witness pattern.

Acceptance criteria:

• Refinery resolves town root via workspace.Find(e.rig.Path) before calling CheckConvoysForIssueWithAutoStore
• Pattern matches witness implementation (graceful fallback if town root not found)
• Import workspace package added to engineer.go
• BUG(S-17) comment in engineer.go removed after fix

4. Critical Invariants

5. Failure Modes

Event Poll

Stranded Scan

Lifecycle

6. File Map

Core Implementation

Integration Points

Tests

Design Documents

7. Review Findings -> Story Mapping

8. Non-Goals (This Spec)

These are documented in convoy-lifecycle.md as future work but are not in scope for this spec:

• Convoy owner/requester field and targeted notifications (P2 in lifecycle doc)
• Convoy timeout/SLA (due_at field, overdue surfacing) (P3 in lifecycle doc)
• Convoy reopen command (implicit via add, explicit command deferred)
• Test clock injection for ConvoyManager (P3 -- useful but not blocking)

Test Harness & Remaining Gaps

Harness Scorecard

Test Clock Injection (P3)

Problem: ConvoyManager uses time.Ticker with 30s default. Testing "runs at interval" requires waiting or injecting a clock.

Proposal: Add clock field to ConvoyManager (interface with NewTicker(d)) defaulting to real time. Tests inject fake clock with immediate tick.

Compound Value: All periodic daemon components benefit.

Status: Not implemented. Tests use long intervals (10min) to prevent ticker firing during test.

Remaining Test Gaps

• Add TestProcessLine_EmptyIssueID (close event with empty issue_id)
• Expand integration test coverage for multi-rig event polling