Repository navigation
feat(sdk): define* authoring builders for every extension registration surface - #342
Conversation
Complete the SDK author->register->test ergonomics tripod. The SDK already exposes defineExtension (module), assert* (verify), and run* (invoke) helpers, but had no authoring-time typed builder for the individual registration definitions package authors hand to api.register*. Add src/sdk/define.ts with 19 zero-cost identity builders: - object-def (generic <T extends Def>(x:T):T; preserves literals and still infers nested handler params): defineCommand, defineFlag, defineItemType, defineItemField, defineMigration, defineSearchProvider, defineVectorStoreAdapter - function-def (non-generic (fn:Fn):Fn so a bare .js arrow's parameter is contextually typed instead of falling back to any): defineCommandOverride, defineParserOverride, definePreflightOverride, defineServiceOverride, defineRendererOverride, defineImporter, defineExporter, defineBeforeCommandHook, defineAfterCommandHook, defineOnWriteHook, defineOnReadHook, defineOnIndexHook Value: in plain-JavaScript packages (the pm package init default) a bare object literal is unchecked; wrapping it in a builder restores contract checking + handler-context inference and lets definitions be colocated, exported, reused, and unit-tested apart from activate(). Validation stays in register*/loader + assert* (one source of truth), mirroring defineExtension and the ecosystem defineConfig/defineComponent pattern. Re-exported via the SDK barrel (export * from ./define.js); barrel identity locked in tests/unit/sdk/sdk-define.spec.ts. Documented in docs/SDK.md (new Authoring Builders section + Public Exports list). 100% line+branch coverage (4843 tests); tsc (src + tsconfig.packages.json) and build clean. Verified end-to-end in a temp dir: a real package authored entirely with define* from @unbrained/pm-cli/sdk activates, registers, and runs through @unbrained/pm-cli/sdk/testing. Closes pm-12tj (epic pm-ugqx); ADR rationale in pm-3mph (living record). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Lwzzhhu7sVjqEZAwMxN4Wo
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
@coderabbitai full review |
|
Warning Review limit reached
More reviews will be available in 41 minutes and 43 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?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 credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. 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, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a new ChangesSDK define* typed identity builders
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
|
To use Codex here, create a Codex account and connect to github. |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== PR Info ==="
gh pr view 342 --repo unbraind/pm-cli --json title,state,baseRefName,headRefName,files
echo ""
echo "=== Changed Files ==="
gh pr diff 342 --repo unbraind/pm-cliLength of output: 42968
Review:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces 19 zero-cost typed identity builders (define*) in src/sdk/define.ts for all extension registration surfaces, enabling type-checking and contextual parameter inference in plain-JavaScript packages. It also updates the SDK barrel exports, adds comprehensive documentation in docs/SDK.md, and includes unit tests in tests/unit/sdk/sdk-define.spec.ts. Feedback on the tests suggests importing and using runRegisteredHookForTest to actually invoke and assert the side-effects of the registered afterCommand hook, ensuring it is fully verified.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| defineVectorStoreAdapter as defineVectorStoreAdapterFromBarrel, | ||
| type ExtensionApi, | ||
| } from "../../../src/sdk/index.js"; | ||
| import { activateExtensionForTest, runRegisteredCommandForTest } from "../../../src/sdk/testing.js"; |
There was a problem hiding this comment.
Import runRegisteredHookForTest to allow exercising and asserting the registered afterCommand hook in the integration test below.
| import { activateExtensionForTest, runRegisteredCommandForTest } from "../../../src/sdk/testing.js"; | |
| import { activateExtensionForTest, runRegisteredCommandForTest, runRegisteredHookForTest } from "../../../src/sdk/testing.js"; |
| const run = await runRegisteredCommandForTest(activation.commands, { command: "demo run" }); | ||
| expect(run).toEqual({ handled: true, result: { ok: true, command: "demo run" }, warnings: [] }); | ||
| }); |
There was a problem hiding this comment.
The test registers an afterCommand hook and defines a local observed array to capture the executed command name, but never actually invokes the hook or asserts its side-effects. Use runRegisteredHookForTest to execute the hook and assert that observed contains the expected command name, ensuring the hook registration and builder are fully verified.
const run = await runRegisteredCommandForTest(activation.commands, { command: "demo run" });
expect(run).toEqual({ handled: true, result: { ok: true, command: "demo run" }, warnings: [] });
await runRegisteredHookForTest(activation.hooks, {
kind: "after_command",
context: { command: "demo run", args: [], pm_root: "", ok: true },
});
expect(observed).toEqual(["demo run"]);
});
There was a problem hiding this comment.
Code Review
This pull request introduces a complete set of 19 zero-cost typed identity builders (define*) in src/sdk/define.ts to improve authoring ergonomics for extension registration surfaces. These builders enable full type-checking and contextual parameter inference in plain-JavaScript packages. The changes also include re-exporting these builders from the SDK barrel, comprehensive unit tests verifying their identity behavior and integration with the activation harness, updated documentation in docs/SDK.md, and changelog updates. I have no feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@docs/SDK.md`:
- Around line 578-579: The Authoring Builders section heading in docs/SDK.md
lacks the required tracker references as per coding guidelines. Locate the "##
Authoring Builders" section heading and add tracker references near the top
following the pattern used in CHANGELOG.md. Include references to both pm-12tj
(the task reference) and pm-3mph (the ADR reference) either as an HTML comment
or as inline text at the start of the section, as suggested in the comment
details.
In `@tests/unit/sdk/sdk-define.spec.ts`:
- Around line 137-150: The test mutates the observed array inside the
defineAfterCommandHook callback but does not assert that this side effect
actually occurred, so the test could pass even if hook dispatch regresses. After
the runRegisteredCommandForTest call that executes the "demo run" command, add
an assertion to verify that the observed array contains the expected command
value to ensure the afterCommand hook was properly dispatched and executed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 982eeb8e-0be4-470e-9e94-3379ee4babd1
📒 Files selected for processing (9)
.agents/pm/decisions/pm-3mph.toon.agents/pm/features/pm-12tj.toon.agents/pm/history/pm-12tj.jsonl.agents/pm/history/pm-3mph.jsonlCHANGELOG.mddocs/SDK.mdsrc/sdk/define.tssrc/sdk/index.tstests/unit/sdk/sdk-define.spec.ts
Address review feedback on PR #342: - Both CodeRabbit and Gemini flagged that the activation e2e test mutated an `observed` array inside the defineAfterCommandHook callback but never asserted it, so the test could pass even if hook dispatch regressed. Fire the registered after_command hook through runRegisteredHookForTest and assert it recorded the command (observed === ["demo run"]) plus zero runner warnings. Confirms runRegisteredCommandForTest does not double-fire lifecycle hooks. - CodeRabbit flagged the new docs/SDK.md "Authoring Builders" section lacks the tracker reference AGENTS.md requires for pm-driven doc changes. Add a "Tracked: pm-12tj (ADR pm-3mph)" line under the heading. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Lwzzhhu7sVjqEZAwMxN4Wo
|
Addressed review feedback in 282e9b3:
Sourcery was rate-limited (no findings); Codex connector is not configured on this repo. @coderabbitai full review |
There was a problem hiding this comment.
Code Review
This pull request introduces a complete set of 19 zero-cost identity define* builders in src/sdk/define.ts to support authoring-time type-checking and contextual parameter inference across all extension registration surfaces (such as commands, flags, overrides, and hooks). These builders are re-exported via the SDK barrel, documented in docs/SDK.md, and backed by comprehensive unit tests in tests/unit/sdk/sdk-define.spec.ts along with corresponding ADR and feature records. There are no review comments, and I have no feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== PR Info ==="
gh pr view 342 --repo unbraind/pm-cli --json title,state,baseRefName,headRefName,commits
echo ""
echo "=== Changed Files ==="
gh pr diff 342 --repo unbraind/pm-cliLength of output: 46721
✅ Hook assertion — ✅ Docs tracker ref — Two minor doc nits from the first pass are still open in
Everything else is in order — implementation, barrel re-export, test coverage, and documentation are all solid. Approving as-is once you decide whether to fold those nits in. [approve] |
CodeRabbit round-2 doc-accuracy nits (approved with these as non-blocking):
- defineFlag JSDoc: use `api.registerFlags(...)` to match every other
builder's JSDoc.
- defineMigration JSDoc: `run` is optional, so word the inference as
conditional ("when `run` is provided") instead of unconditional.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Lwzzhhu7sVjqEZAwMxN4Wo
|
Folded in the two round-2 doc-accuracy nits in 1d5a9d1: @coderabbitai full review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== PR Info ==="
gh pr view 342 --repo unbraind/pm-cli --json title,state,baseRefName,headRefName,commits
echo ""
echo "=== Changed Files ==="
gh pr diff 342 --repo unbraind/pm-cliLength of output: 47623
All three rounds of feedback are fully resolved. The implementation, barrel re-export, test coverage (identity + barrel lock + activation e2e with the hook dispatch assertion), docs, ADR, and CHANGELOG are all in order. [approve] |
There was a problem hiding this comment.
Code Review
This pull request introduces a complete set of 19 zero-cost authoring-time typed builders (define*) in src/sdk/define.ts to cover every extension registration surface, enabling full type-checking and contextual parameter inference in both TypeScript and plain JavaScript packages. These builders are re-exported via the SDK barrel, fully documented in docs/SDK.md, and thoroughly tested in tests/unit/sdk/sdk-define.spec.ts. There are no review comments provided, and the implementation is clean, well-documented, and fully tested, so I have no additional feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Summary
Completes the SDK author → register → test ergonomics tripod. The SDK already shipped
defineExtension(the module), theassert*helpers (verify a registration), and therun*helpers (invoke a registration) — but there was no authoring-time typed builder for the individual registration definitions a package author hands toapi.register*. This PR adds the missing leg.New
src/sdk/define.tsexports 19 zero-cost identity builders covering everyExtensionApiregistration surface:<T extends Def>(x: T): T, preserve the narrow literal type and still infer nested handler params:defineCommand,defineFlag,defineItemType,defineItemField,defineMigration,defineSearchProvider,defineVectorStoreAdapter(fn: Fn): Fnso a bare arrow's parameter is contextually typed instead of falling back toany:defineCommandOverride,defineParserOverride,definePreflightOverride,defineServiceOverride,defineRendererOverride,defineImporter,defineExporter,defineBeforeCommandHook,defineAfterCommandHook,defineOnWriteHook,defineOnReadHook,defineOnIndexHookWhy
In plain-JavaScript packages — the default
pm package initscaffold — authors cannot write TypeScript annotations, so a bareconst cmd = { ... }is completely unchecked. Wrapping it asconst cmd = defineCommand({ ... })restores full contract checking and infers the nested handler'scontextparameter through the editor's TypeScript language service. It also lets a definition be colocated, exported, reused, and unit-tested apart fromactivate().Every builder is a pure identity function (returns its argument unchanged), exactly like
defineExtensionand the widerdefineConfig/defineComponentconvention — the value is entirely at the type level. Validation deliberately stays inregister*/loader and theassert*helpers (one source of truth). Design rationale recorded in ADRpm-3mph.Surface
export * from "./define.js"); barrel identity locked in a dedicated spec.docs/SDK.md(new Authoring Builders section + Public Exports list).Verification
tsc --noEmit(src andtsconfig.packages.json) clean;buildclean.changelog:pm:checkclean.define*(defineCommand+defineFlag+defineAfterCommandHook+defineSearchProvider) imported from@unbrained/pm-cli/sdkactivates via@unbrained/pm-cli/sdk/testing, registers its search provider, and runsgreet hello alice --loudreturning{greeting:'hi alice',loud:true}. Confirmeddist/sdk/define.d.tsdeclares all 19 builders and the barrel re-exports them for JS IntelliSense.pm tracking
🤖 Generated with Claude Code