Skip to content

fix(runner): stream --ids sources and cap selection to the pinned set - #47

Merged
steipete merged 3 commits into
openclaw:mainfrom
SebTardif:fix/ids-stream-semantic-bound
Sep 16, 2026
Merged

steipete merged 3 commits into
openclaw:mainfrom
SebTardif:fix/ids-stream-semantic-bound

Conversation

@SebTardif

@SebTardif SebTardif commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

Summary

Loading a large local or HTTP --ids source buffered the entire input before parsing. Stream both sources through the same parser, bound the selection to the pinned SkillTrustBench set (5,520 IDs), and reject IDs over 256 bytes or retained ID text over 256 KiB. These bounds apply to extracted IDs, so full-set JSONL with larger metadata still loads.

Trimmed text IDs now own their bytes: otherwise 64 short IDs padded with whitespace retained 17,303,696 bytes of source lines despite the new ID-text budget. The reader is closed on success and rejection, including HTTP responses that have not finished streaming. Operator documentation and CLI help describe every limit.

Follow-up to #45. Thanks @SebTardif for the original implementation; contributor commits and authorship are preserved.

Scope

  • CLI behavior
  • Judge/profile/benchmark behavior

Security / Trust Impact

  • Security/trust impact explained

Prevents unbounded buffering and retention of benchmark selection inputs. This preserves the existing order, duplicate rejection, and selection digest. The published artifact schema is unchanged.

Verification

  • go test -count=1 ./...
  • go vet ./...
  • Built CLI --help and synthetic clawscan-static scan
  • go test -count=1 -run TestLoadBenchmarkIDSelection -v ./internal/runner
  • go test -race -count=1 -run TestLoadBenchmarkIDSelection ./internal/runner
  • make docs-site
  • All 27 npm-wrapper and script Node tests; npm package build with --pack --smoke
  • Independent Codex review through P2

Red proof: current main accepts a 257-byte ID and a 5,521-ID selection; regression tests fail. The contributor implementation without the padding fix retains over 16 MiB for 64 short padded IDs. All regressions pass after these changes.

Runtime proof: the built public CLI rejects synthetic oversized-ID, count-overflow, and retained-text-overflow files. A loopback HTTP server sends a 257-byte ID and holds the response open; the CLI rejects it in 0.040 seconds, before EOF. Fixture tests load all 5,520 IDs from a 4,134,480-byte JSONL file and HTTP response, with matching ordered IDs and SHA-256.

Notes

Proof uses synthetic local files and loopback HTTP, without live Hugging Face or scanner credentials. Generated docs and package outputs are excluded. No release or version change.

@clawsweeper

clawsweeper Bot commented Aug 16, 2026

Copy link
Copy Markdown

🦞👀
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 merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Aug 16, 2026
@clawsweeper

clawsweeper Bot commented Aug 16, 2026

Copy link
Copy Markdown

Codex review: needs changes before merge. Reviewed September 14, 2026, 6:54 PM ET / 22:54 UTC (Revision 53).

ClawSweeper review

What this changes

The PR streams local and HTTP benchmark ID lists, limits selected ID count and text size, and updates tests, CLI help, and usage guidance.

Merge readiness

Needs changes before merge - 2 items remain

This remains useful: current main and v0.1.8 still buffer entire ID sources. The supplied runtime proof addresses the earlier proof request, while the previously reported documentation gap remains unresolved.

Priority: P2
Reviewed head: 6baa16f5fe70e10ca1f69925ae1fe25d034a2b9a

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused, useful patch with sufficient runtime proof and one remaining documentation correction.
Proof confidence 🐚 platinum hermit (4/6) Sufficient (terminal): The captured macOS CLI transcript at the reviewed head exercises the production selection loader, shows all three limit rejections, and demonstrates that a 299,600-byte JSONL source reaches downstream case lookup; HTTP loading also reports the expected count and digest.
Patch quality 🐚 platinum hermit (4/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Verified Sufficient (terminal): The captured macOS CLI transcript at the reviewed head exercises the production selection loader, shows all three limit rejections, and demonstrates that a 299,600-byte JSONL source reaches downstream case lookup; HTTP loading also reports the expected count and digest.
Evidence reviewed 8 items Applicable repository policy: Read the complete root AGENTS.md. No applicable nested AGENTS.md or maintainer-notes directory was found. Applied benchmark, documentation, and generated-artifact guidance; the read-only review contract supersedes running repository review helpers.
Current main still needs the fix: Inspected current-main benchmark.go lines 295–435: readBenchmarkIDSource still uses io.ReadAll for HTTP and os.ReadFile for local sources, followed by an unbounded selection collection. The release tag v0.1.8 points to this same main revision.
Release identity: The tag lookup returned v0.1.8 for the inspected current-main revision; this release therefore retains the old loader.
Findings 1 actionable finding [P3] Document the per-ID and retained-text limits
Security None None.

How this fits together

ClawScan’s benchmark runner loads selected case IDs from a file or URL before fetching benchmark cases and running scanners. Selection order and a digest are recorded in the resulting artifact.

flowchart LR
  A[Benchmark command] --> B[File or HTTP ID source]
  B --> C[Streaming line parser]
  C --> D{Selection limits satisfied}
  D -->|No| E[Input error]
  D -->|Yes| F[Select benchmark cases]
  F --> G[Scanner results and artifact]
Loading

Before merge

  • Document the per-ID and retained-text limits (P3) - The parser now rejects an ID longer than 256 bytes and aggregate ID text above 256 KiB, but this paragraph and the CLI skill describe only the 5,520-ID limit. Operators following the guidance cannot determine the full acceptance contract. Document both byte limits and explain that they do not cap total JSONL source size. This previously reported finding remains unresolved at the same head.
  • Complete next step (P2) - Document the per-ID and aggregate ID-text byte limits in the benchmark guide and CLI skill, explicitly distinguishing them from total streamed source size.

Findings

  • [P3] Document the per-ID and retained-text limits — docs/benchmarks.md:14-17
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and test delta Production +24 net lines; tests +85 lines Production growth implements shared streaming and selection bounds, with focused acceptance and rejection coverage.

Root-cause cluster

Relationship: canonical
Canonical: #47
Summary: This is the active follow-up for the same unbounded ID-source read addressed by the closed earlier proposal.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Technical review

Best possible solution:

Keep the shared streaming loader and publish all three selection limits, clearly distinguishing retained ID text from total source size.

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

Yes, from source: current main reads an entire local or HTTP ID source before parsing, so input size directly drives allocation. No memory-exhaustion run was performed.

Is this the best way to solve the issue?

Yes: streaming both supported source types follows the direction requested in the prior discussion and avoids the rejected whole-body cap; the operator guidance still needs the byte limits.

Full review comments:

  • [P3] Document the per-ID and retained-text limits — docs/benchmarks.md:14-17
    The parser now rejects an ID longer than 256 bytes and aggregate ID text above 256 KiB, but this paragraph and the CLI skill describe only the 5,520-ID limit. Operators following the guidance cannot determine the full acceptance contract. Document both byte limits and explain that they do not cap total JSONL source size. This previously reported finding remains unresolved at the same head.
    Confidence: 0.99

Overall correctness: patch is correct
Overall confidence: 0.91

AGENTS.md: found and applied where relevant.

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

Labels

Label justifications:

  • P2: This addresses a concrete benchmark-input memory problem with limited scope and no evidence of an active core-runtime outage.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The captured macOS CLI transcript at the reviewed head exercises the production selection loader, shows all three limit rejections, and demonstrates that a 299,600-byte JSONL source reaches downstream case lookup; HTTP loading also reports the expected count and digest.
  • proof: sufficient: Contributor real behavior proof is sufficient. The captured macOS CLI transcript at the reviewed head exercises the production selection loader, shows all three limit rejections, and demonstrates that a 299,600-byte JSONL source reaches downstream case lookup; HTTP loading also reports the expected count and digest.

Evidence

Acceptance criteria:

  • [P1] make docs-site.
  • [P1] git diff --check.

What I checked:

  • Applicable repository policy: Read the complete root AGENTS.md. No applicable nested AGENTS.md or maintainer-notes directory was found. Applied benchmark, documentation, and generated-artifact guidance; the read-only review contract supersedes running repository review helpers. (AGENTS.md:1, 6baa16f5fe70)
  • Current main still needs the fix: Inspected current-main benchmark.go lines 295–435: readBenchmarkIDSource still uses io.ReadAll for HTTP and os.ReadFile for local sources, followed by an unbounded selection collection. The release tag v0.1.8 points to this same main revision. (internal/runner/benchmark.go:332, 6190d96d7fc4)
  • Release identity: The tag lookup returned v0.1.8 for the inspected current-main revision; this release therefore retains the old loader. (6190d96d7fc4)
  • Introduced implementation and regression coverage: The verified merge-base-to-head diff replaces full-source reads with readers, closes them on completion, and enforces 5,520 IDs, 256 bytes per ID, and 262,144 bytes of aggregate ID text. Three added tests cover large JSONL input through file and HTTP paths and rejection of excessive ID length, aggregate text, and count. Tests were inspected, not executed in this read-only review. (internal/runner/benchmark.go:322, 6baa16f5fe70)
  • Prior documentation finding remains: The benchmark guide and CLI skill describe only the count limit, omitting both newly enforced byte limits. The preceding completed review identified this exact gap at the same head; comparison against that reviewed SHA shows no subsequent documentation changes. (docs/benchmarks.md:14, 6baa16f5fe70)
  • After-fix production CLI proof: The fully supplied PR body, captured under sourceRevision 9e48e20999e1a3a6abb323d7c593a3de215676819949f99081450d4488a23ddf, records macOS public-CLI output at the reviewed head rejecting a 257-byte ID, aggregate-text overflow, and 5,521 IDs. A 299,600-byte JSONL file reaches the subsequent missing-case error, demonstrating successful selection parsing. The HTTP run reports 400 loaded IDs and a matching digest. A full live 5,520-row source was explicitly not tested. The later missing-case error is emitted by benchmark_registry.go after selection loading. (internal/runner/benchmark_registry.go:326, 6baa16f5fe70)

Likely related people:

  • unknown: The claimed source-line change could not be verified from bounded local history. (role: source history unknown; confidence: low)
  • steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Document the 256-byte per-ID and 256-KiB aggregate ID-text limits in the benchmark guide and CLI skill, distinguishing them from source size.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (52 earlier review cycles; latest 8 shown)
  • reviewed 2026-08-31T05:33:23.076Z sha 6baa16f :: needs changes before merge. :: [P3] Document every enforced --ids limit
  • reviewed 2026-08-31T07:58:44.770Z sha 6baa16f :: needs changes before merge. :: [P3] Document the per-ID and retained-text limits
  • reviewed 2026-09-02T19:41:19.588Z sha 6baa16f :: needs changes before merge. :: [P3] Document the per-ID and retained-text limits
  • reviewed 2026-09-03T15:43:06.156Z sha 6baa16f :: needs changes before merge. :: [P3] Document the per-ID and retained-text limits
  • reviewed 2026-09-04T11:53:58.834Z sha 6baa16f :: blocked before merge. :: [P3] Document the per-ID and retained-text limits
  • reviewed 2026-09-05T12:54:44.023Z sha 6baa16f :: needs changes before merge. :: [P3] Document the per-ID and retained-text limits
  • reviewed 2026-09-06T01:18:24.786Z sha 6baa16f :: needs changes before merge. :: [P3] Document the per-ID and retained-text limits
  • reviewed 2026-09-07T15:54:05.469Z sha 6baa16f :: needs changes before merge. :: [P3] Document the per-ID and retained-text limits

@SebTardif

Copy link
Copy Markdown
Contributor Author

@clawsweeper

[P2] Bound total retained ID bytes
The 5,520-item guard runs after each arbitrary ID has been inserted into both ids and seen; because the scanner permits records up to 1 MiB and IDs only reject whitespace, a hostile HTTP source can still retain several GiB across 5,520 unique records. Add an aggregate byte or ID-length bound that still accepts the documented

Pushed 6baa16f. One extracted id is capped at 256 bytes. Retained id text is capped at 256 KiB. That is not a JSONL file-size limit, so the documented 5,520-id / 1.3 MiB SkillTrustBench stream still parses.

@clawsweeper re-review

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. proof: sufficient Contributor real behavior proof is sufficient. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. labels Aug 16, 2026
@SebTardif

Copy link
Copy Markdown
Contributor Author

@clawsweeper

The PR has useful terminal proof at b501d31, but the final retained-text limits were added in 6baa16f and need a redacted current-head runtime transcript.

Refreshed the PR body with public CLI output from 6baa16f: 256-byte id reject, 262144-byte retained-id reject, 5520-id reject, and a 299600-byte JSONL source that loads then fails on a missing SkillTrustBench row. Also ran make docs-site (Built 7 docs page(s) in dist/docs-site); generated dist/ was not committed.

@clawsweeper re-review

@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. and removed status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. labels Aug 16, 2026
@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. and removed rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. labels Aug 23, 2026
@clawsweeper clawsweeper Bot added merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: needs maintainer proof decision A ClawSweeper-authored PR needs a maintainer proof capture or override decision. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. status: needs maintainer proof decision A ClawSweeper-authored PR needs a maintainer proof capture or override decision. labels Aug 24, 2026
@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. labels Sep 3, 2026
SebTardif and others added 3 commits September 15, 2026 20:41
PLAN: HTTP and file --ids loaders used unbounded ReadAll. PR 45 capped
HTTP at 256 KiB, which rejects the valid 1.3 MiB full SkillTrustBench
JSONL list and left local files unbounded.

DO: stream both sources through the existing line parser and reject
more than 5520 unique IDs, the pinned SkillTrustBench full set.

Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
The 5,520-id cap ran after each extracted id was stored. A hostile
HTTP source could still retain thousands of unique megabyte-sized
ids. Cap one id at 256 bytes and retained id text at 256 KiB. That
still accepts the documented SkillTrustBench set.

Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
Release whitespace-padded source lines, cover full-set streams and early HTTP rejection, and document all selection limits.

Co-authored-by: Sebastien Tardif <sebtardif@ncf.ca>
@steipete
steipete force-pushed the fix/ids-stream-semantic-bound branch from 6baa16f to 6db87f2 Compare September 16, 2026 03:48
@steipete
steipete requested a review from a team as a code owner September 16, 2026 03:48
@clawsweeper

clawsweeper Bot commented Sep 16, 2026

Copy link
Copy Markdown

ClawSweeper status: review started.

I am starting a fresh review of this pull request: fix(runner): stream --ids sources and cap selection to the pinned set This is item 1/1 in the current shard. Shard 0/1.

This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking.

Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted.

@steipete
steipete merged commit 4b644ab into openclaw:main Sep 16, 2026
8 of 9 checks passed
@steipete

steipete commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

Merged as 4b644ab. Thanks @SebTardif.

Validated with go test -count=1 ./..., go vet ./..., focused TestLoadBenchmarkIDSelection tests with and without -race, all 27 Node wrapper/script tests, make docs-site, and the npm package --pack --smoke build. Independent Codex P0–P2 review was scoped-clean.

The built CLI rejected all three selection-limit fixtures and rejected an invalid loopback HTTP stream in 0.040 seconds before EOF. Regression tests accepted all 5,520 IDs from a 4,134,480-byte JSONL file and HTTP response with matching order/digest, and verified that 64 whitespace-padded IDs no longer retain over 16 MiB. These are synthetic fixture proofs; no live Hugging Face or scanner API was used.

Exact-head CI passed on 6db87f2422471eb56db5fb82424ab6f7cd3c12b1: CI, CodeQL, and submission validation. No failed checks or reruns. Main has been pulled and verified clean.

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

Labels

P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. 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.

2 participants