fix: reduce Genie DX false positives - #2481
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds v2-aware setup completeness to doctor, recognizes self-managed ~/.genie/bin/genie in installer resolution, makes stale-executor depend on heartbeat-expected states plus latest activity, and silently skips ineligible turn-aware workers in scheduler recovery. ChangesSetup v2 config completion diagnostics
Genie-managed binary classification
Heartbeat-aware executor staleness detection
Turn-aware recovery silent filtering
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves the 'doctor' diagnostic tool by correctly identifying setup completion for v2 configurations and recognizing Genie-managed binaries. It also refines agent health monitoring to reduce false 'stale_executor' alerts by incorporating tool activity signals and restricting heartbeat checks to active agent states. Additionally, it streamlines scheduler daemon logging and recovery logic. Feedback identifies a potential regression in 'boot' mode recovery where the new turn-aware filter might incorrectly skip agents in 'error' or 'spawning' states, and points out redundant logic in the 'handleDeadPane' function.
| if (turnAware && worker.state !== 'idle' && !TURN_AWARE_RESUMABLE_STATES.has(worker.state as AgentState)) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
This check introduces a regression for boot mode recovery. In boot mode, the reconciler is intended to bypass the D1/D3 gates to rehydrate agents that were mid-turn (or in an error/spawning state) when the daemon died. By applying the TURN_AWARE_RESUMABLE_STATES filter here without checking the mode, agents in error or spawning states will be skipped during startup recovery, even if they have autoResume: true and a valid session. This filter should be restricted to sweep mode to preserve the aggressive rehydration behavior on boot.
if (turnAware && mode !== 'boot' && worker.state !== 'idle' && !TURN_AWARE_RESUMABLE_STATES.has(worker.state as AgentState)) {
continue;
}| if (turnAware && !TURN_AWARE_RESUMABLE_STATES.has(worker.state as AgentState)) { | ||
| deps.log({ | ||
| timestamp: deps.now().toISOString(), | ||
| level: 'debug', | ||
| event: 'agent_resume_skipped_turn_aware', | ||
| daemon_id: daemonId, | ||
| agent_id: worker.id, | ||
| state: worker.state, | ||
| reason: 'state_not_in_d3', | ||
| }); | ||
| return 'skipped'; | ||
| } |
There was a problem hiding this comment.
This block is now redundant. For mode === 'sweep', the condition is already covered by the hoisted check at line 1225 (for non-idle states) and the idle block at line 1070. For mode === 'boot', the function returns early at line 1069. Removing this block simplifies the function and avoids duplicate logic.
![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: 5980fa0357
ℹ️ 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".
| if (turnAware && worker.state !== 'idle' && !TURN_AWARE_RESUMABLE_STATES.has(worker.state as AgentState)) { | ||
| continue; |
There was a problem hiding this comment.
This new guard runs before handleDeadPane, so it now skips non-D3 states in both sweep and boot passes. In turn-aware mode (default-on), dead-pane workers in states like spawning are now dropped during boot and never reach the boot recovery path that previously attempted resume for legitimate mid-turn crashes; they are also not terminalized here, so they can remain stuck indefinitely after daemon restart.
Useful? React with 👍 / 👎.
| const recentToolAt = row.recentLastToolAt ? Date.parse(row.recentLastToolAt) : Number.NaN; | ||
| const latestActivity = Math.max( | ||
| Number.isFinite(executorUpdated) ? executorUpdated : Number.NEGATIVE_INFINITY, | ||
| Number.isFinite(recentToolAt) ? recentToolAt : Number.NEGATIVE_INFINITY, |
There was a problem hiding this comment.
Using recentLastToolAt as an unconditional liveness proof can hide real stale executors because that timestamp is agent-wide activity, not scoped to the current executor/session. If a previous session for the same agent emitted a recent tool event, latestActivity becomes fresh and suppresses stale_executor even when the current executor heartbeat is stale, producing false negatives in health reporting.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/genie-commands/__tests__/doctor.test.ts`:
- Around line 41-50: Add tests exercising edge cases for
isSetupEffectivelyComplete: assert that isSetupEffectivelyComplete(false, {
version: 1 }) returns false (pre-v2), isSetupEffectivelyComplete(false, {})
returns false (config present but no version), and
isSetupEffectivelyComplete(true, null) returns true (explicit true takes
precedence over missing config). Place these new expectations alongside the
existing describe('setup completion diagnostics') tests so they run with the
current suite and reference the isSetupEffectivelyComplete function.
In `@src/genie-commands/installer-resolution.test.ts`:
- Around line 71-80: Add a unit test that exercises the realPath branch of
isGenieManagedBinary by invoking classifyInstallerResolution with a resolved
object whose path is a non-Genie location and whose realPath points to the
Genie-managed binary (e.g., resolved('/usr/local/bin/genie',
'/home/alice/.genie/bin/genie')); assert the returned rows length is 1,
rows[0].status === 'pass', and rows[0].message contains 'Genie managed binary'
so the symlink/realPath case is covered.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 89fa594a-52ea-495d-96da-6a1e56746759
📒 Files selected for processing (8)
src/genie-commands/__tests__/doctor.test.tssrc/genie-commands/doctor.tssrc/genie-commands/installer-resolution.test.tssrc/genie-commands/installer-resolution.tssrc/lib/agent-observability.test.tssrc/lib/agent-observability.tssrc/lib/scheduler-daemon.test.tssrc/lib/scheduler-daemon.ts
Verified with focused DX tests, lint, build, dogfood checks, and independent review.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/genie-commands/__tests__/doctor.test.ts (1)
46-49:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd boundary assertions for pre-v2 and missing version configs.
The new tests still miss threshold/shape edges for
isSetupEffectivelyComplete(version: 1,{}, and explicittruewithnullconfig).Suggested additions
describe('setup completion diagnostics', () => { test('treats an existing modern v2 config as effectively complete even if the legacy setup flag is false', () => { expect(isSetupEffectivelyComplete(false, { version: 2 })).toBe(true); }); test('still honors explicit setup completion and missing configs', () => { expect(isSetupEffectivelyComplete(true, { version: 2 })).toBe(true); expect(isSetupEffectivelyComplete(false, null)).toBe(false); + expect(isSetupEffectivelyComplete(true, null)).toBe(true); }); + + test('rejects pre-v2 and missing-version configs', () => { + expect(isSetupEffectivelyComplete(false, { version: 1 })).toBe(false); + expect(isSetupEffectivelyComplete(false, {})).toBe(false); + }); });As per coding guidelines, "Test edge cases and boundaries of every command, flag, and plugin contract."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/genie-commands/__tests__/doctor.test.ts` around lines 46 - 49, Add assertions to cover boundary/shape edges for isSetupEffectivelyComplete: assert that isSetupEffectivelyComplete(true, null) === true (explicit setup true with null config), isSetupEffectivelyComplete(false, { version: 1 }) === false (pre-v2 version), and isSetupEffectivelyComplete(false, {}) === false (empty config object) so tests exercise version:1, empty config, and explicit-true-with-null scenarios; update the test block containing isSetupEffectivelyComplete to include these expectations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/genie-commands/doctor.ts`:
- Around line 274-277: The call to loadGenieConfigSync passed into
isSetupEffectivelyComplete can throw on malformed/unreadable configs and abort
`genie doctor`; wrap the genieConfigExists() / loadGenieConfigSync() usage in a
guarded block so loadGenieConfigSync is invoked inside a try/catch, on error
catch and log a warning (include the error) and pass null (or a safe fallback)
into isSetupEffectivelyComplete; update the code around
isSetupEffectivelyComplete/isSetupComplete/genieConfigExists/loadGenieConfigSync
so setupComplete is computed using the guarded result.
In `@src/lib/scheduler-daemon.ts`:
- Around line 1225-1227: The new pre-filter condition is a single long line and
fails Biome formatting; rewrite the if condition in scheduler-daemon.ts to a
Biome-compliant multiline form by breaking the logical checks onto separate
lines and aligning them—for example, put the primary mode check on its own line,
the turnAware check on the next, and the worker.state/resumable-state checks on
subsequent lines, keeping the same boolean logic around mode, turnAware,
worker.state, and TURN_AWARE_RESUMABLE_STATES (and the AgentState cast) so the
body (continue) is unchanged.
---
Duplicate comments:
In `@src/genie-commands/__tests__/doctor.test.ts`:
- Around line 46-49: Add assertions to cover boundary/shape edges for
isSetupEffectivelyComplete: assert that isSetupEffectivelyComplete(true, null)
=== true (explicit setup true with null config),
isSetupEffectivelyComplete(false, { version: 1 }) === false (pre-v2 version),
and isSetupEffectivelyComplete(false, {}) === false (empty config object) so
tests exercise version:1, empty config, and explicit-true-with-null scenarios;
update the test block containing isSetupEffectivelyComplete to include these
expectations.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 8a719f44-1a13-4d50-bc38-d3087fa8b8d0
📒 Files selected for processing (8)
src/genie-commands/__tests__/doctor.test.tssrc/genie-commands/doctor.tssrc/genie-commands/installer-resolution.test.tssrc/genie-commands/installer-resolution.tssrc/lib/agent-observability.test.tssrc/lib/agent-observability.tssrc/lib/scheduler-daemon.test.tssrc/lib/scheduler-daemon.ts
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
src/genie-commands/installer-resolution.test.ts (1)
71-80: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd test coverage for the
realPathbranch.The test verifies the path-match case, but
isGenieManagedBinarychecks bothresolved.pathORresolved.realPath. A symlink scenario wherepathdoesn't match butrealPathdoes remains untested.🧪 Suggested test case
test('passes when realPath is Genie-managed but path is a symlink elsewhere', () => { const rows = classifyInstallerResolution({ resolved: resolved('/usr/local/bin/genie', '/home/alice/.genie/bin/genie'), installers: [], }); expect(rows).toHaveLength(1); expect(rows[0].status).toBe('pass'); expect(rows[0].message).toContain('Genie managed binary'); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/genie-commands/installer-resolution.test.ts` around lines 71 - 80, Add a test covering the realPath branch of isGenieManagedBinary by creating a resolved object whose path is a non-matching symlink and whose realPath points to the Genie-managed location; call classifyInstallerResolution with that resolved (e.g. resolved('/usr/local/bin/genie', '/home/alice/.genie/bin/genie')), then assert rows length is 1, rows[0].status === 'pass' and rows[0].message contains 'Genie managed binary' to ensure the realPath check is exercised.src/genie-commands/doctor.ts (1)
274-277:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
loadGenieConfigSync()to avoid abortinggenie doctor.Line 276 can throw on malformed/unreadable config and stop the whole diagnostics run instead of returning a warning row.
Proposed fix
- const setupComplete = isSetupEffectivelyComplete( - isSetupComplete(), - genieConfigExists() ? loadGenieConfigSync() : null, - ); + let loadedConfig: { version?: number } | null = null; + if (genieConfigExists()) { + try { + loadedConfig = loadGenieConfigSync(); + } catch { + loadedConfig = null; + } + } + const setupComplete = isSetupEffectivelyComplete(isSetupComplete(), loadedConfig);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/genie-commands/doctor.ts` around lines 274 - 277, The call to loadGenieConfigSync() inside the setupComplete calculation can throw on a malformed/unreadable config and abort the whole "genie doctor" run; wrap the config load in a safe guard: check genieConfigExists(), then attempt loadGenieConfigSync() inside a try/catch, on error return null (or a sentinel) and record a warning row for the diagnostics instead of letting the exception bubble. Update the expression used by isSetupEffectivelyComplete(...) (the setupComplete variable) to use the guarded result and ensure any caught error is converted into a warning entry so the doctor command continues running.src/genie-commands/__tests__/doctor.test.ts (1)
46-50:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd the missing boundary assertions for setup completion.
Line 46’s test title says explicit completion + missing config behavior, but it still misses
isSetupEffectivelyComplete(true, null). Also add pre-v2 and missing-version config cases to lock the threshold behavior.As per coding guidelines: "Test edge cases and boundaries of every command, flag, and plugin contract".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/genie-commands/__tests__/doctor.test.ts` around lines 46 - 50, The test is missing boundary assertions for isSetupEffectivelyComplete; update the test block containing isSetupEffectivelyComplete(true, { version: 2 }) / isSetupEffectivelyComplete(false, null) to also assert isSetupEffectivelyComplete(true, null) (explicit completion with missing config), isSetupEffectivelyComplete(false, { version: 1 }) (pre-v2 config should be treated as incomplete), and isSetupEffectivelyComplete(false, { }) or isSetupEffectivelyComplete(false, { version: undefined }) (missing-version config case); ensure expected boolean values reflect that explicit true overrides missing config, version < 2 is incomplete, and missing version is treated as incomplete.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/genie-commands/installer-resolution.ts`:
- Around line 116-118: In isGenieManagedBinary(resolved: ResolvedBinary) the
regex /(^|\/)\.genie\/bin\/genie$/ is duplicated; extract it to a single named
constant (e.g., GENIE_MANAGED_BIN_REGEX) and use that constant for both
resolved.path and resolved.realPath tests so future changes to the path
convention only need one update; update any references in that function to use
the new constant and keep the test logic identical.
In `@src/lib/agent-observability.test.ts`:
- Around line 105-111: The test creates a row with executorUpdatedAt computed
from Date.now() but calls assessHealth(row) without passing the same explicit
time; update the test to compute a single const now = Date.now(), use that to
set row.executorUpdatedAt (now - STALE_EXECUTOR_WINDOW_MS -
60_000).toISOString(), and pass that same now as the second argument to
assessHealth(row, now) so the test is deterministic and consistent with the
earlier test pattern (referencing assessHealth, baseRow, and
STALE_EXECUTOR_WINDOW_MS).
In `@src/lib/agent-observability.ts`:
- Around line 161-177: recentLastToolAt is computed agent-wide but is being used
to decide per-executor liveness (masking stale executors); update the logic so
executor staleness only uses executor-scoped timestamps or change the SQL to
produce an executor/session-scoped recentLastToolAt. Concretely, either (A)
modify the DB view/migrations so recentLastToolAt is aggregated by
executor/session (so the value returned to the code is executor-scoped), or (B)
change the staleness check in the block that uses executorUpdatedAt,
recentLastToolAt, LIVE_EXECUTOR_STATES, HEARTBEAT_EXPECTED_AGENT_STATES and
STALE_EXECUTOR_WINDOW_MS to ignore recentLastToolAt for determining
'stale_executor' and rely only on executorUpdatedAt (executorUpdatedAt vs now).
Ensure the updated behavior still sets flags.push('stale_executor') correctly
when an executor is truly stale.
---
Duplicate comments:
In `@src/genie-commands/__tests__/doctor.test.ts`:
- Around line 46-50: The test is missing boundary assertions for
isSetupEffectivelyComplete; update the test block containing
isSetupEffectivelyComplete(true, { version: 2 }) /
isSetupEffectivelyComplete(false, null) to also assert
isSetupEffectivelyComplete(true, null) (explicit completion with missing
config), isSetupEffectivelyComplete(false, { version: 1 }) (pre-v2 config should
be treated as incomplete), and isSetupEffectivelyComplete(false, { }) or
isSetupEffectivelyComplete(false, { version: undefined }) (missing-version
config case); ensure expected boolean values reflect that explicit true
overrides missing config, version < 2 is incomplete, and missing version is
treated as incomplete.
In `@src/genie-commands/doctor.ts`:
- Around line 274-277: The call to loadGenieConfigSync() inside the
setupComplete calculation can throw on a malformed/unreadable config and abort
the whole "genie doctor" run; wrap the config load in a safe guard: check
genieConfigExists(), then attempt loadGenieConfigSync() inside a try/catch, on
error return null (or a sentinel) and record a warning row for the diagnostics
instead of letting the exception bubble. Update the expression used by
isSetupEffectivelyComplete(...) (the setupComplete variable) to use the guarded
result and ensure any caught error is converted into a warning entry so the
doctor command continues running.
In `@src/genie-commands/installer-resolution.test.ts`:
- Around line 71-80: Add a test covering the realPath branch of
isGenieManagedBinary by creating a resolved object whose path is a non-matching
symlink and whose realPath points to the Genie-managed location; call
classifyInstallerResolution with that resolved (e.g.
resolved('/usr/local/bin/genie', '/home/alice/.genie/bin/genie')), then assert
rows length is 1, rows[0].status === 'pass' and rows[0].message contains 'Genie
managed binary' to ensure the realPath check is exercised.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: ee6deb87-8288-4ca6-902e-ba0efb187db0
📒 Files selected for processing (12)
.claude-plugin/marketplace.jsonpackage.jsonplugins/genie/.claude-plugin/plugin.jsonplugins/genie/package.jsonsrc/genie-commands/__tests__/doctor.test.tssrc/genie-commands/doctor.tssrc/genie-commands/installer-resolution.test.tssrc/genie-commands/installer-resolution.tssrc/lib/agent-observability.test.tssrc/lib/agent-observability.tssrc/lib/scheduler-daemon.test.tssrc/lib/scheduler-daemon.ts
Verified with focused DX tests, lint, build, dogfood checks, and independent review.
2 participants
Summary
stale_executoralerts when the agent is idle or recent tool activity proves livenessstate_not_in_d3lifecycle flicker for non-D3 states~/.genie/bin/geniemanaged binaries as healthy in doctorVerification
GENIE_TEST_SKIP_PGSERVE=1 bun test src/genie-commands/__tests__/doctor.test.ts src/lib/agent-observability.test.ts src/lib/scheduler-daemon.test.ts src/genie-commands/installer-resolution.test.ts --timeout 20000→ 127 pass, 13 skip, 0 failbun run typecheck→ passbun run lint→ passbun run build→ passtypecheck,lint,dead-code,skills:lint,wishes:lint,lint:emit→ passstate_not_in_d3, 0agent_resume_skipped_turn_aware, 0stale_executorafter removing stale duplicate foreground serveSummary by CodeRabbit
Bug Fixes
Tests
Chores