Skip to content

fix(skills): keep HAR capture usable after a bad action - #85100

Open
Adolanium wants to merge 1 commit into
NousResearch:mainfrom
Adolanium:fix/har-derived-capture-and-derive
Open

Adolanium wants to merge 1 commit into
NousResearch:mainfrom
Adolanium:fix/har-derived-capture-and-derive

Conversation

@Adolanium

Copy link
Copy Markdown

What does this PR do?

Fixes the capture and derivation bugs in the har-derived-api-client skill described in #85099:

  1. A failed --action no longer costs you the capture. Both capture scripts share one validated run_action (new scripts/har_actions.py). Malformed specs (fill:onlysel, bare click, sleep:, empty selectors) fail with a ValueError naming the expected shape instead of IndexError. Local capture closes the context in a finally, so Playwright flushes the HAR even when an action fails, and the error still propagates, so there's no false "HAR written" on a run that produced nothing.
  2. CDP capture keeps its hands off the tab Hermes is driving. Request/response listeners attach to every existing context instead of contexts[0].pages[0], --goto opens a new tab, and actions run on that same tab.
  3. In-flight requests survive --wait. Leftovers are written as entries with a null response, after the listeners detach. The flush deliberately never calls Playwright's request.response(): it blocks with no timeout until a response arrives (long-poll or a stuck XHR would hang the capture), and a response arriving mid-flush would be recorded twice since the response event also appends an entry.
  4. Derivation reads form and base64 bodies. request_body_sample() handles params-only urlencoded postData, and response_body_text() decodes encoding: "base64" content so {"ok":true} prints as JSON, not eyJvayI6dHJ1ZX0=. The same helper treats postData: null as empty (it has to, to reach params at all), which supersedes fix(har-derived-api-client): handle postData: null in HAR entries #84977.
  5. The source-reading tests are gone. The tests that read_text() the scripts and regexed for call sites are replaced with behavioral ones: real HAR fixtures through har_to_client.main(), and fake page/context/request objects for the action and CDP helpers. The flush test asserts response() is never called so the hang can't quietly come back.

Deliberately untouched: header redaction, the printed scheme, and CDP queryString. Those belong to #85053, which edits the same grouping loop, so whichever of the two lands second gets a small rebase.

Related Issue

Fixes #85099

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • optional-skills/web-development/har-derived-api-client/scripts/har_actions.py: new shared helper with parse_action/run_action (validated specs), choose_drive_page (new-tab policy for CDP --goto), and flush_pending (null-response entries, no waiting calls)
  • optional-skills/web-development/har-derived-api-client/scripts/har_capture.py: context.close() in a finally so a failed action still flushes the HAR
  • optional-skills/web-development/har-derived-api-client/scripts/har_capture_cdp.py: context-level listeners on every context, --goto opens a new tab, detach-then-flush in a finally, the drive error re-raised after the HAR is written
  • optional-skills/web-development/har-derived-api-client/scripts/har_to_client.py: request_body_sample() (params + null postData) and response_body_text() (base64 decode)
  • optional-skills/web-development/har-derived-api-client/SKILL.md: pitfalls now say a failed action writes a partial HAR and that CDP --goto opens a new tab
  • tests/skills/test_har_derived_api_client_skill.py: behavioral tests replacing the source-text assertions

How to Test

  1. scripts/run_tests.sh tests/skills/test_har_derived_api_client_skill.py -q (11 pass, stdlib + pytest only, no network, no playwright needed)
  2. With playwright installed: python3 optional-skills/web-development/har-derived-api-client/scripts/har_capture.py https://example.com out.har --action "fill:#q" fails with ValueError: fill needs fill:SELECTOR:TEXT and out.har still contains the page-load traffic (before this PR: IndexError and no file)
  3. Start Chromium with --remote-debugging-port=9222, open a page in the existing tab, then python3 .../har_capture_cdp.py http://127.0.0.1:9222 out.har --goto https://example.com. The original tab is untouched and the HAR contains the new tab's requests

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: Windows 11

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Screenshots / Logs

$ python -m pytest tests/skills/test_har_derived_api_client_skill.py -q
...........
11 passed in 0.57s

Ruff is clean on the changed files. Overlap notes: #84977's null-postData case is covered here by request_body_sample(), and #85053's redaction/scheme/queryString lines are untouched.

@Enough1122

Copy link
Copy Markdown

AI code review — automated review for reference, author can ignore or act on any point.

fix(skills): keep HAR capture usable after a bad action — flushes a partial HAR on action failure, extracts shared action parsing to har_actions.py, and adds base64/form handling in har_to_client.py. Solid improvement overall.

  1. scripts/har_capture_cdp.py + har_actions.flush_pending: pending requests are flushed with make_entry(req, None), i.e. _har_entry(req, None). The old code only ever called _har_entry with a real response; unless _har_entry explicitly guards resp is None, the partial-flush path will crash on the first leftover request (e.g. resp.status on None). Please confirm that guard exists — the test covers the flush with a fake make_entry, not the real _har_entry.
  2. har_capture_cdp.py: _attach_network(contexts, ...) and choose_drive_page(...) run outside the new try/finally. If attaching fails (e.g. a context that never delivers events) or page selection raises before the try, the HAR file is never written and pending is never flushed — the exact "no file" failure this PR is meant to eliminate. Consider moving attach + page selection inside the try.
  3. har_capture.py / har_capture_cdp.py: sys.path.insert(0, ...) + from har_actions import ... mutates global sys.path. Fine for standalone scripts, but it shadows any module named har_actions elsewhere in the process if these files are ever imported as modules; a relative-import or explicit loader would be safer.
  4. request_body_sample: when postData has only params, the form fields are joined raw without URL-decoding — acceptable for a sample, but a value containing & or = produces an ambiguous body. Minor.

@alt-glitch alt-glitch added type/bug Something isn't working tool/browser Browser automation (CDP, Playwright) tool/skills Skills system (list, view, manage) P3 Low — cosmetic, nice to have labels Aug 16, 2026
@Adolanium

Copy link
Copy Markdown
Author

I rechecked this PR after the refactor. The remaining failed check is the amd64 Docker build in run 31674658945. I attempted to rerun the failed job, but GitHub returned HTTP 403: repository admin rights are required. Could a maintainer rerun that failed job? I have left the code unchanged because the reviewed failure is in CI infrastructure.

Failed --action specs IndexError and skip context.close(), so the HAR
never lands. CDP attach drives pages[0] (and --goto navigates it), and
derivation drops form params and base64 response bodies.

Share a validated run_action helper (bad specs fail with a clear
ValueError, including empty selectors), flush the Playwright HAR in
finally, listen on every CDP context, and open a new tab for --goto so
it cannot wipe the tab Hermes is driving. In-flight requests left after
--wait are flushed as entries with a null response, after detaching the
listeners. Calling the waiting request.response() there could hang the
capture (no timeout) or record a late response twice. har_to_client now
reads params-only postData and decodes base64 content.

Does not change header redaction, scheme printing, or CDP queryString
(NousResearch#85053). Null postData is handled by the new body helper, which
supersedes NousResearch#84977.
@Adolanium
Adolanium force-pushed the fix/har-derived-capture-and-derive branch from f371e8c to c6b4bcd Compare October 1, 2026 11:51
@Adolanium

Copy link
Copy Markdown
Author

Rebased onto current main at c6b4bcd. The conflict is gone, and GitHub opened new CI, Docker, and Nix runs for that push.

Those runs are waiting on approval. I still get HTTP 403 when I try to approve them. Could a maintainer approve the runs on c6b4bcd?

This branch has not been deployed

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

Labels

P3 Low — cosmetic, nice to have tool/browser Browser automation (CDP, Playwright) tool/skills Skills system (list, view, manage) type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: har-derived-api-client: failed --action loses the HAR, CDP capture drives Hermes's tab, derivation drops form/base64 bodies

3 participants