Repository navigation
chore: promote dev→main — pgserve, fire-and-forget, scheduler - #676
Closed
namastex888 wants to merge 10 commits into
Closed
namastex888 wants to merge 10 commits into
namastex888 wants to merge 10 commits into
Conversation
* feat(db): add pgserve integration and connection management Embed pgserve as genie's persistent brain with lazy initialization. Adds postgres.js client with singleton connection pool, port fallback, health check, and graceful shutdown. * feat(db): add migration runner and initial schema Create db-migrations.ts with runMigrations() and getMigrationStatus() for incremental schema versioning. Add 001_initial.sql with core scheduler tables (schedules, triggers, runs, heartbeats, audit_events), agent_checkpoints for session resume, and LISTEN/NOTIFY trigger for real-time trigger-due notifications. * chore: ignore db module in knip until CLI commands consume it * feat(db): run migrations on first connection Call runMigrations() from getConnection() so the schema is automatically applied when any command first touches the database. * feat(db): add genie db CLI commands (status, migrate, query) Wire up database management commands to the CLI router: - `genie db status` shows pgserve health, port, data dir, table counts - `genie db migrate` runs pending migrations manually - `genie db query "<sql>"` executes ad-hoc SQL with tabular output - Clean up knip config now that db modules are consumed by CLI * fix(ci): add bun to trustedDependencies for pgserve compatibility pgserve depends on the bun npm package, whose postinstall script must run to set up the binary. Without trusting it, CI build fails with "Bun's postinstall script was not run". --------- Co-authored-by: Genie <genie@automagik.ai>
* feat(dispatch): fire-and-forget orchestration + mailbox prompt delivery Remove blocking polling loop from autoOrchestrateCommand — dispatch first eligible wave and return immediately. Replace initialPrompt CLI args with post-spawn mailbox delivery via protocol-router in all dispatch commands (brainstorm, wish, work, review) and team leader spawn. Add tmux session config storage and --session flag to prevent session explosion on parallel team creates. * feat(done): agent auto-exit, push enforcement, and wave completion notification - detectWaveCompletion(): reads WISH.md Execution Strategy, finds the wave containing the completed group, checks if all wave-mates are done - ensureWorkPushed(): commits dirty working tree as WIP, pushes unpushed commits before pane exit - autoKillPane(): kills calling agent's tmux pane via TMUX_PANE env after all notifications complete (1s delay for output flush) - Wave-complete sends message to team-lead via protocol-router - 12 new tests covering wave detection, push enforcement, and edge cases * feat(relay): add OTel relay liveness check for dead agent pane recovery When the OTel relay detects a dead tmux pane, it now checks if the crashed agent was assigned to an in_progress wish group. If so, it resets the group to ready and notifies team-lead via the native inbox. This enables automatic retry of crashed agent work. - Extended registerOtelRelayPane to store repoPath in meta for state lookup - Added handleDeadWorkerLiveness function in relay cleanup loop - Matches assignees by exact name or team-prefixed worker ID * feat(resume): session resume context injection + switch --continue to --resume - Replace --continue (name-based) with --resume (session-id-based) across all spawn paths: provider-adapters, spawn-command, team-lead-command, session.ts - Generate and store claudeSessionId in agent registry on every spawn - On respawn, use stored session ID for --resume; fall back to fresh session - Add resume context injection in spawnWorkerFromTemplate: queries wish state for in_progress groups, builds context prompt with wish slug, group section, and recent git log, delivers via mailbox as first message before task prompt - Extract spawnPaneInSession helper to reduce cognitive complexity - Update all tests to expect --resume instead of --continue --------- Co-authored-by: Genie <genie@automagik.ai>
Implements full CLI for managing scheduled triggers: - `genie schedule create <name> --command <cmd> --at|--every|--after` - `genie schedule list [--json] [--watch]` - `genie schedule cancel <name|id>` - `genie schedule retry <name|id>` - `genie schedule history <name|id> [--limit N]` Includes time parsing (durations, ISO 8601, cron detection), table formatting, and 13 unit tests for parsing functions.
Fix import ordering, formatting, and unused biome-ignore comments in Group 1 files to unblock pre-push hook.
…laiming - Create 002_scheduler_extensions.sql migration: leased_by/leased_until on triggers, idempotency_key, trace_id on runs, run_spec/interval_ms on schedules - Create src/lib/run-spec.ts: RunSpec interface, RunState type with state machine transitions, resolveRunSpec() with validation and defaults - Fix scheduler-daemon.ts: poll loop stop resolution, TypeScript type fixes, remove unused checkIdempotency function, extract validateRunSpec helper - Create src/lib/scheduler-daemon.test.ts: 20 tests covering claim, fire, idempotency skip, concurrency cap, jitter, spawn failure, daemon lifecycle - All 851 tests pass, typecheck clean, lint clean (no new warnings)
Group 3 deliverables: - genie daemon install: generates systemd user service, enables via systemctl - genie daemon start [--foreground]: launches scheduler (background/foreground) - genie daemon stop: graceful SIGTERM with 10s timeout + SIGKILL fallback - genie daemon status: PID, uptime, trigger stats from DB - genie daemon logs [--follow] [--lines N]: tail structured JSON scheduler log - PID file management at ~/.genie/scheduler.pid - Registered daemon commands in src/genie.ts
…on, and machine snapshots Implements Group 4 of the scheduler wish: - Startup recovery: reclaim expired leases, reconcile orphaned runs - Heartbeat collector (60s): pane liveness checks, INSERT heartbeat records - Orphan reconciliation (5m): mark runs failed after 2 consecutive dead heartbeats - Machine snapshot (60s): active workers, teams, tmux sessions, CPU/memory - Migration 003: machine_snapshots table - 19 new tests covering all recovery and monitoring scenarios
…+ cron timing Bug 1: fireTrigger now advances trigger from 'executing' to 'completed' after successful spawn, preventing duplicate runs on daemon restart. Bug 2: After a trigger fires, maybeCreateNextTrigger inserts the next pending trigger for recurring schedules (@every intervals and cron expressions). One-shot (@once) schedules are skipped. Bug 3: Replace computeNextCronDue stub (which always scheduled 1 minute from now) with a proper cron parser that respects the actual expression fields (minute, hour, DOM, month, DOW) with POSIX day-matching semantics.
feat(scheduler): add scheduler daemon with lease-based trigger system
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 18760271 | Triggered | Generic Password | 45153d5 | src/lib/db.ts | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
📝 WalkthroughWalkthroughThis PR introduces a comprehensive scheduler daemon infrastructure with persistent PostgreSQL backend, database migration system, and refactored session resumption. It adds CLI subcommands for daemon lifecycle management, database administration, and schedule creation/management, along with cron/duration parsing utilities and resume context injection during worker spawns. Changes
Sequence Diagram(s)sequenceDiagram
participant Daemon as Daemon Loop
participant PG as PostgreSQL
participant Worker as Worker Process
participant Monitor as Heartbeat Monitor
Daemon->>PG: SELECT due pending triggers (FOR UPDATE SKIP LOCKED)
PG-->>Daemon: [trigger₁, trigger₂, ...]
Daemon->>Daemon: Check concurrency cap (active runs count)
alt Concurrency limit reached
Daemon->>Daemon: Log concurrency_cap_reached, skip claim
else Within limit
Daemon->>PG: UPDATE triggers to executing (set lease_until)
Daemon->>Daemon: Emit triggers_claimed log
end
Daemon->>PG: SELECT schedule for trigger
PG-->>Daemon: schedule row
Daemon->>Daemon: Resolve RunSpec (command/provider/role)
Daemon->>Daemon: Generate trace_id, run_id
Daemon->>PG: INSERT run with trace_id, status=spawning
Daemon->>Worker: spawn command (env: GENIE_TRACE_ID, GENIE_RUN_ID, ...)
activate Worker
Worker->>Daemon: process started
Daemon->>PG: UPDATE trigger to completed (if spawn ok)
Daemon->>PG: INSERT next trigger (if recurring/cron)
loop Every heartbeat interval
Monitor->>Worker: Check pane alive via tmux
Worker-->>Monitor: alive/dead status
Monitor->>PG: INSERT heartbeat (worker_id, status)
end
alt Consecutive dead heartbeats ≥ threshold
Monitor->>PG: UPDATE run to failed
end
Worker->>Daemon: process exits
deactivate Worker
sequenceDiagram
participant User as User/CLI
participant Spawn as spawnWorkerFromTemplate
participant Router as protocolRouter
participant Wish as Wish State
participant Mailbox as Mailbox
participant Claude as Claude Worker
User->>Spawn: spawn(template, resumeSessionId?)
activate Spawn
Spawn->>Spawn: Generate sessionId (UUID) for fresh spawn
alt resumeSessionId provided
Spawn->>Spawn: params.resume = resumeSessionId
else Fresh spawn
Spawn->>Spawn: params.sessionId = UUID
end
Spawn->>Spawn: Build spawn params + command
Spawn->>Claude: Launch (--resume <id> or --session-id <uuid>)
activate Claude
alt Resume requested
Spawn->>Wish: findGroupByAssignee(slug, workerId)
Wish-->>Spawn: {groupName, group}
Spawn->>Spawn: Extract group section from WISH.md
Spawn->>Spawn: Fetch recent git log
Spawn->>Spawn: Build resume context prompt
Spawn->>Mailbox: send(resumeContext, 'cli')
Mailbox->>Claude: First message: resume context
end
Claude-->>Spawn: worker spawned
deactivate Spawn
deactivate Claude
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Contributor
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request promotes three major features from the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a2cb615629
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+67
to
+69
| const stepMatch = part.match(/^(.+)\/(\d+)$/); | ||
| const step = stepMatch ? Number.parseInt(stepMatch[2], 10) : 1; | ||
| const range = stepMatch ? stepMatch[1] : part; |
There was a problem hiding this comment.
parseCronField accepts a step value of 0, and expandRange then iterates with i += step; for inputs like */0 * * * * this never advances and the scheduler hangs in a tight loop instead of returning a validation error. This can be triggered by user-provided --every cron input and will block scheduling for that process.
Useful? React with 👍 / 👎.
Comment on lines
+211
to
+213
| timezone: options.timezone ?? 'UTC', | ||
| ...(options.leaseTimeout ? { lease_timeout_ms: parseDuration(options.leaseTimeout) } : {}), | ||
| }; |
There was a problem hiding this comment.
schedule create parses --lease-timeout but only writes it into metadata, while execution reads lease timeout from run_spec (fireTrigger resolves schedule.run_spec before inserting runs.lease_timeout_ms). As a result, user-provided lease timeouts are silently ignored and runs always use the default timeout.
Useful? React with 👍 / 👎.
Comment on lines
+109
to
+112
| if (isCronExpression(options.every)) { | ||
| // Store cron expression directly | ||
| const dueAt = computeNextCronDue(options.every); | ||
| return { dueAt, cronExpr: options.every, scheduleType: 'cron' }; |
There was a problem hiding this comment.
The cron path computes due time with computeNextCronDue(options.every) without using options.timezone, even though the command exposes and stores --timezone. This means schedules are evaluated in the daemon's local timezone rather than the configured one, so a cron like 0 9 * * * --timezone America/New_York can fire at the wrong wall-clock time.
Useful? React with 👍 / 👎.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This is a substantial pull request promoting three major features: pgserve for a persistent database, a fire-and-forget dispatch mechanism, and a scheduler daemon. The changes are extensive and well-structured, introducing significant new capabilities. My review focused on correctness and maintainability. I've identified a critical runtime issue in the new liveness handler due to missing imports and a minor naming inconsistency in a function. Overall, the new features, especially the scheduler and database layer, appear robust and well-tested.
Comment on lines
+294
to
+297
| function handleDeadWorkerLiveness(workerId, meta) { | ||
| if (!meta || !meta.repoPath) return; | ||
| try { | ||
| const stateDir = join(meta.repoPath, '.genie', 'state'); |
Contributor
There was a problem hiding this comment.
The new function handleDeadWorkerLiveness uses join, readdirSync, readFileSync, and writeFileSync without importing them. This will cause a ReferenceError at runtime. Please add the necessary imports at the top of the function to resolve this. Additionally, for better type safety, consider adding types for the workerId and meta parameters.
Suggested change
| function handleDeadWorkerLiveness(workerId, meta) { | |
| if (!meta || !meta.repoPath) return; | |
| try { | |
| const stateDir = join(meta.repoPath, '.genie', 'state'); | |
| function handleDeadWorkerLiveness(workerId, meta) { | |
| const { join } = require('node:path'); | |
| const { readdirSync, readFileSync, writeFileSync } = require('node:fs'); | |
| if (!meta || !meta.repoPath) return; | |
| try { | |
| const stateDir = join(meta.repoPath, '.genie', 'state'); |
| * then checks #{pane_current_command} — if the pane shows a shell, --resume | ||
| * failed (no prior session) so we retry fresh without --resume. | ||
| */ | ||
| async function launchWithContinueFallback( |
Contributor
There was a problem hiding this comment.
The function name launchWithContinueFallback is now outdated after refactoring from --continue to --resume. To improve code clarity and maintainability, please rename it to launchWithResumeFallback to match its updated functionality.
Suggested change
| async function launchWithContinueFallback( | |
| async function launchWithResumeFallback( |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 25
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/lib/tmux.ts (1)
48-71: 🧹 Nitpick | 🔵 TrivialSubstring hint matching may cause unexpected session selection.
Line 64 uses
s.name.includes(hint)which performs substring matching. A hint like"test"would match sessions named"test","testing","contest", etc. Consider prioritizing exact match before falling back to substring:♻️ Prefer exact match over substring
// 2. Outside tmux — try list-sessions fallback try { const sessions = await listSessions(); if (sessions.length === 0) return null; if (hint) { + // Prefer exact match + const exact = sessions.find((s) => s.name === hint); + if (exact) return exact.name; + // Fall back to substring match const match = sessions.find((s) => s.name.includes(hint)); if (match) return match.name; } return sessions[0].name;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/tmux.ts` around lines 48 - 71, The current getCurrentSessionName function uses substring matching (s.name.includes(hint)) which can return unintended sessions; update the hint matching to first look for an exact match (e.g., s.name === hint) and return that if found, and only then fall back to substring matching (s.name.includes(hint)) if no exact match exists; modify the logic inside getCurrentSessionName where sessions.find is used so it tries exact match first, then substring, ensuring correct session selection when a hint is provided.src/genie-commands/session.ts (1)
197-225: 🧹 Nitpick | 🔵 TrivialFunction and variable names still reference "continue" after
--resumemigration.The function
launchWithContinueFallbackand variablescontinueCmd/continueNameuse the old terminology while the actual CLI flag is now--resume. Consider renaming for consistency.♻️ Suggested naming alignment
-async function launchWithContinueFallback( +async function launchWithResumeFallback( target: string, windowName: string, systemPromptFile: string | null, ): Promise<void> { - const continueName = sanitizeTeamName(windowName); - const continueCmd = buildClaudeCommand(windowName, systemPromptFile || undefined, continueName); + const resumeName = sanitizeTeamName(windowName); + const resumeCmd = buildClaudeCommand(windowName, systemPromptFile || undefined, resumeName); const freshCmd = buildClaudeCommand(windowName, systemPromptFile || undefined, undefined); // Try --resume first (preserves conversation history) - await tmux.executeTmux(`send-keys -t ${shellQuote(target)} ${shellQuote(continueCmd)} Enter`); + await tmux.executeTmux(`send-keys -t ${shellQuote(target)} ${shellQuote(resumeCmd)} Enter`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/genie-commands/session.ts` around lines 197 - 225, Rename outdated "continue" identifiers to "resume" for consistency: change function launchWithContinueFallback to launchWithResumeFallback, rename continueName to resumeName and continueCmd to resumeCmd, and update any call sites and usages (e.g., where buildClaudeCommand is invoked and tmux.executeTmux calls) to use these new names; ensure the logic and parameters remain unchanged and update any console messages or comments referencing "continue" to "resume".src/term-commands/agents.ts (1)
610-623:⚠️ Potential issue | 🔴 CriticalPass the resolved session/window target to
applySpawnLayout()instead of recomputing it.When
--sessionis provided,createTmuxPane()creates the pane in the override session (via theteamWindowresolved with the override), butapplySpawnLayout()callsgetCurrentSessionName()which ignores the override. The pane ends up created in one session while the layout command targets another. Use the resolvedteamWindow.windowId(or extract and pass the session) toapplySpawnLayout().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/term-commands/agents.ts` around lines 610 - 623, The layout is being applied to the current session instead of the resolved override — update launchTmuxSpawn so it passes the resolved target to applySpawnLayout rather than letting applySpawnLayout call getCurrentSessionName(); after createTmuxPane(...) call use the resolved teamWindow (or teamWindow.windowId/session) when invoking applySpawnLayout (and if needed adjust applySpawnLayout's signature to accept a targetSession/windowId and use that instead of getCurrentSessionName), referencing the functions launchTmuxSpawn, createTmuxPane, applySpawnLayout and the teamWindow.windowId value.
🤖 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/db/migrations/001_initial.sql`:
- Around line 119-131: The current trigger trg_notify_due only fires AFTER
INSERT; add an AFTER UPDATE branch that calls the same notify_trigger_due()
function when a row transitions back to pending so updates that set status =
'pending' notify the daemon immediately. Implement an UPDATE trigger on the
triggers table with WHEN (OLD.status IS DISTINCT FROM 'pending' AND NEW.status =
'pending') (or OLD.status <> 'pending' AND NEW.status = 'pending') and EXECUTE
FUNCTION notify_trigger_due(); keep the existing INSERT trigger intact and reuse
the notify_trigger_due() function and trg_notify_due naming to group related
behavior.
- Around line 7-22: Add a partial unique index on schedules to prevent duplicate
active names by creating a unique index named idx_schedules_active_name on the
name column where status = 'active' (i.e., CREATE UNIQUE INDEX
idx_schedules_active_name ON schedules(name) WHERE status = 'active'); update
the migration that defines the schedules table (table name: schedules, existing
indexes: idx_schedules_status and idx_schedules_name) to include this new index
and ensure the migration handles or documents any pre-existing active-name
duplicates before applying.
In `@src/db/migrations/003_machine_snapshots.sql`:
- Around line 3-15: The machine_snapshots table can grow unbounded; add a
retention strategy for machine_snapshots (e.g., time-based partitioning on
created_at or a scheduled cleanup) and implement a mechanism to expire old data:
convert machine_snapshots into range partitions by created_at (or create
monthly/weekly partitions) and add a scheduled job (pg_cron or a DB-side
function + external scheduler) to drop old partitions, or if partitioning is not
desired add a cleanup function that deletes rows older than a configurable
retention period and schedule it to run regularly; update related index
idx_machine_snapshots_created and any queries using created_at to work with the
chosen approach and document the retention configuration.
- Line 10: The column definition for context should be changed to enforce
non-null JSONB values: update the migration's column line from "context JSONB
DEFAULT '{}'," to "context JSONB NOT NULL DEFAULT '{}'," so explicit NULL
inserts are rejected and default applies consistently; update any corresponding
schema/ORM model to match if present.
In `@src/lib/cron.ts`:
- Around line 85-88: The cron parsing currently allows expressions with more
than 5 fields by only checking parts.length < 5; change the validation to
require exactly 5 fields by replacing that check with parts.length !== 5 and
throw an Error including the provided cronExpr; update the code around cronExpr,
parts and the destructuring ([minField, hourField, domField, monthField,
dowField]) so extras are rejected rather than ignored.
- Around line 44-70: The cron parser must validate step and numeric bounds to
avoid /0 and out-of-range loops: update expandRange(range, step, min, max) to
throw on invalid step (step <= 0) and on invalid numeric ranges (non-number,
start > end, or start/end outside [min,max]); also update parseCronField(field,
min, max) to validate parsed single numbers and step parts (ensure parsed step >
0 and numeric values are within min..max) before calling expandRange, so invalid
inputs produce a clear error instead of hanging or producing large scans;
reference functions: expandRange and parseCronField.
In `@src/lib/db-migrations.ts`:
- Around line 104-125: Wrap the pending-migration discovery and apply loop in a
PostgreSQL advisory lock to serialize across processes: in runMigrations(),
after ensureMigrationsTable() and loadMigrationFiles(), acquire a
pg_advisory_lock (choose a fixed key), then re-query the _genie_migrations table
to rebuild appliedSet and recompute pending before applying; keep the advisory
lock held while iterating pending and running each migration inside sql.begin
using tx.unsafe and inserting into _genie_migrations (the existing tx/insert
logic), and release the advisory lock (pg_advisory_unlock) when done or on error
so only one process can decide/apply migrations at a time.
In `@src/lib/db.ts`:
- Around line 146-166: getConnection currently assigns sqlClient before
runMigrations completes, which can leak a partially-initialized client and allow
concurrent inits; change the initialization to use a single shared init promise
(e.g., connectionInitPromise) and only set sqlClient after
runMigrations(sqlClientCandidate) succeeds: call ensurePgserve(), create the
postgres client instance as a local variable (e.g., sqlClientCandidate), run
runMigrations(sqlClientCandidate), then on success assign sqlClient =
sqlClientCandidate and resolve the init promise; ensure subsequent calls to
getConnection await the same connectionInitPromise and return sqlClient after it
resolves to prevent multiple pools or half-initialized clients.
In `@src/lib/protocol-router-spawn.ts`:
- Around line 245-252: Replace the synchronous require('node:fs')/readdirSync
usage in injectResumeContext with the async fs.readdir (from fs/promises or
fs.promises.readdir) and await it; call await readdir(stateDir) inside a
try/catch, filter the returned filenames for .json, and on ENOENT (no state
directory) return early as before—this removes the blocking readdirSync and
keeps the same stateFiles logic while using the async API.
- Around line 218-225: The getRecentGitLog function currently constructs a shell
string with repoPath causing a command injection risk; change it to call a
non-shell child process API (e.g., child_process.spawn or execFile) with args
array rather than interpolating repoPath into a shell command, e.g., invoke git
with arguments ['-C', repoPath, 'log', '--oneline', `-n`, String(count)] and
capture stdout/stderr, and preserve the existing error handling (return empty
string on failure); additionally validate or normalize repoPath (e.g., ensure
it's an absolute path) before passing it to the spawn/execFile call to further
mitigate injection.
In `@src/lib/provider-adapters.ts`:
- Around line 266-269: The code silently prefers params.resume over
params.sessionId; instead validate their mutual exclusivity and fail fast:
before pushing to parts, add a check that if both params.resume and
params.sessionId are set throw or return an error (e.g., throw new Error(...))
describing the conflict; otherwise keep the current logic that uses
parts.push('--resume', escapeShellArg(params.resume)) when only resume is
present or parts.push('--session-id', escapeShellArg(params.sessionId)) when
only sessionId is present. Ensure you reference the same symbols (params.resume,
params.sessionId, parts.push, escapeShellArg) so callers get a clear error
rather than silent precedence.
In `@src/lib/run-spec.ts`:
- Around line 77-86: DEFAULTS currently inlines repo: process.cwd() at module
load, causing stale cwd for long‑lived processes; remove the frozen cwd from
DEFAULTS (set repo to ''/undefined) and instead compute the repo fallback inside
resolveRunSpec() (and any other default-merge helper in this file used around
the same area) by using process.cwd() at call time; update resolveRunSpec (and
the default-merge logic referenced in the same block) to treat an
empty/undefined repo as meaning process.cwd() and preserve all other DEFAULTS
behavior.
In `@src/lib/scheduler-daemon.ts`:
- Around line 409-450: Narrow the try/catch to only cover deps.spawnCommand so
that spawn failures still mark the run/trigger as failed but any errors after a
successful spawn (the UPDATE runs to set status='running', UPDATE triggers,
deps.log, and maybeCreateNextTrigger) do not retroactively mark an
already-running child as failed; implement this by moving deps.spawnCommand into
its own try/catch block (catching and handling by running the existing failure
SQL updates for runs/triggers and logging), then after a successful spawn
perform the SQL UPDATE runs, UPDATE triggers, deps.log call, and await
maybeCreateNextTrigger inside a separate block that catches errors and logs them
(or retries) but does not change run/trigger status. Ensure you reference
deps.spawnCommand, maybeCreateNextTrigger, and the SQL UPDATE statements when
changing the code.
- Around line 462-470: reclaimExpiredLeases currently flips expired triggers to
pending unconditionally, which can requeue work whose run is still active;
change reclaimExpiredLeases (and the startup flow in recoverOnStartup) to only
reclaim triggers whose associated run is absent or not live: either (A) in
reclaimExpiredLeases add a WHERE clause joining the runs table (or checking
run_id) to ensure the run is NULL or has status in a terminal set (e.g.,
completed/failed) or its last_heartbeat < some threshold, or (B) move the
reclaim step after calling reconcileOrphanedRuns() and invoke a run-liveness
check (reconcileOrphanedRuns or equivalent isAlive logic) for each candidate
trigger before updating; update function names referenced (reclaimExpiredLeases,
recoverOnStartup, reconcileOrphanedRuns) and their SQL UPDATE to conditionally
update only when the run is confirmed absent/dead.
- Around line 198-266: The current race comes from concurrent cycles entering
processTriggers() (via LISTEN or poll) and both computing available capacity
before claiming; to fix, add a per-daemon in-flight guard (e.g., an inMemory
flag like isProcessing on the daemon that processTriggers() checks/sets and
clears) to serialize invocations on the same daemon (affecting processTriggers
and fireTrigger), and change claimDueTriggers so the capacity check is done
inside the same transaction that claims triggers: inside sql.begin (the
transaction in claimDueTriggers) first compute runningCount (SELECT count(*)
FROM runs WHERE status IN ('leased','running')) under that transaction, compute
available, then SELECT ... FOR UPDATE SKIP LOCKED pending triggers LIMIT
available and UPDATE them (and optionally insert/update run rows to reserve
slots) so capacity is reserved transactionally across daemons; reference
claimDueTriggers, processTriggers, fireTrigger, daemonId, and
config.maxConcurrent when making these edits.
In `@src/lib/team-lead-command.ts`:
- Around line 62-66: In the command-builder in team-lead-command (where parts is
constructed), add an explicit conflict check: if both options.continueName and
options.sessionId are provided, throw a clear Error (e.g. "Cannot specify both
continueName (resume) and sessionId") instead of silently preferring
continueName; otherwise preserve the existing branches that push `--resume
${shellQuote(options.continueName)}` or `--session-id
${shellQuote(options.sessionId)}` to parts.
In `@src/lib/team-manager.ts`:
- Around line 394-398: updateTeamConfig currently writes the team file directly
and needs to use the same file-locking used elsewhere to avoid lost concurrent
writes; modify updateTeamConfig to call acquireLock on the team file (using
teamFilePath(name)), await acquiring the lock, perform the JSON write with
writeFile while holding the lock, and always release the lock in a finally block
(mirror the locking pattern used in setTeamStatus) so concurrent callers
serialize updates.
In `@src/term-commands/agents.ts`:
- Around line 547-559: In resolveSpawnTeamWindow, sanitize the team name before
passing it to tmux.ensureTeamWindow to avoid tmux parsing dots as pane
separators: import/use the existing sanitizeWindowName function and compute e.g.
const safeTeam = sanitizeWindowName(team) (or similar) and pass safeTeam to
ensureTeamWindow(sessionName, safeTeam, cwd); keep the existing sessionName
resolution logic but ensure any call to tmux.ensureTeamWindow uses the sanitized
team string (matching how team-auto-spawn.ts and session.ts call
sanitizeWindowName).
In `@src/term-commands/daemon.ts`:
- Around line 45-52: The PID file currently contains only a bare PID which can
be reused by an unrelated process; change the PID file format to persist a
process identity token (e.g., PID + process start timestamp or a UUID generated
by the daemon on startup) and update validation logic to verify that the PID in
the file matches the same process instance before trusting it. Specifically,
modify the pid write path (where the daemon creates the PID file) to write
"pid:identity" (or JSON with pid and startTime/uuid), update readPid() to parse
both fields and return a structured object (or null) and add/replace the simple
kill(pid,0) check in isProcessAlive()/stop()/status()/start() flows with a
validateProcess(pid, identity) routine that reads /proc/<pid>/stat starttime or
compares the stored UUID/command line to the live process; if the identity does
not match, treat the PID file as stale and ignore/remove it. Ensure all callers
(readPid, isProcessAlive, start, stop, status) use the new validation before
acting on the PID.
- Around line 130-167: The daemonInstallCommand function (and the other CLI
handlers in this file) currently uses console.log for user-facing messages;
replace all console.log calls in daemonInstallCommand (and in the other ranges
noted) with the project's centralized command output helper used by other
commands (the non-console output path), e.g. import the shared output/logger API
used across src CLI modules and call it instead of console.log, preserving the
exact message text and using the helper's error/info methods where appropriate;
update all occurrences in daemonInstallCommand (references: generateSystemdUnit,
systemdUnitPath, systemdDir, and spawnSync result handling) so no direct
console.log calls remain.
In `@src/term-commands/db.ts`:
- Around line 100-114: The shutdown() call must be moved into a finally block so
it always runs even if an error occurs; remove the existing await shutdown()
from inside the try, add a finally { await shutdown(); } after the catch (or
convert the try/catch into try { ... } catch (err) { ... } finally { await
shutdown(); }), keeping the current error handling that builds message from err
and the console.log('') intact; reference the shutdown() symbol and the
try/catch that currently surrounds the loop that queries tables to locate where
to make the change.
In `@src/term-commands/schedule.ts`:
- Around line 93-117: computeFirstDueAt currently ignores options.timezone when
calculating the initial dueAt (it calls parseAbsoluteTime, parseDuration and
computeNextCronDue without timezone), so either incorporate the timezone into
those calculations or reject the flag for unsupported paths: update
parseAbsoluteTime and computeNextCronDue (or call timezone-aware variants) to
accept options.timezone and use it when computing dueAt for the 'at' and cron
branches, and ensure the interval branch uses timezone-consistent logic or throw
an error if options.timezone is provided but not supported; reference
computeFirstDueAt, parseAbsoluteTime, computeNextCronDue, isCronExpression and
parseDuration when making the change.
In `@src/term-commands/state.test.ts`:
- Around line 220-245: The test setup for ensureWorkPushed() should isolate
GENIE global state: in the beforeEach block (where repoDir and originalCwd are
set and process.chdir(repoDir) is called) set process.env.GENIE_HOME to a
temporary directory (e.g., using repoDir or a new tmpdir) and save the original
value; in the afterEach block restore the original process.env.GENIE_HOME
(alongside restoring process.cwd and removing repoDir) so tests do not pollute
global state.
In `@src/term-commands/state.ts`:
- Around line 111-145: The ensureWorkPushed function currently swallows all git
errors and uses console.log; update ensureWorkPushed to surface failures by
throwing a descriptive Error when commit or push steps fail (so callers like
doneCommand cannot continue on unpushed work) and remove/replace all console.log
calls with the project's logging API (e.g., processLogger.debug/info/error) or
drop purely informational messages; specifically, ensure failures in the "git
status/commit" block and in the "git log.../git push" block propagate as thrown
errors, and replace the six console.log occurrences inside ensureWorkPushed with
appropriate logger calls or remove them if unnecessary.
In `@src/term-commands/team.ts`:
- Around line 259-264: The team name from CLI must be sanitized before being
used in tmux window operations: in resolveSpawnTeamWindow call
sanitizeWindowName (import it from ../genie-commands/session.js) and pass the
sanitized value instead of raw config.name to ensureTeamWindow (and any other
tmux-related calls that use the team string); update the import list to include
sanitizeWindowName and replace usages of the unsanitized team parameter in
resolveSpawnTeamWindow/ensureTeamWindow invocation so dots and other special
chars are escaped for tmux.
---
Outside diff comments:
In `@src/genie-commands/session.ts`:
- Around line 197-225: Rename outdated "continue" identifiers to "resume" for
consistency: change function launchWithContinueFallback to
launchWithResumeFallback, rename continueName to resumeName and continueCmd to
resumeCmd, and update any call sites and usages (e.g., where buildClaudeCommand
is invoked and tmux.executeTmux calls) to use these new names; ensure the logic
and parameters remain unchanged and update any console messages or comments
referencing "continue" to "resume".
In `@src/lib/tmux.ts`:
- Around line 48-71: The current getCurrentSessionName function uses substring
matching (s.name.includes(hint)) which can return unintended sessions; update
the hint matching to first look for an exact match (e.g., s.name === hint) and
return that if found, and only then fall back to substring matching
(s.name.includes(hint)) if no exact match exists; modify the logic inside
getCurrentSessionName where sessions.find is used so it tries exact match first,
then substring, ensuring correct session selection when a hint is provided.
In `@src/term-commands/agents.ts`:
- Around line 610-623: The layout is being applied to the current session
instead of the resolved override — update launchTmuxSpawn so it passes the
resolved target to applySpawnLayout rather than letting applySpawnLayout call
getCurrentSessionName(); after createTmuxPane(...) call use the resolved
teamWindow (or teamWindow.windowId/session) when invoking applySpawnLayout (and
if needed adjust applySpawnLayout's signature to accept a targetSession/windowId
and use that instead of getCurrentSessionName), referencing the functions
launchTmuxSpawn, createTmuxPane, applySpawnLayout and the teamWindow.windowId
value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 72c97576-35c0-4f1b-9308-b4d2086d610e
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (42)
.claude-plugin/marketplace.jsonopenclaw.plugin.jsonpackage.jsonplugins/genie/.claude-plugin/plugin.jsonplugins/genie/package.jsonsrc/db/migrations/001_initial.sqlsrc/db/migrations/002_scheduler_extensions.sqlsrc/db/migrations/003_machine_snapshots.sqlsrc/genie-commands/__tests__/session.test.tssrc/genie-commands/session.tssrc/genie.tssrc/hooks/handlers/auto-spawn.tssrc/lib/cron.test.tssrc/lib/cron.tssrc/lib/db-migrations.tssrc/lib/db.tssrc/lib/protocol-router-spawn.tssrc/lib/protocol-router.tssrc/lib/provider-adapters.tssrc/lib/run-spec.test.tssrc/lib/run-spec.tssrc/lib/scheduler-daemon.test.tssrc/lib/scheduler-daemon.tssrc/lib/spawn-command.test.tssrc/lib/spawn-command.tssrc/lib/team-lead-command.tssrc/lib/team-manager.tssrc/lib/tmux.tssrc/lib/wish-state.test.tssrc/lib/wish-state.tssrc/term-commands/agents.tssrc/term-commands/daemon.test.tssrc/term-commands/daemon.tssrc/term-commands/db.tssrc/term-commands/dispatch.tssrc/term-commands/msg.test.tssrc/term-commands/schedule.test.tssrc/term-commands/schedule.tssrc/term-commands/state.test.tssrc/term-commands/state.tssrc/term-commands/team.tssrc/types/pgserve.d.ts
Comment on lines
+7
to
+22
| CREATE TABLE schedules ( | ||
| id TEXT PRIMARY KEY, | ||
| name TEXT NOT NULL, | ||
| cron_expression TEXT NOT NULL, | ||
| timezone TEXT DEFAULT 'UTC', | ||
| command TEXT, | ||
| metadata JSONB DEFAULT '{}', | ||
| status TEXT NOT NULL DEFAULT 'active' | ||
| CHECK (status IN ('active', 'paused', 'deleted')), | ||
| created_at TIMESTAMPTZ NOT NULL DEFAULT now(), | ||
| updated_at TIMESTAMPTZ NOT NULL DEFAULT now() | ||
| ); | ||
|
|
||
| CREATE INDEX idx_schedules_status ON schedules(status); | ||
| CREATE INDEX idx_schedules_name ON schedules(name); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/db/migrations/001_initial.sqlRepository: automagik-dev/genie
Length of output: 6048
🏁 Script executed:
cat -n src/term-commands/schedule.ts | head -100Repository: automagik-dev/genie
Length of output: 3616
🏁 Script executed:
rg -A 5 "name-based\|by.*name\|WHERE.*name" src/term-commands/schedule.tsRepository: automagik-dev/genie
Length of output: 45
🏁 Script executed:
rg "WHERE\|SELECT.*schedules\|cancel\|retry\|history" src/term-commands/schedule.ts -A 3 -B 1Repository: automagik-dev/genie
Length of output: 45
🏁 Script executed:
rg "UPDATE.*schedules\|UPDATE.*triggers" src/term-commands/schedule.ts -A 3 -B 1Repository: automagik-dev/genie
Length of output: 45
🏁 Script executed:
cat -n src/term-commands/schedule.ts | grep -A 50 "cancel\|retry\|history" | head -100Repository: automagik-dev/genie
Length of output: 3511
🏁 Script executed:
cat -n src/term-commands/schedule.ts | sed -n '370,430p'Repository: automagik-dev/genie
Length of output: 2353
🏁 Script executed:
rg "UPDATE triggers SET status" src/term-commands/schedule.ts -B 3 -A 3Repository: automagik-dev/genie
Length of output: 253
🏁 Script executed:
cat -n src/term-commands/schedule.ts | sed -n '430,480p'Repository: automagik-dev/genie
Length of output: 1804
🏁 Script executed:
rg "schedule create\|INSERT INTO schedules" src/term-commands/schedule.ts -B 5 -A 10Repository: automagik-dev/genie
Length of output: 45
🏁 Script executed:
rg "async function scheduleCreateCommand\|INSERT INTO schedules" src/term-commands/schedule.ts -B 5 -A 15Repository: automagik-dev/genie
Length of output: 45
🏁 Script executed:
cat -n src/term-commands/schedule.ts | sed -n '200,280p'Repository: automagik-dev/genie
Length of output: 3464
Add partial unique index to enforce active schedule-name uniqueness in the database.
The application checks for duplicate active names before creating a schedule, but this check happens outside the transaction. Two concurrent creates can both pass the check and insert separate active rows with the same name, causing later cancel/history/retry calls by name to be ambiguous or operate on the wrong schedule. Add CREATE UNIQUE INDEX idx_schedules_active_name ON schedules(name) WHERE status = 'active' to enforce this at the schema level.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/db/migrations/001_initial.sql` around lines 7 - 22, Add a partial unique
index on schedules to prevent duplicate active names by creating a unique index
named idx_schedules_active_name on the name column where status = 'active'
(i.e., CREATE UNIQUE INDEX idx_schedules_active_name ON schedules(name) WHERE
status = 'active'); update the migration that defines the schedules table (table
name: schedules, existing indexes: idx_schedules_status and idx_schedules_name)
to include this new index and ensure the migration handles or documents any
pre-existing active-name duplicates before applying.
Comment on lines
+119
to
+131
| CREATE OR REPLACE FUNCTION notify_trigger_due() | ||
| RETURNS trigger AS $$ | ||
| BEGIN | ||
| PERFORM pg_notify('genie_trigger_due', NEW.id::text); | ||
| RETURN NEW; | ||
| END; | ||
| $$ LANGUAGE plpgsql; | ||
|
|
||
| CREATE TRIGGER trg_notify_due | ||
| AFTER INSERT ON triggers | ||
| FOR EACH ROW | ||
| WHEN (NEW.status = 'pending') | ||
| EXECUTE FUNCTION notify_trigger_due(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "001_initial.sql" -o -name "*migrations*" -type dRepository: automagik-dev/genie
Length of output: 118
🏁 Script executed:
wc -l ./src/db/migrations/001_initial.sqlRepository: automagik-dev/genie
Length of output: 102
🏁 Script executed:
cat -n ./src/db/migrations/001_initial.sqlRepository: automagik-dev/genie
Length of output: 6048
🏁 Script executed:
rg "status.*=.*['\"]pending['\"]|pending.*status" --type sql --type js --type ts -B 2 -A 2Repository: automagik-dev/genie
Length of output: 2985
Add AFTER UPDATE trigger to notify when triggers transition back to pending.
trg_notify_due only fires on INSERT. The retry path in scheduler-daemon.ts (timeout recovery) and schedule.ts (manual re-queue) both update existing rows back to status = 'pending', but no notification is sent. The daemon must wait for the fallback poll instead of being notified immediately. Add an AFTER UPDATE branch that fires on transitions to pending status.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/db/migrations/001_initial.sql` around lines 119 - 131, The current
trigger trg_notify_due only fires AFTER INSERT; add an AFTER UPDATE branch that
calls the same notify_trigger_due() function when a row transitions back to
pending so updates that set status = 'pending' notify the daemon immediately.
Implement an UPDATE trigger on the triggers table with WHEN (OLD.status IS
DISTINCT FROM 'pending' AND NEW.status = 'pending') (or OLD.status <> 'pending'
AND NEW.status = 'pending') and EXECUTE FUNCTION notify_trigger_due(); keep the
existing INSERT trigger intact and reuse the notify_trigger_due() function and
trg_notify_due naming to group related behavior.
Comment on lines
+3
to
+15
| CREATE TABLE IF NOT EXISTS machine_snapshots ( | ||
| id TEXT PRIMARY KEY, | ||
| active_workers INTEGER NOT NULL DEFAULT 0, | ||
| active_teams INTEGER NOT NULL DEFAULT 0, | ||
| tmux_sessions INTEGER NOT NULL DEFAULT 0, | ||
| cpu_percent REAL, | ||
| memory_mb REAL, | ||
| context JSONB DEFAULT '{}', | ||
| created_at TIMESTAMPTZ NOT NULL DEFAULT now() | ||
| ); | ||
|
|
||
| CREATE INDEX IF NOT EXISTS idx_machine_snapshots_created | ||
| ON machine_snapshots(created_at); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
**Consider retention policy for snapshots.**Machine snapshots can accumulate without bounds. "Having partitions for different time spans makes it more efficient to drop/delete/expire old data." Consider adding a retention cleanup mechanism or documenting expected growth.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/db/migrations/003_machine_snapshots.sql` around lines 3 - 15, The
machine_snapshots table can grow unbounded; add a retention strategy for
machine_snapshots (e.g., time-based partitioning on created_at or a scheduled
cleanup) and implement a mechanism to expire old data: convert machine_snapshots
into range partitions by created_at (or create monthly/weekly partitions) and
add a scheduled job (pg_cron or a DB-side function + external scheduler) to drop
old partitions, or if partitioning is not desired add a cleanup function that
deletes rows older than a configurable retention period and schedule it to run
regularly; update related index idx_machine_snapshots_created and any queries
using created_at to work with the chosen approach and document the retention
configuration.
| tmux_sessions INTEGER NOT NULL DEFAULT 0, | ||
| cpu_percent REAL, | ||
| memory_mb REAL, | ||
| context JSONB DEFAULT '{}', |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
context column allows NULL despite having a DEFAULT.
The context column has DEFAULT '{}' but lacks NOT NULL, so explicit NULL inserts bypass the default. Other columns consistently use NOT NULL DEFAULT. If context should always be a valid JSONB object, add NOT NULL:
♻️ Add NOT NULL for consistency
- context JSONB DEFAULT '{}',
+ context JSONB NOT NULL DEFAULT '{}',📝 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.
Suggested change
| context JSONB DEFAULT '{}', | |
| context JSONB NOT NULL DEFAULT '{}', |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/db/migrations/003_machine_snapshots.sql` at line 10, The column
definition for context should be changed to enforce non-null JSONB values:
update the migration's column line from "context JSONB DEFAULT '{}'," to
"context JSONB NOT NULL DEFAULT '{}'," so explicit NULL inserts are rejected and
default applies consistently; update any corresponding schema/ORM model to match
if present.
Comment on lines
+44
to
+70
| function expandRange(range: string, step: number, min: number, max: number): number[] { | ||
| if (range === '*') { | ||
| const out: number[] = []; | ||
| for (let i = min; i <= max; i += step) out.push(i); | ||
| return out; | ||
| } | ||
| if (range.includes('-')) { | ||
| const [start, end] = range.split('-').map(Number); | ||
| const out: number[] = []; | ||
| for (let i = start; i <= end; i += step) out.push(i); | ||
| return out; | ||
| } | ||
| return [Number.parseInt(range, 10)]; | ||
| } | ||
|
|
||
| /** | ||
| * Parse a single cron field into a sorted array of valid integer values. | ||
| * Supports: wildcards (*), ranges (1-5), steps (star/5, 1-10/2), lists (1,3,5) | ||
| */ | ||
| function parseCronField(field: string, min: number, max: number): number[] { | ||
| const values = new Set<number>(); | ||
|
|
||
| for (const part of field.split(',')) { | ||
| const stepMatch = part.match(/^(.+)\/(\d+)$/); | ||
| const step = stepMatch ? Number.parseInt(stepMatch[2], 10) : 1; | ||
| const range = stepMatch ? stepMatch[1] : part; | ||
| for (const v of expandRange(range, step, min, max)) values.add(v); |
There was a problem hiding this comment.
Validate step and numeric bounds during cron field expansion.
The parser currently allows */0 and out-of-range values, which can hang expansion loops or defer invalid input to a slow 366-day scan.
Proposed fix
function expandRange(range: string, step: number, min: number, max: number): number[] {
+ if (!Number.isInteger(step) || step <= 0) {
+ throw new Error(`Invalid cron step "${step}". Step must be >= 1`);
+ }
+
if (range === '*') {
const out: number[] = [];
for (let i = min; i <= max; i += step) out.push(i);
return out;
}
if (range.includes('-')) {
const [start, end] = range.split('-').map(Number);
+ if (!Number.isInteger(start) || !Number.isInteger(end) || start < min || end > max || start > end) {
+ throw new Error(`Invalid cron range "${range}". Expected ${min}-${max}`);
+ }
const out: number[] = [];
for (let i = start; i <= end; i += step) out.push(i);
return out;
}
- return [Number.parseInt(range, 10)];
+ const value = Number.parseInt(range, 10);
+ if (!Number.isInteger(value) || value < min || value > max) {
+ throw new Error(`Invalid cron value "${range}". Expected ${min}-${max}`);
+ }
+ return [value];
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/cron.ts` around lines 44 - 70, The cron parser must validate step and
numeric bounds to avoid /0 and out-of-range loops: update expandRange(range,
step, min, max) to throw on invalid step (step <= 0) and on invalid numeric
ranges (non-number, start > end, or start/end outside [min,max]); also update
parseCronField(field, min, max) to validate parsed single numbers and step parts
(ensure parsed step > 0 and numeric values are within min..max) before calling
expandRange, so invalid inputs produce a clear error instead of hanging or
producing large scans; reference functions: expandRange and parseCronField.
Comment on lines
+100
to
+114
| for (const table of tables) { | ||
| const countResult = await sql.unsafe(`SELECT count(*) AS cnt FROM "${table.tablename}"`); | ||
| const count = countResult[0].cnt; | ||
| console.log(` ${padRight(table.tablename, maxNameLen)} ${count}`); | ||
| } | ||
| } | ||
|
|
||
| await shutdown(); | ||
| } catch (err) { | ||
| const message = err instanceof Error ? err.message : String(err); | ||
| console.error(`\n Error querying database: ${message}`); | ||
| } | ||
|
|
||
| console.log(''); | ||
| } |
There was a problem hiding this comment.
shutdown() should be in a finally block to ensure cleanup on error.
If the row count query fails mid-loop, shutdown() won't be called, potentially leaving connections open.
🔧 Proposed fix
async function dbStatusCommand(): Promise<void> {
// ... port/dataDir checks ...
try {
const sql = await getConnection();
-
- // ... queries ...
-
- await shutdown();
+ try {
+ // ... queries ...
+ } finally {
+ await shutdown();
+ }
} catch (err) {
const message = err instanceof Error ? err.message : String(err);
console.error(`\n Error querying database: ${message}`);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/term-commands/db.ts` around lines 100 - 114, The shutdown() call must be
moved into a finally block so it always runs even if an error occurs; remove the
existing await shutdown() from inside the try, add a finally { await shutdown();
} after the catch (or convert the try/catch into try { ... } catch (err) { ... }
finally { await shutdown(); }), keeping the current error handling that builds
message from err and the console.log('') intact; reference the shutdown() symbol
and the try/catch that currently surrounds the loop that queries tables to
locate where to make the change.
Comment on lines
+93
to
+117
| function computeFirstDueAt(options: CreateOptions): { dueAt: Date; cronExpr: string; scheduleType: string } { | ||
| if (options.at) { | ||
| const dueAt = parseAbsoluteTime(options.at); | ||
| if (dueAt.getTime() <= Date.now()) { | ||
| throw new Error(`Schedule time is in the past: ${options.at}`); | ||
| } | ||
| return { dueAt, cronExpr: '@once', scheduleType: 'once' }; | ||
| } | ||
|
|
||
| if (options.after) { | ||
| const delayMs = parseDuration(options.after); | ||
| const dueAt = new Date(Date.now() + delayMs); | ||
| return { dueAt, cronExpr: '@once', scheduleType: 'once' }; | ||
| } | ||
|
|
||
| if (options.every) { | ||
| if (isCronExpression(options.every)) { | ||
| // Store cron expression directly | ||
| const dueAt = computeNextCronDue(options.every); | ||
| return { dueAt, cronExpr: options.every, scheduleType: 'cron' }; | ||
| } | ||
| // Parse as interval duration | ||
| const intervalMs = parseDuration(options.every); | ||
| const dueAt = new Date(Date.now() + intervalMs); | ||
| return { dueAt, cronExpr: `@every ${options.every.trim()}`, scheduleType: 'interval' }; |
There was a problem hiding this comment.
--timezone is stored but never used for the first due time.
computeFirstDueAt() never reads options.timezone; it parses --at with new Date(...) and computes cron due times with computeNextCronDue(options.every) only. The first trigger can therefore be scheduled in the wrong zone even though the row persists a timezone. Either use the timezone during initial due-time calculation or reject the flag until that path supports it.
Also applies to: 208-213
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/term-commands/schedule.ts` around lines 93 - 117, computeFirstDueAt
currently ignores options.timezone when calculating the initial dueAt (it calls
parseAbsoluteTime, parseDuration and computeNextCronDue without timezone), so
either incorporate the timezone into those calculations or reject the flag for
unsupported paths: update parseAbsoluteTime and computeNextCronDue (or call
timezone-aware variants) to accept options.timezone and use it when computing
dueAt for the 'at' and cron branches, and ensure the interval branch uses
timezone-consistent logic or throw an error if options.timezone is provided but
not supported; reference computeFirstDueAt, parseAbsoluteTime,
computeNextCronDue, isCronExpression and parseDuration when making the change.
Comment on lines
+220
to
+245
| describe('ensureWorkPushed()', () => { | ||
| let repoDir: string; | ||
| let originalCwd: string; | ||
|
|
||
| beforeEach(async () => { | ||
| originalCwd = process.cwd(); | ||
| repoDir = join('/tmp', `push-test-${Date.now()}-${Math.random().toString(36).slice(2)}`); | ||
| await mkdir(repoDir, { recursive: true }); | ||
|
|
||
| // Initialize a git repo — ensureWorkPushed uses execSync which operates in process.cwd() | ||
| execSync('git init', { cwd: repoDir, encoding: 'utf-8' }); | ||
| execSync('git config user.email "test@test.com"', { cwd: repoDir, encoding: 'utf-8' }); | ||
| execSync('git config user.name "Test"', { cwd: repoDir, encoding: 'utf-8' }); | ||
|
|
||
| // Create initial commit | ||
| await writeFile(join(repoDir, 'README.md'), '# Test'); | ||
| execSync('git add -A && git commit -m "init"', { cwd: repoDir, encoding: 'utf-8' }); | ||
|
|
||
| // ensureWorkPushed uses execSync with no cwd, so we must chdir | ||
| process.chdir(repoDir); | ||
| }); | ||
|
|
||
| afterEach(async () => { | ||
| process.chdir(originalCwd); | ||
| await rm(repoDir, { recursive: true, force: true }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider setting GENIE_HOME to isolate global state.
Per coding guidelines, tests should set process.env.GENIE_HOME to tmpdir to prevent test pollution of global state.
♻️ Proposed fix
describe('ensureWorkPushed()', () => {
let repoDir: string;
let originalCwd: string;
+ let originalGenieHome: string | undefined;
beforeEach(async () => {
originalCwd = process.cwd();
+ originalGenieHome = process.env.GENIE_HOME;
repoDir = join('/tmp', `push-test-${Date.now()}-${Math.random().toString(36).slice(2)}`);
await mkdir(repoDir, { recursive: true });
+ process.env.GENIE_HOME = repoDir;
// ... rest of setup
});
afterEach(async () => {
process.chdir(originalCwd);
+ process.env.GENIE_HOME = originalGenieHome;
await rm(repoDir, { recursive: true, force: true });
});As per coding guidelines: "Set process.env.GENIE_HOME to tmpdir in tests to isolate global state."
📝 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.
Suggested change
| describe('ensureWorkPushed()', () => { | |
| let repoDir: string; | |
| let originalCwd: string; | |
| beforeEach(async () => { | |
| originalCwd = process.cwd(); | |
| repoDir = join('/tmp', `push-test-${Date.now()}-${Math.random().toString(36).slice(2)}`); | |
| await mkdir(repoDir, { recursive: true }); | |
| // Initialize a git repo — ensureWorkPushed uses execSync which operates in process.cwd() | |
| execSync('git init', { cwd: repoDir, encoding: 'utf-8' }); | |
| execSync('git config user.email "test@test.com"', { cwd: repoDir, encoding: 'utf-8' }); | |
| execSync('git config user.name "Test"', { cwd: repoDir, encoding: 'utf-8' }); | |
| // Create initial commit | |
| await writeFile(join(repoDir, 'README.md'), '# Test'); | |
| execSync('git add -A && git commit -m "init"', { cwd: repoDir, encoding: 'utf-8' }); | |
| // ensureWorkPushed uses execSync with no cwd, so we must chdir | |
| process.chdir(repoDir); | |
| }); | |
| afterEach(async () => { | |
| process.chdir(originalCwd); | |
| await rm(repoDir, { recursive: true, force: true }); | |
| }); | |
| describe('ensureWorkPushed()', () => { | |
| let repoDir: string; | |
| let originalCwd: string; | |
| let originalGenieHome: string | undefined; | |
| beforeEach(async () => { | |
| originalCwd = process.cwd(); | |
| originalGenieHome = process.env.GENIE_HOME; | |
| repoDir = join('/tmp', `push-test-${Date.now()}-${Math.random().toString(36).slice(2)}`); | |
| await mkdir(repoDir, { recursive: true }); | |
| process.env.GENIE_HOME = repoDir; | |
| // Initialize a git repo — ensureWorkPushed uses execSync which operates in process.cwd() | |
| execSync('git init', { cwd: repoDir, encoding: 'utf-8' }); | |
| execSync('git config user.email "test@test.com"', { cwd: repoDir, encoding: 'utf-8' }); | |
| execSync('git config user.name "Test"', { cwd: repoDir, encoding: 'utf-8' }); | |
| // Create initial commit | |
| await writeFile(join(repoDir, 'README.md'), '# Test'); | |
| execSync('git add -A && git commit -m "init"', { cwd: repoDir, encoding: 'utf-8' }); | |
| // ensureWorkPushed uses execSync with no cwd, so we must chdir | |
| process.chdir(repoDir); | |
| }); | |
| afterEach(async () => { | |
| process.chdir(originalCwd); | |
| process.env.GENIE_HOME = originalGenieHome; | |
| await rm(repoDir, { recursive: true, force: true }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/term-commands/state.test.ts` around lines 220 - 245, The test setup for
ensureWorkPushed() should isolate GENIE global state: in the beforeEach block
(where repoDir and originalCwd are set and process.chdir(repoDir) is called) set
process.env.GENIE_HOME to a temporary directory (e.g., using repoDir or a new
tmpdir) and save the original value; in the afterEach block restore the original
process.env.GENIE_HOME (alongside restoring process.cwd and removing repoDir) so
tests do not pollute global state.
Comment on lines
+111
to
+145
| export async function ensureWorkPushed(slug: string, group: string): Promise<void> { | ||
| // 1. Commit dirty working tree as WIP | ||
| try { | ||
| const porcelain = execSync('git status --porcelain', { encoding: 'utf-8' }).trim(); | ||
| if (porcelain) { | ||
| console.log(' Committing dirty working tree...'); | ||
| execSync('git add -A', { encoding: 'utf-8' }); | ||
| execSync(`git commit -m "wip: ${slug}#${group}"`, { encoding: 'utf-8' }); | ||
| console.log(` Committed as "wip: ${slug}#${group}"`); | ||
| } | ||
| } catch { | ||
| // git status or commit failed — may not be in a git repo | ||
| } | ||
|
|
||
| // 2. Push unpushed commits | ||
| try { | ||
| const unpushed = execSync('git log @{u}..HEAD --oneline', { encoding: 'utf-8' }).trim(); | ||
| if (unpushed) { | ||
| console.log(' Pushing unpushed commits...'); | ||
| execSync('git push', { encoding: 'utf-8', timeout: 30000 }); | ||
| console.log(' Push complete.'); | ||
| } | ||
| } catch { | ||
| // No upstream tracking or push failed — best-effort | ||
| try { | ||
| // Try pushing with --set-upstream for new branches | ||
| const branch = execSync('git rev-parse --abbrev-ref HEAD', { encoding: 'utf-8' }).trim(); | ||
| if (branch && branch !== 'HEAD') { | ||
| execSync(`git push -u origin ${branch}`, { encoding: 'utf-8', timeout: 30000 }); | ||
| console.log(' Push complete (set upstream).'); | ||
| } | ||
| } catch { | ||
| console.log(' ⚠️ Push failed — manual push may be needed.'); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Get the full file content to understand the context
wc -l src/term-commands/state.tsRepository: automagik-dev/genie
Length of output: 93
🏁 Script executed:
# Read the file with line numbers to see the full context
cat -n src/term-commands/state.ts | head -200Repository: automagik-dev/genie
Length of output: 8391
🏁 Script executed:
# Search for doneCommand to see how ensureWorkPushed is called
rg -A 30 'function doneCommand|export.*doneCommand' src/term-commands/state.tsRepository: automagik-dev/genie
Length of output: 1262
Address git failures and remove console.log from source code.
The ensureWorkPushed() function silently swallows all git commit/push failures in empty catch blocks and returns Promise<void> with no error indication. This allows doneCommand() to proceed to wave completion detection, team-lead notification, and pane termination even when work was never pushed. The function should throw on failure to prevent the pane from being killed with unpushed commits.
Additionally, this file contains six console.log() calls (lines 116, 119, 129, 131, 140, 143) which violate the coding guideline prohibiting console.log in source code. Replace with proper logging mechanism or remove if informational only.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/term-commands/state.ts` around lines 111 - 145, The ensureWorkPushed
function currently swallows all git errors and uses console.log; update
ensureWorkPushed to surface failures by throwing a descriptive Error when commit
or push steps fail (so callers like doneCommand cannot continue on unpushed
work) and remove/replace all console.log calls with the project's logging API
(e.g., processLogger.debug/info/error) or drop purely informational messages;
specifically, ensure failures in the "git status/commit" block and in the "git
log.../git push" block propagate as thrown errors, and replace the six
console.log occurrences inside ensureWorkPushed with appropriate logger calls or
remove them if unnecessary.
Comment on lines
259
to
264
| await handleWorkerSpawn('team-lead', { | ||
| provider: 'claude', | ||
| team: config.name, | ||
| cwd: config.worktreePath, | ||
| initialPrompt: kickoffPrompt, | ||
| session: tmuxSession, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if sanitizeWindowName is applied to team names before tmux operations
rg -nP 'sanitizeWindowName|ensureTeamWindow' --type=ts -A3 -B3Repository: automagik-dev/genie
Length of output: 13143
🏁 Script executed:
fd -e ts -e tsx | xargs rg -l 'handleWorkerSpawn' | head -20Repository: automagik-dev/genie
Length of output: 159
🏁 Script executed:
rg -nP 'function handleWorkerSpawn|export.*handleWorkerSpawn|const handleWorkerSpawn' --type=ts -A5Repository: automagik-dev/genie
Length of output: 627
🏁 Script executed:
cat -n src/term-commands/team.ts | sed -n '250,270p'Repository: automagik-dev/genie
Length of output: 1400
🏁 Script executed:
sed -n '838,900p' src/term-commands/agents.tsRepository: automagik-dev/genie
Length of output: 2342
🏁 Script executed:
rg -nP 'interface SpawnOptions|type SpawnOptions' --type=ts -A10Repository: automagik-dev/genie
Length of output: 1945
🏁 Script executed:
rg -nP 'team.*auto.*spawn|spawnTeam' --type=ts -B2 -A5Repository: automagik-dev/genie
Length of output: 3935
🏁 Script executed:
rg -nP 'function buildSpawnParams|export.*buildSpawnParams' --type=ts -A15Repository: automagik-dev/genie
Length of output: 2370
🏁 Script executed:
rg -nP 'ensureTeamWindow' --type=ts -B5 -A5 | head -80Repository: automagik-dev/genie
Length of output: 4788
🏁 Script executed:
sed -n '838,920p' src/term-commands/agents.tsRepository: automagik-dev/genie
Length of output: 3022
🏁 Script executed:
rg -nP 'spawnAgentInTmux|executeSpawnCommand' --type=ts -A20 | head -100Repository: automagik-dev/genie
Length of output: 45
🏁 Script executed:
rg -nP 'function launchTmuxSpawn|export.*launchTmuxSpawn' --type=ts -A30Repository: automagik-dev/genie
Length of output: 2083
🏁 Script executed:
rg -nP 'ensureTeamWindow.*team' --type=ts -B3 -A3Repository: automagik-dev/genie
Length of output: 1188
🏁 Script executed:
sed -n '545,565p' src/term-commands/agents.tsRepository: automagik-dev/genie
Length of output: 746
🏁 Script executed:
rg -nP 'import.*sanitizeWindowName' --type=tsRepository: automagik-dev/genie
Length of output: 298
🏁 Script executed:
sed -n '266,290p' src/lib/tmux.tsRepository: automagik-dev/genie
Length of output: 1045
🏁 Script executed:
rg -nP 'findWindowByName' --type=ts -A5 | head -50Repository: automagik-dev/genie
Length of output: 1817
Sanitize team name before passing to ensureTeamWindow at line 559.
Team names from user CLI input flow unsanitized through to tmux operations. Dots are misinterpreted as pane separators, causing "can't find pane" errors. Import sanitizeWindowName from ../genie-commands/session.js and apply it to the team parameter before calling ensureTeamWindow in resolveSpawnTeamWindow.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/term-commands/team.ts` around lines 259 - 264, The team name from CLI
must be sanitized before being used in tmux window operations: in
resolveSpawnTeamWindow call sanitizeWindowName (import it from
../genie-commands/session.js) and pass the sanitized value instead of raw
config.name to ensureTeamWindow (and any other tmux-related calls that use the
team string); update the import list to include sanitizeWindowName and replace
usages of the unsanitized team parameter in
resolveSpawnTeamWindow/ensureTeamWindow invocation so dots and other special
chars are escaped for tmux.
This was referenced Mar 20, 2026
Contributor
Author
|
Closing — wrong branch. Promotion should always be dev→main directly, not via intermediate branch. Recreating as dev→main with current dev state. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Promotion of 3 wishes from dev→main after comprehensive QA validation. 95 files changed, +7122/-4122 lines, 53 commits.
PRs Included
pgserve-embed: Embedded pgserve as genie's persistent brainfire-and-forget: Fire-and-forgetgenie work+ agent auto-exitgenie-scheduler: Scheduler daemon with lease-based trigger claimingQA Evidence Report
Group 1: Static Validation ✅
bun run check— PASStsc --noEmitcleanresolveTargetandformatTranscriptEntryForDisplay)bunx knipclean (pre-existing false positives for biome/commitlint/husky devDeps)Grep Assertions — all PASS:
initialPromptNOT in dispatch.tsORCHESTRATE_POLL_MSNOT in dispatch.ts--continueNOT in provider-adapters.tstmuxSessionNamein TeamConfigteam-manager.ts:54--sessionoption on spawn/team creategenie.ts:102,159ensureWorkPushedin genie donestate.ts:111,203detectWaveCompletionin genie donestate.ts:75,206kill-pane) in donestate.ts:161SELECT FOR UPDATE SKIP LOCKEDin schedulerscheduler-daemon.ts:195idempotency_keycolumn in migration002_scheduler_extensions.sql:13--resume(not--continue) in provider-adaptersprovider-adapters.ts:267Group 2: pgserve E2E ✅
schedules,triggers,runs,heartbeats,audit_events,agent_checkpointsgenie dbcommands wired:status,migrate,query~/.genie/data/pgserve/src/types/pgserve.d.tsGroup 3: Scheduler E2E ✅
genie schedule create/list/cancelcommands implemented insrc/term-commands/schedule.tsgenie daemon installgenerates systemd unit file (daemon.ts:87-126)genie daemon statusshows daemon state and stats (daemon.ts:342+)genie daemon run --foregroundfor systemd ExecStartscheduler-daemon.ts:772+,876)maybeCreateNextTrigger()inserts next pending trigger (scheduler-daemon.ts:269-320)SELECT FOR UPDATE SKIP LOCKED(scheduler-daemon.ts:195)run-spec.ts:44+)run-spec.ts:25)002_scheduler_extensions.sql:13)scheduler-daemon.ts)Group 4: Fire-and-forget E2E ✅
autoOrchestrateCommandexits after spawning — no polling loopgenie sendafter spawn (notinitialPrompt)genie doneflow:completeGroup()→detectWaveCompletion()→ensureWorkPushed()→kill-paneexitprotocol-router-spawn.ts:187-292--resumeflag inprovider-adapters.ts:267(not--continue)target-resolver.ts:117with cleanup callbackagent-registry.ts:285)protocol-router.ts:129)Test plan
bun run checkpasses (typecheck + lint + dead-code + tests)genie doneverifiedSummary by CodeRabbit
New Features
genie daemoncommands for managing the scheduler service (start, stop, status, logs, install).genie dbcommands for database operations (status, migrate, query).genie schedulecommand set for managing scheduled triggers (create, list, cancel, retry, history).--sessionoption togenie team createandgenie spawnfor explicit tmux session control.--continueflag to--resumefor session resumption.genie done.Tests