Skip to content

fix(rest): map StageResult failures on the machine code, not the prose #264

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
divya0795 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(rest): map StageResult failures on the machine code, not the prose #264

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 13 additions & 4 deletions src/daemon/rest/errors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,14 @@ const SEMANTIC_VALIDATION_REASONS = new Set([
'unsupported_scheme',
]);

/** Upstream HTTP failures are coded `http_<status>` (see tools/fetch.ts) → 502. */
const HTTP_STATUS_CODE = /^http_\d{3}$/;

/**
* A tool failure. NOTE the field convention here is the inverse of
* `ErrorEnvelope` above: on a StageResult `error` is the machine code and
* `error_reason` is the human sentence.
*/
export interface StageFailure {
error: string;
error_reason: string;
Expand All @@ -134,12 +142,13 @@ export interface StageFailure {
/**
* Map a StageResult failure to an HTTP status. Conservative + table-driven:
* 503 for known unavailability, 502 for fetch-stage upstream failures, 400 for
* the explicit semantic-validation allowlist, else 500. Never substring-scans.
* 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;
Comment on lines +145 to +151

@coderabbitai coderabbitai Bot Aug 1, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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 tests

Repository: 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.ts

Repository: 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.

return 500;
}

Expand Down
11 changes: 10 additions & 1 deletion tests/unit/daemon/rest-dispatch.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,13 +57,22 @@ describe('dispatchTool — fetch', () => {

it('failure maps via errors.ts status table (fetch upstream → 502)', async () => {
vi.mocked(handleFetch).mockResolvedValue({
ok: false, error: 'blocked', error_reason: 'blocked_by_challenge', stage: 'fetch',
ok: false, error: 'blocked_by_challenge', error_reason: 'the site returned a bot challenge', stage: 'fetch',
} as never);
const r = await dispatchTool('fetch', { url: 'https://x.com' }, fakeCtx());
expect(r.status).toBe(502);
expect((r.body as { ok: boolean }).ok).toBe(false);
});

it('invalid input from the tool maps to 400, not 500', async () => {
vi.mocked(handleFetch).mockResolvedValue({
ok: false, error: 'invalid_url', error_reason: 'url is not a valid absolute URL', stage: 'fetch',
} as never);
const r = await dispatchTool('fetch', { url: 'not a url' }, fakeCtx());
expect(r.status).toBe(400);
expect((r.body as { ok: boolean }).ok).toBe(false);
});

it('applies the serve-mode target guard before dispatch (non-loopback bind, loopback target → 400)', async () => {
const ctx = fakeCtx();
ctx.bindIsLoopback = false;
Expand Down
35 changes: 26 additions & 9 deletions tests/unit/daemon/rest-errors.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,24 +86,41 @@ describe('error envelope builders', () => {
});
});

// On a StageResult the machine code lives in `error` and the human sentence in
// `error_reason` — the inverse of the REST envelope. Every case below is shaped
// the way src/tools/*.ts actually emits failures.
describe('statusForStageResult', () => {
it('unavailability code → 503', () => {
expect(statusForStageResult({ error: 'x', error_reason: 'browser_engine_unavailable', stage: 'fetch' })).toBe(503);
expect(statusForStageResult({ error: 'browser_engine_unavailable', error_reason: 'playwright is not installed', stage: 'fetch' })).toBe(503);
});
it('fetch-stage upstream code → 502', () => {
expect(statusForStageResult({ error: 'x', error_reason: 'blocked_by_challenge', stage: 'fetch' })).toBe(502);
expect(statusForStageResult({ error: 'x', error_reason: 'fetch_failed', stage: 'fetch' })).toBe(502);
expect(statusForStageResult({ error: 'blocked_by_challenge', error_reason: 'the site returned a bot challenge', stage: 'fetch' })).toBe(502);
expect(statusForStageResult({ error: 'fetch_failed', error_reason: 'connection refused', stage: 'fetch' })).toBe(502);
});
it('fetch-stage http_<status> code → 502', () => {
expect(statusForStageResult({ error: 'http_404', error_reason: 'Upstream returned HTTP 404', stage: 'fetch' })).toBe(502);
expect(statusForStageResult({ error: 'http_503', error_reason: 'Upstream returned HTTP 503', stage: 'fetch' })).toBe(502);
});
it('semantic-validation allowlist → 400', () => {
expect(statusForStageResult({ error: 'x', error_reason: 'invalid_url', stage: 'validate' })).toBe(400);
expect(statusForStageResult({ error: 'invalid_url', error_reason: 'url is not a valid absolute URL', stage: 'fetch' })).toBe(400);
});
it('unknown reason → 500', () => {
expect(statusForStageResult({ error: 'x', error_reason: 'some_novel_reason', stage: 'extract' })).toBe(500);
it('unknown code → 500', () => {
expect(statusForStageResult({ error: 'some_novel_reason', error_reason: 'something new broke', stage: 'extract' })).toBe(500);
});
it('NEGATIVE: a reason containing the word "timeout" does NOT map to 504', () => {
expect(statusForStageResult({ error: 'connection timeout occurred', error_reason: 'network_timeout', stage: 'fetch' })).not.toBe(504);
it('NEGATIVE: a reason sentence containing the word "timeout" does NOT map to 504', () => {
expect(statusForStageResult({ error: 'network_timeout', error_reason: 'connection timeout occurred', stage: 'fetch' })).not.toBe(504);
// network_timeout is not in the fetch upstream allowlist nor unavailability → 500
expect(statusForStageResult({ error: 'connection timeout occurred', error_reason: 'network_timeout', stage: 'fetch' })).toBe(500);
expect(statusForStageResult({ error: 'network_timeout', error_reason: 'connection timeout occurred', stage: 'fetch' })).toBe(500);
});
it('NEGATIVE: a code that only appears in the reason sentence is NOT matched', () => {
expect(statusForStageResult({ error: 'x', error_reason: 'invalid_url', stage: 'fetch' })).toBe(500);
expect(statusForStageResult({ error: 'x', error_reason: 'browser_engine_unavailable', stage: 'fetch' })).toBe(500);
});
it('NEGATIVE: an http_-prefixed free-text reason is not a status code', () => {
expect(statusForStageResult({ error: 'http_gateway_wobble', error_reason: 'upstream misbehaved', stage: 'fetch' })).toBe(500);
});
it('upstream codes only map to 502 on the fetch stage', () => {
expect(statusForStageResult({ error: 'fetch_failed', error_reason: 'connection refused', stage: 'extract' })).toBe(500);
});
});

Expand Down