Repository navigation
feat(cli)!: split the compiled execution graph out of CLISchema and seal it - #101
Conversation
|
Important Review skippedAuto reviews are limited based on label configuration. 🏷️ Required labels (at least one) (1)
🚫 Excluded labels (none allowed) (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 Walkthroughfeat!: split compiled CLI state from public schemas
WalkthroughThe CLI now separates sealed public schemas from compiled execution state. Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLIBuilder
participant CompiledCLI
participant Planner
participant Dispatch
participant Executor
CLIBuilder->>CompiledCLI: retain compiled handlers and routes
CompiledCLI->>Planner: provide compiled command graph
Planner->>Dispatch: resolve command arguments
Dispatch->>Planner: return matched CompiledCommand
Planner->>Executor: pass handler, steps, and merged schema
Possibly related PRs
Suggested labels: Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 3 warnings)
✅ Passed checks (7 passed)
Comment |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
dreamcli | 434a334 | Commit Preview URL Branch Preview URL |
Aug 10 2026, 08:22 PM |
commit: |
This comment was marked as resolved.
This comment was marked as resolved.
coderabbitai
Bot
added
area: cli
This comment was marked as resolved.
This comment was marked as resolved.
kjanat
changed the title
CLISchema and seal it
9b5cc06 to
17c1554
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
17c1554 to
4b43249
Compare
coderabbitai
Bot
added
area: completion
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/core/schema/command.ts (1)
652-660: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winMark
buildCommandSchema()as internal.
buildCommandSchema()is a module-private API. Its JSDoc must include@internalso generated documentation does not present it as supported API.Proposed documentation change
* `@throws` {`@link` CLIError} `INVALID_BUILDER_STATE` or `DUPLICATE_STDIN_ARG` when * an arg breaks the ordering or stdin invariants. + * `@internal` */As per coding guidelines,
src/**/*.tsmust use@internalto mark non-public APIs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/schema/command.ts` around lines 652 - 660, Update the JSDoc for buildCommandSchema() to include the `@internal` tag, marking this module-private API as unsupported for generated documentation while preserving the existing documentation.Source: Coding guidelines
src/core/schema/schema-sealing.test.ts (1)
248-257: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRebuild the schema in this regression test.
The test name states that it verifies rebuilding and deep equality. It only checks the initial schema construction. A regression in
createCommandSchema(built)can alter or remove nestedinteractiveresolvers while this test still passes.Proposed test change
const built = createCommandSchema({ name: 'deploy', interactive: resolver, commands: [{ name: 'status', interactive: resolver }], }); + const rebuilt = createCommandSchema(built); - expect(built.interactive).toBe(resolver); - expect(built.commands[0]?.interactive).toBe(resolver); + expect(rebuilt).toEqual(built); + expect(rebuilt.interactive).toBe(resolver); + expect(rebuilt.commands[0]?.interactive).toBe(resolver);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/schema/schema-sealing.test.ts` around lines 248 - 257, Update the regression test around createCommandSchema to rebuild the initially created schema by passing built back into createCommandSchema, then assert the rebuilt schema retains deep equality with built and preserves the top-level and nested interactive resolver references. Keep the existing nested-command setup and resolver identity checks focused on the rebuilt result.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/core/schema/command.ts`:
- Around line 652-660: Update the JSDoc for buildCommandSchema() to include the
`@internal` tag, marking this module-private API as unsupported for generated
documentation while preserving the existing documentation.
In `@src/core/schema/schema-sealing.test.ts`:
- Around line 248-257: Update the regression test around createCommandSchema to
rebuild the initially created schema by passing built back into
createCommandSchema, then assert the rebuilt schema retains deep equality with
built and preserves the top-level and nested interactive resolver references.
Keep the existing nested-command setup and resolver identity checks focused on
the rebuilt result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: e5715ead-235f-4def-84c5-b998a16358f3
📒 Files selected for processing (3)
CHANGELOG.mdsrc/core/schema/command.tssrc/core/schema/schema-sealing.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: test (node latest, bun latest)
- GitHub Check: test (node 22.22.2, bun latest)
- GitHub Check: Workers Builds: dreamcli
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (5)
CHANGELOG.md
📄 CodeRabbit inference engine (Custom checks)
If a version bump is detected in CHANGELOG.md, the new section header MUST match the exact new version number in the format: '## [X.Y.Z] - YYYY-MM-DD'. If NO version bump is detected, changes MUST be added under the existing '## [Unreleased]' section with descriptive entries (e.g., Added, Changed, Fixed, Removed).
Files:
CHANGELOG.md
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use tabs, single quotes, semicolons, and LF line endings in TypeScript files.
Useimport typefor type-only imports.
Include.tsextensions in all relative imports.
Use explicit named re-exports; do not useexport *.
Do not add runtime dependencies.
Files:
src/core/schema/schema-sealing.test.tssrc/core/schema/command.ts
src/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.ts: Add@moduleJSDoc at the top of source files and use@internalto mark non-public APIs.
Keep the public API factory-first, using factories such ascli(),command(),flag.*(),createOutput(), andcreateAdapter().
Files:
src/core/schema/schema-sealing.test.tssrc/core/schema/command.ts
**/*.test.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/*.test.ts: Co-locate tests as*.test.ts; usedescribe()andit(), em dashes in suite titles, and// ===and// ---section markers.
Do not use lifecycle hooks, snapshots, or module mocks in tests.
Include a trailing\nin output assertions.
Files:
src/core/schema/schema-sealing.test.ts
src/core/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
src/core/**/*.ts: Keep core code runtime-agnostic; route host I/O throughRuntimeAdapter,WriteFn, orReadFn.
Do not useprocess.*or other runtime-specific APIs insrc/core/.
Files:
src/core/schema/schema-sealing.test.tssrc/core/schema/command.ts
🧠 Learnings (11)
📚 Learning: 2026-02-10T16:25:08.867Z
Learnt from: kjanat
Repo: kjanat/dreamcli PR: 1
File: AGENTS.md:12-12
Timestamp: 2026-02-10T16:25:08.867Z
Learning: In repository kjanat/dreamcli, prefer using the filename syntax (e.g., GOALS.md) when referencing files in Markdown documentation, and treat references that do not follow this style as non-issues. This pattern applies to all Markdown files across the project.
Applied to files:
CHANGELOG.md
📚 Learning: 2026-02-11T11:57:27.623Z
Learnt from: kjanat
Repo: kjanat/dreamcli PR: 3
File: src/core/cli/cli-completion-e2e.test.ts:9-16
Timestamp: 2026-02-11T11:57:27.623Z
Learning: Enforce explicit .ts extensions in all relative imports for TypeScript source files (enabled by allowImportingTsExtensions and noEmit in tsconfig.json). This applies to both static and dynamic imports. For each TypeScript file, update relative import paths to end with .ts (e.g., './utils/helper' -> './utils/helper.ts'). Ensure tooling (linting/eslint rules or a codemod) flags missing extensions and provides a quick fix. This guideline should cover all TypeScript files in the repo, not just a single test or module.
Applied to files:
src/core/schema/schema-sealing.test.tssrc/core/schema/command.ts
📚 Learning: 2026-02-12T15:52:24.166Z
Learnt from: kjanat
Repo: kjanat/dreamcli PR: 4
File: src/core/resolve/resolve-arg-env.test.ts:1-7
Timestamp: 2026-02-12T15:52:24.166Z
Learning: In test files, do not require the module JSDoc tag at the top. This rule applies to all test files, while non-test source files should still require the module tag. Use the pattern **/*.test.{ts,tsx} to cover all test TypeScript test files (adjust if your project uses only .ts tests).
Applied to files:
src/core/schema/schema-sealing.test.ts
📚 Learning: 2026-04-02T00:14:17.279Z
Learnt from: kjanat
Repo: kjanat/dreamcli PR: 7
File: src/core/cli/cli.test.ts:14-14
Timestamp: 2026-04-02T00:14:17.279Z
Learning: In kjanat/dreamcli test files (src/**/*.test.ts), do not flag differences in section separator style between `// ---` and `// ===`. This style is intentionally used by the author for all section levels (including top-level sections) in tests, so separator casing/choice should not be treated as a guideline violation in code review.
Applied to files:
src/core/schema/schema-sealing.test.ts
📚 Learning: 2026-04-06T11:25:57.444Z
Learnt from: kjanat
Repo: kjanat/dreamcli PR: 9
File: examples/spinner-progress.ts:2-12
Timestamp: 2026-04-06T11:25:57.444Z
Learning: In this repo, enforce that TypeScript source files under `src/` include the required `module` JSDoc tag (where the project’s lint/review rules expect it). Do not flag missing `module` tags in files under `examples/`, since those are documentation/example scripts (e.g., `examples/*.ts`).
Applied to files:
src/core/schema/schema-sealing.test.tssrc/core/schema/command.ts
📚 Learning: 2026-04-06T11:26:11.173Z
Learnt from: kjanat
Repo: kjanat/dreamcli PR: 9
File: examples/json-mode.ts:2-15
Timestamp: 2026-04-06T11:26:11.173Z
Learning: In this repository, `module` JSDoc tags are required for TypeScript source files under `src/` (e.g., `src/**/*.ts`). Runnable demo/example scripts under `examples/` are not part of this requirement and should not be flagged for missing `module`.
Applied to files:
src/core/schema/schema-sealing.test.tssrc/core/schema/command.ts
📚 Learning: 2026-04-06T11:28:55.224Z
Learnt from: kjanat
Repo: kjanat/dreamcli PR: 9
File: docs/.vitepress/theme/index.ts:1-16
Timestamp: 2026-04-06T11:28:55.224Z
Learning: In the kjanat/dreamcli repo, enforce the `module` JSDoc tag requirement only for TypeScript source files under `src/` (i.e., `src/**/*.ts`). Do not require `module` for files under `docs/`, including anything in `docs/.vitepress/**`, since those are part of the VitePress docs pipeline.
Applied to files:
src/core/schema/schema-sealing.test.tssrc/core/schema/command.ts
📚 Learning: 2026-04-06T11:26:04.030Z
Learnt from: kjanat
Repo: kjanat/dreamcli PR: 9
File: examples/middleware.ts:2-11
Timestamp: 2026-04-06T11:26:04.030Z
Learning: In the kjanat/dreamcli repository, require the `module` JSDoc tag only for library source files under `src/` (e.g., `src/**/*.ts`). Files outside `src/` such as runnable demo/example scripts under `examples/` are not considered source code for this rule and should not be required to include `module`.
Applied to files:
src/core/schema/schema-sealing.test.tssrc/core/schema/command.ts
📚 Learning: 2026-04-06T18:24:44.371Z
Learnt from: kjanat
Repo: kjanat/dreamcli PR: 9
File: docs/reference/api.md:8-8
Timestamp: 2026-04-06T18:24:44.371Z
Learning: In the kjanat/dreamcli repo, the explicit type annotation `Readonly<Record<string, string>>` applied to `const` object literals is an intentional documentation/intent pattern. Do not flag it as redundant or unnecessary (e.g., for being inferable) when reviewing TypeScript/TSX files; treat this explicitness as acceptable by design.
Applied to files:
src/core/schema/schema-sealing.test.tssrc/core/schema/command.ts
📚 Learning: 2026-04-06T18:40:01.263Z
Learnt from: kjanat
Repo: kjanat/dreamcli PR: 9
File: docs/.vitepress/vite-plugins/source-artifacts.ts:62-83
Timestamp: 2026-04-06T18:40:01.263Z
Learning: For this repository, Windows is explicitly not a supported/targeted platform. During code review, do not raise issues about Windows-specific path separators or normalization differences (e.g., use of `path.sep`, handling forward-slash vs backslash, or `normalize()`-related concerns). These checks should be treated as out-of-scope because the project is intended for Linux/macOS only.
Applied to files:
src/core/schema/schema-sealing.test.tssrc/core/schema/command.ts
📚 Learning: 2026-06-23T16:38:50.547Z
Learnt from: kjanat
Repo: kjanat/dreamcli PR: 37
File: src/core/output/output.test.ts:76-76
Timestamp: 2026-06-23T16:38:50.547Z
Learning: In this repository’s test files (`*.test.ts`), do not suggest or require em dashes (—) in `describe()` suite titles. When reviewing, treat the suite-title punctuation as a repo-specific convention: do not flag missing/absent em dashes or recommend adding them to `describe(...)` block names.
Applied to files:
src/core/schema/schema-sealing.test.ts
🔍 Remote MCP DeepWiki, GitHub Grep
Additional review context
-
The repository
kjanat/dreamcliremains unavailable through DeepWiki, so its implementation and documentation could not be independently verified. -
WeakMap-backed derived state is a demonstrated TypeScript pattern: Angular uses
WeakMapto associate cached/internal state with object identity without mutating the object. This supports reviewing whether everyCLIBuilder._from()path preserves the intended compiled-state association. -
Immutable descriptor implementations commonly freeze nested structures, not just the outer object. Prisma examples clone and freeze records, arrays, nested fields, and execution steps. Review whether
CLISchemasealing protects nested command/config structures against mutation. -
Commander validates aliases against both the command’s own name and existing sibling commands, throwing on collisions. This supports checking that
assertNoSiblingRouteConflictrejects every name/alias collision consistently and reports the conflicting route clearly.
🔇 Additional comments (3)
src/core/schema/schema-sealing.test.ts (1)
2-8: LGTM!Also applies to: 22-29, 83-104, 159-224, 289-310, 328-411
CHANGELOG.md (1)
17-36: LGTM!Also applies to: 40-60, 70-75
src/core/schema/command.ts (1)
529-529: LGTM!Also applies to: 662-692, 710-733, 743-784, 950-959, 978-979, 1049-1059, 1544-1578, 1711-1711
4b43249 to
a0b6886
Compare
kjanat
added
cr:skip
a0b6886 to
434a334
Compare
…eal it CompiledCommand carries schema (by reference), handler, steps, and subcommands; CompiledCLI holds the command graph and plugins in a module-private WeakMap keyed by builder instance. CLISchema becomes a descriptive tree (commands and defaultCommand are CommandSchema, plugins gone) sealed with the cli brand alongside ConfigSettings; CLIDefinition, ConfigSettingsDefinition, and createCLISchema complete the ladder. CLIBuilder is factory-only: private constructor plus an internal _from that seeds the compiled state. ErasedCommand and its _command/_execute duality are deleted; executeCommand takes the executable handler/steps pair and planner, dispatch, and preflight receive schema and compiled graph as separate inputs. The compiled node's schema identity with the public tree is pinned by tests, including across builder rebuilds.
434a334 to
ee281c9
Compare
Validate routes created from plain CLI definitions, preserve default commands in input schemas, and keep compiled state across every builder rebuild path. Export the missing public configuration types and strip internal declarations from published types.
1 participant
Breaking, targets v4. Stacked on #100. Implements L3 of the approved v4 plan — the big one.
Compiled graph, minimal and private
CompiledCommand { schema, handler, steps, subcommands }/CompiledCLI { commands, defaultCommand, plugins }in internalcli/compiled.ts;compileCommand(builder)extracts once, retains no builder. Compiled state lives in a module-privateWeakMap<CLIBuilder, CompiledCLI>; nothing reaches any d.mts on the package entry.CLIBuilderis factory-only:private constructor+@internal static _from. All 14 rebuild sites plus the preflight rebuild carry compiled state (each traced in review — a missed site would silently drop handlers).CompiledCommand.schemais the same object as the public schema-tree entry) is pinned by tests at root, default, nested level, and across builder rebuilds — review added the WeakMap-held-state assertions after finding the original tests only covered freshly-compiled nodes.CLISchema descriptive + sealed
commands: readonly CommandSchema[],defaultCommand: CommandSchema | undefined,pluginsgone; sealed with the'cli'brand;ConfigSettingssealed as'config'with theConfigSettingsDefinitioninput rung;CLIDefinition+createCLISchemacomplete the ladder (one sanctioned cast, insealCLISchemaso builder-internal config rebuilds don't re-normalize the command tree and break identity).ErasedCommandand the_command/_executeduality deleted;executeCommandtakes{ handler, steps }; planner/dispatch/preflight take{ schema, compiled }as separate inputs (a mid-flighttoExecutableViewreconstitution of the mixed shape was caught and removed — it rebuilt exactly what this layer exists to split).plugins, so nothing needed threading.Behavioral note:
cli('')now throwsINVALID_SCHEMA(previously accepted). No call site existed; recorded in the changelog.Fixture migration: ~160 wrapper call sites unwrapped; planner/dispatch fixtures build real
command()trees +compileCommand; assertion-line diffs verified zero-delta across all four rewritten fixture files (identity assertions kept — planner/dispatch never execute, so recording actions would have been dead code).Gates: typecheck exit 0, lint clean, fmt clean, 2981/2981 tests (93 files), build (attw+publint) clean, docs build clean, meta-descriptions up to date.