Skip to content

Commit 4f0508a

Browse files
dratnerclaude
dratner
and
claude
authored
Add durable asks and incidents for PM memory and idle detection (#205)
* Add durable asks and incidents for PM memory and architect idle detection (#200, #201) Phase 1 of the durable asks/incidents system. PM persists a singular UserAsk that survives AWAIT_USER re-entry, and architect opens/closes Incident objects for story_blocked, clarification_needed, and system_idle conditions. Hash-based change detection prevents duplicate summary injection on WORKING re-entry. Schema v22 adds persistence columns for both agents. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Fix five reviewer P1s: nil map, data race, nondeterministic hash, debounce, bare assert - Initialize PM openIncidents map in production constructor (panic on first incident) - Add incidentsMu sync.Mutex to architect Driver; all incident map access is now thread-safe between requeue worker goroutine and main FSM loop - Sort incident IDs before building pending summary so hash is deterministic - Track monitoringIdleSince from when idle predicate first becomes true, not from MONITORING entry — prevents false system_idle after long monitoring periods - Replace bare type assertion with utils.SafeAssert per project convention Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Add incident_action tool with resume action (Phase 1.5) Closes the recovery loop: PM detects incident → tells user → user says retry → PM calls incident_action(resume) → architect recovers work. - incident_action tool (PM-facing) with resume as sole action - Architect handler routes by incident kind with state-aware recovery: system_idle: orphan sweep + re-dispatch story_blocked: RetryFailedStory (failed) or release holds (on_hold) clarification_needed: release held stories + re-dispatch - Recover-first pattern: incident stays open if recovery fails - Failure-group resolution: all incidents sharing a FailureID close together - incident_action_result RESPONSE payload delivers typed success/failure to PM - AllowedActions validation on both PM and architect sides - Queue methods: RetryFailedStory, RequeueOrphanedDispatched - Unit tests for payloads, queue operations, and protocol routing Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Fix orphan sweep using wrong ownership source and on-hold resume ignoring empty releases Orphan sweep: RequeueOrphanedDispatched now uses the dispatcher's lease table (the actual source of truth) instead of QueuedStory.AssignedAgent which is never populated during live dispatch. Adds GetLeasedStoryIDs() to the dispatcher. On-hold resume: Both resumeStoryBlocked (StatusOnHold) and resumeClarification now check len(released) > 0 before resolving incidents, matching the existing repair_complete path. Empty releases report failure and keep the incident open. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Add full action semantics for incident_action tool (Phase 2) Implement try_again, skip, and change_request actions alongside existing resume. Add StatusSkipped terminal state with reverse-dependency gating, kind+action routing in architect handlers, and DB migration v23. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Fix four Phase 2 reviewer P1s: budget reset, sessions, suppression, AllowedActions - Reset per-class retry budgets (not just AttemptCount) in change_request - Add 'skipped' to resumable-session terminal set in sessions.go - Remove dispatch unsuppression from skip/change_request (story-local actions) - Only advertise change_request for story/attempt-scoped failures Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Add belt-and-suspenders guard for change_request on held stories Creation-time: also check story status (not just scope) before offering change_request — prerequisite failures hold the trigger story before opening the incident. Handler-time: reject change_request when siblings are held by the same failure ID. AllowedActions is metadata, not a trust boundary; persisted incidents or future refactors can drift. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Fix Copilot PR findings: map iteration safety, resolution docs, missing-incident guard - Snapshot map keys before deleting in resolveAllIncidents and resolveIncidentsByFailureID to avoid delete-during-range ambiguity - Update Resolution field comment with new values from Phase 2 - Fail fast in PM when incident_id is not in openIncidents map Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
1 parent dab9755 commit 4f0508a

35 files changed

Lines changed: 3232 additions & 92 deletions

‎docs/DURABLE_ASKS_AND_INCIDENTS.md‎

Lines changed: 219 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,219 @@
1+
# Durable Asks & Incidents
2+
3+
*Design document for fixes to GitHub issues #200 (PM silent in AWAIT_USER) and #201 (architect silent in MONITORING).*
4+
5+
## Problem Statement
6+
7+
Maestro goes silent for hours when work stalls. Two root causes:
8+
9+
1. **PM memory loss (#200):** PM sends ACTION REQUIRED via `chat_ask_user`, transitions to AWAIT_USER, and waits indefinitely. When the user eventually responds, PM has no structured memory of what's pending, so it gives stale or incorrect status.
10+
11+
2. **Architect silence (#201):** When all coders die (watchdog kills) or stall, the architect loops in MONITORING every 30s checking for messages that will never arrive. No escalation to PM or user.
12+
13+
## Design Principle
14+
15+
"Waiting on user" should be a durable product-state object, not a timer problem. Timers compensate for missing memory; durable asks and incidents solve the actual bug.
16+
17+
Timers are intentionally not part of this design. Edge-triggered communication (one notification per incident open/close) replaces polling and reminders.
18+
19+
---
20+
21+
## Data Model
22+
23+
### Incident
24+
25+
An **incident** is an architect-owned operational blocker. The architect is the sole authority for opening and closing incidents.
26+
27+
```go
28+
type Incident struct {
29+
ID string // incident-{kind}-{storyID|system}-{failureID|timestamp}
30+
Kind IncidentKind // story_blocked | clarification_needed | system_idle
31+
Scope string // "story" | "system"
32+
StoryID string // set for story-scoped incidents
33+
FailureID string // cross-reference to failure record
34+
Title string
35+
Summary string
36+
AffectedStoryIDs []string // for system_idle: which stories are stuck
37+
AllowedActions []IncidentAction // advisory in Phase 1
38+
Blocking bool
39+
OpenedAt string
40+
ResolvedAt string
41+
Resolution string // how it was resolved
42+
}
43+
```
44+
45+
**Incident kinds:**
46+
- `story_blocked` — Story abandoned after exhausting retries. Scoped to a single story.
47+
- `clarification_needed` — Failure requires human input (credentials, unclear requirements). Scoped to a story.
48+
- `system_idle` — No active coders making progress but pending work exists. Scoped to system.
49+
50+
### UserAsk
51+
52+
A **UserAsk** is a PM-owned conversational obligation. Created when PM calls `chat_ask_user`, resolved when the user responds.
53+
54+
```go
55+
type UserAsk struct {
56+
ID string // ask-{kind}-{timestamp}
57+
Prompt string
58+
Kind string // "interview_question" | "clarification" | "decision_required"
59+
RelatedIncidentID string // optional link to triggering incident
60+
OpenedAt string
61+
ResolvedAt string
62+
}
63+
```
64+
65+
**Constraint:** At most one active ask at a time. A new ask implicitly supersedes any prior unresolved ask.
66+
67+
### IncidentAction
68+
69+
Recovery actions a user can take. Advisory metadata in Phase 1.
70+
71+
- `try_again` — Retry the failed story with same or edited content
72+
- `change_request` — Modify story requirements before retry
73+
- `skip` — Abandon the story permanently
74+
- `resume` — Signal that the blocking condition has been resolved externally
75+
76+
---
77+
78+
## Ownership Rules
79+
80+
These rules are the heart of the design:
81+
82+
1. **Asks are PM-owned.** PM creates asks; user replies resolve asks.
83+
2. **Incidents are architect-owned.** Architect opens incidents; architect-side recovery closes incidents.
84+
3. **User replies do NOT automatically resolve incidents.** A user chatting with PM may provide information that PM relays to the architect, but the incident closes only when the architect observes recovery (story requeued, hold released, coder becomes active).
85+
4. **Architect does NOT create or resolve asks.** The PM decides when to ask the user for input.
86+
87+
---
88+
89+
## Lifecycle Rules
90+
91+
### Incident Lifecycle
92+
93+
**Opening triggers:**
94+
| Kind | Trigger | Location |
95+
|------|---------|----------|
96+
| `story_blocked` | Story abandoned (exhausted retries, `!willRetry`) | `notifyPMOfBlockedStory` in `request.go` |
97+
| `clarification_needed` | Failure requires human input | `notifyPMOfClarificationNeeded` in `request.go` |
98+
| `system_idle` | No active coders + pending work + idle > 60s | `checkAndOpenIdleIncident` in `monitoring.go` |
99+
100+
**Closing triggers:**
101+
| Kind | Trigger | Resolution value |
102+
|------|---------|-----------------|
103+
| `story_blocked` | Story status no longer `failed`/`on_hold` | `"story_requeued"` |
104+
| `clarification_needed` | Hold released via `repair_complete` | `"manual"` |
105+
| `system_idle` | Idle predicate false (coder becomes active) | `"work_resumed"` |
106+
| Any | All stories terminal | `"all_terminal"` |
107+
108+
**Idle detection specifics:**
109+
- **Opening** uses a 60s debounce guard (2 heartbeats) to avoid false positives during dispatch transitions.
110+
- **Closing** is predicate-based only — no timing guard. Once work resumes, close immediately.
111+
- The `monitoringIdleSince` timestamp is a debounce guard, not durable business state. It is not persisted.
112+
113+
### UserAsk Lifecycle
114+
115+
**Opening:** PM calls `chat_ask_user` → `SignalAwaitUser` handler creates `UserAsk`.
116+
117+
**Closing:** User sends a chat message while PM is in AWAIT_USER → current ask resolved.
118+
119+
**Supersession:** If PM issues a new `chat_ask_user` before the prior ask is resolved, the new ask replaces the old one. Only one ask can be active.
120+
121+
---
122+
123+
## Communication Pattern
124+
125+
Incidents use edge-triggered, not level-triggered, communication:
126+
127+
1. Architect opens incident → sends `incident_opened` payload to PM (once)
128+
2. PM stores incident in `openIncidents` and injects context into LLM
129+
3. Architect closes incident → sends `incident_resolved` payload to PM (once)
130+
4. PM removes incident from `openIncidents`
131+
132+
No polling, no reminders, no timers. The PM's `maybeInjectPendingItemsSummary()` re-injects the summary only when the digest changes (hash-based deduplication), preventing context bloat from `handleWorking()` re-entry loops.
133+
134+
---
135+
136+
## Persistence
137+
138+
Both asks and incidents must survive process restart:
139+
140+
- **PM:** `currentAsk` and `openIncidents` serialized as JSON in `PMState` (explicit DB columns via schema migration).
141+
- **Architect:** `openIncidents` serialized as JSON in `ArchitectState` (explicit DB column).
142+
- **State data mirroring:** Runtime state is also mirrored into state data keys (`StateKeyCurrentAsk`, `StateKeyOpenIncidents`) for FSM visibility through existing inspection tools.
143+
144+
---
145+
146+
## Phase Boundaries
147+
148+
### Phase 1 (this implementation)
149+
150+
- Durable `UserAsk` and `Incident` models
151+
- `incident_opened` / `incident_resolved` payload kinds
152+
- Architect incident lifecycle (open on story_blocked/clarification, system_idle detection, reconciliation)
153+
- PM durable ask (singular, created on `chat_ask_user`, resolved on user reply)
154+
- PM mirrored incidents (stored on `incident_opened`, removed on `incident_resolved`)
155+
- Pending items summary injection with hash-based change detection
156+
- Persistence via explicit JSON columns (schema migration v22)
157+
- `AllowedActions` populated as advisory metadata
158+
- User acts through natural language via PM; PM routes to architect as needed
159+
160+
### Phase 1.5 — `incident_action` tool with `resume`
161+
162+
Adds a structured `incident_action` PM tool with `resume` as the only supported action. Closes the loop: PM detects incident → tells user → user says retry → PM calls tool → architect recovers.
163+
164+
**Routing:** `PayloadKindIncidentAction` maps to `RequestKindExecution` (no new request kind).
165+
166+
**Resume semantics per incident kind:**
167+
168+
| Kind | Story Status | Recovery Action |
169+
|------|-------------|-----------------|
170+
| `system_idle` | N/A | Sweep orphaned dispatched stories (no live coder), resume dispatch |
171+
| `story_blocked` | `StatusFailed` | `RetryFailedStory` — reset to pending for fresh attempt (preserves attempt count) |
172+
| `story_blocked` | `StatusOnHold` | Release held stories by failure ID (same as repair_complete path) |
173+
| `clarification_needed` | `StatusOnHold` | Release held stories by failure ID, resume dispatch |
174+
175+
**Key rules:**
176+
- Recovery executes *before* incident resolution. If recovery fails, incident stays open and PM gets a failure result.
177+
- When resuming by failure ID, *all* related incidents for that failure are resolved (not just the clicked one). A single prerequisite failure can open both `story_blocked` and `clarification_needed`.
178+
- Both PM (tool side) and architect (handler side) validate `resume` ∈ `AllowedActions`.
179+
- `incident_action_result` RESPONSE payload delivers typed success/failure back to PM.
180+
- Orphan recovery in `system_idle`: dispatched stories whose assigned coder is no longer active are requeued to pending before re-dispatching.
181+
182+
### Phase 2: Full Action Semantics
183+
184+
Implemented. Four actions on the `incident_action` tool:
185+
186+
| Action | Description |
187+
|---|---|
188+
| `resume` | External blocker resolved — release held stories and re-dispatch |
189+
| `try_again` | Identical to `resume` (PM picks the word that fits the context) |
190+
| `skip` | Intentionally abandon a story — marks it `StatusSkipped` (new terminal state) |
191+
| `change_request` | Append user instructions to story content, reset retry budget, requeue |
192+
193+
**Action × incident-kind matrix:**
194+
195+
| Action | `system_idle` | `story_blocked` | `clarification_needed` |
196+
|---|---|---|---|
197+
| `resume` / `try_again` | orphan sweep + re-dispatch | Failed→retry; OnHold→release holds | release holds + re-dispatch |
198+
| `skip` | N/A | mark story skipped (no sibling release) | N/A |
199+
| `change_request` | N/A | StatusFailed only; append + reset + retry | N/A |
200+
201+
**Key design decisions:**
202+
203+
- **`StatusSkipped`** is a new terminal status distinct from `StatusFailed`. Terminal guards, `AllStoriesTerminal()`, deadlock detection, session summaries, and PM notifications all include it. `AllStoriesCompleted()` does *not* — skipped ≠ completed. Dependencies on a skipped story are never satisfied.
204+
- **Reverse-dependency gating:** `SkipStory` rejects if non-terminal stories depend on the target (they would become permanently unstartable).
205+
- **Skip does not release siblings:** Failure-group holds represent shared blockers. Skipping one story doesn't resolve the shared condition, so siblings stay on hold. Only `resume` releases failure groups.
206+
- **`change_request` restricted to `StatusFailed`:** For group-scoped incidents (`StatusOnHold`, `clarification_needed`), annotating one story and resuming the group would leave siblings without the annotation. `change_request` only operates on story-local (failed) incidents.
207+
- **AttemptCount reset:** `change_request` resets attempts to 0 — a user-supplied change is a new direction and deserves a fresh retry budget.
208+
- **Content annotation:** Appends `"## Change Request (User)"` section to story content, consistent with existing `"## Implementation Notes (Auto-generated)"` and `"## Failure Context (Auto-generated)"` patterns.
209+
210+
---
211+
212+
## Relationship to Existing Notifications
213+
214+
Phase 1 layers incidents on top of existing notifications. The existing `story_blocked` and `clarification_request` payloads continue to flow and inject immediate LLM context. The new `incident_opened` messages arrive separately and add durable state. Both coexist:
215+
216+
- **Existing notification** → immediate context injection into PM's LLM conversation
217+
- **New incident** → durable state that survives resume and provides accurate status on re-entry
218+
219+
This avoids a risky migration while still solving the core problem. Phase 2+ may consolidate.

‎pkg/architect/STATES.md‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -107,6 +107,36 @@ Each type uses appropriate review logic and tools for its domain.
107107

108108
---
109109

110+
## Durable Incident Lifecycle
111+
112+
The architect maintains a map of open `Incident` objects — architect-owned operational blockers. See `docs/DURABLE_ASKS_AND_INCIDENTS.md` for the full design.
113+
114+
### Incident Opening Triggers
115+
| Kind | Trigger | Location |
116+
|------|---------|----------|
117+
| `story_blocked` | Story abandoned (exhausted retries) | `notifyPMOfBlockedStory` |
118+
| `clarification_needed` | Failure requires human input | `notifyPMOfClarificationNeeded` |
119+
| `system_idle` | No active coders + pending work + idle > 60s | `checkAndOpenIdleIncident` in MONITORING |
120+
121+
### Incident Closing Triggers
122+
| Kind | Trigger | Resolution |
123+
|------|---------|------------|
124+
| `story_blocked` | Story no longer failed/on_hold | `"story_requeued"` |
125+
| `clarification_needed` | Hold released via `repair_complete` | `"manual"` |
126+
| `system_idle` | Idle predicate false (coder becomes active) | `"work_resumed"` |
127+
| Any | All stories terminal | `"all_terminal"` |
128+
129+
### System Idle Detection (MONITORING state)
130+
Each monitoring tick runs `reconcileOpenIncidents()` then `checkAndOpenIdleIncident()`:
131+
- **Opening** uses a 60s debounce guard to avoid false positives during dispatch transitions
132+
- **Closing** is predicate-based only — no timing guard; once work resumes, close immediately
133+
- `monitoringIdleSince` is a non-persistent debounce timestamp, set when the idle predicate first becomes true, reset when predicate becomes false or a coder message arrives
134+
135+
### Ownership Rule
136+
Architect opens and closes incidents. User replies do NOT automatically resolve incidents — only PM-owned asks. The PM receives `incident_opened`/`incident_resolved` payloads and mirrors the state.
137+
138+
---
139+
110140
## Error handling
111141

112142
* The agent enters **ERROR** when:

‎pkg/architect/dispatching.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -277,7 +277,7 @@ func (d *Driver) detectDeadlock() bool {
277277
var unreleasedStories []*QueuedStory
278278
for _, story := range allStories {
279279
status := story.GetStatus()
280-
if status != StatusDone && status != StatusFailed && status != StatusOnHold {
280+
if status != StatusDone && status != StatusFailed && status != StatusSkipped && status != StatusOnHold {
281281
unreleasedStories = append(unreleasedStories, story)
282282
}
283283
}

‎pkg/architect/driver.go‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -115,6 +115,9 @@ type Driver struct {
115115
scopeWidener *ScopeWidener // Tracks failure recurrence for scope auto-escalation
116116
workDir string // Workspace directory
117117
reviewStreaks map[string]map[string]int // Per-coder, per-review-type consecutive NEEDS_CHANGES count
118+
incidentsMu sync.Mutex // Protects openIncidents from concurrent access
119+
openIncidents map[string]*proto.Incident // Durable incidents keyed by incident ID
120+
monitoringIdleSince time.Time // Debounce guard for system_idle detection (not persisted)
118121
pmAllCompleteNotified bool // Guard: PM "all stories complete" notification already sent
119122
pmAllTerminalNotified bool // Guard: PM "all stories terminal" (with failures) notification already sent
120123
}
@@ -190,6 +193,7 @@ func NewDriver(architectID, _ string, dispatcher *dispatch.Dispatcher, workDir s
190193
BaseStateMachine: sm,
191194
agentContexts: make(map[string]*contextmgr.ContextManager), // Initialize context map
192195
reviewStreaks: make(map[string]map[string]int), // Initialize streak tracking
196+
openIncidents: make(map[string]*proto.Incident), // Initialize incident tracking
193197
toolLoop: nil, // Set via SetLLMClient
194198
renderer: renderer,
195199
workDir: workDir,
@@ -916,7 +920,7 @@ func (d *Driver) processRequeueRequests(ctx context.Context) {
916920

917921
// Check if all stories are now terminal — if so, notify PM and finish
918922
if d.queue.AllStoriesTerminal() {
919-
d.logger.Info("📋 All stories are terminal (done or failed). Transitioning to DONE.")
923+
d.logger.Info("📋 All stories are terminal (done, failed, or skipped). Transitioning to DONE.")
920924
if err := d.notifyPMAllStoriesTerminal(ctx); err != nil {
921925
d.logger.Warn("⚠️ Failed to notify PM of all stories terminal: %v", err)
922926
}

0 commit comments

Comments
 (0)