Skip to content

test: build faked API error answers with a contract-checked helper - #3612

Merged
freeqaz merged 2 commits into
mainfrom
test-dry-envelope
Oct 25, 2026
Merged

freeqaz merged 2 commits into
mainfrom
test-dry-envelope

Conversation

@freeqaz

@freeqaz freeqaz commented Oct 25, 2026

Copy link
Copy Markdown
Collaborator

Problem

Browser tests fake API refusals by writing the error envelope by hand in route.fulfill (about 146 copies in 33 files). Nothing checks those copies against the API contract, and many have drifted from what the API can send:

  • Codes the API does not have: INTERNAL, PERMISSION_DENIED, VALIDATION_FAILED, UNAVAILABLE, TEST.
  • A stale-generation conflict detail without its code. The API sends INVALID_VALUE.
  • Request IDs that break the contract pattern (req_test_…), or no meta at all, or no message.

The repository's test rules say a test must use the API's actual response envelopes. A test that fakes an answer the API never sends proves less than it appears to.

Change

  • tests/helpers/api-error.mjs gets apiErrorResponse(status, code, message, { details, requestId }). It returns a route.fulfill() answer with the API's envelope, { error: { code, message, details? }, meta: { requestId } }, and checks that envelope against the contract's ErrorResponse schema (TypeBox Check).
    • A code, detail or request ID the API cannot send throws. The error names each schema problem.
    • A status outside 4xx/5xx throws.
    • Without a requestId, it generates a valid one.
  • 26 hand-built envelopes in 13 browser files now use it. Each keeps its status and message. Where the old envelope broke the contract, it now carries the real value:
    • INTERNAL becomes INTERNAL_ERROR, PERMISSION_DENIED becomes FORBIDDEN, VALIDATION_FAILED becomes INVALID_REQUEST, and UNAVAILABLE becomes DEPENDENCY_UNAVAILABLE.
    • The two TEST outcomes become RESOURCE_CONFLICT (409) and INTERNAL_ERROR (500).
    • The stale-generation detail gains code: "INVALID_VALUE".
    • Envelopes without meta get one.
  • console-status-block: the Share Agent refusal's faked 403 had no meta, so the console showed no request ID. A real 403 always carries one, and the console prints it. The exact-text check now includes Request ID: ….
  • Files: console-runtime-credentials (4), console-status-block (3), console-agents-list-return (3), console-create-agent-member (3), console-form-fields-2 (3), console-agent-deploy-again (2), console-json-error-feedback (2), console-form-fields (1), console-agent-saves (1), console-namespace-not-ready (1), console-namespace-recheck (1), console-refocus-focus (1), console-retained-revalidation (1).

Left as they are, on purpose:

  • Answers that are deliberately not the API's: a proxy's 503 without a code, a gateway's 502, and the BAD_GATEWAY 502 in console-status-block.
  • The RATE_LIMITED 429 cases in console-secrets and console-form-fields. The Secret API never sends this code, so these need their own fix.
  • The 120 envelopes in files that other open PRs are changing. A later change can migrate them.

Verification

On a remote test VM:

  • checks-browser (24 files) and checks-browser-2 (25 files) pass with this change.

The validator refuses each kind of drift above. Called directly, it refuses:

  • 500 INTERNAL
  • requestId: "req_test"
  • a /expectedGeneration detail without a code
  • an empty message

Mutant: apiErrorResponse answers with status 200. 9 of the 13 caller files fail at a migrated site. The other 4 pass: console-runtime-credentials, console-namespace-recheck, console-refocus-focus and console-retained-revalidation. There the console reads the error body whatever the status is, or the check covers only the reread.

Locally (CPUs 20-27):

  • 4 of the API tests that use assertApiError from the same module pass: 96/96.
  • tsc --build tsconfig.json tests/tsconfig.json, ESLint and Prettier are clean.

Open PRs:

  • No open PR touches any of the 14 changed files.
  • git merge-tree with current main is clean.

Risks

Test-only. A faked refusal now goes through the schema check when the module loads or the test runs, so a later contract change that drops a code fails these tests at the helper, with the schema problem named. That is the purpose of the check.

apiErrorResponse() in tests/helpers/api-error.mjs writes the API's error
envelope and checks it against the ErrorResponse contract schema, so a faked
refusal cannot carry a code, detail or request ID the API never sends. 26
hand-built envelopes in 13 browser files that no open PR touches now use it.
Codes the API does not have become the real ones (INTERNAL -> INTERNAL_ERROR,
PERMISSION_DENIED -> FORBIDDEN, VALIDATION_FAILED -> INVALID_REQUEST,
UNAVAILABLE -> DEPENDENCY_UNAVAILABLE, TEST -> the status's code), the
stale-generation detail gains its INVALID_VALUE code, and envelopes without
meta get a valid request ID.
…refusal

The faked 403 had no meta, so the console showed no Request ID; a real API
refusal always carries one and the console prints it.
@freeqaz
freeqaz requested a review from a team as a code owner October 25, 2026 10:10
@clawsweeper

clawsweeper Bot commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-repro Preliminary submission classification; full source review pending. P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-repro Preliminary submission classification; full source review pending. labels Oct 25, 2026
@clawsweeper

clawsweeper Bot commented Oct 25, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge.

What this changes

This PR validates simulated API error responses against the existing contract and migrates 26 fixtures across 13 browser-test files.

Example: Browser tests simulate API refusals with hand-built error envelopes, including values the API contract does not allow.

  • Before: Fixtures can silently use invalid codes such as INTERNAL, omit required metadata, or provide malformed request IDs.
  • After: apiErrorResponse validates the envelope against ErrorResponse, rejects invalid fixtures, and supplies a valid request ID when one is omitted.

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) A focused, source-supported test repair with passing head checks; validation consists of browser harnesses and reported mutation exercises.
Proof confidence 🦐 gold shrimp (3/6) This maintainer-authored test-only change alters fixture construction, with passing browser checks on the reviewed head and reported rejection and mutation exercises.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Product

Kind: Test only · Worth it: Yes · Fix scope: Complete
User problem: Malformed simulated responses can let Console tests pass without exercising the API’s supported error envelope.
Reason: The maintainer-authored change corrects demonstrated fixture drift within the existing test owner and adds no product contract.

Merge readiness

✅ Ready for maintainer review

Keep this PR open for normal landing: it repairs invalid browser-test fixtures with a focused shared helper, and no concrete introduced defect was found.

Priority: P3
Reviewed head: 68f74f944e49aa2f6b8e6d445f990ed45e209e0b

Before merge

None.

Findings

None.

Agent review details

How this fits together

Browser tests inject HTTP refusals into the Console’s real API client; the existing error helper now builds those responses using the contracts package’s ErrorResponse schema.

flowchart LR
  A[Browser test scenario] --> B[API error helper]
  C[ErrorResponse contract] --> B
  B --> D[Playwright route response]
  D --> E[Console API client]
  E --> F[Existing UI assertions]
Loading

Technical review

Best possible solution:

Land the focused helper and fixture corrections while preserving independent UI expectations and deliberately malformed proxy-response scenarios.

Do we have a high-confidence way to reproduce the issue?

The original fixtures visibly contain codes absent from ErrorResponse, missing required metadata, and an incomplete detail object; no target code was executed during this review.

Is this the best way to solve the issue?

Extending the existing test helper with the authoritative schema is proportionate and preserves independent behavior assertions without copying validation rules.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against c000db1d561d.

Provenance checked

  • Shared API error helper keeps the original intent (5f94137: Consolidate strict status and code assertions while making failures show the response body.)
  • Console refusal, status, and focus scenarios keeps the original intent (22283f6: Consolidate browser setup while preserving each scenario and its assertions.)

Testing

Proof path: in-process harness.

Security

None.

Evidence

What I checked:

  • Introduced change: The pinned introduction diff changes only browser tests and their existing helper; production code, dependencies, schemas, and persisted formats are unchanged. (tests/helpers/api-error.mjs:34, 68f74f944e49)
  • Contract and production response agree: ErrorResponse requires a supported code, nonempty message, valid request ID, and coded details; canonicalFailure emits that envelope, including request metadata. (packages/contracts/src/api/common.ts:1004, 68f74f944e49)
  • Request ID assertion remains independent: The Share Agent test now supplies a fixed valid request ID and independently expects its rendered text; other migrated cases retain their observable assertions. (tests/browser/console-status-block.test.ts:275, 68f74f944e49)
  • Current-main ownership and prior intent: Current main retains the assertion-only helper introduced by test: assert API error responses through a shared helper #3558; this PR extends that owner while preserving assertApiError. Earlier browser consolidation and TypeScript migrations likewise preserve scenario assertions. (tests/helpers/api-error.mjs:15, c000db1d561d)
  • Reviewed-head validation: REST check results report successful Console Browser, Console Browser 2, static checks, and CI Required on the reviewed head; the body additionally reports rejection probes and a status mutation exercise. (68f74f944e49)
  • Repository policy: The complete root instructions apply, with no nested test instructions found; contract-valid fixture guidance supports this change, and the requested single-reviewer, read-only boundary was preserved. (AGENTS.md, 68f74f944e49)

Likely related people:

  • Free Wortley: Raw commit 5f94137 adds tests/helpers/api-error.mjs:8 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: 5f94137a9bfc; files: tests/helpers/api-error.mjs)

Review metrics

Metric Value Why it matters
Change footprint Production +0/-0; tests and helper +106/-168 The existing helper replaces duplicated envelopes, reducing the test surface by 62 lines without adding production machinery.

Labels

Label changes:

  • add P3: This is test-fixture maintenance with no change to shipped behavior.
  • add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR.

Label justifications:

  • P3: This is test-fixture maintenance with no change to shipped behavior.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR.

Rating scale

6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior.

Workflow

ClawSweeper edits this one comment on every review. Comment @clawsweeper re-review for a fresh review only. Repair and merge execution lanes are retired.

Reviewed October 25, 2026, 6:23 AM ET / 10:23 UTC.

@freeqaz
freeqaz merged commit 674990f into main Oct 25, 2026
73 checks passed
@freeqaz
freeqaz deleted the test-dry-envelope branch October 25, 2026 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant