Skip to content

test(perf): compare saved CLI startup benchmarks#85400

Closed
FelixIsaac wants to merge 1 commit into
openclaw:mainfrom
FelixIsaac:perf/cli-startup-bench-compare
Closed

test(perf): compare saved CLI startup benchmarks#85400
FelixIsaac wants to merge 1 commit into
openclaw:mainfrom
FelixIsaac:perf/cli-startup-bench-compare

Conversation

@FelixIsaac

@FelixIsaac FelixIsaac commented May 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

  • add saved-report comparison mode to scripts/bench-cli-startup.ts
  • emit duration and RSS deltas as either terminal text or JSON
  • keep the existing entry-vs-entry comparison path for same-run experiments
  • refresh shrinkwrap snapshots so check-guards passes under the current pnpm lockfile

Why

The follow-up performance PRs need repeatable before/after numbers. The benchmark harness could already save JSON reports, but comparing two saved reports required ad-hoc one-off scripts. This makes the comparison reusable for future PR bodies and review proof.

Real behavior proof (required for external PRs)

  • Behavior or issue addressed: saved CLI startup benchmark reports can now be compared directly, with text output for PR proof and JSON output for downstream tooling.
  • Real environment tested: Real OpenClaw checkout on Linux, Node.js v24.2.0, using saved benchmark JSON reports produced from the perf(cli): skip config load for default root help #85396 root-help performance test.
  • Exact steps or command run after this patch:
node scripts/bench-cli-startup.ts --compare-baseline /tmp/root-help-before.json --compare-candidate /tmp/root-help-after.json
node scripts/bench-cli-startup.ts --compare-baseline /tmp/root-help-before.json --compare-candidate /tmp/root-help-after.json --json
corepack pnpm exec oxfmt --write --threads=1 scripts/bench-cli-startup.ts
node scripts/generate-npm-shrinkwrap.mjs --all --check
--help -1072.3ms (-96.0%) rss -209.9MB (-79.5%)

JSON mode emitted machine-readable deltas[] entries containing durationAvgDeltaMs, durationAvgDeltaPct, maxRssAvgDeltaMb, and maxRssAvgDeltaPct.

  • Observed result after fix: The text comparison reports the expected startup and RSS reduction from the saved before/after reports, and JSON mode exposes the same deltas as structured fields for automation.
  • What was not tested: I did not rerun the original slow benchmark captures in this PR; this PR validates comparison of already-saved real benchmark reports.
  • Before evidence (optional but encouraged): Before this patch, comparing two saved benchmark JSON files required ad-hoc one-off scripts instead of a reusable harness mode.

Tests

  • corepack pnpm exec oxfmt --write --threads=1 scripts/bench-cli-startup.ts (commit hook)
  • git diff --check -- scripts/bench-cli-startup.ts
  • corepack pnpm check:no-conflict-markers && node scripts/generate-npm-shrinkwrap.mjs --all --check
  • manual smoke commands above

Hygiene

  • committed with GitHub noreply author email
  • staged only scripts/bench-cli-startup.ts in the original feature commit and only generated npm-shrinkwrap.json files in the follow-up guard-fix commit
  • checked the staged diffs for obvious credential patterns before committing

Related follow-up to #85353, #85368, and #85396; this covers the benchmark/profiling harness bucket for the performance-audit series.

This was referenced May 22, 2026
Closed
Closed
Closed
@openclaw-barnacle openclaw-barnacle Bot added scripts Repository scripts size: S triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels May 22, 2026
@clawsweeper

clawsweeper Bot commented May 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs changes before merge. Reviewed June 19, 2026, 1:08 AM ET / 05:08 UTC.

Summary
The PR adds saved-report comparison flags to scripts/bench-cli-startup.ts and emits benchmark duration/RSS deltas as text or JSON.

PR surface: Other +67. Total +67 across 1 file.

Reproducibility: yes. for the PR defect: source inspection of the merge ref shows the compare branch returns before the existing options.output writer runs. I did not run the script because this review was read-only.

Review metrics: 1 noteworthy metric.

  • Script CLI options: 2 added, 1 existing output path bypassed. The PR adds compare flags but the new early branch skips the existing --output file writer.

Merge readiness
Overall: 🦐 gold shrimp
Proof: 🐚 platinum hermit
Patch quality: 🦐 gold shrimp
Result: needs maintainer review before merge.

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

Rank-up moves:

  • Write compare-mode JSON to --output using the existing parent-directory creation behavior.
  • [P2] Add a focused test or smoke for compare mode with --output if practical.

Next step before merge

  • [P2] The remaining blocker is a narrow script-contract repair that the automerge repair lane can make on the PR branch.

Security
Cleared: The diff reads local JSON files supplied by the caller and adds no dependencies, workflow changes, package metadata, secret handling, or remote code execution paths.

Review findings

  • [P2] Honor --output in compare mode — scripts/bench-cli-startup.ts:1018-1023
Review details

Best possible solution:

Keep the saved-report comparison mode in the benchmark script, but make it honor the existing JSON file-output convention before automerge.

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

Yes for the PR defect: source inspection of the merge ref shows the compare branch returns before the existing options.output writer runs. I did not run the script because this review was read-only.

Is this the best way to solve the issue?

No. The benchmark script is the right layer for saved-report comparison, but the implementation should also write the comparison JSON when --output is supplied.

Full review comments:

  • [P2] Honor --output in compare mode — scripts/bench-cli-startup.ts:1018-1023
    --output is still documented and used as the machine-readable JSON file path, but this new compare branch returns after stdout handling and never reaches the existing file writer. A saved-report comparison invoked with --output will silently produce no file, so write the comparison JSON to options.output before returning, mirroring the normal report path.
    Confidence: 0.88

Overall correctness: patch is incorrect
Overall confidence: 0.86

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 5e7a0b155808.

Label changes

Label changes:

  • add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🐚 platinum hermit and patch quality is 🦐 gold shrimp.
  • add status: 🚀 automerge armed: This PR is in ClawSweeper's automerge lane. Sufficient (live_output): The PR body includes after-patch live terminal output from saved-report comparison plus JSON-mode smoke details, which is suitable proof for the non-visual script behavior it claims.
  • remove rating: 🐚 platinum hermit: Current PR rating is rating: 🦐 gold shrimp, so this older rating label is no longer current.
  • remove status: 👀 ready for maintainer look: Current PR status label is status: 🚀 automerge armed.

Label justifications:

  • P3: This is a low-risk performance benchmark harness improvement with limited blast radius.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🐚 platinum hermit and patch quality is 🦐 gold shrimp.
  • status: 🚀 automerge armed: This PR is in ClawSweeper's automerge lane. Sufficient (live_output): The PR body includes after-patch live terminal output from saved-report comparison plus JSON-mode smoke details, which is suitable proof for the non-visual script behavior it claims.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes after-patch live terminal output from saved-report comparison plus JSON-mode smoke details, which is suitable proof for the non-visual script behavior it claims.
Evidence reviewed

PR surface:

Other +67. Total +67 across 1 file.

View PR surface stats
Area Files Added Removed Net
Source 0 0 0 0
Tests 0 0 0 0
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 1 79 12 +67
Total 1 79 12 +67

Acceptance criteria:

  • [P1] node scripts/run-vitest.mjs test/scripts/bench-cli-startup.test.ts.
  • [P1] git diff --check -- scripts/bench-cli-startup.ts test/scripts/bench-cli-startup.test.ts.

What I checked:

  • Root policy read: Root AGENTS.md was read fully and requires whole-module PR review, scoped AGENTS.md checks, current-main behavior, tests, and history before verdicts. (AGENTS.md:1, 5e7a0b155808)
  • Scripts policy read: scripts/AGENTS.md was read fully; its script-wrapper and generated-output guidance applies to this scripts-only PR. (scripts/AGENTS.md:1, 5e7a0b155808)
  • Live PR state: GitHub reports head 77414e4 as mergeable/clean, with one changed file and maintainerCanModify=true. (77414e48cc35)
  • Merge-result source review: The pull-request merge ref contains both the compare-mode additions and current-main helpers such as strict integer parsing, failed-sample collection, and the guarded main() export pattern. (scripts/bench-cli-startup.ts:815, 3da2552d2dc6)
  • Output contract evidence: Current package scripts and docs use --output to save CLI startup benchmark JSON reports, while the merge-ref compare path returns before the normal options.output writer runs. (package.json:1879, 5e7a0b155808)
  • Adjacent tests: Existing tests cover benchmark failure reporting and option parsing helpers; there is no current-main saved-report compare-mode or compare --output coverage. (test/scripts/bench-cli-startup.test.ts:28, 5e7a0b155808)

Likely related people:

  • Vincent Koc: Current-main history/blame identify this person as the checkout author of the benchmark script, tests, docs reference, and package scripts, and the PR discussion includes a maintainer automerge command from the same login. (role: recent area contributor; confidence: high; commits: 6c82a9fb1866; files: scripts/bench-cli-startup.ts, test/scripts/bench-cli-startup.test.ts, docs/reference/test.md)
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 proof: sufficient ClawSweeper judged the real behavior proof convincing. 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. P3 Low-priority cleanup, docs, polish, ergonomics, or speculative work. labels May 22, 2026
@clawsweeper

clawsweeper Bot commented May 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

ClawSweeper PR egg

✨ Hatched: 🥚 common Clockwork Shellbean

Hatch command

Comment @clawsweeper hatch when this PR is hatchable.

Hatchability rules:

  • Merged PRs are hatchable.
  • Open PRs are hatchable when they are status: 👀 ready for maintainer look, status: 🚀 automerge armed, or labeled clawsweeper:automerge.
  • Closed unmerged PRs are hatchable only when one of those hatchable labels is still present in the durable record.

Rarity: 🥚 common.
Trait: collects tiny proofs.
Image traits: location status garden; accessory little merge flag; palette violet, aqua, and starlight; mood bright-eyed; pose leaning over a miniature review desk; shell smooth pearl shell; lighting calm overcast light; background subtle branch markers.
Share on X: post this hatch
Copy: My PR egg hatched a 🥚 common Clockwork Shellbean in ClawSweeper.

What is this egg doing here?
  • Eggs appear after the PR passes real-behavior proof. It is here for vibes, not verdicts: it does not change labels, ratings, merge decisions, or automation.
  • The shell reacts to review momentum: open follow-up work warms it up, re-review makes it wobble, and a clean final review lets it hatch.
  • Hatchability usually comes from sufficient real-behavior proof, no blocking P0/P1/P2 findings, no security attention needed, and clean correctness. A merged PR is already final, so merge makes the egg hatchable independently.
  • The hatch is seeded from this repository and PR number, so the same PR keeps the same creature; the reviewed head SHA can only change safe visual details.
  • Rarity is just collectible sparkle: 🥚 common, 🌱 uncommon, 💎 rare, ✨ glimmer, and 🌈 legendary.

@FelixIsaac
FelixIsaac requested a review from a team as a code owner May 22, 2026 20:21
@github-actions github-actions Bot added the dependencies-changed PR changes dependency-related files label May 22, 2026
@openclaw-barnacle openclaw-barnacle Bot added channel: discord Channel integration: discord channel: msteams Channel integration: msteams channel: slack Channel integration: slack channel: whatsapp-web Channel integration: whatsapp-web channel: zalouser Channel integration: zalouser extensions: diagnostics-otel Extension: diagnostics-otel extensions: memory-lancedb Extension: memory-lancedb channel: feishu Channel integration: feishu channel: twitch Channel integration: twitch extensions: acpx extensions: codex extensions: amazon-bedrock extensions: anthropic-vertex extensions: diffs labels May 22, 2026
@FelixIsaac

FelixIsaac commented May 22, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Pushed refreshed shrinkwrap snapshots for check-guards and normalized the Real behavior proof section with copied live benchmark comparison output.

@openclaw-barnacle openclaw-barnacle Bot removed the triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. label May 22, 2026
@FelixIsaac

FelixIsaac commented May 22, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Rebuilt the branch on current main and replayed only the saved-report benchmark comparison change, with no shrinkwrap deltas. Focused checks passed: node scripts/generate-npm-shrinkwrap.mjs --all --check, git diff --check -- scripts/bench-cli-startup.ts npm-shrinkwrap.json extensions/*/npm-shrinkwrap.json, and a JSON compare-mode smoke using temporary saved benchmark reports.

@clawsweeper

clawsweeper Bot commented May 22, 2026 •
edited
Loading

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:

@clawsweeper clawsweeper Bot added 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: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. labels May 22, 2026
@RomneyDa

RomneyDa commented May 29, 2026

Copy link
Copy Markdown
Member

Heads up: this PR needs to be updated against current main before the new required Dependency Guard check can pass.

@vincentkoc

vincentkoc commented Jun 19, 2026

Copy link
Copy Markdown
Member

/clownfish automerge

@openclaw-clownfish openclaw-clownfish Bot added clownfish:automerge Maintainer opted this Clownfish PR into bounded ClawSweeper-reviewed automerge clawsweeper:automerge Maintainer opted this PR into bounded ClawSweeper-reviewed automerge labels Jun 19, 2026
@openclaw-clownfish

openclaw-clownfish Bot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Clownfish is on the reef for this PR. 🐠

I tagged clownfish:automerge and sent ClawSweeper over this exact head. If the sweep finds rough coral, failing checks, or needs-human, I will take another bounded repair lap and ask for a fresh review.

A maintainer can call /clownfish stop any time and I will drift this back to human review.

@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 🚀 automerge armed This PR is in ClawSweeper's automerge lane. and removed 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. labels Jun 19, 2026
@clawsweeper

clawsweeper Bot commented Jun 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🔧
ClawSweeper picked up the repair feedback.

Source: clawsweeper[bot]
Feedback: structured ClawSweeper marker: fix-required (finding=review-feedback sha=77414e48cc35d88fa21f16ecba7756d6a26c858f)
Action: repair worker queued. Run: https://github.com/openclaw/clawsweeper/actions/runs/27806746713

I will update this PR branch, or open a safe credited replacement, if the repair worker finds a narrow fix.

Automerge progress:

  • 2026-06-19 05:08:56 UTC review requested repair 77414e48cc35 (structured ClawSweeper marker: fix-required (finding=review-feedback sha=77414e...)

@clawsweeper clawsweeper Bot mentioned this pull request Jun 19, 2026
Merged
@clawsweeper

clawsweeper Bot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

ClawSweeper 🐠 reef update

Thanks for the work on this. ClawSweeper did not have permission to update this branch directly, so it opened a narrow replacement PR instead. that's a branch access thing, not a knock on the contribution.

Why replacement: ClawSweeper could not update the source PR branch directly; GitHub did not grant sufficient push rights to the bot for that branch.
Replacement PR: #94812
Why close: this run explicitly closes the superseded source PR after the credited replacement PR is open, so review continues in one place.
This source PR is being closed only under the explicit source-close setting for this ClawSweeper run.
Contributor credit is copied into the replacement PR notes and release-note context.
Co-author credit kept:

fish notes: reasoning high; reviewed against 32da7b8.

@clawsweeper clawsweeper Bot closed this Jun 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

clawsweeper:automerge Maintainer opted this PR into bounded ClawSweeper-reviewed automerge clownfish:automerge Maintainer opted this Clownfish PR into bounded ClawSweeper-reviewed automerge P3 Low-priority cleanup, docs, polish, ergonomics, or speculative work. proof: sufficient ClawSweeper judged the real behavior proof convincing. proof: supplied External PR includes structured after-fix real behavior proof. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. scripts Repository scripts size: S status: 🚀 automerge armed This PR is in ClawSweeper's automerge lane.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@FelixIsaac @RomneyDa @vincentkoc