Skip to content

feat: add bsk network read-only command#8

Open
hjxccc wants to merge 1 commit into
Tencent:mainfrom
hjxccc:feat/console-network-commands
Open

feat: add bsk network read-only command#8
hjxccc wants to merge 1 commit into
Tencent:mainfrom
hjxccc:feat/console-network-commands

Conversation

@hjxccc

@hjxccc hjxccc commented Jul 7, 2026

Copy link
Copy Markdown

Refit to network-only per @BB-fat's go-ahead — console landed in #7, this fills the remaining half of #2.

Adds bsk network as a sibling of the console command, reading buffered network responses / failures for a tab. It mirrors the console command's conventions exactly.

bsk network --session <id> [--tab-id N] [--since C] [--limit N] [--max-text-chars N]

What

The extension enables Network on attach (best-effort) and buffers responseReceived / loadingFailed per tab with a monotonic sequence, correlating them to requestWillBeSent for method/URL. Reads are cursor-paginated (sincenext_since) and bounded (limit, max_text_chars) for agent-context safety — same shape as console. Entries carry the real HTTP status code (response) or the CDP failure reason (failure), which performance.getEntriesByType('resource') can't provide. Classified read-only; defaults to the Agent Window's active tab.

Layers

  • bsk-protocolMethod::ToolNetwork, Network{Params,Result,Entry} + NetworkEntryKind, generated schema, read-only classification.
  • bsk-clinetwork subcommand (mirrors console) + daemon dispatch allow-list.
  • extensiontools/network.ts handler + dispatcher route; buffering in chromium-cdp.ts (ensureNetworkCapture / networkEntriesSince).

Testing

  • Rustcargo check / cargo clippy clean; cargo test green for the touched units (protocol round-trips, cli_parse, method classification); rustfmt clean; schema regenerated via dump-schema (no drift to existing files). tools_ipc network round-trip mirrors the console one.
  • Extensiontsc clean, vitest green (incl. new network.test.ts handler tests), biome clean.

No console changes remain — this is purely additive on top of #7. Refs #2.

@BB-fat

BB-fat commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

Thanks for following up on #2 and for putting this together.

Since the issue was inactive for a while, I went ahead and implemented bsk console separately, and it has already landed on main. The current console implementation now supports cursor-based reads (since / next_since), output bounds (limit, max_text_chars), truncation metadata, sequence numbers, columns, and optional structured stack frames.

Because of that, we should not merge the console part of this PR as-is, since it would duplicate and partially regress the existing console API.

The network part is still useful and very much aligned with #2. Would you be open to rebasing this PR on main and narrowing it to bsk network only? Please use the existing console implementation as the reference design for network: a dedicated handler, bounded/cursor-based reads, stable protocol structs/schema, read-only method classification, CLI wiring, and focused extension tests.

Happy to review that version.

@hjxccc hjxccc force-pushed the feat/console-network-commands branch from 8e46308 to f4c7e2d Compare July 7, 2026 08:44
@hjxccc

hjxccc commented Jul 7, 2026

Copy link
Copy Markdown
Author

Heads-up / coordination: I opened this right as #7 (your console command) landed, so the console half here now overlaps and the PR shows as conflicting. Your console implementation is nicer than mine (the since cursor, limit / max-text-chars caps, include-stack + structured frames) — happy to defer to it entirely.

That leaves network, which #7 didn't cover. I'm glad to refit this PR down to a network-only change, rebased on current main and matching your console command's conventions (sibling tools/network.rs + tools/network.ts modules, the same params surface — since / limit / max-text-chars — network buffering in chromium-cdp.ts, and parallel cli_parse / tools_ipc / handler tests).

Want me to take network this way, or are you already picking it up? Either's fine — just don't want us doing it in parallel. If you'd rather I close this and start clean, say the word.

@BB-fat

BB-fat commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

Thanks, that sounds great. Please go ahead and refit this PR as network-only.

Also, thanks for jumping in here as a first-time GitHub contributor. The console overlap is just a timing/coordination issue, not a problem with your contribution. Your work on #2 and this PR helped shape the direction, and the network part is still very useful.

For network, please use the existing bsk console implementation as the reference: dedicated module/CLI/protocol wiring, bounded cursor-based reads (since, limit, output cap), stable schema, read-only method classification, and focused tests.

Once it’s rebased on current main and the console changes are removed, I’ll be happy to review it.

Add `bsk network` as a sibling of the `console` command from Tencent#7,
reading the buffered network responses / failures for a tab. Fills the
remaining half of the request in Tencent#2 (console landed in Tencent#7; network did
not).

Mirrors the console command's conventions exactly:

  bsk network --session <id> [--tab-id N] [--since C]
                             [--limit N] [--max-text-chars N]

The extension enables `Network` on attach (best-effort) and buffers
`responseReceived` / `loadingFailed` events per tab with a monotonic
sequence, correlating them to `requestWillBeSent` for method/URL. Reads
are cursor-paginated (`since` -> `next_since`) and bounded (`limit`,
`max_text_chars`) for agent-context safety, same as console. Entries
carry the real HTTP status code (response) or the CDP failure reason
(failure) — things `performance.getEntriesByType('resource')` can't
give.

Classified read-only (pass-through under the pending-interrupt gate);
defaults to the Agent Window's active tab.

Wires bsk-protocol (Method + params/result/entry structs + schema),
bsk-cli (network subcommand + daemon dispatch allow-list) and the
extension (handler + dispatcher route), with unit / parse / IPC tests
paralleling the console command.

Refs Tencent#2.
@hjxccc hjxccc force-pushed the feat/console-network-commands branch from f4c7e2d to 965e922 Compare July 7, 2026 16:42
@hjxccc hjxccc changed the title feat: add bsk console and bsk network read-only commands feat: add bsk network read-only command Jul 7, 2026
@hjxccc

hjxccc commented Jul 7, 2026

Copy link
Copy Markdown
Author

Done — force-pushed the refit. This PR is now network-only, rebased on current main (on top of your console in #7), and mergeable. It mirrors the console command's conventions throughout: same --since / --limit / --max-text-chars params, cursor pagination (sincenext_since), per-tab sequence + ring buffer, ensureNetworkCapture on read, and parallel unit / parse / IPC tests. Entries are response (with real status) or failure (with the CDP error reason).

cargo check / clippy / rustfmt and the extension tsc / vitest / biome are all green locally; schema regenerated with no drift to existing files. Thanks for the warm welcome — happy to adjust anything in review.

@BB-fat BB-fat left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This PR adds bsk network in a way that generally follows the existing console command, but there are still several issues that should be addressed in this PR before merging: network event state isolation, Network.enable failure semantics, request metadata lifecycle, and shared helper extraction for duplicated buffered-read logic.

  1. apps/extension/src/browser-driver/chromium-cdp.ts: networkRequestMeta is currently a global Map<string, NetworkRequestMeta> keyed only by requestId. It is not scoped by tabId, and clearNetworkState(tabId) does not clear the request metadata for that tab.

    In multi-tab scenarios, if requestId values are reused or collide, a response/failure may pick up the URL/method/resourceType from another tab, producing incorrectly attributed network entries. Please make this per-tab, for example Map<number, Map<string, NetworkRequestMeta>>, and have rememberNetworkRequest, parseNetworkResponse, and parseNetworkFailure all look up and clean up metadata for the current tabId only.

  2. apps/extension/src/browser-driver/chromium-cdp.ts: enableNetworkDomains() marks a tab as attempted before calling Network.enable, and then only debug-logs failures. After one failure, later calls will not retry, so an explicit bsk network call may just return an empty result, leaving the user with no indication that the Network domain failed to enable.

    Please separate best-effort attach-time behavior from explicit ensureNetworkCapture() behavior. When the user explicitly reads network events, the code should either retry enabling the domain or return a clear failure to the handler, ultimately surfacing as cdp_failed.

  3. apps/extension/src/browser-driver/chromium-cdp.ts: networkRequestMeta entries are not removed after responseReceived / loadingFailed; they are only bounded by MAX_NETWORK_REQUEST_META. The stored URL is also the raw, untruncated URL. Network events can be high-volume, and long URLs or data URLs can make this map grow substantially in bytes even with an entry-count cap.

    Please limit stored metadata size when writing it, preserve truncation information, and remove the corresponding request metadata once a response/failure has been processed. This can be handled together with the per-tab metadata change.

  4. apps/extension/src/browser-driver/chromium-cdp.ts: parseNetworkFailure() writes "(unknown)" into the structured url field when request metadata is missing. That is a presentation-layer placeholder, but here it becomes part of the structured protocol result, and clients may treat it as a real URL.

    Please avoid putting display placeholders into the protocol layer. Options include making the URL optional in the protocol, carrying a request id / unknown marker, or displaying unknown only in the CLI human renderer.

  5. apps/extension/src/tools/network.ts / apps/extension/src/tools/shared.ts: ensureNetworkCapture? and networkEntriesSince? were added as optional methods on CdpRunner, which forces handleNetwork to carry a runtime compatibility branch. In production, ChromiumCdp should necessarily support these methods for the new command.

    Please define a more precise dependency type for the network handler where these methods are required, and remove the runtime if (!deps.cdp.ensureNetworkCapture || !deps.cdp.networkEntriesSince) branch. Test fakes can implement that type directly, avoiding as unknown as CdpRunner.

  6. apps/extension/src/tools/network.ts: parseNetworkBounds() / boundedOptionalInteger() largely duplicate the since / limit / max_text_chars parsing logic from the console handler. Since this PR introduces a second buffered-read command, this shared behavior should be factored out in this PR instead of duplicated.

    Please move the positive-integer bounds parsing, or a more specific cursor-read bounds parser, into tools/shared.ts; for example, a shared boundedOptionalInteger() or parseBufferedReadBounds(). This keeps console/network defaults, caps, and error messages from drifting over time.

  7. apps/extension/src/browser-driver/chromium-cdp.ts: the network bounded-buffer append, pagination, truncation aggregation, and text truncation logic closely duplicate the existing console logic. Since this PR explicitly aims for network to “mirror console exactly”, the consistency should be enforced by shared code rather than copy/paste.

    Please extract small, local helpers in this PR. Low-risk examples are appendBoundedEntry() and truncateText(); if the change remains contained, the buffered pagination logic can also be shared. This reduces the chance of future fixes landing on only one side.

  8. crates/bsk-protocol/src/tools/network.rs: NetworkEntryKind::as_str() is newly added but currently unused. Please remove it to avoid exposing unused API surface, or use it from the CLI renderer.

  9. crates/bsk-cli/src/cli/network.rs: the human failure output uses a Unicode separator. CLI output should stay simple ASCII and consistent with nearby commands. Please use something like #{} FAILED {method} {} - {err} instead.

  10. apps/extension/src/tools/__tests__/network.test.ts: the network tests duplicate console test fixtures and bounds behavior coverage. Since the bounds parsing should become a shared helper, please move that coverage to tests for the shared helper, and keep the network handler tests focused on network-specific CDP forwarding, tab/session authorization, and error behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants