feat: surface playbook STATUS.json in Auto Run UI#732
feat: surface playbook STATUS.json in Auto Run UI#732pedramamini wants to merge 3 commits intomainfrom
Conversation
- Standardized multi-phase auto-run docs into one flat, dated subdirectory 📁 - Explicitly banned nested project/feature folder structures for phase outputs 🚫 - Improved guidance for clean lexicographic sorting with zero-padded phases 🔢 - Made it easier to add entire effort folders to auto-run at once ➕ - Clarified organization rules so related phase documents stay tightly grouped 🧭
) Playbooks can now write a .maestro/STATUS.json file during auto-run to surface rich execution context (feature, phase, test results, summary) in the Maestro UI progress panel. - Add PlaybookStatus type contract in shared/types - Watch .maestro/STATUS.json via chokidar during batch runs - Dispatch status updates through batch reducer - Display feature, phase, test results, and summary in RightPanel
📝 WalkthroughWalkthroughThis PR implements real-time playbook status tracking by adding a Changes
Sequence DiagramsequenceDiagram
participant Renderer as Renderer Process
participant Preload as Preload API
participant Main as Main Process
participant FS as File System
participant State as Batch State
Renderer->>Preload: watchStatus(projectPath)
Preload->>Main: ipcRenderer.invoke('autorun:watchStatus')
Main->>FS: Check/create .maestro directory
Main->>FS: Start fs.watch() on STATUS.json
Main->>FS: Read initial STATUS.json
Main->>Preload: Return initial status
Preload->>Renderer: Promise resolves with status
Renderer->>State: Dispatch UPDATE_PLAYBOOK_STATUS
FS-->>Main: STATUS.json modified
Main->>FS: Read updated STATUS.json
Main->>Preload: ipcRenderer.send('autorun:statusChanged')
Preload->>Renderer: Trigger onStatusChanged callback
Renderer->>State: Dispatch UPDATE_PLAYBOOK_STATUS
Renderer->>Renderer: Re-render UI with new status
Renderer->>Preload: unwatchStatus(projectPath)
Preload->>Main: ipcRenderer.invoke('autorun:unwatchStatus')
Main->>FS: Close watcher for project
Main->>Preload: Resolve
Preload->>Renderer: Promise resolves
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
docs/releases.md (1)
174-174: Consider correcting "Github" to "GitHub".The official name of the platform is "GitHub" with a capital H.
📝 Proposed fix
-🌳 Github Worktree support was added. Any agent bound to a Git repository has the option to enable worktrees, each of which show up as a sub-agent with their own write-lock and Auto Run capability. Now you can truly develop in parallel on the same project and issue PRs when you're ready, all from within Maestro. Huge improvement, major thanks to `@petersilberman`. +🌳 GitHub Worktree support was added. Any agent bound to a Git repository has the option to enable worktrees, each of which show up as a sub-agent with their own write-lock and Auto Run capability. Now you can truly develop in parallel on the same project and issue PRs when you're ready, all from within Maestro. Huge improvement, major thanks to `@petersilberman`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/releases.md` at line 174, The release note text uses "Github" instead of the official "GitHub"; update the sentence beginning "Github Worktree support was added..." in docs/releases.md to spell "GitHub" (capital H) so it reads "GitHub Worktree support was added." Ensure only the branding spelling is changed and other wording remains untouched.src/main/preload/autorun.ts (2)
29-38: Prefer reusing sharedPlaybookStatusinstead of duplicating shape locally.Keeping a second local interface for the same IPC payload invites drift over time. Alias this to the shared canonical type.
💡 Suggested refactor
import { ipcRenderer } from 'electron'; +import type { PlaybookStatus } from '../../shared/types'; ... -export interface PlaybookStatusInfo { - feature?: string; - phase?: string; - summary?: string; - tests?: { pass: number; fail: number }; - artifact?: string; -} +export type PlaybookStatusInfo = PlaybookStatus;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/preload/autorun.ts` around lines 29 - 38, Replace the duplicated local interface PlaybookStatusInfo with the shared canonical PlaybookStatus type: remove the PlaybookStatusInfo declaration and import (or re-export) PlaybookStatus from the module that defines the canonical type, then update any local references to use PlaybookStatus; ensure the import target is the module that currently exports PlaybookStatus so the IPC payload shape is reused rather than duplicated.
138-139: TightenunwatchStatusreturn typing for contract parity.This currently infers
Promise<any>. Explicitly type it to match the renderer contract and avoidanyleakage.💡 Suggested refactor
- unwatchStatus: (projectPath: string) => + unwatchStatus: (projectPath: string): Promise<Record<string, never>> => ipcRenderer.invoke('autorun:unwatchStatus', projectPath),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/preload/autorun.ts` around lines 138 - 139, unwatchStatus currently infers Promise<any>; change its declaration to explicitly return the renderer contract type (e.g. Promise<void>) to avoid any leakage — update the method signature in autorun.ts from unwatchStatus: (projectPath: string) => ipcRenderer.invoke('autorun:unwatchStatus', projectPath) to unwatchStatus: (projectPath: string): Promise<void> => ipcRenderer.invoke('autorun:unwatchStatus', projectPath) (or the exact contract type from your renderer types; import that type if needed).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/ipc/handlers/autorun.ts`:
- Line 60: The module-level debounce timer statusWatchDebounceTimer causes
cross-project cancellations; replace it with a per-project timer map (e.g.,
statusWatchDebounceTimers: Map<string, NodeJS.Timeout | null>) and update all
usages (where statusWatchDebounceTimer is cleared, set, or checked—including the
handlers around the status-watch logic and the blocks referenced at the other
occurrences) to use a project-specific key (projectId or runId) to get/clear/set
the timer, and ensure timers are cleaned up when a project/watch stops to avoid
leaks.
- Around line 1292-1297: The current try/catch around reading and parsing
statusFilePath swallows all errors (including JSON parse and permission errors)
and sets initialStatus silently; change it to only ignore ENOENT
(file-not-found) errors and rethrow all others: when awaiting
fs.readFile(statusFilePath, 'utf-8') catch errors and if (err?.code ===
'ENOENT') return/continue, otherwise rethrow; then perform JSON.parse(content)
in its own try/catch and if parsing fails rethrow (or wrap and rethrow with
context) so JSON syntax errors are not swallowed; preserve the use of
initialStatus and PlaybookStatus for types and ensure unexpected errors
propagate for Sentry.
In `@src/renderer/hooks/batch/useBatchProcessor.ts`:
- Around line 842-867: The STATUS.json watcher currently swallows errors and
only cleans up on the success path; refactor the watcher setup using a
try/catch/finally so that statusCleanup is always called in the finally block
(ensure statusCleanup is invoked if non-null) and replace the empty catch with
explicit error reporting using the Sentry helpers (captureException or
captureMessage) imported from src/utils/sentry.ts, providing context such as
statusProjectPath and sessionId; keep the existing behavior of dispatching
'UPDATE_PLAYBOOK_STATUS' for initialStatus and onStatusChanged, but ensure any
failure from window.maestro.autorun.watchStatus or onStatusChanged is logged to
Sentry rather than being silently ignored.
---
Nitpick comments:
In `@docs/releases.md`:
- Line 174: The release note text uses "Github" instead of the official
"GitHub"; update the sentence beginning "Github Worktree support was added..."
in docs/releases.md to spell "GitHub" (capital H) so it reads "GitHub Worktree
support was added." Ensure only the branding spelling is changed and other
wording remains untouched.
In `@src/main/preload/autorun.ts`:
- Around line 29-38: Replace the duplicated local interface PlaybookStatusInfo
with the shared canonical PlaybookStatus type: remove the PlaybookStatusInfo
declaration and import (or re-export) PlaybookStatus from the module that
defines the canonical type, then update any local references to use
PlaybookStatus; ensure the import target is the module that currently exports
PlaybookStatus so the IPC payload shape is reused rather than duplicated.
- Around line 138-139: unwatchStatus currently infers Promise<any>; change its
declaration to explicitly return the renderer contract type (e.g. Promise<void>)
to avoid any leakage — update the method signature in autorun.ts from
unwatchStatus: (projectPath: string) =>
ipcRenderer.invoke('autorun:unwatchStatus', projectPath) to unwatchStatus:
(projectPath: string): Promise<void> =>
ipcRenderer.invoke('autorun:unwatchStatus', projectPath) (or the exact contract
type from your renderer types; import that type if needed).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0468fb71-da24-41e9-b6b4-4b6860c823e5
📒 Files selected for processing (11)
docs/releases.mdsrc/__tests__/main/ipc/handlers/autorun.test.tssrc/main/ipc/handlers/autorun.tssrc/main/preload/autorun.tssrc/prompts/maestro-system-prompt.mdsrc/renderer/components/RightPanel.tsxsrc/renderer/global.d.tssrc/renderer/hooks/batch/batchReducer.tssrc/renderer/hooks/batch/useBatchProcessor.tssrc/renderer/types/index.tssrc/shared/types.ts
|
|
||
| // Playbook STATUS.json watchers (keyed by project path) | ||
| const statusWatchers = new Map<string, FSWatcher>(); | ||
| let statusWatchDebounceTimer: NodeJS.Timeout | null = null; |
There was a problem hiding this comment.
Use per-project debounce timers to avoid cross-project status loss.
A single module-level debounce timer means one project’s status update can cancel another project’s pending update. This can drop or misorder events when multiple runs/watchers are active.
💡 Suggested fix (isolate debounce state by project)
- let statusWatchDebounceTimer: NodeJS.Timeout | null = null;
+ const statusWatchDebounceTimers = new Map<string, NodeJS.Timeout>();
...
- if (statusWatchDebounceTimer) {
- clearTimeout(statusWatchDebounceTimer);
- }
- statusWatchDebounceTimer = setTimeout(async () => {
- statusWatchDebounceTimer = null;
+ const existingTimer = statusWatchDebounceTimers.get(projectPath);
+ if (existingTimer) {
+ clearTimeout(existingTimer);
+ }
+ const timer = setTimeout(async () => {
+ statusWatchDebounceTimers.delete(projectPath);
try {
const content = await fs.readFile(statusFilePath, 'utf-8');
const status = JSON.parse(content) as PlaybookStatus;
...
} catch (err) {
logger.warn(`${LOG_CONTEXT} Failed to read STATUS.json: ${err}`, LOG_CONTEXT);
}
}, 500);
+ statusWatchDebounceTimers.set(projectPath, timer);
...
if (statusWatchers.has(projectPath)) {
statusWatchers.get(projectPath)?.close();
statusWatchers.delete(projectPath);
+ const timer = statusWatchDebounceTimers.get(projectPath);
+ if (timer) {
+ clearTimeout(timer);
+ statusWatchDebounceTimers.delete(projectPath);
+ }
logger.info(`Stopped watching STATUS.json in: ${projectPath}`, LOG_CONTEXT);
}
...
for (const [projectPath, watcher] of statusWatchers) {
watcher.close();
logger.info(`Cleaned up STATUS.json watcher for: ${projectPath}`, LOG_CONTEXT);
}
statusWatchers.clear();
+for (const timer of statusWatchDebounceTimers.values()) {
+ clearTimeout(timer);
+}
+statusWatchDebounceTimers.clear();Also applies to: 1313-1317, 1366-1369, 1383-1387
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/ipc/handlers/autorun.ts` at line 60, The module-level debounce timer
statusWatchDebounceTimer causes cross-project cancellations; replace it with a
per-project timer map (e.g., statusWatchDebounceTimers: Map<string,
NodeJS.Timeout | null>) and update all usages (where statusWatchDebounceTimer is
cleared, set, or checked—including the handlers around the status-watch logic
and the blocks referenced at the other occurrences) to use a project-specific
key (projectId or runId) to get/clear/set the timer, and ensure timers are
cleaned up when a project/watch stops to avoid leaks.
| try { | ||
| const content = await fs.readFile(statusFilePath, 'utf-8'); | ||
| initialStatus = JSON.parse(content) as PlaybookStatus; | ||
| } catch { | ||
| // File doesn't exist yet — that's fine | ||
| } |
There was a problem hiding this comment.
Don’t swallow non-ENOENT errors while loading initial STATUS.json.
This catch currently hides JSON parse and filesystem permission errors as if the file simply does not exist, which makes failures hard to diagnose and can keep stale/empty UI state.
💡 Suggested fix (handle expected errors, rethrow unexpected ones)
let initialStatus: PlaybookStatus | null = null;
try {
const content = await fs.readFile(statusFilePath, 'utf-8');
initialStatus = JSON.parse(content) as PlaybookStatus;
- } catch {
- // File doesn't exist yet — that's fine
+ } catch (error) {
+ const err = error as NodeJS.ErrnoException;
+ if (err.code === 'ENOENT') {
+ // File doesn't exist yet — expected
+ } else if (error instanceof SyntaxError) {
+ logger.warn(`${LOG_CONTEXT} Invalid STATUS.json format: ${statusFilePath}`, LOG_CONTEXT);
+ } else {
+ throw error;
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| const content = await fs.readFile(statusFilePath, 'utf-8'); | |
| initialStatus = JSON.parse(content) as PlaybookStatus; | |
| } catch { | |
| // File doesn't exist yet — that's fine | |
| } | |
| try { | |
| const content = await fs.readFile(statusFilePath, 'utf-8'); | |
| initialStatus = JSON.parse(content) as PlaybookStatus; | |
| } catch (error) { | |
| const err = error as NodeJS.ErrnoException; | |
| if (err.code === 'ENOENT') { | |
| // File doesn't exist yet — expected | |
| } else if (error instanceof SyntaxError) { | |
| logger.warn(`${LOG_CONTEXT} Invalid STATUS.json format: ${statusFilePath}`, LOG_CONTEXT); | |
| } else { | |
| throw error; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/ipc/handlers/autorun.ts` around lines 1292 - 1297, The current
try/catch around reading and parsing statusFilePath swallows all errors
(including JSON parse and permission errors) and sets initialStatus silently;
change it to only ignore ENOENT (file-not-found) errors and rethrow all others:
when awaiting fs.readFile(statusFilePath, 'utf-8') catch errors and if
(err?.code === 'ENOENT') return/continue, otherwise rethrow; then perform
JSON.parse(content) in its own try/catch and if parsing fails rethrow (or wrap
and rethrow with context) so JSON syntax errors are not swallowed; preserve the
use of initialStatus and PlaybookStatus for types and ensure unexpected errors
propagate for Sentry.
| // Start watching .maestro/STATUS.json for playbook status updates | ||
| const statusProjectPath = effectiveCwd; | ||
| let statusCleanup: (() => void) | null = null; | ||
| try { | ||
| const { status: initialStatus } = | ||
| await window.maestro.autorun.watchStatus(statusProjectPath); | ||
| if (initialStatus) { | ||
| dispatch({ | ||
| type: 'UPDATE_PLAYBOOK_STATUS', | ||
| sessionId, | ||
| status: initialStatus, | ||
| }); | ||
| } | ||
| statusCleanup = window.maestro.autorun.onStatusChanged((data) => { | ||
| if (data.projectPath === statusProjectPath) { | ||
| dispatch({ | ||
| type: 'UPDATE_PLAYBOOK_STATUS', | ||
| sessionId, | ||
| status: data.status ?? undefined, | ||
| }); | ||
| } | ||
| }); | ||
| } catch { | ||
| // STATUS.json watching is optional — don't fail the batch | ||
| } | ||
|
|
There was a problem hiding this comment.
Make STATUS watcher lifecycle non-leaky and non-silent.
At Line 864 and Line 1762, errors are swallowed silently, and cleanup is only on the normal completion path. If any awaited operation throws before cleanup, the status listener/watch can remain active for that session. Please move watcher teardown into a finally block and explicitly report failures (Sentry/context logging) instead of empty catches.
Suggested direction
+// import { captureException } from '../../../utils/sentry';
let statusCleanup: (() => void) | null = null;
const statusProjectPath = effectiveCwd;
-try {
+try {
const { status: initialStatus } = await window.maestro.autorun.watchStatus(statusProjectPath);
...
statusCleanup = window.maestro.autorun.onStatusChanged(...);
-} catch {
- // STATUS.json watching is optional — don't fail the batch
+} catch (error) {
+ // expected-recoverable path: watcher unavailable
+ // captureException(error, { context: { sessionId, statusProjectPath, phase: 'watchStatus:init' } });
}
+try {
+ // existing batch execution body
+} finally {
+ try {
+ statusCleanup?.();
+ } catch (error) {
+ // captureException(error, { context: { sessionId, statusProjectPath, phase: 'watchStatus:unsubscribe' } });
+ }
+
+ try {
+ await window.maestro.autorun.unwatchStatus(statusProjectPath);
+ } catch (error) {
+ // captureException(error, { context: { sessionId, statusProjectPath, phase: 'watchStatus:unwatch' } });
+ }
+}As per coding guidelines, src/**/*.{ts,tsx}: “Do not silently swallow errors… Use Sentry utilities (captureException, captureMessage) from src/utils/sentry.ts for explicit error reporting with context.”
Also applies to: 1756-1765
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/hooks/batch/useBatchProcessor.ts` around lines 842 - 867, The
STATUS.json watcher currently swallows errors and only cleans up on the success
path; refactor the watcher setup using a try/catch/finally so that statusCleanup
is always called in the finally block (ensure statusCleanup is invoked if
non-null) and replace the empty catch with explicit error reporting using the
Sentry helpers (captureException or captureMessage) imported from
src/utils/sentry.ts, providing context such as statusProjectPath and sessionId;
keep the existing behavior of dispatching 'UPDATE_PLAYBOOK_STATUS' for
initialStatus and onStatusChanged, but ensure any failure from
window.maestro.autorun.watchStatus or onStatusChanged is logged to Sentry rather
than being silently ignored.
Greptile SummaryThis PR adds the
Confidence Score: 4/5Safe to merge after fixing the shared debounce timer in the main process; the bug silently drops status updates for users running multiple concurrent auto-run sessions in different directories. One P1 bug (shared statusWatchDebounceTimer singleton) that causes silent data loss for the core use case of Maestro — multiple concurrent agents in different project directories. All other layers (reducer, preload, RightPanel) are correct. src/main/ipc/handlers/autorun.ts — shared debounce timer and unnecessary mkdir side effect Important Files Changed
Sequence DiagramsequenceDiagram
participant R as Renderer (useBatchProcessor)
participant P as Preload Bridge
participant M as Main Process (autorun.ts)
participant F as .maestro/STATUS.json
R->>P: watchStatus(projectPath)
P->>M: IPC: autorun:watchStatus
M->>F: readFile (initial status)
M->>M: chokidar.watch(statusFilePath)
M-->>R: { status: initialStatus }
R->>R: dispatch UPDATE_PLAYBOOK_STATUS
R->>P: onStatusChanged(handler)
Note over R: renderer listener registered
loop STATUS.json changes during auto-run
F-->>M: add / change event
M->>M: debounce 500ms
M->>F: readFile
M-->>P: IPC: autorun:statusChanged
P-->>R: handler({ projectPath, status })
R->>R: dispatch UPDATE_PLAYBOOK_STATUS
end
F-->>M: unlink event
M-->>P: IPC: autorun:statusChanged (null)
R->>R: dispatch UPDATE_PLAYBOOK_STATUS (undefined)
Note over R: auto-run ends
R->>P: statusCleanup() — remove listener
R->>P: unwatchStatus(projectPath)
P->>M: IPC: autorun:unwatchStatus
M->>M: watcher.close()
R->>R: dispatch COMPLETE_BATCH (clears playbookStatus)
Reviews (1): Last reviewed commit: "style: format docs/releases.md with pret..." | Re-trigger Greptile |
| const statusWatchers = new Map<string, FSWatcher>(); | ||
| let statusWatchDebounceTimer: NodeJS.Timeout | null = null; |
There was a problem hiding this comment.
Shared debounce timer broken for concurrent watchers
statusWatchDebounceTimer is a module-level singleton, but the statusWatchers map can hold multiple concurrent watchers (one per projectPath). Each watcher's handleStatusChange closure at line 1312 reads and writes this same timer. When two projects have active watchers and both STATUS.json files change within 500ms of each other, watcher B's callback clears watcher A's pending timeout — A's status update is silently dropped and never sent to the renderer, until the next STATUS.json write for that project.
Replace the singleton with a per-path map:
// Replace line 60 with:
const statusWatchDebounceTimers = new Map<string, NodeJS.Timeout>();Then in handleStatusChange (lines 1312–1331), scope the debounce to projectPath:
const handleStatusChange = async () => {
const existing = statusWatchDebounceTimers.get(projectPath);
if (existing) clearTimeout(existing);
const timer = setTimeout(async () => {
statusWatchDebounceTimers.delete(projectPath);
try {
const content = await fs.readFile(statusFilePath, 'utf-8');
const status = JSON.parse(content) as PlaybookStatus;
const mainWindow = getMainWindow();
if (isWebContentsAvailable(mainWindow)) {
mainWindow.webContents.send('autorun:statusChanged', { projectPath, status });
}
} catch (err) {
logger.warn(`${LOG_CONTEXT} Failed to read STATUS.json: ${err}`, LOG_CONTEXT);
}
}, 500);
statusWatchDebounceTimers.set(projectPath, timer);
};Also clear pending timers in unwatchStatus (line 1366) and before-quit (line 1383) to prevent stale callbacks from firing after a watcher is closed.
| // Watch the .maestro directory for STATUS.json changes | ||
| const maestroDir = path.join(projectPath, '.maestro'); | ||
| try { | ||
| await fs.stat(maestroDir); | ||
| } catch { | ||
| await fs.mkdir(maestroDir, { recursive: true }); | ||
| } |
There was a problem hiding this comment.
watchStatus creates .maestro/ directory as an unexpected side effect
This block unconditionally creates the .maestro directory the first time any Auto Run starts, even if the user has never written a STATUS.json file. Users who don't use playbooks will find a .maestro directory appearing in their project root.
chokidar supports watching a non-existent file path — it will detect the file when it is eventually created. The mkdir guard is unnecessary here and can be removed.
| // Watch the .maestro directory for STATUS.json changes | |
| const maestroDir = path.join(projectPath, '.maestro'); | |
| try { | |
| await fs.stat(maestroDir); | |
| } catch { | |
| await fs.mkdir(maestroDir, { recursive: true }); | |
| } | |
| const watcher = chokidar.watch(statusFilePath, { |
Summary
Closes #233
PlaybookStatustype contract: playbooks can write.maestro/STATUS.jsonwithfeature,phase,summary,tests, andartifactfieldsHow it works
.maestro/STATUS.jsonin the project directoryExample STATUS.json a playbook would write:
{ "feature": "F-13", "phase": "IMPLEMENT", "summary": "Vector Health Watchdog - 11/14 tasks complete", "tests": { "pass": 394, "fail": 0 } }Test plan
npm run lint)npm run lint:eslint)npm run test) including updated autorun handler registration test.maestro/STATUS.jsonfile while auto-run is active and verify it appears in the progress panelSummary by CodeRabbit
Release Notes
New Features
Documentation