Skip to content

refactor(cli): simplify lazy command registration - #139108

Merged
steipete merged 1 commit into
mainfrom
codex/deslop-cli-registration
Sep 5, 2026
Merged

refactor(cli): simplify lazy command registration#139108
steipete merged 1 commit into
mainfrom
codex/deslop-cli-registration

Conversation

@steipete

@steipete steipete commented Sep 5, 2026

Copy link
Copy Markdown
Contributor
Additional instructions

Repository maintainers have normal access to this same-repository branch.

What Problem This Solves

Fixes an issue where uncached root help shows the retired crestodian command even though cached help and the registered command hide it. This also implements the maintainer-requested simplification of builtin CLI registration.

Why This Change Was Made

Builtin command groups now use typed lazy registrar callbacks directly. This removes the string-based export lookup, descriptor-catalog wrappers, intermediate mapping passes, and unused core argument plumbing: 231 fewer production lines. Cheap descriptor-only help, grouped aliases, the completion import boundary, plugin registration order, and completion-specific channel options retain their existing owners.

The shared lazy placeholder now carries existing hidden metadata into Commander. The public placeholder type gains only an optional field, and the dynamically loaded private QA registrar retains its boundary validation.

User Impact

Existing command names, aliases, options, parsing, and shell completions are preserved. The retired alias stays callable but is consistently omitted from root help. No configuration, environment-variable, default, or persistence surfaces change.

Evidence

  • The real setup-alias parser test passes through direct registration but fails through lazy registration before the hidden-metadata fix. Both cases pass after the fix and still dispatch the same action arguments.
  • 447 tests across 14 files pass, covering builtin command trees and JSON parsing, startup, setup flags, grouped/plugin aliases, nested lazy help, and completion. Removed implementation-only adapter tests; net 82 fewer test lines.
  • The complete changed-file gate passes, including type checks, full core lint, dead-export scans, zero runtime import cycles, and repository guards.
  • Independent review found no actionable P0–P2 issues.
  • Full pnpm build passes, including CLI bootstrap import and plugin SDK export checks.
  • 80 real built launcher comparisons against the baseline: 79 successful help/completion cases and the unchanged private-QA unavailable-artifact error. Output is byte-identical except for removal of the retired alias line from uncached root help; cached, uncached, and eager help all hide it. Bash, Zsh, Fish, and PowerShell completion scripts are byte-identical.
  • Live commands use node openclaw.mjs with an isolated synthetic home/config. The private QA artifact is absent from the normal build, so its availability error is checked instead of claiming a private-QA execution proof.

@clawsweeper

clawsweeper Bot commented Sep 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@steipete
steipete marked this pull request as ready for review September 5, 2026 13:17
@openclaw-barnacle openclaw-barnacle Bot added cli CLI command changes size: L maintainer Maintainer-authored PR labels Sep 5, 2026
@clawsweeper clawsweeper Bot added P3 Low-priority cleanup, docs, polish, ergonomics, or speculative work. 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 Sep 5, 2026
@clawsweeper

clawsweeper Bot commented Sep 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed September 5, 2026, 1:45 PM ET / 17:45 UTC.

ClawSweeper review

What this changes

Simplifies deferred CLI command registration and consistently hides the retired setup alias from help while keeping it callable.

Merge readiness

✅ Ready for maintainer review

Useful work remains absent from main and v2026.9.1. No blocking introduced defects were found, and direct inspection of Commander 15.0.0 resolves the previous review’s dependency limitation.

Priority: P3
Reviewed head: b12b49c07f446d84581f396e20ce05d51fcce441

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A coherent simplification with meaningful parser coverage, reported launcher comparisons, and no blocking findings.
Proof confidence 🌊 off-meta tidepool Not applicable: The maintainer-authored PR is exempt from the contributor-proof gate. Its captured evidence reports built-launcher help/completion comparisons that exercise the changed registration owner, with the retired alias removed from uncached help and other output preserved.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: The maintainer-authored PR is exempt from the contributor-proof gate. Its captured evidence reports built-launcher help/completion comparisons that exercise the changed registration owner, with the retired alias removed from uncached help and other output preserved.
Evidence reviewed 13 items Introduced scope and merge identity: Reviewed the pinned merge-base-to-head changes. The test merge’s raw parents are the supplied base followed by the exact PR head; its changed paths match the introduced scope, without the unrelated base-only changes.
Current main still has the visibility defect: The lazy placeholder calls program.command(name) without forwarding hidden metadata, whereas the descriptor renderer and direct setup registrar already honor it. Main also retains the adapter layers removed by this PR.
Latest release comparison: v2026.9.1 contains the same lazy-placeholder omission and already marks the retired alias hidden in its core descriptors. The release tag was peeled to the supplied release commit.
Findings None None.
Security None None.

How this fits together

The CLI registration layer turns command descriptors and invocation arguments into help entries and executable commands. It loads command implementations on demand and supplies the command tree used for shell completion.

flowchart TD
  A[CLI arguments] --> C[Command registration]
  B[Command descriptors] --> C
  C --> D[Lightweight placeholders]
  D --> E[Help with visibility rules]
  D --> F[Load selected command group]
  F --> G[Parse and run command]
  F --> H[Shell completion tree]
Loading

Before merge

None.

Agent review details

Security

None.

PR surface

Source -231, Tests -82. Total -313 across 16 files.

View PR surface stats
Area Files Added Removed Net
Source 10 223 454 -231
Tests 6 22 104 -82
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 16 245 558 -313

Review metrics

Metric Value Why it matters
Production versus tests Production net −231 lines; tests net −82 lines Deletes adapter layers and their implementation-focused tests while extending actual alias-parser coverage.
Public placeholder contract 1 optional field added; 0 existing fields removed Visibility metadata is additive and uses the existing Commander contract.

Technical review

Best possible solution:

Keep typed lazy callbacks and propagate descriptor visibility through the shared placeholder owner, preserving existing alias and completion behavior.

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

Yes, from source: uncached root help uses lazy placeholders that discard hidden metadata on main, unlike direct registration. This read-only review did not execute a runtime reproduction.

Is this the best way to solve the issue?

Yes. Forwarding existing metadata at the shared registration boundary fixes the inconsistency without alias-specific filtering, while typed callbacks remove redundant adapters.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 402f514805ce.

Labels

Label justifications:

  • P3: This is bounded registration cleanup with a minor help-visibility correction and preserved command invocation.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The maintainer-authored PR is exempt from the contributor-proof gate. Its captured evidence reports built-launcher help/completion comparisons that exercise the changed registration owner, with the retired alias removed from uncached help and other output preserved.

Evidence

What I checked:

  • Introduced scope and merge identity: Reviewed the pinned merge-base-to-head changes. The test merge’s raw parents are the supplied base followed by the exact PR head; its changed paths match the introduced scope, without the unrelated base-only changes. (0a95fd0719ae)
  • Current main still has the visibility defect: The lazy placeholder calls program.command(name) without forwarding hidden metadata, whereas the descriptor renderer and direct setup registrar already honor it. Main also retains the adapter layers removed by this PR. (src/cli/program/register-lazy-command.ts:28, 402f514805ce)
  • Latest release comparison: v2026.9.1 contains the same lazy-placeholder omission and already marks the retired alias hidden in its core descriptors. The release tag was peeled to the supplied release commit. (src/cli/program/register-lazy-command.ts:28, ad6fe23aecb9)
  • Repair at the shared owner: Command-group registration forwards existing descriptor visibility into the shared lazy placeholder. Group replacement and reparsing remain unchanged; the setup test covers both direct and lazy registration, hidden help, and dispatch with identical action arguments. (src/cli/program/register-lazy-command.ts:30, b12b49c07f44)
  • Behavior regression coverage: The extended existing parser test asserts that help omits crestodian and that invoking it still dispatches the expected message, yes, and json values. Removed tests exercised the deleted catalog and import adapters. (src/cli/program/register.setup.test.ts:346, b12b49c07f44)
  • Applicable dependency boundary: The changed placeholder directly uses Commander’s command options, and package.json pins Commander 15.0.0. The inspected registration path does not depend on the Codex runtime or protocol. (package.json:2120, b12b49c07f44)

Likely related people:

  • Peter Steinberger: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
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.

Workflow

  • 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.

History

Review history (1 earlier review cycle)
  • reviewed 2026-09-05T13:53:32.211Z sha cac48b0 :: blocked before merge. :: none

@steipete
steipete force-pushed the codex/deslop-cli-registration branch from cac48b0 to b12b49c Compare September 5, 2026 17:12
@steipete

steipete commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

The failed typecheck/tooling jobs came from the prebuilt UI harness importing the removed readiness helper. Main already fixed that in #139123. This branch is now rebased onto that fix; all 15 recorded candidate file hashes and the test deletion match the reviewed/live-tested version.

The renewed 447-test run, 80 real CLI comparisons and independent P2 review passed. The 13 prebuilt-harness tests also pass on the integrated branch. Fresh CI is running; no helper compatibility shim or assertion/timeout change was added.

@steipete

steipete commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

Native prepare passed for b12b49c07f446d84581f396e20ce05d51fcce441. The current ClawSweeper review has no required moves; independent review, 447 tests, 80 real CLI comparisons, and 13 integrated admission tests pass. CI attempt 2 is green after retrying the failed dependency download. Ready to land the 231-line production reduction and hidden-alias correction.

@steipete
steipete merged commit 0afd8f0 into main Sep 5, 2026
295 of 297 checks passed
@steipete
steipete deleted the codex/deslop-cli-registration branch September 5, 2026 18:27
@steipete

steipete commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

Merged via squash.

vincentkoc added a commit that referenced this pull request Sep 5, 2026
@vincentkoc
chore: merge current main into fs-safe repair
37d889a 
* commit '911d7d5b0c107f7274f2415ca9655dcc6105f0e8': (867 commits)
  fix(skills): deliver pending Workshop approval controls (#139248)
  fix(gateway): deliver loopback ask_user prompts to the originating channel (#138831)
  docs: keep root agent instructions focused on repository policy (#139258)
  fix(matrix): exclude archived state roots from scans and selection (#112329)
  perf(agents): prepare exact tool names once per catalog batch (#139267)
  fix: preserve Swarm progress and simplify owner plumbing (#139269)
  fix(tailscale): release claims after gateway process-group exit (#139261)
  refactor(cli): simplify lazy command registration (#139108)
  test(ui): retain synthetic widget failure evidence (#139229)
  fix(buzz): keep implicit replies at thread root (#138879)
  test(diagnostics): await graceful Gateway shutdown before cleanup (#139247)
  fix(ci): unblock releases with different tooling package managers (#139259)
  feat(ui): add social preview links for private sessions (#139250)
  fix(codex): retain failed conversations for guarded continuation (#139246)
  chore(ui): refresh control ui locales (#139200)
  fix(ci): run browser checks for real-gateway inputs (#139256)
  fix(llama-cpp): reconcile managed router state before requests (#136363)
  chore(i18n): refresh native locales (#139237)
  refactor(worker): unify inference payload handling (#139245)
  refactor(terminal): replay wrapped prefixes in one pass (#139240)
  ...
@github-actions github-actions Bot mentioned this pull request Sep 5, 2026
Open
github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Sep 6, 2026
@steipete
refactor(cli): simplify lazy command registration (openclaw#139108)
ced8153
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

cli CLI command changes maintainer Maintainer-authored PR P3 Low-priority cleanup, docs, polish, ergonomics, or speculative work. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. size: L status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@steipete