Compare commits
2 Commits
| Author | SHA1 | Date | |
|---|---|---|---|
| 6e31ec33ac | |||
| f94808969f |
@@ -177,6 +177,7 @@ Options:
|
|||||||
--git-remote <remote> Git remote for PR URL detection (default: origin)
|
--git-remote <remote> Git remote for PR URL detection (default: origin)
|
||||||
--skip-context Skip context gathering phase
|
--skip-context Skip context gathering phase
|
||||||
--no-post Skip post-processing (GitHub comment flow)
|
--no-post Skip post-processing (GitHub comment flow)
|
||||||
|
--fail-fast Abort the entire review immediately if any reviewer fails
|
||||||
--plan-only Generate review plan without executing
|
--plan-only Generate review plan without executing
|
||||||
--reanalyze Force re-analyze features (ignore cache)
|
--reanalyze Force re-analyze features (ignore cache)
|
||||||
|
|
||||||
@@ -206,6 +207,7 @@ Options:
|
|||||||
--reviewers <ids> Comma-separated reviewer IDs
|
--reviewers <ids> Comma-separated reviewer IDs
|
||||||
-a, --all Use all configured reviewers
|
-a, --all Use all configured reviewers
|
||||||
-d, --devil-advocate Add a Devil's Advocate to challenge consensus
|
-d, --devil-advocate Add a Devil's Advocate to challenge consensus
|
||||||
|
--fail-fast Abort the entire discussion immediately if any reviewer fails
|
||||||
--list List all discuss sessions
|
--list List all discuss sessions
|
||||||
--resume <id> Resume a discuss session with follow-up question
|
--resume <id> Resume a discuss session with follow-up question
|
||||||
```
|
```
|
||||||
@@ -436,6 +438,20 @@ magpie review 12345 --no-converge
|
|||||||
|
|
||||||
Set `defaults.check_convergence: false` in config to disable by default.
|
Set `defaults.check_convergence: false` in config to disable by default.
|
||||||
|
|
||||||
|
### Failure Handling
|
||||||
|
|
||||||
|
By default, Magpie is **resilient**: if a single reviewer fails (network error, rate limit, model unavailable), the round continues with the surviving reviewers and only aborts if *all* reviewers fail. The failed reviewer's slot shows `[Review failed: ...]` and is excluded from subsequent rounds.
|
||||||
|
|
||||||
|
Use `--fail-fast` to flip to strict mode — any single reviewer failure (or context-gathering failure) immediately terminates the entire flow with an error:
|
||||||
|
|
||||||
|
```bash
|
||||||
|
# Strict mode: abort the moment anything fails
|
||||||
|
magpie review 12345 --fail-fast
|
||||||
|
magpie discuss "Should we use microservices?" --fail-fast
|
||||||
|
```
|
||||||
|
|
||||||
|
Useful when you want to guarantee every configured reviewer participated, or when you're debugging provider/auth issues and don't want failures swallowed.
|
||||||
|
|
||||||
### Markdown Rendering
|
### Markdown Rendering
|
||||||
|
|
||||||
All outputs (analysis, reviewer comments, final conclusion) are rendered with proper markdown formatting in terminal - headers, bold, tables, code blocks all display correctly.
|
All outputs (analysis, reviewer comments, final conclusion) are rendered with proper markdown formatting in terminal - headers, bold, tables, code blocks all display correctly.
|
||||||
|
|||||||
@@ -199,6 +199,7 @@ interface DiscussOptions {
|
|||||||
list?: boolean
|
list?: boolean
|
||||||
resume?: string
|
resume?: string
|
||||||
devilAdvocate?: boolean
|
devilAdvocate?: boolean
|
||||||
|
failFast?: boolean
|
||||||
}
|
}
|
||||||
|
|
||||||
async function runDiscussion(
|
async function runDiscussion(
|
||||||
@@ -302,6 +303,7 @@ async function runDiscussion(
|
|||||||
checkConvergence,
|
checkConvergence,
|
||||||
language: lang,
|
language: lang,
|
||||||
interruptState,
|
interruptState,
|
||||||
|
failFast: !!options.failFast,
|
||||||
onWaiting: (reviewerId) => {
|
onWaiting: (reviewerId) => {
|
||||||
flushBuffer()
|
flushBuffer()
|
||||||
if (spinnerRef.spinner) spinnerRef.spinner.stop()
|
if (spinnerRef.spinner) spinnerRef.spinner.stop()
|
||||||
@@ -443,6 +445,7 @@ export const discussCommand = new Command('discuss')
|
|||||||
.option('--reviewers <ids>', 'Comma-separated reviewer IDs')
|
.option('--reviewers <ids>', 'Comma-separated reviewer IDs')
|
||||||
.option('-a, --all', 'Use all reviewers')
|
.option('-a, --all', 'Use all reviewers')
|
||||||
.option('-d, --devil-advocate', "Add a Devil's Advocate to challenge consensus")
|
.option('-d, --devil-advocate', "Add a Devil's Advocate to challenge consensus")
|
||||||
|
.option('--fail-fast', 'Abort the entire discussion immediately if any reviewer fails')
|
||||||
.option('--list', 'List all discuss sessions')
|
.option('--list', 'List all discuss sessions')
|
||||||
.option('--resume <id>', 'Resume a discuss session')
|
.option('--resume <id>', 'Resume a discuss session')
|
||||||
.action(async (topic: string | undefined, options: DiscussOptions) => {
|
.action(async (topic: string | undefined, options: DiscussOptions) => {
|
||||||
|
|||||||
@@ -55,6 +55,7 @@ export const reviewCommand = new Command('review')
|
|||||||
.option('--export <file>', 'Export completed review to markdown')
|
.option('--export <file>', 'Export completed review to markdown')
|
||||||
.option('--skip-context', 'Skip context gathering phase')
|
.option('--skip-context', 'Skip context gathering phase')
|
||||||
.option('--no-post', 'Skip post-processing (GitHub comment flow)')
|
.option('--no-post', 'Skip post-processing (GitHub comment flow)')
|
||||||
|
.option('--fail-fast', 'Abort the entire review immediately if any reviewer (or context gatherer) fails')
|
||||||
.action(async (pr: string | undefined, options) => {
|
.action(async (pr: string | undefined, options) => {
|
||||||
const spinner = ora('Loading configuration...').start()
|
const spinner = ora('Loading configuration...').start()
|
||||||
|
|
||||||
@@ -433,6 +434,7 @@ export const reviewCommand = new Command('review')
|
|||||||
checkConvergence,
|
checkConvergence,
|
||||||
language: config.defaults.language,
|
language: config.defaults.language,
|
||||||
interruptState,
|
interruptState,
|
||||||
|
failFast: !!options.failFast,
|
||||||
onWaiting: (reviewerId) => {
|
onWaiting: (reviewerId) => {
|
||||||
// Flush previous reviewer's buffer before showing spinner
|
// Flush previous reviewer's buffer before showing spinner
|
||||||
flushBuffer()
|
flushBuffer()
|
||||||
|
|||||||
@@ -352,6 +352,9 @@ Then on the LAST line, respond with EXACTLY one word: CONVERGED or NOT_CONVERGED
|
|||||||
const diff = this.extractDiffFromPrompt(prompt)
|
const diff = this.extractDiffFromPrompt(prompt)
|
||||||
this.gatheredContext = await this.contextGatherer!.gather(diff, label, 'main')
|
this.gatheredContext = await this.contextGatherer!.gather(diff, label, 'main')
|
||||||
} catch (error) {
|
} catch (error) {
|
||||||
|
if (this.options.failFast) {
|
||||||
|
throw new Error(`Context gathering failed (fail-fast): ${error instanceof Error ? error.message : String(error)}`)
|
||||||
|
}
|
||||||
logger.warn('Context gathering failed:', error)
|
logger.warn('Context gathering failed:', error)
|
||||||
}
|
}
|
||||||
})()
|
})()
|
||||||
@@ -489,6 +492,10 @@ Then on the LAST line, respond with EXACTLY one word: CONVERGED or NOT_CONVERGED
|
|||||||
duration: (endTime - startTime) / 1000
|
duration: (endTime - startTime) / 1000
|
||||||
}
|
}
|
||||||
this.options.onParallelStatus?.(round, statuses)
|
this.options.onParallelStatus?.(round, statuses)
|
||||||
|
if (this.options.failFast) {
|
||||||
|
// Re-throw so Promise.all rejects immediately and aborts the whole flow
|
||||||
|
throw new Error(`Reviewer ${reviewer.id} failed in round ${round} (fail-fast): ${err instanceof Error ? err.message : String(err)}`)
|
||||||
|
}
|
||||||
logger.warn(`Reviewer ${reviewer.id} failed in round ${round}:`, err)
|
logger.warn(`Reviewer ${reviewer.id} failed in round ${round}:`, err)
|
||||||
return { reviewer, fullResponse: '', inputText: '', failed: true as const, error: err }
|
return { reviewer, fullResponse: '', inputText: '', failed: true as const, error: err }
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -56,6 +56,7 @@ export interface OrchestratorOptions {
|
|||||||
onPostAnalysisQA?: () => Promise<{ target: string; question: string } | null>
|
onPostAnalysisQA?: () => Promise<{ target: string; question: string } | null>
|
||||||
onContextGathered?: (context: GatheredContext) => void // Context gathering complete callback
|
onContextGathered?: (context: GatheredContext) => void // Context gathering complete callback
|
||||||
interruptState?: { interrupted: boolean } // External interrupt signal (e.g., Ctrl+C)
|
interruptState?: { interrupted: boolean } // External interrupt signal (e.g., Ctrl+C)
|
||||||
|
failFast?: boolean // Abort the entire flow as soon as any reviewer (or context gatherer) fails
|
||||||
}
|
}
|
||||||
|
|
||||||
/** Structured issue from a reviewer */
|
/** Structured issue from a reviewer */
|
||||||
|
|||||||
@@ -63,4 +63,26 @@ describe('DebateOrchestrator resilience', () => {
|
|||||||
await expect(orchestrator.runStreaming('test', 'Review this code'))
|
await expect(orchestrator.runStreaming('test', 'Review this code'))
|
||||||
.rejects.toThrow('All reviewers failed')
|
.rejects.toThrow('All reviewers failed')
|
||||||
})
|
})
|
||||||
|
|
||||||
|
it('should abort immediately when failFast is enabled and any reviewer fails', async () => {
|
||||||
|
const goodProvider = makeProvider('good', 'LGTM, no issues found.')
|
||||||
|
const badProvider = makeFailingProvider('bad')
|
||||||
|
|
||||||
|
const reviewers = [
|
||||||
|
makeReviewer('good-reviewer', goodProvider),
|
||||||
|
makeReviewer('bad-reviewer', badProvider),
|
||||||
|
]
|
||||||
|
const summarizer = makeReviewer('summarizer', makeProvider('sum', 'Final conclusion.'))
|
||||||
|
const analyzer = makeReviewer('analyzer', makeProvider('analyzer', 'Analysis done.'))
|
||||||
|
|
||||||
|
const orchestrator = new DebateOrchestrator(reviewers, summarizer, analyzer, {
|
||||||
|
maxRounds: 1,
|
||||||
|
interactive: false,
|
||||||
|
checkConvergence: false,
|
||||||
|
failFast: true,
|
||||||
|
})
|
||||||
|
|
||||||
|
await expect(orchestrator.runStreaming('test', 'Review this code'))
|
||||||
|
.rejects.toThrow(/bad-reviewer.*fail-fast/)
|
||||||
|
})
|
||||||
})
|
})
|
||||||
|
|||||||
@@ -50,7 +50,6 @@ describe('DebateOrchestrator', () => {
|
|||||||
expect(result.prNumber).toBe('123')
|
expect(result.prNumber).toBe('123')
|
||||||
expect(result.analysis).toBe('PR analysis result')
|
expect(result.analysis).toBe('PR analysis result')
|
||||||
expect(result.messages.length).toBe(4) // 2 reviewers * 2 rounds
|
expect(result.messages.length).toBe(4) // 2 reviewers * 2 rounds
|
||||||
expect(result.summaries.length).toBe(2)
|
|
||||||
expect(result.finalConclusion).toBe('Final conclusion')
|
expect(result.finalConclusion).toBe('Final conclusion')
|
||||||
})
|
})
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user