chore(perf): say when the clap ratio slides, and record why the derive is stricter - #996
Conversation
|
Warning Review limit reached
Next review available in: 5 minutes Limit details: You’ve used all 4 included reviews currently available under your plan. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThe PR documents the derive parser’s intentional duplicate-flag policy and adds a non-gating warning when the clap-to-usage benchmark ratio falls below its expected floor.
Confidence Score: 5/5The PR appears safe to merge because no blocking failure remains. No blocking failure remains. Important Files Changed
Reviews (2): Last reviewed commit: "chore(perf): say when the clap ratio sli..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| # | ||
| # `::warning::` when running under GitHub Actions, a plain line otherwise, so the same script is | ||
| # useful at a terminal. | ||
| clap_ratio=$(awk -v a="$clap_cold" -v b="$usage_cold" 'BEGIN { printf "%d", a / b }') |
There was a problem hiding this comment.
The new warning always formats clap_ratio as an integer, while the existing report preserves one decimal below 10x. A ratio below 1 therefore becomes 0x, producing a misleading claim that usage is “0x cheaper than clap” precisely when the warning needs to explain a severe regression.
Knowledge Base Used: Compiled argv parsing and derives
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
…e is stricter Two loose ends from the differential-fuzzing work, both mine and both left as open questions rather than decisions. **The ratio had nobody watching it.** `tak.toml` explains at length why the shadow benchmarks are not gated — the fixture grows on purpose, so an absolute count fires on a bigger fixture rather than a slower parse — and then says what *is* worth watching: "the ratio between the two frameworks, which is a property of the parsers rather than of the fixture." Nothing acted on that. The number went 117x to 97x across a run of feature work, unremarked, because the only place it appeared was one line of a report nobody diffs, and clap is flat so the movement was ours. So `perf-shadow.sh` now says so: below 80x it emits a `::warning::` under Actions and a stderr line at a terminal. Not a gate — the workflow runs this with `|| true`, and a shared runner is no place to fail a build on an instruction count — and 80 rather than 97 because the point is to catch a slide rather than pin today's figure. Checked on both sides of the floor: quiet at 97x, fires at 73x. **And the derive's duplicate rule is a decision, not an oversight.** The corpus specifies the opposite for a spec-driven parse — a repeat is a correction, the later occurrence wins — and usage-argv's parser and usage-lib both honour it. The rule exists only in the derive's post-binding layer. Kept, because the two cases differ. A spec parsed at run time may be describing someone else's command line, which is the wrapper the corpus has in mind. A derived CLI is an authored surface where `--jobs 1 --jobs 2` is likelier a mistake than an amendment, and it is what clap does — so an adopter sees no change, where dropping it would have a derived binary quietly accept a line clap rejects today. That is the direction that costs an adopter rather than helping. Written down because it reads as an inconsistency and is not one; the gate's `differential.rs` carries the same note from the other end.
6837c47 to
de2a01f
Compare
Instruction counts
No instruction-count regression above 1%. Only instruction counts gate. Wall clock is shown for context — on identical hardware it moves 4-20% run to run. Measured by tak — instruction-counted CLI benchmarks, stored in this repository's git notes. Shadow comparisonParsing
|
1 participant
Two loose ends from the differential-fuzzing work, both mine and both left as
open questions rather than decisions.
The ratio had nobody watching it.
tak.tomlexplains at length why theshadow benchmarks are not gated — the fixture grows on purpose, so an absolute
count fires on a bigger fixture rather than a slower parse — and then says what
is worth watching: "the ratio between the two frameworks, which is a property
of the parsers rather than of the fixture." Nothing acted on that. The number
went 117x to 97x across a run of feature work, unremarked, because the only
place it appeared was one line of a report nobody diffs, and clap is flat so the
movement was ours.
So
perf-shadow.shnow says so: below 80x it emits a::warning::underActions and a stderr line at a terminal. Not a gate — the workflow runs this
with
|| true, and a shared runner is no place to fail a build on aninstruction count — and 80 rather than 97 because the point is to catch a slide
rather than pin today's figure. Checked on both sides of the floor: quiet at
97x, fires at 73x.
And the derive's duplicate rule is a decision, not an oversight. The corpus
specifies the opposite for a spec-driven parse — a repeat is a correction, the
later occurrence wins — and usage-argv's parser and usage-lib both honour it.
The rule exists only in the derive's post-binding layer.
Kept, because the two cases differ. A spec parsed at run time may be describing
someone else's command line, which is the wrapper the corpus has in mind. A
derived CLI is an authored surface where
--jobs 1 --jobs 2is likelier amistake than an amendment, and it is what clap does — so an adopter sees no
change, where dropping it would have a derived binary quietly accept a line clap
rejects today. That is the direction that costs an adopter rather than helping.
Written down because it reads as an inconsistency and is not one; the gate's
differential.rscarries the same note from the other end.Stack created with GitHub Stacks CLI • Give Feedback 💬
Note
Low Risk
Comments and non-failing CI warnings only; no runtime or parsing behavior changes.
Overview
Documentation and observability only — no change to duplicate-flag behavior or parse logic.
tasks/perf-shadow.shnow definesCLAP_RATIO_FLOOR=80. After the shadow report is written, if the cold-parse ratio (clap vs usage) falls below that floor, the script emits a::warning::on GitHub Actions or a stderr warning locally. This is explicitly not a gate (|| truein CI); it flags parser-side regression when clap’s count stays flat.In
derive/src/codegen.rs, a long comment onrejects_duplicatewas moved belowreset_per_leveland expanded. It records why the derive rejects repeated non-repeatable flags (clap-aligned,Error::DuplicateFlagfrom this layer) while spec-driven parsing treats repeats as “last wins,” and points readers todifferential.rsfor the other side of that story.Reviewed by Cursor Bugbot for commit de2a01f. Bugbot is set up for automated code reviews on this repo. Configure here.
AI-assisted — Tool: Claude Code; model: anthropic/claude-opus-5; version: unavailable.