fix(spec): deduplicate derived completers - #1072
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
bbd9312 to
4615168
Compare
4615168 to
cda8d65
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
|
Documents `parse_from`/`parse_from_argv` as the explicit interception point for applications that need work before rendering help or version output, and the successful-value path for work before dispatch. This confirms the existing API covers Aube’s customized version handling and hk’s pre-dispatch re-exec without adding a hidden hook lifecycle. Marks the corresponding PLAN gap complete. Stacked on #1072. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Documentation and planning updates only; no runtime or API changes in this diff. > > **Overview** > **Documents** that post-parse behavior is intentionally application-owned: `parse_from` / `parse_from_argv` return `Error::Help` and `Error::Version` instead of exiting, and successful parses hand back a value so embedders can run notifiers, customize version output, or re-exec (e.g. hk `--cd`) before dispatch. The help guide now calls out the `match` on those errors as the interception point and states there is no hidden hook lifecycle—`parse()` stays the print-and-exit shortcut. > > **PLAN** marks the “no home for post-parse hooks” gap **done**, replacing the open question with the decided pattern. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 9ec5460. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
Emits each identical built-in completion node only once per command. Multiple fields sharing a completion name and type (for example path-valued flags with
value_name = "PATH") no longer produce repeatedcomplete "path"nodes. Distinct names or types retain declaration order.The parser and facade test suites pass, including a regression that parses both fields and asserts one emitted completer. PLAN now records this portion of the broader canonical-KDL gap as complete.
Stacked on #1071.
Note
Low Risk
Cold-path spec emission and tests only; parsing and binding behavior are unchanged.
Overview
Derived KDL emission now writes each identical built-in
completenode only once per command. When several flags or arguments share the same completion name and type (for example twovalue_name = "PATH"fields with a path hint),write_completion_typestracks what was already emitted instead of repeatingcomplete "path" type="path".A facade regression test (
CompletionDedup) parses both flags and asserts a single matching completer line into_kdl(). PLAN.md records that this slice of the broader canonical-KDL work is done; full parse/serialize canonicalization is still open.Reviewed by Cursor Bugbot for commit cda8d65. Bugbot is set up for automated code reviews on this repo. Configure here.