Preserve pipeline work by using waiting_approval on post-completion errors#836
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds a pipelineCompleted flag to pipeline execution/resume flows and changes error handling: if an error occurs after pipeline steps finished, fallback status is Changes
Sequence Diagram(s)sequenceDiagram
actor Orchestrator
participant ExecutionService
participant Agent
participant FeatureStore
Orchestrator->>ExecutionService: resumeFromStep(context)
ExecutionService->>Agent: executePipeline(steps)
Agent-->>ExecutionService: steps complete
ExecutionService-->>ExecutionService: set pipelineCompleted = true
ExecutionService->>FeatureStore: update feature (normal completion)
alt error after completion
Agent--xExecutionService: error thrown
ExecutionService->>FeatureStore: load current feature
ExecutionService->>FeatureStore: update -> waiting_approval (if not terminal)
ExecutionService->>Orchestrator: emit auto_mode_error
else error before completion
Agent--xExecutionService: error thrown
ExecutionService->>FeatureStore: update -> backlog
ExecutionService->>Orchestrator: emit auto_mode_error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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 enhances the reliability and user experience of Automaker by preserving pipeline work, improving git command execution, and adding features for orphaned feature resolution. It also introduces a mock provider for testing purposes and improves provider context resolution. 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. Ignored Files
Activity
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
|
![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 introduces a pipelineCompleted flag to preserve work by setting a feature's status to waiting_approval instead of backlog if an error occurs after the pipeline has successfully finished. This logic is correctly implemented in both execution-service.ts for new executions and pipeline-orchestrator.ts for resumed features. The new unit tests in execution-service.test.ts effectively validate this behavior. The changes are well-aligned with the goal of preventing loss of completed work. I have one minor suggestion to improve code consistency.
| // If pipeline steps completed successfully, don't send the feature back to backlog. | ||
| // The pipeline work is done — set to waiting_approval so the user can review. | ||
| const fallbackStatus = pipelineCompleted ? 'waiting_approval' : 'backlog'; | ||
| if (pipelineCompleted) { | ||
| logger.info( | ||
| `[resumeFromStep] Feature ${featureId} failed after pipeline completed. ` + | ||
| `Setting status to waiting_approval instead of backlog to preserve pipeline work.` | ||
| ); | ||
| } | ||
| logger.error(`Pipeline resume failed for ${featureId}:`, error); | ||
| await this.updateFeatureStatusFn(projectPath, featureId, 'backlog'); | ||
| await this.updateFeatureStatusFn(projectPath, featureId, fallbackStatus); |
There was a problem hiding this comment.
For consistency with execution-service.ts, it's a good practice to log the error as soon as it's caught, at the beginning of the else block. This improves readability and ensures error logging is the first action in the error handling path.
| // If pipeline steps completed successfully, don't send the feature back to backlog. | |
| // The pipeline work is done — set to waiting_approval so the user can review. | |
| const fallbackStatus = pipelineCompleted ? 'waiting_approval' : 'backlog'; | |
| if (pipelineCompleted) { | |
| logger.info( | |
| `[resumeFromStep] Feature ${featureId} failed after pipeline completed. ` + | |
| `Setting status to waiting_approval instead of backlog to preserve pipeline work.` | |
| ); | |
| } | |
| logger.error(`Pipeline resume failed for ${featureId}:`, error); | |
| await this.updateFeatureStatusFn(projectPath, featureId, 'backlog'); | |
| await this.updateFeatureStatusFn(projectPath, featureId, fallbackStatus); | |
| logger.error(`Pipeline resume failed for ${featureId}:`, error); | |
| // If pipeline steps completed successfully, don't send the feature back to backlog. | |
| // The pipeline work is done — set to waiting_approval so the user can review. | |
| const fallbackStatus = pipelineCompleted ? 'waiting_approval' : 'backlog'; | |
| if (pipelineCompleted) { | |
| logger.info( | |
| `[resumeFromStep] Feature ${featureId} failed after pipeline completed. ` + | |
| `Setting status to waiting_approval instead of backlog to preserve pipeline work.` | |
| ); | |
| } | |
| await this.updateFeatureStatusFn(projectPath, featureId, fallbackStatus); |
![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 the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/services/execution-service.ts`:
- Around line 546-555: The post-pipeline fallback unconditionally calls
updateFeatureStatusFn and can clobber a terminal "merge_conflict" state; modify
the executeFeature flow so before calling updateFeatureStatusFn(projectPath,
featureId, fallbackStatus) you read the current feature status (or use the
feature object available in scope) and only call updateFeatureStatusFn if
currentStatus !== 'merge_conflict' (or not any other terminal states you want
preserved); reference updateFeatureStatusFn, executeFeature, pipelineCompleted,
fallbackStatus, and merge_conflict when making this guard.
In `@apps/server/src/services/pipeline-orchestrator.ts`:
- Around line 444-455: The catch block currently calls
updateFeatureStatusFn(projectPath, featureId, fallbackStatus) unconditionally
and can overwrite an existing 'merge_conflict' status; before calling
updateFeatureStatusFn, read the feature's current status (e.g., via an existing
accessor like this.getFeatureById / this.getFeatureStatus / fetching the feature
by featureId) and only call updateFeatureStatusFn if currentStatus !==
'merge_conflict'; reference the symbols featureId, fallbackStatus,
updateFeatureStatusFn and the surrounding resumeFromStep error handling so the
guard skips the status update when a merge_conflict is already set.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d5207c54-3189-43cb-ae1b-105a6871b8f9
📒 Files selected for processing (3)
apps/server/src/services/execution-service.tsapps/server/src/services/pipeline-orchestrator.tsapps/server/tests/unit/services/execution-service.test.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/server/src/services/execution-service.ts (1)
556-559:⚠️ Potential issue | 🟠 MajorError handler can throw while handling an error.
At Line 556,
await this.loadFeatureFn(...)is insidecatchbut not locally guarded. If reload fails, the catch path throws again and aborts fallback status handling (matches the pipeline failure signal around Line 556).Suggested hardening
- const currentFeature = await this.loadFeatureFn(projectPath, featureId); + const currentFeature = await this.loadFeatureFn(projectPath, featureId).catch((loadErr) => { + logger.warn( + `[executeFeature] Failed to reload feature ${featureId} in error handler; applying fallback status.`, + loadErr + ); + return null; + }); if (currentFeature?.status !== 'merge_conflict') { await this.updateFeatureStatusFn(projectPath, featureId, fallbackStatus); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/execution-service.ts` around lines 556 - 559, The catch handler currently calls this.loadFeatureFn(projectPath, featureId) which can itself throw and abort the fallback status work; wrap the load+status-check in a local try/catch (or use a safe helper) so failures to reload the feature are caught and logged but do not rethrow, then proceed to call this.updateFeatureStatusFn(projectPath, featureId, fallbackStatus) when appropriate; reference the loadFeatureFn, updateFeatureStatusFn, projectPath, featureId, and fallbackStatus symbols so you locate and harden that catch path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/server/src/services/execution-service.ts`:
- Around line 556-559: The catch handler currently calls
this.loadFeatureFn(projectPath, featureId) which can itself throw and abort the
fallback status work; wrap the load+status-check in a local try/catch (or use a
safe helper) so failures to reload the feature are caught and logged but do not
rethrow, then proceed to call this.updateFeatureStatusFn(projectPath, featureId,
fallbackStatus) when appropriate; reference the loadFeatureFn,
updateFeatureStatusFn, projectPath, featureId, and fallbackStatus symbols so you
locate and harden that catch path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 78efc308-c685-48c2-a330-b2b6ae79c534
📒 Files selected for processing (2)
apps/server/src/services/execution-service.tsapps/server/src/services/pipeline-orchestrator.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/server/src/services/pipeline-orchestrator.ts
1 participant
Summary
waiting_approvalinstead ofbacklogwhen errors occur after pipeline steps finishChanges
pipelineCompletedflag to track successful pipeline execution. When a post-pipeline error occurs, set feature status towaiting_approvalinstead ofbacklogto preserve the completed work.pipelineCompletedflag in the resume path with the same logic to maintain consistency across both execution flows.waiting_approvalis used when errors occur after pipeline completionbacklogis still used when errors occur during pipeline execution itselfSummary by CodeRabbit
Bug Fixes
Tests