feat(bench): enforce runner timing policy - #1899
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. |
clawsweeper
Bot
added
P2
|
Codex review: needs maintainer review before merge. Reviewed September 6, 2026, 11:09 PM ET / September 7, 2026, 03:09 UTC. ClawSweeper reviewWhat this changesAdds a local benchmark check command that enforces sample, failure-count, and runner-duration limits, with structured output, documentation, and tests. Merge readiness✅ Ready for maintainer review The policy gate remains a distinct, useful addition beyond current main’s descriptive reports. The reconciled candidate resolves the previous conflict blocker, and this review found no actionable correctness or security defects. Priority: P2 Review scores
Verification
How this fits togetherCrabbox records execution timings in a local JSONL ledger and groups those observations for comparison. The new check command reads those groups and returns policy results and an exit code for scripts or CI. flowchart LR
A[Local timing ledger] --> B[Read and validate records]
C[Provider and recency filters] --> D[Group matching observations]
B --> D
D --> E[Evaluate samples and timing limits]
F[Policy flags] --> E
E --> G[Text or JSON results]
E --> H[Pass or fail exit code]
Before mergeNone. Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Keep enforcement opt-in through the shared benchmark aggregation path while preserving descriptive reports and existing ledger compatibility. Do we have a high-confidence way to reproduce the issue? Not applicable: this adds an explicit policy-checking capability rather than repairing a reported existing-behavior failure. Is this the best way to solve the issue? Yes: reusing the existing reader, filters, and aggregation avoids a competing benchmark implementation, and enforcement remains an explicitly invoked command. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 7de92efad414. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (7 earlier review cycles)
|
b9ce9c3 to
bc819ab
Compare
|
@clawsweeper re-review Rebased onto current |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
@clawsweeper re-review The exact reviewed head remains |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
Merge current main into the existing benchmark policy branch without rewriting its history. Resolve only the changelog conflict, retaining the benchmark entry under Unreleased with its PR link and verified contributor credit. The benchmark documentation, implementation, tests, and CLI registration remain byte-identical to the previously reviewed head. Related: #1899 Punchcard-Session: calm-lantern-orchard-dn
Merge main at 7de92ef without rewriting the benchmark policy branch. Preserve both Unreleased entries. The four benchmark documentation, implementation, test, and registration files remain byte-identical to the audited contributor head. Related: #1899 Thanks @vincentkoc. Punchcard-Session: calm-lantern-orchard-dn
1 participant
What Problem This Solves
Resolves a problem where local benchmark reports described runner timings but
could not enforce a repeatable performance policy in scripts or CI. A slow,
failed, undersampled, or legacy-only group could not fail a benchmark gate.
Related: #1896
Why This Change Was Made
Adds
crabbox bench check, reusing the report command's store, provider,command-fingerprint, and recency filters. The command evaluates every matched
group against successful-sample, failure-count, runner-telemetry, and required
p95 runner-total limits.
Policy failures return exit 1 after emitting the complete human or schema-v1
JSON result. Invalid flags, durations, and stores return exit 2. Check JSON is
deterministic and omits store paths, raw records, command text, command
fingerprints, and lease or run IDs.
The reporting prerequisite in #1896 is
merged. This branch preserves its existing history and merges main at
7de92efad4149a343b317e086051365a224be32b, following the earlier reconciliationat
3eb7469107ab33471b4be0420af521c845b9dd94. Only changelog conflicts wereresolved; the four benchmark documentation, implementation, test, and CLI
registration files are byte-identical to audited head
bc819abc4f30c20a2c7d5e79c8326b5e6818c95b.User Impact
Developers can gate local runner performance:
The default policy requires three successful runner-total samples per group
and allows zero failed observations. Existing
bench reportbehavior remainsdescriptive and now exposes
runnerTotalN.No provider, coordinator, Worker, workflow, credential, or image-publication
behavior changes. Thanks @vincentkoc.
Evidence
5ea4563c1f9059b01d826fe5d4d741c62040d87b.GOTOOLCHAIN=go1.26.5 go test -race -timeout=20m ./internal/cli -run 'TestBench|TestBenchmark|TestPrintBenchmark' -count=1: passed.GOTOOLCHAIN=go1.26.5 go vet ./internal/cli: passed.GOTOOLCHAIN=go1.26.5 bash scripts/check-docs.sh: passed, including docs-site build and all 16 docs tests.GOTOOLCHAIN=go1.26.5 go build -trimpath -o <task-binary> ./cmd/crabbox: passed.e616acf28375b41dc3a481ca17668f929efc9b0d: all nine contract clauses passed with no findings. Synthetic-ledger invocations verified exit codes 0/1/2, threshold boundaries, failure/sample budgets, filters, all-group semantics, privacy, deterministic JSON, report compatibility, and unchanged binary/ledger hashes. Help-only clarification changed no production code. Validated binary SHA-256:9e6c9cc9ded02bad98acf1adc656130e3ef30ada9f1751f9fc50829ef116cb0a.eb8bb555ceb61e333e9f626df4ebe7f27e7b6bf4c04105740356569daf4719af; no new source-blind run is claimed for this binary.git diff --check 7de92efad4149a343b317e086051365a224be32b: passed.Punchcard-Session: calm-lantern-orchard-dn