Wave0/browser integration - #4
Conversation
Preserve the pre-existing launcher-test formatting pass as an isolated change before Browser integration work. Test names, timeout behavior, subprocess commands, assertions, and lock-race semantics remain unchanged.
Run the existing Node test suite and shell syntax checks on Ubuntu with Node 20 using immutable official action pins. Add focused manifest validation for TOML parsing, version parity, entrypoint existence and containment, and executable script modes.
Replace session-derived recording paths with a private, run-keyed state machine that pins the start session through stop. Validate artifacts before atomically completing metadata with byte size and SHA-256, while retaining failed stops for retry and leaving legacy recordings untouched.\n\nAdd focused containment, traversal, duplicate-start, environment drift, special-file, oversized-file, failure, and integrity tests. Document the observational non-attestation boundary and release Browser 0.6.0 without changing action IDs or adding dependencies.
Validate exact pointer and manifest generations before any Stop engine call, atomically claim run directories across workspaces, and journal Start state before spawning the recorder. Compensate failed starts or retain an explicit retryable needs-attention generation when compensation fails.\n\nUse no-follow descriptors for bounded JSON reads and stable artifact chmod/hash operations, make completion recovery idempotent with durable pointer removal, and reclaim only well-formed locks whose local owner is proven dead. Cover corrupt generations, replacement races, concurrency, crash recovery, and injected persistence failures.
Correct the renderer ownership and navigation description, replace the literal source NUL with an escaped separator, and align the Carbonyl installation guidance with the documented next channel.\n\nRun ShellCheck in hosted CI and keep these source-level drift contracts under repository tests.
Make the live-stream cooldown unit supply its own WebSocket capability and establish an actual live renderer state for the action-failure banner unit. This keeps Node 20 CI and machines without agent-browser from changing the behavior under test.
Journal a stop-pending state before invoking the non-idempotent engine and refuse automatic recovery when an interrupted outcome is unknown. Once Stop succeeds in-process, continue artifact validation, manifest finalization, and durable pointer removal even if the intermediate stopped-pointer publication fails. Apply the same transition to successful Start compensation and cover write, fsync, and rename faults with a non-idempotent recorder stub.
feat: surface failed network requests in the console region
|
Tip For best results, initiate chat on the files or code changes.
You are interacting with an AI system. |
|
Warning Review limit reached
Next review available in: 48 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (15)
📝 WalkthroughWalkthroughThe change adds secure run-scoped browser recording, failed-network reporting for snapshot and live modes, repository manifest validation, CI checks, documentation, and version updates. ChangesNetwork failure reporting
Browser recording lifecycle
Repository validation and release checks
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Renderer
participant Browser
participant Console
Renderer->>Browser: Poll network entries
Browser-->>Renderer: Return normalized requests
Renderer->>Renderer: Diff and deduplicate failures
Renderer->>Console: Display failed requests
sequenceDiagram
participant User
participant RecordCLI
participant AgentBrowser
participant RecordingBundle
User->>RecordCLI: Start recording
RecordCLI->>RecordingBundle: Publish active manifest and pointer
RecordCLI->>AgentBrowser: Start pinned session recording
User->>RecordCLI: Stop recording
RecordCLI->>AgentBrowser: Stop recording
RecordCLI->>RecordingBundle: Inspect artifact and publish completion manifest
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR advances the herdr-browser integration by (1) surfacing failed network requests in the pane console (both polling and live-stream modes via a shared, guarded poll), and (2) replacing the legacy “recordings/*.webm” behavior with run-scoped recording bundles (manifest + artifact) driven by a new Node-based recorder.
Changes:
- Add a failed-network-request feed: diffing/deduping/formatting helpers, renderer integration (tick + live timer), and extensive tests.
- Introduce run-scoped recording bundles via
bin/record.mjs, wire it intoscripts/record.sh, and add comprehensive tests for safety/atomicity/recovery. - Add CI + manifest validation scaffolding, and update docs/versioning to reflect the new features and release.
Reviewed changes
Copilot reviewed 12 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
bin/renderer.mjs |
Implements network failure diffing/formatting + poll integration and live-mode timer. |
bin/record.mjs |
Adds run-scoped recording bundle creation/completion with strict validation and durable state handling. |
scripts/record.sh |
Switches record actions to a Node entrypoint and exports pinned recording context env. |
scripts/lib.sh |
Adds shared launcher helpers (state/session/workspace sanitization, timeout, URL validation). |
scripts/check-manifest.mjs |
Adds repository/manifest validation by parsing herdr-plugin.toml. |
scripts/browse-pane.sh |
Updates carbonyl install guidance. |
tests/renderer.test.mjs |
Adds/updates tests for network failure feed + live timer behavior. |
tests/record.test.mjs |
Adds a new test suite covering recording bundle lifecycle, validation, and fault recovery. |
tests/manifest.test.mjs |
Adds tests ensuring release/CI/manifest invariants remain stable. |
tests/launchers.test.mjs |
Removes legacy record-action coverage and refactors a couple of launcher tests. |
.github/workflows/ci.yml |
Adds CI workflow running tests, shell checks, and manifest validation. |
README.md |
Documents failed network requests and the new recording bundle model. |
package.json |
Bumps version to 0.6.0. |
herdr-plugin.toml |
Bumps version to 0.6.0 while keeping action IDs stable. |
docs/plans/2026-08-03-001-feat-network-failures-console-plan.md |
Adds the implementation plan/requirements for network failure reporting. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (13)
scripts/check-manifest.mjs (1)
17-18: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDeclare the Python runtime dependency.
python3importstomllib, which requires Python 3.11 or later. The workflow does not pin Python, and the local requirement is undocumented. Add a pinned Python 3.11+ setup step and document the requirement, or replace this parser with a declared Node dependency.🤖 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 `@scripts/check-manifest.mjs` around lines 17 - 18, Declare the Python runtime requirement used by the manifest-check script: either add a pinned Python 3.11+ setup step to the workflow and document the local prerequisite, or replace the tomllib-based parser invoked by spawnSync in check-manifest with a declared Node dependency.bin/record.mjs (2)
736-743: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDerive the pointer path once.
Line 738 rebuilds
active-${workspaceId}.jsonby hand, and line 743 derives the same path throughpathsFor. Two places now encode the pointer filename. Compute the pointer path through a single source so a later rename cannot diverge.🤖 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 `@bin/record.mjs` around lines 736 - 743, Update the withWorkspaceLock flow in record to derive the active recording pointer path through pathsFor once, then reuse that value for pathEntryExists and readJsonFile instead of manually constructing active-${workspaceId}.json. Keep the existing validation and error behavior unchanged.
613-616: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueResolve the target with the root in
sha256File.
rootcomes fromrealpathSync, butfilestays unresolved. If any parent component is a symlink,assertSafeParentrejects the call and the helper throws for a legitimate file. Join the resolved directory with the basename.♻️ Proposed refactor
export function sha256File(file) { const root = fs.realpathSync(path.dirname(file)); - return inspectArtifact(root, file).sha256; + return inspectArtifact(root, path.join(root, path.basename(file))).sha256; }🤖 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 `@bin/record.mjs` around lines 613 - 616, Update sha256File to construct the target file path by joining the resolved root directory from realpathSync with the original file’s basename before passing it to inspectArtifact, so parent symlinks resolve consistently while preserving the existing SHA-256 return behavior.tests/record.test.mjs (2)
262-262: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConfirm sparse-file support for the oversized case.
This case creates a 10 GiB + 1 file in
os.tmpdir(). The test stays cheap only where the temporary filesystem supports sparse files. If a CI runner uses a filesystem without sparse support, the run allocates 10 GiB and can exhaust the disk. Verify the CI temporary filesystem, or assert the boundary throughinspectArtifactwith an injected descriptor size instead.🤖 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 `@tests/record.test.mjs` at line 262, Update the oversized case in the record tests to avoid unconditionally creating a 10 GiB file: either verify that the CI temporary filesystem supports sparse files before relying on fs.truncateSync, or test the MAX_RECORDING_BYTES boundary by injecting a descriptor size into inspectArtifact. Preserve coverage that artifacts larger than MAX_RECORDING_BYTES are rejected without risking disk exhaustion.
503-548: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe ordinal fault injection couples tests to the write sequence.
failOperationNumbertargets exact call ordinals (writeFileSync#2and#3,fsyncSync#6and#8). Any new write or fsync added tostart()before the engine call shifts the injected operation to a different site, and the test then verifies a different fault than its name states. Consider keying the injection on the target path, for example the first write whose path ends withevidence.jsonoractive-workspace_1.json.🤖 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 `@tests/record.test.mjs` around lines 503 - 548, The fault tests in failOperationNumber are coupled to fragile write/fsync call ordinals. Replace ordinal-based matching with target-path matching for each named fault, injecting failures when the operation targets the relevant manifest or pointer path (such as evidence.json or active-workspace_1.json), while preserving the existing assertions that the engine does not start and no calls or pointer remain.scripts/record.sh (1)
23-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGuard the context assignments.
The script runs with
set -uo pipefailand no-e. Ifstate_dir,ws_id, orsession_namefails, the assignment succeeds with an empty value and the script still execs node.bin/record.mjsthen fails closed, but it reports a generic message such as "plugin state directory must be absolute" instead of the real wrapper failure. Fail at the assignment so the operator sees the cause.♻️ Proposed refactor
-HERDR_BROWSER_STATE_DIR="$(state_dir)" -HERDR_BROWSER_WORKSPACE_ID="$(ws_id)" -HERDR_BROWSER_SESSION_PINNED="$(session_name)" +HERDR_BROWSER_STATE_DIR="$(state_dir)" || exit 1 +HERDR_BROWSER_WORKSPACE_ID="$(ws_id)" || exit 1 +HERDR_BROWSER_SESSION_PINNED="$(session_name)" || exit 1 +if [ -z "$HERDR_BROWSER_STATE_DIR" ] || [ -z "$HERDR_BROWSER_WORKSPACE_ID" ] || + [ -z "$HERDR_BROWSER_SESSION_PINNED" ]; then + echo "herdr-browser: could not resolve the recording context." >&2 + exit 1 +fi export HERDR_BROWSER_STATE_DIR HERDR_BROWSER_WORKSPACE_ID HERDR_BROWSER_SESSION_PINNED exec node bin/record.mjs "$1"🤖 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 `@scripts/record.sh` around lines 23 - 27, Guard the HERDR_BROWSER_STATE_DIR, HERDR_BROWSER_WORKSPACE_ID, and HERDR_BROWSER_SESSION_PINNED assignments in the wrapper so failures from state_dir, ws_id, or session_name stop execution immediately. Preserve the existing exports and node invocation for successful assignments, while allowing the original helper error to be reported instead of continuing with empty values.README.md (1)
203-213: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the per-poll cap line.
The implementation caps output at five failures per poll and appends
✖ …and N more failed requests. A user who sees that line has no documentation for it. Add one sentence after the deduplication note.♻️ Proposed addition
identical failures are collapsed within a 60-second window, and on very long sessions the feed turns itself off with a one-time note once the daemon's request log outgrows the pane's read buffer. +A single poll paints at most five failures; the rest collapse into an +`✖ …and N more failed requests` line.🤖 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 `@README.md` around lines 203 - 213, Add a sentence after the 60-second deduplication note documenting that output is limited to five failures per poll and additional failures are summarized as `✖ …and N more failed requests`.bin/renderer.mjs (3)
389-392: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
methodbypasses the bound thaturlgets.
formatNetworkFailuresanitizes and capsurl, but passesmethodthrough unchanged.methodcomes from the same page-controlled daemon JSON.pushConsolesanitizes the composed line at Line 946, so escape bytes cannot reach the terminal, andrenderConsoletruncates at paint time. Only stored line length is affected. Capmethodfor symmetry if you want the stored line length to stay bounded on its own.♻️ Optional hardening
export function formatNetworkFailure({ method, url, status }) { const shownUrl = truncate(sanitizeText(url), 200); - return `${status ?? "no response"} ${method} ${shownUrl}`; + const shownMethod = truncate(sanitizeText(String(method ?? "GET")), 16); + return `${status ?? "no response"} ${shownMethod} ${shownUrl}`; }🤖 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 `@bin/renderer.mjs` around lines 389 - 392, Update formatNetworkFailure to sanitize and truncate the page-controlled method value before composing the returned failure string, matching the existing url handling and keeping the stored line length bounded independently.
972-976: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReset the poll counters on re-attach too.
The re-attach path resets
networkState,networkBaselinePending, andnetworkOff, but leavesnetworkPollErrorsandnetworkIdleTicksat their values from the dead session. Neither changes behavior today, because a successful poll clearsnetworkPollErrorsand the live timer resetsnetworkIdleTickson activity. Reset them here so the state block stays a single source of truth.♻️ Proposed change
this.networkState = newNetworkState(); this.networkBaselinePending = true; this.networkOff = false; + this.networkPollErrors = 0; + this.networkIdleTicks = 0;🤖 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 `@bin/renderer.mjs` around lines 972 - 976, Update the re-attach state reset block around newNetworkState() to also reset networkPollErrors and networkIdleTicks to their initial values, keeping all dead-session network counters cleared alongside networkState, networkBaselinePending, and networkOff.
706-723: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winOne transient timeout latches the feed off for the whole attach.
err.killed === trueis set when Node kills the child on the 10 s exec timeout, not only when the log is too large. A single slow daemon response therefore disables network reporting until the next attach ornavigate(), and prints "request log too large", which does not describe a timeout.Consider latching only after consecutive fatal-class failures, and separating the two messages.
♻️ Proposed refinement
- const fatal = - /maxBuffer/i.test(err?.message ?? "") || err?.killed === true; - if (fatal) { + const oversized = /maxBuffer/i.test(err?.message ?? ""); + const timedOut = err?.killed === true; + // A single timeout can be a busy machine; an oversized read never + // shrinks. Require repetition before giving up on timeouts. + if (timedOut) this.networkPollErrors++; + if (oversized || (timedOut && this.networkPollErrors >= 3)) { this.networkOff = true; const hadConsole = this.consoleLines.length > 0; this.pushConsole( - [{ text: "network reporting off — request log too large", type: "error" }], + [ + { + text: oversized + ? "network reporting off — request log too large" + : "network reporting off — request log reads keep timing out", + type: "error", + }, + ], false, ); this.queueConsolePaint(hadConsole); } else { this.networkPollErrors++; }Note: the test at
tests/renderer.test.mjsLines 1721-1743 asserts a latch after onemaxBuffererror, which this change preserves.🤖 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 `@bin/renderer.mjs` around lines 706 - 723, Update the catch handling around the network polling request so a single timeout does not set networkOff or report the payload-too-large message: distinguish maxBuffer failures from exec-timeout failures, preserve immediate latching for maxBuffer errors as covered by the existing test, and use a separate timeout message or transient error path for err.killed === true. Keep retry/error counting behavior intact for non-fatal failures.tests/renderer.test.mjs (3)
1495-1626: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCoverage matches the plan's U1 scenarios.
One uncovered branch:
diffNetworkFailuresskips entries whoserequestIdis missing or not a string (Line 338 ofbin/renderer.mjs). The daemon JSON is external input, so a malformed-entry assertion would pin that guard.♻️ Optional added case
+test("network diff: entries without a usable requestId are skipped", () => { + const st = newNetworkState(); + const out = diffNetworkFailures( + st, + [{ status: 500 }, { requestId: "", status: 500 }, { requestId: 7, status: 500 }], + T0, + ); + assert.equal(out.failures.length, 0); + assert.equal(st.seen.size, 0); +});🤖 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 `@tests/renderer.test.mjs` around lines 1495 - 1626, Add a test covering malformed network entries without a string requestId, exercising the guard in diffNetworkFailures that skips such entries. Assert the malformed entry produces no failure and does not disrupt processing of valid entries.
1858-1888: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThese two live-timer tests depend on wall-clock timing.
Both start a real timer with a 5 ms base and then sleep for 40-60 ms. Under a loaded CI runner the timer callback plus the awaited
network()poll may not complete inside that window, so the assertions can fail intermittently. The plan for this work states the opposite approach atdocs/plans/2026-08-03-001-feat-network-failures-console-plan.mdLines 144 and 148: drive the timer hook directly, and assert the state transitions rather than the cadence.Also note that the first assertion in the backoff test cannot fail.
networkIdleTicksstarts at0, soassert.equal(r.networkIdleTicks, 0)passes whether the timer fired or not. Assert that at least one poll happened first.♻️ Proposed deterministic shape for the backoff test
test("live timer: painted polls hold base cadence, empty polls back off", async () => { const r = quietPoll(mkRenderer()); r.attached = true; r.live = { ws: { close: () => {} } }; r.browser = { network: async () => [] }; let painted = true; - r.pollNetwork = async () => painted; - r.startNetworkTimer(5); - await new Promise((res) => setTimeout(res, 40)); - assert.equal(r.networkIdleTicks, 0, "painted failures reset the counter"); + let polls = 0; + r.pollNetwork = async () => { + polls++; + return painted; + }; + r.startNetworkTimer(5); + while (polls < 2) await new Promise((res) => setTimeout(res, 5)); + assert.equal(r.networkIdleTicks, 0, "painted failures reset the counter"); painted = false; - await new Promise((res) => setTimeout(res, 40)); + const before = polls; + while (polls < before + 2) await new Promise((res) => setTimeout(res, 5)); r.stopNetworkTimer(); assert.ok(r.networkIdleTicks > 0, "quiet polls accumulate idle ticks"); });Apply the same wait-for-condition pattern to the test at Lines 1858-1872 instead of the fixed 60 ms sleep.
🤖 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 `@tests/renderer.test.mjs` around lines 1858 - 1888, Make the live-timer tests deterministic by driving the timer hook directly or waiting for observable poll/state completion instead of fixed 40–60 ms sleeps. In “live timer: fires the shared poll and paints while streaming,” wait until the network poll completes before asserting calls and consoleLines; in “painted polls hold base cadence, empty polls back off,” assert at least one poll occurred before checking networkIdleTicks, then verify painted polls reset it and empty polls increase it. Keep the existing timer setup and stopNetworkTimer cleanup.
1669-1685: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the
netQueuedrain semantics.
network()shifts entries while more than one remains, then repeats the last element forever. Tests rely on this in two different ways:[a, b]yieldsaonce thenbforever, and mutatingnetQueue[0]changes the steady-state response. Both are load-bearing at Lines 1696-1699 and Line 1771. A one-line comment prevents a future reader from "simplifying" the ternary.♻️ Proposed comment
network: async () => { calls.push("network"); + // Drain queued responses, then repeat the last one forever. Tests + // either queue a sequence or mutate netQueue[0] to change steady state. const next = netQueue.length > 1 ? netQueue.shift() : netQueue[0];🤖 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 `@tests/renderer.test.mjs` around lines 1669 - 1685, Add a concise inline comment in pollFake’s network method immediately before the netQueue selection logic, documenting that entries are shifted while more than one remains, then the final entry is reused indefinitely; note that this supports both staged responses such as [a, b] and mutations to netQueue[0] affecting steady-state responses.
🤖 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 `@bin/record.mjs`:
- Around line 511-527: Preserve the primary callback outcome when cleanup fails.
In bin/record.mjs lines 511-527, update withWorkspaceLock to retain any callback
error, invoke releaseWorkspaceLock only for cleanup, and rethrow its error only
when the callback succeeded; update releaseWorkspaceLock to call fs.rmSync with
recursive and force enabled. In bin/record.mjs lines 140-144, remove the ENOENT
rethrow so temporary-file unlink errors are swallowed.
- Around line 140-144: Update the temporary-file cleanup around
fs.unlinkSync(temp) so cleanup failures are always swallowed, including errors
other than ENOENT. Remove the rethrow from the catch branch while preserving the
existing cleanup attempt and primary write/compensation outcome.
In `@docs/plans/2026-08-03-001-feat-network-failures-console-plan.md`:
- Line 11: Update the affected Markdown code span in the plan text to remove the
trailing space from `✖`; place any required separator space outside the code
span while preserving the intended error-prefix wording.
- Line 49: Update the plan’s state-shape description to document the shipped
newNetworkState contract: use seen and recent, omit a separate
pending-candidates set, leave unaged null-status IDs out of seen for
reclassification, prune seen against the current log, and prune recent by its
60-second window. Revise the diffNetworkFailures description to state that it
mutates the provided state in place and returns { failures, overflow }, rather
than returning a next-state object.
---
Nitpick comments:
In `@bin/record.mjs`:
- Around line 736-743: Update the withWorkspaceLock flow in record to derive the
active recording pointer path through pathsFor once, then reuse that value for
pathEntryExists and readJsonFile instead of manually constructing
active-${workspaceId}.json. Keep the existing validation and error behavior
unchanged.
- Around line 613-616: Update sha256File to construct the target file path by
joining the resolved root directory from realpathSync with the original file’s
basename before passing it to inspectArtifact, so parent symlinks resolve
consistently while preserving the existing SHA-256 return behavior.
In `@bin/renderer.mjs`:
- Around line 389-392: Update formatNetworkFailure to sanitize and truncate the
page-controlled method value before composing the returned failure string,
matching the existing url handling and keeping the stored line length bounded
independently.
- Around line 972-976: Update the re-attach state reset block around
newNetworkState() to also reset networkPollErrors and networkIdleTicks to their
initial values, keeping all dead-session network counters cleared alongside
networkState, networkBaselinePending, and networkOff.
- Around line 706-723: Update the catch handling around the network polling
request so a single timeout does not set networkOff or report the
payload-too-large message: distinguish maxBuffer failures from exec-timeout
failures, preserve immediate latching for maxBuffer errors as covered by the
existing test, and use a separate timeout message or transient error path for
err.killed === true. Keep retry/error counting behavior intact for non-fatal
failures.
In `@README.md`:
- Around line 203-213: Add a sentence after the 60-second deduplication note
documenting that output is limited to five failures per poll and additional
failures are summarized as `✖ …and N more failed requests`.
In `@scripts/check-manifest.mjs`:
- Around line 17-18: Declare the Python runtime requirement used by the
manifest-check script: either add a pinned Python 3.11+ setup step to the
workflow and document the local prerequisite, or replace the tomllib-based
parser invoked by spawnSync in check-manifest with a declared Node dependency.
In `@scripts/record.sh`:
- Around line 23-27: Guard the HERDR_BROWSER_STATE_DIR,
HERDR_BROWSER_WORKSPACE_ID, and HERDR_BROWSER_SESSION_PINNED assignments in the
wrapper so failures from state_dir, ws_id, or session_name stop execution
immediately. Preserve the existing exports and node invocation for successful
assignments, while allowing the original helper error to be reported instead of
continuing with empty values.
In `@tests/record.test.mjs`:
- Line 262: Update the oversized case in the record tests to avoid
unconditionally creating a 10 GiB file: either verify that the CI temporary
filesystem supports sparse files before relying on fs.truncateSync, or test the
MAX_RECORDING_BYTES boundary by injecting a descriptor size into
inspectArtifact. Preserve coverage that artifacts larger than
MAX_RECORDING_BYTES are rejected without risking disk exhaustion.
- Around line 503-548: The fault tests in failOperationNumber are coupled to
fragile write/fsync call ordinals. Replace ordinal-based matching with
target-path matching for each named fault, injecting failures when the operation
targets the relevant manifest or pointer path (such as evidence.json or
active-workspace_1.json), while preserving the existing assertions that the
engine does not start and no calls or pointer remain.
In `@tests/renderer.test.mjs`:
- Around line 1495-1626: Add a test covering malformed network entries without a
string requestId, exercising the guard in diffNetworkFailures that skips such
entries. Assert the malformed entry produces no failure and does not disrupt
processing of valid entries.
- Around line 1858-1888: Make the live-timer tests deterministic by driving the
timer hook directly or waiting for observable poll/state completion instead of
fixed 40–60 ms sleeps. In “live timer: fires the shared poll and paints while
streaming,” wait until the network poll completes before asserting calls and
consoleLines; in “painted polls hold base cadence, empty polls back off,” assert
at least one poll occurred before checking networkIdleTicks, then verify painted
polls reset it and empty polls increase it. Keep the existing timer setup and
stopNetworkTimer cleanup.
- Around line 1669-1685: Add a concise inline comment in pollFake’s network
method immediately before the netQueue selection logic, documenting that entries
are shifted while more than one remains, then the final entry is reused
indefinitely; note that this supports both staged responses such as [a, b] and
mutations to netQueue[0] affecting steady-state responses.
🪄 Autofix
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: 3f0ad3be-ee2c-47cc-9921-92de07c715a3
📒 Files selected for processing (15)
.github/workflows/ci.ymlREADME.mdbin/record.mjsbin/renderer.mjsdocs/plans/2026-08-03-001-feat-network-failures-console-plan.mdherdr-plugin.tomlpackage.jsonscripts/browse-pane.shscripts/check-manifest.mjsscripts/lib.shscripts/record.shtests/launchers.test.mjstests/manifest.test.mjstests/record.test.mjstests/renderer.test.mjs
| function releaseWorkspaceLock(root, held) { | ||
| const current = readJsonFile(root, held.ownerPath, "recording lock owner"); | ||
| validateLockOwner(current); | ||
| if (current.nonce !== held.owner.nonce || current.pid !== held.owner.pid) | ||
| throw new Error("recording lock ownership changed unexpectedly"); | ||
| fs.rmSync(held.lock, { recursive: true }); | ||
| syncDirectory(held.runs); | ||
| } | ||
|
|
||
| function withWorkspaceLock(root, workspaceId, callback) { | ||
| const held = acquireWorkspaceLock(root, workspaceId); | ||
| try { | ||
| return callback(); | ||
| } finally { | ||
| releaseWorkspaceLock(root, held); | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Cleanup code throws from finally and replaces the primary error. Both sites let a cleanup failure become the reported outcome, so the real recording failure is lost. The shared fix is to keep the primary result and suppress the cleanup error.
bin/record.mjs#L511-L527: rethrow thereleaseWorkspaceLockerror only when the callback succeeded, and callfs.rmSync(held.lock, { recursive: true, force: true }).bin/record.mjs#L140-L144: drop theif (error.code !== "ENOENT") throw error;rethrow and swallow the temporary-file unlink error.
📍 Affects 1 file
bin/record.mjs#L511-L527(this comment)bin/record.mjs#L140-L144
🤖 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 `@bin/record.mjs` around lines 511 - 527, Preserve the primary callback outcome
when cleanup fails. In bin/record.mjs lines 511-527, update withWorkspaceLock to
retain any callback error, invoke releaseWorkspaceLock only for cleanup, and
rethrow its error only when the callback succeeded; update releaseWorkspaceLock
to call fs.rmSync with recursive and force enabled. In bin/record.mjs lines
140-144, remove the ENOENT rethrow so temporary-file unlink errors are
swallowed.
Source: Linters/SAST tools
|
|
||
| ## Summary | ||
|
|
||
| Closes issue #1: failed network requests (4xx/5xx and no-response failures) appear in the pane's console region with the `✖ ` error prefix, in both poll and live-stream modes. Implemented as a requestId-set diff over `agent-browser network requests --json` — agent-browser 0.33.2's push stream carries no network event, so polling is the only path. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the trailing space inside the code span.
markdownlint reports MD038 for `✖ `. A code span must not start or end with a space. Use `✖` and put the space outside, or use ✖ if the trailing space is significant.
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 11-11: Spaces inside code span elements
(MD038, no-space-in-code)
🤖 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 `@docs/plans/2026-08-03-001-feat-network-failures-console-plan.md` at line 11,
Update the affected Markdown code span in the plan text to remove the trailing
space from `✖`; place any required separator space outside the code span while
preserving the intended error-prefix wording.
Source: Linters/SAST tools
| - **Poll-diff, not stream.** Verified against the vercel-labs/agent-browser 0.33.2 source: the per-session WebSocket emits only `url`, `frame`, `console`, `page_error`, `tabs`. Issue approach 1 (consume a network stream event) is impossible today; a follow-up upstream feature request is deferred work. | ||
| - **Separate exec, not the snapshot batch.** The snapshot batch uses `--bail` and throws on a short result array — an agent-browser version lacking the `network` subcommand would brick every tick (no frames, no console) if the command joined the batch. A separate `network requests --type xhr,fetch,document --json` exec degrades independently, and live mode needs a standalone call anyway. `--type` also bounds payload (spike observed `data:` URLs with full base64 payloads in the log; `maxBuffer` is 16 MiB). | ||
| - **Optional duck-typed `network()` method on `makeBrowser`.** Every call site guards `typeof this.browser.network === "function"` — the `streamEnable` precedent (`bin/renderer.mjs` `goLive()`) — so the dozens of existing object-literal test fakes stay green. | ||
| - **Diff by requestId with prune-to-current-log, not count-based reconcile.** A live spike on 0.33.2 observed the request log wiped without `--clear` (browser relaunch after a failed navigation recreated the page target), so `reconcileConsole`-style count/tail matching is unsafe here. Seen-set membership = ids already reported OR resolved to a non-failure status (2xx/3xx); unreported null-status ids live only in a pending-candidates state and are reclassified every poll — report immediately when status lands at 400–599, drop at 200–399, report as "no response" past the age threshold. This closes the slow-failure hole where a request observed in-flight at poll N would swallow its 500 arriving at poll N+1. Both sets are pruned to ids present in the current log (bounds growth; `pid.N`-format requestIds can restart after relaunch, so pruning makes stale collisions harmless). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The plan describes a state shape the implementation does not use.
newNetworkState ships two fields: seen (a Set) and recent (a Map). There is no separate pending-candidates set — an unaged null-status id is simply left out of seen and reclassified next poll. Only seen is pruned to the current log; recent is pruned by the 60 s window.
Line 88 also states the function returns "new failure descriptors plus the next state". diffNetworkFailures mutates state in place and returns { failures, overflow }.
Update both lines so the plan matches the shipped contract.
Also applies to: 88-88
🧰 Tools
🪛 LanguageTool
[grammar] ~49-~49: Ensure spelling is correct
Context: ...rent log (bounds growth; pid.N-format requestIds can restart after relaunch, so pruning ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 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 `@docs/plans/2026-08-03-001-feat-network-failures-console-plan.md` at line 49,
Update the plan’s state-shape description to document the shipped
newNetworkState contract: use seen and recent, omit a separate
pending-candidates set, leave unaged null-status IDs out of seen for
reclassification, prune seen against the current log, and prune recent by its
60-second window. Revise the diffNetworkFailures description to state that it
mutates the provided state in place and returns { failures, overflow }, rather
than returning a next-state object.
Summary by CodeRabbit
New Features
Documentation
Chores