fix(runner): stream --ids sources and cap selection to the pinned set - #47
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
Codex review: needs changes before merge. Reviewed September 14, 2026, 6:54 PM ET / 22:54 UTC (Revision 53). ClawSweeper reviewWhat this changesThe 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 Review scores
Verification
How this fits togetherClawScan’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]
Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Technical reviewBest 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:
Overall correctness: patch is correct AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 6190d96d7fc4. LabelsLabel justifications:
EvidenceAcceptance criteria:
What I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (52 earlier review cycles; latest 8 shown)
|
Pushed @clawsweeper re-review |
Refreshed the PR body with public CLI output from @clawsweeper re-review |
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>
6baa16f to
6db87f2
Compare
|
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. |
|
Merged as 4b644ab. Thanks @SebTardif. Validated with 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 |
Summary
Loading a large local or HTTP
--idssource 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
Security / Trust Impact
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 ./...--helpand syntheticclawscan-staticscango test -count=1 -run TestLoadBenchmarkIDSelection -v ./internal/runnergo test -race -count=1 -run TestLoadBenchmarkIDSelection ./internal/runnermake docs-site--pack --smokeRed 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.