[freeqaz]

Problem

Browser tests hold console requests in two ways: the shared holdRequests helper and about 40 hand-written page.route gates. Nineteen of the hand-written gates do exactly what holdRequests does: hold a request (optionally after the server answered), signal that it arrived, and deliver it on release. Each copy has its own release, teardown and signalling code. Several have no teardown release at all, and two use Playwright's { times: 1 }, which the helper's comment explains can release other held requests.

They could not use the helper as it was:

• holdRequests starts a 10 s deadline when a test awaits requested. The hand-written gates wait as long as the test does, so moving them would add a new way to fail.
• Some read the server's answer (status, body) while the request is held.
• Two deliver a network failure on release after the server applied the write (a lost response).

Change

• holdRequests gets three options. Existing callers do not change.
  • timeout: the deadline for requested in ms. The default is still 10 s, and 0 waits as long as the test does.
  • With afterServer, requested resolves with the server's answer (APIResponse).
  • loseResponse (needs afterServer): the page's request fails on release although the server applied it.
• 19 hand-written holds in 8 files now call holdRequests. Each keeps its method filter, its hold-first or fetch-first order, its status and body checks, and its wait with no deadline (timeout: 0 wherever the test awaits requested).
  • console-agent-channels: 2
  • console-agent-detail: 3
  • console-agent-draft-guards: 5
  • console-agent-plugin-approvers: 1
  • console-agent-repositories: 3
  • console-agent-state: 2 ({ times: 1 } becomes once: true)
  • console-agents: 2
  • console-namespace-create: 1
• Every migrated hold is now released at teardown.

Left as they are:

• The gates that pick the nth request, run a side effect while held, or answer with simulated provider data built per request.
• The gates in files that another open PR is changing (console-agent-detail-2, console-agent-presets, console-agents-2, console.test.mjs).

Verification

On a remote test VM:

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

Mutants of the helper, each run through both lanes:

• loseResponse ignored: both callers fail at their "Outcome unknown" check (console-agent-draft-guards x2, console-agent-repositories).
• requested resolves without the server's answer: all 4 callers that read it fail (console-agent-detail, console-agent-draft-guards x3 tests, console-agent-repositories).
• timeout: 0 ignored (the deadline fires at once): 12 tests in 5 of the 6 files that pass it fail with "No … request was held". The 6th, console-agents, survives because its request is already held before the test awaits requested.
• No hold (the request is delivered at once): 6 of the 8 migrated files fail at a migrated site. The survivors are the 2 console-agent-channels holds, the session hold in console-agent-plugin-approvers and the capabilities hold in console-agent-detail. The hand-written originals of those 4 also pass with their gate removed, so this change keeps their strength as it was.

Static checks:

• tsc --build tsconfig.json tests/tsconfig.json passes.
• ESLint and Prettier are clean on the touched files.
• git merge-tree against every open PR that touches these files (feat: assign configured OpenClaw roles to Agent users #906, fix: preserve native plugin permissions across Harness selection and admission #2258, feat(console): show the authorized native Gateway people roster #2596, feat: add Gateway managed mode to Agent deployments #2982, feat(console): simplify policy denials and add batch rule review #3232, Track Agent deletion through controller-confirmed cleanup #3390) shows no new conflict.

Risks

Test-only. Non-matching methods now fall through with route.fallback() instead of route.continue(). None of the migrated tests registers an earlier route on the same URL, so the requests still reach the API.

[clawsweeper]

🦞👀
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]

Codex review: needs maintainer review before merge.

What this changes

This PR replaces 19 handwritten browser request gates with the shared holdRequests helper while preserving their timing and response behavior.

Example: Saving repository access applies a PATCH successfully, but its response is lost.

• Before: A local route handler fetches the real 200 response, holds it, then aborts delivery.
• After: holdRequests uses afterServer and loseResponse to exercise the same pending controls and “Outcome unknown” recovery.

Review scores

Measure	Result	What it means
Overall readiness	🦐 gold shrimp (3/6)	Useful, focused consolidation with passing browser jobs and no actionable defect; verification remains scoped to the test harness.
Proof confidence	🦐 gold shrimp (3/6)	This maintainer-authored test refactor is outside the ordinary product-proof gate; supplied browser-lane and mutation results exercise the changed helper, while the inspected related-feature screenshots do not establish its behavior.
Patch quality	🐚 platinum hermit (4/6)	No actionable review findings were identified.

Product

Kind: Test only · Worth it: Yes · Fix scope: Complete
User problem: Contributors maintain repeated request-gating code with inconsistent teardown and signaling.
Reason: The maintainer-authored direction reduces duplication while preserving existing assertions, request ordering, and timeout behavior.

Merge readiness

✅ Ready for maintainer review

This PR is a useful test-only consolidation with no introduced correctness or security defect found; keep it open for normal maintainer landing.

Priority: P3
Reviewed head: ed77bed1d9b1b3e621952935c5bd87b762a0332c

Before merge

None.

Findings

None.

Agent review details

How this fits together

The browser-test helper intercepts Console HTTP requests to the in-process controller fixture and releases, fulfills, or aborts them so tests can inspect pending and uncertain outcomes.

flowchart LR
  A[Browser test action] --> B[Console request]
  B --> C[Shared request hold]
  C --> D[Controller fixture]
  D --> C
  C --> E[Release or lose response]
  E --> F[Console state assertions]

Loading

Technical review

Best possible solution:

Land the focused consolidation through the existing helper while retaining the migrated scenarios and assertions.

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

This is test consolidation rather than a reported product bug; source inspection confirms the duplicated gates and their replacement.

Is this the best way to solve the issue?

Extending the existing helper is appropriate: each added option supports an actual migrated scenario without creating another synchronization owner.

AGENTS.md: found and applied where relevant.

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

Provenance checked

• Shared request-hold behavior keeps the original intent (test(console): consolidate the 10-15/16 console browser tests #2560: Consolidate handwritten browser gates, retain interception, and prevent replay when a write's answer is lost.)
• Typed helper and browser consumers keeps the original intent (Migrate Agent save and permission-refusal browser tests to TypeScript #3187: Type-check browser save and permission-refusal scenarios without changing their scheduling or assertions.)
• Pending and uncertain mutation scenarios keeps the original intent (feat(console): improve repository selection and access #374: Preserve repository drafts and block deployment while relevant writes are pending or uncertain; the migrated gates retain those checks.)

Testing

Proof path: in-process harness.

Security

None.

Evidence

What I checked:

• Introduced scope: The pinned introduction changes only browser tests and their existing helper; the diff adds 149 lines and removes 260. (tests/browser/console-agents-browser-helpers.mjs:603, ed77bed1d9b1)
• Preserved helper contract: Existing defaults, method filtering, first-match behavior, fetch-failure handling, and teardown release remain; response inspection, deliberate response loss, and an optional unbounded wait serve migrated callers. (tests/browser/console-agents-browser-helpers.mjs:616, ed77bed1d9b1)
• Current main and original intent: Current main retains the earlier helper contract; the consolidation in test(console): consolidate the 10-15/16 console browser tests #2560 explicitly established shared holds, fallback routing, and preventing write replay after a lost answer. (tests/browser/console-agents-browser-helpers.mjs:585, bda77b0b61fa)
• Observable recovery assertions: The repository-save scenario still checks a real successful write, disabled pending controls, uncertain-outcome feedback, persisted repository choices, and recovery without another PATCH. (tests/browser/console-agent-repositories.test.ts:869, ed77bed1d9b1)
• Browser validation: Console Browser and Console Browser 2 completed successfully for the reviewed head in https://github.com/openclaw/openclaw-enterprise/actions/runs/40183406880; their lane execution steps also succeeded. (ed77bed1d9b1)
• Policy and inspection limits: Applied the complete root policy; no nested test instructions or maintainer-notes directory exist. No tests or review helpers ran, and the checkout remains clean. Some historical blobs were unavailable, and CI log downloads redirected to a blocked Azure host; available source, history, PR discussion, and job metadata supported the review. (AGENTS.md, ed77bed1d9b1)

Likely related people:

• Mrunal Patel: Raw commit e8185da adds tests/browser/console-agent-state.test.mjs:1615 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: medium; commits: e8185dac805b; files: tests/browser/console-agent-state.test.mjs)

Review metrics

Metric	Value	Why it matters
Test-only change	production +0/-0; tests +149/-260; net -111	Reuses the existing request-hold owner and removes duplicated synchronization plumbing.

Labels

Label changes:

• add P3: Test maintenance with no production behavior change or demonstrated urgent user regression.
• 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: Test maintenance with no production behavior change or demonstrated urgent user regression.
• 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:19 AM ET / 10:19 UTC.