Skip to content

feat: add local benchmark timing ledger - #455

Merged
steipete merged 5 commits into
openclaw:mainfrom
TurboTheTurtle:tt/crabbox-342-bench-ledger
Jun 20, 2026
Merged

feat: add local benchmark timing ledger#455
steipete merged 5 commits into
openclaw:mainfrom
TurboTheTurtle:tt/crabbox-342-bench-ledger

Conversation

@TurboTheTurtle

@TurboTheTurtle TurboTheTurtle commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Closes #342.

Summary

  • add an opt-in local benchmark timing ledger around existing TimingReport payloads
  • add run --timing-record, bench run, bench record, and evidence-aware bench report --json
  • keep observations local, report successful sample and failure counts, and mark insufficient evidence instead of ranking providers globally
  • preserve delegated providers' complete timing metadata and the existing single final --timing-json payload contract

Verification

  • go test -race ./internal/cli
  • go vet ./...
  • scripts/check-docs.sh
  • focused benchmark/delegated timing regression tests
  • autoreview: clean after preserving provider-authored delegated sync timing and phases

Real behavior proof

Built the CLI from head 1a62922c and exercised a real Docker-backed local-container workflow:

  • direct run --timing-record: one successful observation, automatic lease cleanup
  • bench run --provider local-container --repeats 2 -- true: two observations, zero failures, automatic cleanup after both runs
  • bench report --provider local-container --min-samples 2 --json: three matched successful observations, providerCategory=local-runtime, insufficientEvidence=false, evidence=sufficient_local_samples

The delegated regression coverage also proves that recording captures provider-authored sync duration, sync phases, and machine metadata; --timing-json remains one final JSON object, and timing JSON is not printed when only recording is requested.

@TurboTheTurtle

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Requesting ClawSweeper review for this new PR.

@clawsweeper

clawsweeper Bot commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@TurboTheTurtle
TurboTheTurtle force-pushed the tt/crabbox-342-bench-ledger branch from ef6178e to 8b739cd Compare June 18, 2026 07:30
@TurboTheTurtle

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@TurboTheTurtle

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed June 19, 2026, 8:43 PM ET / 00:43 UTC.

Summary
The branch adds an opt-in local benchmark timing ledger with run --timing-record, bench run, bench record, bench report, provider-category metadata, docs, and focused CLI regression tests.

Reproducibility: not applicable. this is an additive feature PR, not a bug report. Source and related-issue inspection show the requested surface is new rather than broken existing behavior.

Review metrics: 3 noteworthy metrics.

  • Public CLI surface: 1 command group added, 1 run flag added. The PR changes the user-facing CLI contract, so proof and docs alignment matter before merge.
  • Diff scope: 14 files changed, +1495/-17. The change spans CLI code, tests, docs, generated metadata, and the generator script.
  • Focused tests: 6 benchmark/timing tests added. The new tests cover the highest-risk ledger, delegated timing, fan-out, and report paths.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #342
Summary: This PR is the candidate implementation for the canonical local benchmark ledger feature request.

Members:

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

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🦞 diamond lobster
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P2] Let the in-progress CI jobs finish against 1a62922c before merge.

Risk before merge

  • [P1] Latest GitHub status still reports several CI jobs in progress for head 1a62922c; merge should wait for required checks to finish.
  • [P1] The PR introduces a new opt-in local JSONL/report contract, so maintainers should treat the v1 field names and privacy wording as product surface before merge.

Maintainer options:

  1. Decide the mitigation before merge
    Land this PR after required checks complete and maintainers are comfortable with the opt-in v1 JSONL/report contract, letting the linked feature issue close through the PR.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • No ClawSweeper repair lane is needed; the remaining action is normal maintainer review plus CI gating.

Security
Cleared: No concrete security or supply-chain regression was found; the ledger is opt-in, local, owner-permissioned on new files, and documents private timing payload contents.

Review details

Best possible solution:

Land this PR after required checks complete and maintainers are comfortable with the opt-in v1 JSONL/report contract, letting the linked feature issue close through the PR.

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

Not applicable: this is an additive feature PR, not a bug report. Source and related-issue inspection show the requested surface is new rather than broken existing behavior.

Is this the best way to solve the issue?

Yes for the implementation shape after the latest fixes: persistence is opt-in, reports are local-evidence based, delegated timing details are preserved, and provider categories are populated from generated metadata. Final acceptance still needs normal maintainer product review because this adds CLI and storage surface.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 8bd692605598.

Label changes

Label changes:

  • add proof: sufficient: Contributor real behavior proof is sufficient. A PR comment includes redacted terminal output from a real Docker-backed local-container run showing the new record, run, and JSON report behavior after the latest fixes.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): A PR comment includes redacted terminal output from a real Docker-backed local-container run showing the new record, run, and JSON report behavior after the latest fixes.
  • remove status: 📣 needs proof: Current PR status label is status: 👀 ready for maintainer look.
  • remove merge-risk: 🚨 compatibility: Current PR review selected no merge-risk labels.
  • remove rating: 🦪 silver shellfish: Current PR rating is rating: 🐚 platinum hermit, so this older rating label is no longer current.

Label justifications:

  • P2: This is a normal-priority additive CLI feature with broad enough user-facing surface to need maintainer review and CI, but it is opt-in and limited in blast radius.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): A PR comment includes redacted terminal output from a real Docker-backed local-container run showing the new record, run, and JSON report behavior after the latest fixes.
  • proof: sufficient: Contributor real behavior proof is sufficient. A PR comment includes redacted terminal output from a real Docker-backed local-container run showing the new record, run, and JSON report behavior after the latest fixes.
Evidence reviewed

What I checked:

  • Repository policy read: Read the full target AGENTS.md and applied its provider-neutral boundary, docs/testing guidance, and release-owned changelog guidance during review. (AGENTS.md:1, 8bd692605598)
  • Current main does not implement the feature: Current main has no timing-record, bench run, bench report, or benchKongCmd hits in the CLI/docs paths, so the PR is not obsolete on main. (8bd692605598)
  • PR implements the requested CLI/docs surface: The PR head registers --timing-record, adds bench docs and CLI command wiring, and documents bench run/bench report usage. (internal/cli/run.go:245, 1a62922cf268)
  • Provider category contract is now populated: newBenchmarkTimingRecord fills provider family, kind, and category from provider metadata, and reports backfill missing category data when grouping old rows. (internal/cli/bench.go:404, 1a62922cf268)
  • Delegated timing capture preserves existing timing JSON contract: Delegated providers receive TimingJSON when recording is enabled, captured provider-authored reports are reused for the ledger, and the final defer only prints JSON when --timing-json was requested. (internal/cli/run.go:560, 1a62922cf268)
  • Focused regression coverage added: The added tests cover appending timing records, delegated timing JSON emission while recording, recording without leaking JSON, provider fan-out/repeats, aggregation, and JSON report filtering. (internal/cli/bench_test.go:78, 1a62922cf268)

Likely related people:

  • steipete: Current-main blame attributes the run and TimingReport baseline to Peter Steinberger, and this PR's later commits preserve delegated timing details and populate benchmark provider categories. (role: timing/run surface owner and recent PR repair author; confidence: high; commits: 008f70f30614, e6bf91308636, 9a495606cd85; files: internal/cli/run.go, internal/cli/timing.go, internal/cli/bench.go)
  • coygeek: This handle authored the linked benchmark-ledger issue with the provider-category and delegated timing requirements that define the PR's target behavior. (role: feature requester and adjacent provider-surface contributor; confidence: medium; files: docs/providers/provider-metadata.json, docs/providers/README.md, internal/cli/providers.go)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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.

How this review workflow works
  • 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.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. labels Jun 19, 2026
@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. labels Jun 20, 2026
@steipete

Copy link
Copy Markdown
Contributor

After-fix proof for #455 at 1a62922c.

Fixes:

Redacted real Docker-backed output:

$ crabbox run --provider local-container --timing-record <store> -- true
run summary sync=1.375s command=64ms total=1.454s sync_skipped=false exit=0
lease cleanup stopped=true policy=auto
benchmark timing record appended path=<store> observations=1

$ crabbox bench run --store <store> --provider local-container --repeats 2 -- true
benchmark timing record appended path=<store> observations=1
benchmark timing record appended path=<store> observations=1
benchmark run completed path=<store> observations=2 failures=0

$ crabbox bench report --store <store> --provider local-container --min-samples 2 --json
{"schemaVersion":1,"observationCount":3,"matchedCount":3,"groups":[{"provider":"local-container","providerFamily":"container","providerKind":"ssh-lease","providerCategory":"local-runtime","n":3,"failureCount":0,"insufficientEvidence":false,"evidence":"sufficient_local_samples"}]}

Additional proof: go test -race ./internal/cli, go vet ./..., scripts/check-docs.sh, provider-metadata drift check, and structured autoreview all pass.

@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. and removed 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. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. labels Jun 20, 2026
@steipete
steipete merged commit 4a061f5 into openclaw:main Jun 20, 2026
6 checks passed
@TurboTheTurtle
TurboTheTurtle deleted the tt/crabbox-342-bench-ledger branch June 22, 2026 03:49
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.

Add local benchmark timing records and provider performance reports

2 participants