feat: Add feature state reconciliation on server startup#781
feat: Add feature state reconciliation on server startup#781gsxdsm merged 2 commits intoAutoMaker-Org:v0.15.0rcfrom
Conversation
Summary of ChangesHello @gsxdsm, 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 introduces robust mechanisms for handling feature state consistency, particularly after server restarts or unexpected interruptions. It ensures that features previously stuck in transient states are automatically reset to a stable condition upon server startup, preventing data discrepancies and improving system resilience. Additionally, it provides an on-demand API for manual reconciliation, offering greater control and diagnostic capabilities. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
📝 WalkthroughWalkthroughAdds a startup reconciliation pass and an on-demand reconcile API to detect and reset stuck feature states; implements reconciliation logic in FeatureStateManager, exposes methods through AutoMode services and routes, extends event types and UI invalidation, and adds port validation improvements to the start script. Changes
Sequence Diagram(s)sequenceDiagram
participant Startup as Server Startup
participant Service as AutoModeService
participant Manager as FeatureStateManager
participant FS as File System
participant EventBus as Event Bus
Startup->>Service: reconcileFeatureStates(projectPath)
Service->>Manager: reconcileAllFeatureStates(projectPath)
Manager->>FS: scan feature files for projectPath
FS-->>Manager: feature data
Manager->>Manager: detect stuck/transient states
Manager->>Manager: update statuses (record old→new)
Manager->>EventBus: emit feature_status_changed (per feature)
Manager->>EventBus: emit features_reconciled (bulk)
Manager->>FS: persist updated feature files
Manager-->>Service: return reconciledCount
Service-->>Startup: return reconciledCount
Startup->>Startup: log reconciled summary
sequenceDiagram
participant Client as HTTP Client
participant Route as Reconcile Route Handler
participant Service as AutoModeService
participant Manager as FeatureStateManager
Client->>Route: POST /reconcile { projectPath }
Route->>Route: validate projectPath
Route->>Service: reconcileFeatureStates(projectPath)
Service->>Manager: reconcileAllFeatureStates(projectPath)
Manager->>Manager: reconcile stuck states
Manager-->>Service: return reconciledCount
Service-->>Route: return reconciledCount
Route-->>Client: 200 { success, reconciledCount, message }
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Suggested Labels
Poem
🚥 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 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
The pull request introduces a new feature to reconcile feature states on server startup, improving recovery after restarts. It also adds a new API endpoint for on-demand reconciliation. The changes are well-documented and follow a logical structure. The FeatureStateManager has been updated to handle interrupted and pipeline_* states during reconciliation, which is a good improvement for robustness. Event types and UI invalidation logic have been correctly updated to support the new reconciliation events. Overall, the changes are solid and enhance the system's reliability.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
apps/server/src/services/feature-state-manager.ts (2)
308-339: Sequentialifblocks inresetStuckFeaturesare fragile — prefer the consolidated check used inreconcileAllFeatureStates.Lines 308, 320, and 332 use separate
ifstatements that each checkfeature.statusafter the prior block may have mutated it. This currently works because each block changes status to a non-matching value, but it's brittle for future maintainers. TheisActiveStatepattern used inreconcileAllFeatureStates(line 430) is clearer and safer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/feature-state-manager.ts` around lines 308 - 339, The three sequential if blocks in resetStuckFeatures should be consolidated into a single check that uses the originalStatus (capture feature.status into originalStatus before mutating) and the same active-state logic used by reconcileAllFeatureStates (i.e., use the isActiveState pattern or equivalent predicate to test originalStatus and a startsWith('pipeline_') check) so you only decide and set feature.status once; update resetStuckFeatures to compute hasApprovedPlan from feature.planSpec?.status, set feature.status = hasApprovedPlan ? 'ready' : 'backlog' if originalStatus matches any of the previous three conditions, set needsUpdate = true, and log using originalStatus and feature.id (keep the existing logger message format).
401-510: Significant duplication withresetStuckFeatures.
reconcileAllFeatureStatesduplicates nearly all logic fromresetStuckFeatures(lines 283–386): directory scan, JSON recovery, state resets forin_progress/interrupted/pipeline_*, planSpecgenerating→pending, taskin_progress→pending, and atomic write. The only additions are per-feature event emission, bulk event, and returning a count.Consider refactoring to extract the shared scan-and-reset logic into a private helper, with
resetStuckFeaturesandreconcileAllFeatureStatesdiffering only in their post-update behavior (emit events or not, return count or not). This would eliminate ~80 lines of duplicated code and ensure future reset-logic changes are applied consistently.♻️ Sketch of shared helper approach
+ private async resetTransientFeatures( + projectPath: string, + ): Promise<Array<{ feature: Feature; originalStatus: string; featurePath: string }>> { + const featuresDir = getFeaturesDir(projectPath); + const results: Array<{ feature: Feature; originalStatus: string; featurePath: string }> = []; + const entries = await secureFs.readdir(featuresDir, { withFileTypes: true }); + + for (const entry of entries) { + if (!entry.isDirectory()) continue; + const featurePath = path.join(featuresDir, entry.name, 'feature.json'); + const result = await readJsonWithRecovery<Feature | null>(featurePath, null, { + maxBackups: DEFAULT_BACKUP_COUNT, autoRestore: true, + }); + const feature = result.data; + if (!feature) continue; + + let needsUpdate = false; + const originalStatus = feature.status; + // ... shared reset logic ... + + if (needsUpdate) { + feature.updatedAt = new Date().toISOString(); + await atomicWriteJson(featurePath, feature, { backupCount: DEFAULT_BACKUP_COUNT }); + results.push({ feature, originalStatus, featurePath }); + } + } + return results; + }Then both
resetStuckFeaturesandreconcileAllFeatureStatescall this helper and layer their own logging/events on top.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/feature-state-manager.ts` around lines 401 - 510, reconcileAllFeatureStates duplicates most of resetStuckFeatures (directory scan, readJsonWithRecovery, status resets, task resets, atomicWriteJson); extract the shared scan-and-reset logic into a private helper (e.g., private async scanAndResetFeatures(projectPath: string, options?): Promise<{reconciledFeatureIds: string[], reconciledCount:number, scanned:number}>) that encapsulates getFeaturesDir, secureFs.readdir, readJsonWithRecovery, the status transitions (in_progress/interrupted/pipeline_*, planSpec.generating -> pending, task.in_progress -> pending), updatedAt assignment and atomicWriteJson; update resetStuckFeatures to call this helper and only perform its original logging/emission behavior, and update reconcileAllFeatureStates to call the same helper and then emit per-feature events, the bulk features_reconciled event, and return the count from the helper so both functions share one implementation for the reset logic.apps/server/src/index.ts (1)
389-411: Startup reconciliation looks correct.One minor observation:
settingsService.getGlobalSettings()is called twice in this IIFE (line 373 and line 394). You could hoist the settings into a shared variable to avoid the redundant call, though it's a minor efficiency concern since this runs once at startup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/index.ts` around lines 389 - 411, The code calls settingsService.getGlobalSettings() twice in the startup IIFE — hoist the result into a single local variable (e.g., const settings = await settingsService.getGlobalSettings()) and reuse that variable for both uses instead of calling settingsService.getGlobalSettings() again; update references in the reconcile loop (autoModeService.reconcileFeatureStates(project.path)) and the earlier check to use the hoisted settings variable so the global settings are fetched only once.apps/server/src/services/typed-event-bus.ts (1)
45-47: The| stringescape hatch negates the type safety of the explicit union members.The
| stringon line 47 meansAutoModeEventTypeaccepts any string, making the explicit union members above it purely documentary. This is a pre-existing pattern, not introduced by this PR, but worth noting: if strict event typing is ever desired, removing| stringand exhaustively listing event types would catch typos at compile time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/typed-event-bus.ts` around lines 45 - 47, AutoModeEventType currently ends with a catch-all "| string" which defeats the explicit union's type safety; to enforce strict event typing remove the trailing "| string" from the AutoModeEventType union (symbol: AutoModeEventType) so only the declared literals like 'feature_status_changed' and 'features_reconciled' are allowed, then update any call sites that relied on arbitrary strings to use one of the declared literals or add a new explicit literal to the union if a new event is required; if you must allow unknown events, introduce a separate fallback type (e.g., UnknownAutoModeEvent) or an explicit "other" literal and handle it at use sites instead of using a loose string escape hatch.apps/ui/src/types/electron.d.ts (1)
363-370: Consider using a stricter type forstatusandpreviousStatusif extensibility is not required.
FeatureStatusWithPipelineandFeatureStatustypes already exist in@automaker/typesand could replace barestringhere for better type safety. However, the server-side code intentionally usesstringtype (including an explicit comment in typed-event-bus.ts: "Allow other strings for extensibility"). The current design supports dynamic or custom statuses beyond the predefined union. If the UI and server should enforce the same set of known statuses, consider importingFeatureStatusWithPipelinefrom@automaker/typesand updating the server-side type signatures accordingly—otherwise, the barestringtype is intentional and correct.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/types/electron.d.ts` around lines 363 - 370, The event payload currently types status and previousStatus as plain string; replace those with the stricter union from `@automaker/types` (preferably FeatureStatusWithPipeline or FeatureStatus) by importing the type and using it for the properties in the event interface (the properties named status and previousStatus), and then update corresponding server-side signatures (including the typed-event-bus.ts declarations that currently allow arbitrary strings) to use the same imported type so client and server enforce the same known statuses consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/server/src/index.ts`:
- Around line 389-411: The code calls settingsService.getGlobalSettings() twice
in the startup IIFE — hoist the result into a single local variable (e.g., const
settings = await settingsService.getGlobalSettings()) and reuse that variable
for both uses instead of calling settingsService.getGlobalSettings() again;
update references in the reconcile loop
(autoModeService.reconcileFeatureStates(project.path)) and the earlier check to
use the hoisted settings variable so the global settings are fetched only once.
In `@apps/server/src/services/feature-state-manager.ts`:
- Around line 308-339: The three sequential if blocks in resetStuckFeatures
should be consolidated into a single check that uses the originalStatus (capture
feature.status into originalStatus before mutating) and the same active-state
logic used by reconcileAllFeatureStates (i.e., use the isActiveState pattern or
equivalent predicate to test originalStatus and a startsWith('pipeline_') check)
so you only decide and set feature.status once; update resetStuckFeatures to
compute hasApprovedPlan from feature.planSpec?.status, set feature.status =
hasApprovedPlan ? 'ready' : 'backlog' if originalStatus matches any of the
previous three conditions, set needsUpdate = true, and log using originalStatus
and feature.id (keep the existing logger message format).
- Around line 401-510: reconcileAllFeatureStates duplicates most of
resetStuckFeatures (directory scan, readJsonWithRecovery, status resets, task
resets, atomicWriteJson); extract the shared scan-and-reset logic into a private
helper (e.g., private async scanAndResetFeatures(projectPath: string, options?):
Promise<{reconciledFeatureIds: string[], reconciledCount:number,
scanned:number}>) that encapsulates getFeaturesDir, secureFs.readdir,
readJsonWithRecovery, the status transitions
(in_progress/interrupted/pipeline_*, planSpec.generating -> pending,
task.in_progress -> pending), updatedAt assignment and atomicWriteJson; update
resetStuckFeatures to call this helper and only perform its original
logging/emission behavior, and update reconcileAllFeatureStates to call the same
helper and then emit per-feature events, the bulk features_reconciled event, and
return the count from the helper so both functions share one implementation for
the reset logic.
In `@apps/server/src/services/typed-event-bus.ts`:
- Around line 45-47: AutoModeEventType currently ends with a catch-all "|
string" which defeats the explicit union's type safety; to enforce strict event
typing remove the trailing "| string" from the AutoModeEventType union (symbol:
AutoModeEventType) so only the declared literals like 'feature_status_changed'
and 'features_reconciled' are allowed, then update any call sites that relied on
arbitrary strings to use one of the declared literals or add a new explicit
literal to the union if a new event is required; if you must allow unknown
events, introduce a separate fallback type (e.g., UnknownAutoModeEvent) or an
explicit "other" literal and handle it at use sites instead of using a loose
string escape hatch.
In `@apps/ui/src/types/electron.d.ts`:
- Around line 363-370: The event payload currently types status and
previousStatus as plain string; replace those with the stricter union from
`@automaker/types` (preferably FeatureStatusWithPipeline or FeatureStatus) by
importing the type and using it for the properties in the event interface (the
properties named status and previousStatus), and then update corresponding
server-side signatures (including the typed-event-bus.ts declarations that
currently allow arbitrary strings) to use the same imported type so client and
server enforce the same known statuses consistently.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/server/src/services/feature-state-manager.ts (1)
183-188:⚠️ Potential issue | 🟠 MajorMissing
previousStatusinfeature_status_changedevent — violates the UI type contract.The
AutoModeEventdefinition inelectron.d.tsdeclarespreviousStatus: FeatureStatusWithPipelineas required (not optional). However, the emission here omits it entirely. Any UI code accessingevent.previousStatuswill getundefinedat runtime despite TypeScript claiming it's always present.In contrast,
reconcileAllFeatureStates(line 441) correctly includespreviousStatus.Proposed fix
You need to capture the previous status before overwriting at line 104. For example:
const feature = result.data; if (!feature) { logger.warn(`Feature ${featureId} not found or could not be recovered`); return; } + const previousStatus = feature.status; feature.status = status; feature.updatedAt = new Date().toISOString();Then include it in the event emission:
this.emitAutoModeEvent('feature_status_changed', { featureId, projectPath, status, + previousStatus, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/feature-state-manager.ts` around lines 183 - 188, The emitted 'feature_status_changed' AutoModeEvent is missing the required previousStatus field; before you overwrite the feature's status in the state update (capture the current FeatureStatusWithPipeline for the given featureId/projectPath), store it as previousStatus and include previousStatus in the payload passed to this.emitAutoModeEvent('feature_status_changed', { featureId, projectPath, status, previousStatus }). This mirrors how reconcileAllFeatureStates includes previousStatus and ensures the UI contract is satisfied.libs/types/src/pipeline.ts (1)
22-30:⚠️ Potential issue | 🔴 CriticalMultiple base feature statuses lack UI display configuration.
The type
FeatureStatusWithPipelineincludes'ready','interrupted', and'completed'statuses, but the UI components that render feature status are missing configurations for all three:
- status-badge.tsx
BASE_STATUS_DISPLAYonly includes backlog, in_progress, waiting_approval, and verified. Missing statuses will render with generic fallback styling (muted colors) instead of proper visual distinction.- board-view/constants.ts
COLUMNSonly includes backlog and in_progress (base) and waiting_approval, verified (end). No columns exist for'ready','interrupted', or'completed', so features in these states won't appear in the board view.- getStatusOrder() in status-badge.tsx only defines ordering for backlog, in_progress, waiting_approval, and verified. Missing statuses default to
0, breaking sort order.The server is already setting
'ready'status (feature-state-manager.ts line 334), so these gaps will cause visual bugs immediately. Add all three statuses to BASE_STATUS_DISPLAY, EMPTY_STATE_CONFIGS, and board columns, and define their sort order.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/types/src/pipeline.ts` around lines 22 - 30, The UI is missing display and ordering for the base statuses 'ready', 'interrupted', and 'completed' declared in FeatureStatusWithPipeline; update the status rendering and board configs to include them: add entries for 'ready', 'interrupted', and 'completed' into BASE_STATUS_DISPLAY and EMPTY_STATE_CONFIGS in status-badge.tsx (providing label, colors, and tooltip/empty-state text consistent with existing keys), add corresponding column definitions to COLUMNS in board-view/constants.ts so they show up on the board, and extend getStatusOrder() in status-badge.tsx to assign proper sort ranks for these three statuses (ensure values follow existing ordering semantics); verify this aligns with server usage (feature-state-manager.ts sets 'ready') so features render and sort correctly.
🧹 Nitpick comments (1)
start-automaker.sh (1)
63-85:validate_portsilently succeeds for privileged ports — callers may miss the warning.The function prints a warning for ports < 1024 but still returns 0. Since
resolve_port_conflictsuses this for user-entered ports, the user will see the warning but the port will be accepted without confirmation. This is likely fine for a dev tool, just noting the design choice.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@start-automaker.sh` around lines 63 - 85, The validate_port function currently prints a warning for privileged ports (<1024) but returns 0, which lets callers proceed silently; update validate_port (and its callers like resolve_port_conflicts) so privileged ports require explicit confirmation: have validate_port return a distinct non-zero exit code (e.g., 2) when port <1024 or set a global flag (e.g., PRIVILEGED_PORT=true) and adjust resolve_port_conflicts to detect that condition and prompt the user to confirm or reject the port before accepting it. Ensure the change keeps numeric/range checks the same but surfaces the warning as a blocking condition unless the user confirms.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/index.ts`:
- Around line 401-424: The comment claiming reconciliation finishes "before the
UI is served" is incorrect because this runs inside the un-awaited async IIFE
(the block that calls autoModeService.reconcileFeatureStates for each project)
and startServer(PORT, HOST) is invoked immediately afterwards; update the
comment to state reconciliation is kicked off asynchronously (fire-and-forget)
and may complete after the server starts so clients might briefly see stale
feature states until feature_status_changed / features_reconciled events arrive,
referencing the async IIFE, autoModeService.reconcileFeatureStates, and
startServer to locate the code.
---
Outside diff comments:
In `@apps/server/src/services/feature-state-manager.ts`:
- Around line 183-188: The emitted 'feature_status_changed' AutoModeEvent is
missing the required previousStatus field; before you overwrite the feature's
status in the state update (capture the current FeatureStatusWithPipeline for
the given featureId/projectPath), store it as previousStatus and include
previousStatus in the payload passed to
this.emitAutoModeEvent('feature_status_changed', { featureId, projectPath,
status, previousStatus }). This mirrors how reconcileAllFeatureStates includes
previousStatus and ensures the UI contract is satisfied.
In `@libs/types/src/pipeline.ts`:
- Around line 22-30: The UI is missing display and ordering for the base
statuses 'ready', 'interrupted', and 'completed' declared in
FeatureStatusWithPipeline; update the status rendering and board configs to
include them: add entries for 'ready', 'interrupted', and 'completed' into
BASE_STATUS_DISPLAY and EMPTY_STATE_CONFIGS in status-badge.tsx (providing
label, colors, and tooltip/empty-state text consistent with existing keys), add
corresponding column definitions to COLUMNS in board-view/constants.ts so they
show up on the board, and extend getStatusOrder() in status-badge.tsx to assign
proper sort ranks for these three statuses (ensure values follow existing
ordering semantics); verify this aligns with server usage
(feature-state-manager.ts sets 'ready') so features render and sort correctly.
---
Nitpick comments:
In `@start-automaker.sh`:
- Around line 63-85: The validate_port function currently prints a warning for
privileged ports (<1024) but returns 0, which lets callers proceed silently;
update validate_port (and its callers like resolve_port_conflicts) so privileged
ports require explicit confirmation: have validate_port return a distinct
non-zero exit code (e.g., 2) when port <1024 or set a global flag (e.g.,
PRIVILEGED_PORT=true) and adjust resolve_port_conflicts to detect that condition
and prompt the user to confirm or reject the port before accepting it. Ensure
the change keeps numeric/range checks the same but surfaces the warning as a
blocking condition unless the user confirms.
1 participant
Improve recovery of tasks after a server restart
Summary by CodeRabbit
New Features
Documentation / Chores