fix(docs): address Flight review findings for circuit breaker guide - #659
fix(docs): address Flight review findings for circuit breaker guide#659diberry wants to merge 1 commit into
Conversation
🧪 FIDO — Quality ReviewPR: #659 — fix(docs): address Flight review findings for circuit breaker guide Checklist
Notes
No convention violations found. No blockers. Verdict: APPROVE |
🏗️ Flight — Lead ReviewReviewed against my original findings from the post-merge audit of #647. Checked all four items plus project-rule blockers. ✅ Finding 1 — Phantom
|
| Rule | Status |
|---|---|
No /docs/ prefix on internal links |
✅ No links added or modified |
| Single blank lines only | ✅ No double-blank-line violations introduced |
| Convention consistency | ✅ squad.config.ts used consistently throughout |
Minor observation (non-blocking)
console.log('Request succeeded:', data); was added inside the try block — not in scope from the original findings, but it improves example completeness. No objection.
Verdict: APPROVE
All four findings addressed correctly. No blockers triggered. Clear to merge.
— Flight
There was a problem hiding this comment.
Pull request overview
Addresses post-merge review findings in the “Building resilient agents” circuit breaker guide by correcting configuration/documentation details and removing references to non-existent APIs.
Changes:
- Updated configuration guidance to reference
squad.config.tsand clarifiedsuccessThresholdsemantics. - Replaced the phantom
@squad/resilienceimport with clearly-labeled pseudocode and improved error-handling notes to avoid conflating open-circuit guards with operation failures.
| When building a custom agent, wrap your external calls with circuit breaker protection: | ||
| When building a custom agent, wrap your external calls with circuit breaker protection. | ||
|
|
||
| > **Planned API — example only:** The `@squad/resilience` module is not yet shipped. This pseudocode shows the intended interface. Circuit breaker support is currently available through Squad's built-in agent configuration. Custom implementations should follow this pattern. |
There was a problem hiding this comment.
This note says circuit breaker support is currently available through Squad's built-in agent configuration, but there doesn't appear to be any corresponding resilience/circuitBreaker configuration surface in the repo (the only circuit breaker implementation is the rate-limit predictive breaker used by watch). Please either point to the real, current configuration/API that exists today, or rephrase this section to be purely pattern guidance (i.e., the helper API is planned and users must implement their own circuit breaker for custom agents).
There was a problem hiding this comment.
Fixed — removed phantom config API claim, clarified circuit breaker is built-in to watch
| Squad includes sensible defaults — most agents won't need to change these. Configure the circuit breaker in your agent's `squad.config.ts` or initialization code only if you want to customize: | ||
|
|
||
| ```json | ||
| { |
There was a problem hiding this comment.
The text now says to configure the circuit breaker in squad.config.ts, but the example that follows is JSON (and uses a resilience.circuitBreaker shape that does not appear to be documented/recognized elsewhere). This is likely to confuse readers—either switch the code fence/example to a squad.config.ts TypeScript snippet that matches the SDK config pattern, or change the text to reference the actual JSON-based config file name (if supported).
There was a problem hiding this comment.
Fixed — replaced squad.config.ts reference with neutral JSON pattern for clarity
| |-----------|---------|---------| | ||
| | `failureThreshold` | 5 | Open circuit after this many failures | | ||
| | `successThreshold` | 2 | Close circuit after this many successes in half-open | | ||
| | `successThreshold` | 2 | In half-open state, close circuit after this many consecutive successes (confirms system recovery) | |
There was a problem hiding this comment.
The updated successThreshold description implies the half-open state can allow multiple consecutive successful requests before closing, but earlier the state machine table says HALF-OPEN "Allow a single probe request". Please align the half-open description with the successThreshold behavior (either update the table text, or adjust the threshold semantics described here).
There was a problem hiding this comment.
Fixed — aligned half-open state description with successThreshold behavior
|
@diberry i'll hold this one until you go through the review comments |
- Replace phantom @squad/resilience import with pseudocode - Fix squad.json → squad.config.ts convention - Add successThreshold explanation - Fix pseudocode catch block double-recording on open circuit Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
5b7d9f6 to
72d7277
Compare
|
Closing -- will re-open via fork-first pipeline when fully polished. |
Context
Flight's post-merge review of PR #647 found issues in the circuit breaker guide that was merged without team review.
Fixes
Review
Previously reviewed by Flight (NEEDS FIXES on these specific items) and FIDO (build/test clean).
Cleaned up from diberry/squad PR #74 (closed — mixed concerns with broken CI guard).