Conversation
statusForStageResult() looked its status tables up against `error_reason`, but tools put the machine code in `error` and a human sentence in `error_reason` — the inverse of the REST envelope convention. No lookup ever matched, so every tool failure fell through to 500: invalid input returned 500 instead of 400, upstream fetch failures 500 instead of 502, and a missing browser engine 500 instead of 503. Key the three lookups on `error`, and add the `http_<status>` codes that tools/fetch.ts emits (http_404, http_503, …) to the fetch-stage 502 branch — the existing 'http_error' entry never matched them. The match stays exact/anchored, so a reason sentence is still never substring-scanned. The existing unit tests passed only because they mirrored the bug, putting the code in `error_reason` — a shape no tool produces. They are re-pointed at producer-shaped failures, plus negative cases pinning that a code found only in the reason sentence does not match.
📝 WalkthroughWalkthroughChangesREST status mapping
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/daemon/rest/errors.ts`:
- Around line 145-151: Update the stealth error handling across
SmartRouter.fetch and the src/tools/fetch.ts forwarding path so
playwright_fetch_failed is normalized to browser_engine_unavailable before
becoming a StageError, or explicitly include the intended Playwright codes in
the appropriate statusForStageResult allowlist. Ensure exported stealth
acquisition failures receive the intended non-500 status while preserving
existing mappings for other stage errors.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e0af6477-e5f2-4635-93c3-c4606a14d061
📒 Files selected for processing (3)
src/daemon/rest/errors.tstests/unit/daemon/rest-dispatch.test.tstests/unit/daemon/rest-errors.test.ts
| * the explicit semantic-validation allowlist, else 500. Keyed on the machine | ||
| * code (`error`), never a substring scan of the `error_reason` sentence. | ||
| */ | ||
| export function statusForStageResult(f: StageFailure): number { | ||
| if (UNAVAILABILITY_REASONS.has(f.error_reason)) return 503; | ||
| if (f.stage === 'fetch' && FETCH_UPSTREAM_REASONS.has(f.error_reason)) return 502; | ||
| if (SEMANTIC_VALIDATION_REASONS.has(f.error_reason)) return 400; | ||
| if (UNAVAILABILITY_REASONS.has(f.error)) return 503; | ||
| if (f.stage === 'fetch' && (FETCH_UPSTREAM_REASONS.has(f.error) || HTTP_STATUS_CODE.test(f.error))) return 502; | ||
| if (SEMANTIC_VALIDATION_REASONS.has(f.error)) return 400; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 6 \
'playwright_not_installed|playwright_fetch_failed|browser_engine_unavailable|statusForStageResult' \
src testsRepository: KnockOutEZ/wigolo
Length of output: 28004
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the relevant implementation slices and status-function test slice without modifying anything.
sed -n '110,155p' src/daemon/rest/errors.ts
printf '\n--- src/tools/fetch.ts stage forwarding ---\n'
sed -n '250,275p' src/tools/fetch.ts
printf '\n--- statusForStageResult tests ---\n'
sed -n '89,125p' tests/unit/daemon/rest-errors.test.tsRepository: KnockOutEZ/wigolo
Length of output: 5589
Normalize stealth browser-acquisition error codes before exporting them.
SmartRouter.fetch emits playwright_not_installed and playwright_fetch_failed only on the stealth path, src/tools/fetch.ts forwards each code unchanged, and statusForStageResult maps both to 500. Normalize playwright_fetch_failed to browser_engine_unavailable or add the intended codes to the status allowlists before exposing these StageError values.
🤖 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/daemon/rest/errors.ts` around lines 145 - 151, Update the stealth error
handling across SmartRouter.fetch and the src/tools/fetch.ts forwarding path so
playwright_fetch_failed is normalized to browser_engine_unavailable before
becoming a StageError, or explicitly include the intended Playwright codes in
the appropriate statusForStageResult allowlist. Ensure exported stealth
acquisition failures receive the intended non-500 status while preserving
existing mappings for other stage errors.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
Hi @divya0795 — thank you for this PR, and I'm sorry it's been sitting here without a proper review. I've been heads-down building some pretty big features and improvements for wigolo, and as a solo dev, shipping that while also reviewing and maintaining every issue and PR has been genuinely hard. This one isn't forgotten — it just hasn't had the attention it deserves yet. I'd really appreciate your patience here. I'm going to work through the open PRs and issues properly over the next few weeks (sooner if I can free up), and I'll follow up right here. In the meantime, the wigolo Discord is open if you'd like to follow what's being built, ask questions, or nudge me directly: https://discord.gg/BkUUgz2bNF Thanks again for contributing, and for understanding — it genuinely means a lot. 🙏 |
3 participants
What & why
Fixes #262.
statusForStageResult()looked its status tables up againstf.error_reason, but on aStageResultthe machine code is inerrorand the human sentence is inerror_reason— theinverse of the
ErrorEnvelopeconvention used elsewhere in this file. None of the three lookup setsever matched, so every tool failure fell through to 500:
invalid_urlfetch_failed/blocked_by_challengehttp_404(and otherhttp_<status>)browser_engine_unavailableThis affects
/v1/*viadispatch.tsand the Firecrawl-compat routes viafirecrawl-compat.ts.The response body was always correct —
errorEnvelope()maps the fields properly — so only thestatus line was wrong, which is what makes it easy to miss and also what makes it worth fixing: a
client or proxy that retries on 5xx currently retries requests that can never succeed.
Behavior change worth flagging: REST responses that previously returned 500 now return
400/502/503. That is the intent of the fix, but it is a visible change for API consumers — happy to
add a CHANGELOG entry if you'd like one.
Changes
src/daemon/rest/errors.ts— key the three lookups instatusForStageResultonf.errorinstead of
f.error_reason.http_<status>codes thattools/fetch.tsemits (http_404,http_503,…) to the fetch-stage 502 branch.
FETCH_UPSTREAM_REASONSonly had'http_error', which no toolproduces, so these would still have fallen through to 500 after the field fix. The match is an
anchored
/^http_\d{3}$/, so the "never substring-scan a reason sentence" contract holds — thereis a negative test pinning that
http_gateway_wobbledoes not map to 502.StageFailureso the next reader doesn't repeat this.tests/unit/daemon/rest-errors.test.ts,tests/unit/daemon/rest-dispatch.test.ts— the existingcases passed only because they mirrored the bug, putting the code in
error_reason(e.g.
{ error: 'blocked', error_reason: 'blocked_by_challenge' }), a shape no tool emits. Theyare re-pointed at producer-shaped failures, and I added negative cases pinning that a code found
only in the reason sentence is not matched.
No new dependencies.
Testing
npm testpasses — see the caveat below, it does not fully pass onmaineithernpm run lintpassesOn
npm test— on this machine (Windows, Node 20) the full suite has pre-existing failures intests/unit/repl/shell.test.ts,tests/e2e/init-command.e2e.test.tsand (intermittently)tests/integration/repl-e2e.test.ts, allError: readline was closed/ tty-related. I confirmedthey are not from this change by running the same suite on unmodified
main: baseline is34 failed / 7901 passed across those 3 suites; this branch is 11 failed / 7929 passed across 2 of
the same suites — a strict subset, and the difference is that flaky suite passing on one run.
Everything else is green, including all 43 files under
tests/unit/daemon.The new tests were verified to fail against the pre-fix source (6 failures) and pass after.
Checklist
CONTRIBUTING.mdand agree to its contribution termsSummary by CodeRabbit