feat(m12): Extension Runtime — registry, loader, CLI, and built-in manifests#260
Merged
flyingrobots merged 9 commits intomainfrom Feb 20, 2026
Merged
feat(m12): Extension Runtime — registry, loader, CLI, and built-in manifests#260flyingrobots merged 9 commits intomainfrom
flyingrobots merged 9 commits intomainfrom
Conversation
ajv is needed at runtime by the extension loader to validate manifests against docs/contracts/extension-manifest.schema.json.
…#255) Declarative YAML manifests for the two core built-in domains. These describe the domain vocabulary, views, and lenses that were previously hardcoded only in src/views.js.
Extension runtime: registry, loader, validator, and reset mechanism. Mirrors the lens.js pattern (captureBuiltIns/resetExtensions). AJV validates manifests at load time using the existing extension-manifest schema. Built-in extensions (roadmap, architecture) load from extensions/.
#257) 35 tests across two files: - extension.test.js: loadExtension, registerExtension, validateExtension, registry ops, resetExtensions/captureBuiltIns (20 tests) - extension.integration.test.js: built-in manifests pass schema validation, content checks, registration, registerBuiltinExtensions (15 tests)
Adds three subcommands: - extension list [--json]: show all registered extensions - extension validate <manifest> [--json]: validate without registering - extension add <manifest> [--json]: load and register an extension Registers built-in extensions at CLI startup so roadmap + architecture show up immediately in extension list.
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds an extension runtime and registry with AJV-backed manifest loading/validation, two built-in extension manifests (architecture, roadmap), CLI subcommands to manage extensions (list, validate, add) and startup registration of built-ins, plus unit and integration tests and new public exports for extension operations. Changes
Sequence DiagramsequenceDiagram
participant User as User/CLI
participant CLI as CLI Handler
participant FS as File System
participant Runtime as Extension Runtime
participant Validator as AJV Validator
participant Registry as Extension Registry
participant Views as View System
User->>CLI: git-mind extension add /path/manifest.yaml
CLI->>Runtime: extensionAdd(manifestPath, opts)
Runtime->>FS: read manifest file
FS-->>Runtime: YAML/JSON content
Runtime->>Runtime: parse & normalize manifest
Runtime->>Validator: validate(record)
Validator-->>Runtime: valid/errors
alt Validation fails
Runtime-->>CLI: return validation errors
CLI-->>User: error output
else Validation passes
Runtime->>Registry: registerExtension(record, { builtin: false })
Registry-->>Runtime: stored
Runtime->>Views: declareView(...) for non-builtin views
Views-->>Runtime: acknowledged
Runtime-->>CLI: success response
CLI-->>User: registered confirmation
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/git-mind.js`:
- Around line 13-14: registerBuiltinExtensions() is being invoked
unconditionally at startup which causes unnecessary file I/O and YAML/AJV work
on every CLI run; move its invocation so it only runs when needed (e.g., call
registerBuiltinExtensions() inside the 'extension' command branch in the main
switch/case or implement lazy initialization within views.js or lens.js that
triggers registerBuiltinExtensions() on first access). Update the CLI entry
(where registerBuiltinExtensions() is currently awaited) to remove the
unconditional call and instead ensure code paths that actually require
extensions (the case 'extension' handler or view/lens modules) import/await or
call a lazily-initializing wrapper around registerBuiltinExtensions() so startup
cost is avoided for unrelated commands.
- Around line 13-14: registerBuiltinExtensions() is being executed on every CLI
invocation before args are parsed, causing unnecessary file reads/validation;
move its invocation so it runs lazily only when needed (e.g., call
registerBuiltinExtensions() inside the command handling for extension-related
commands such as the 'extension' case or when an extension-provided view/lens is
first requested). Update the CLI dispatch logic that processes argv (the
switch/case that handles commands) to invoke registerBuiltinExtensions() only
for extension paths or on-demand, ensuring non-extension commands (e.g., init,
link, status) avoid the startup overhead.
- Around line 384-389: The validate/add branches incorrectly use args[2] as the
manifest path which picks up flags like "--json"; update both cases to extract
the positional manifest the same way importCmd does (e.g., use
args.slice(1).find(a => !a.startsWith('--')) or equivalent) and pass that result
into extensionValidate and extensionAdd instead of args[2], so the manifest path
ignores preceding flags and the guard for missing manifest still works (refer to
the 'validate' and 'add' case handlers and the extensionValidate/extensionAdd
call sites).
- Around line 384-389: The handler for the 'validate' and 'add' cases is
incorrectly using args[2] as the manifest path, which breaks when flags like
--json appear before the path; change both calls to locate the first non-flag
positional arg (same approach as the importCmd pattern) by using something like
const manifestPath = args.slice(1).find(a => !a.startsWith('--')); then pass
that manifestPath to extensionValidate and extensionAdd (and keep extFlags.json
as before) so flags like --json no longer get treated as the manifest path.
In `@extensions/architecture/extension.yaml`:
- Around line 6-7: The domain prefixes list includes spec, adr, and doc but the
existing view "architecture" only filters on crate, module, and pkg; update the
extension.yaml to either (a) add view entries in the "views" section for the
spec, adr, and doc prefixes so they are exposed in the architecture domain
(mirror the filter pattern used by the "architecture" view) or (b) add a short
inline comment beside domain.prefixes explaining that spec/adr/doc are
schema-only annotations and intentionally omitted from views; edit the
domain.prefixes and views definitions (referencing the "architecture" view and
the domain.prefixes array) accordingly.
In `@extensions/roadmap/extension.yaml`:
- Line 6: The configured domain prefixes include milestone and feature but no
view exposes them; update the extension so milestone and feature are reachable
by either adding them to an existing view (e.g., include "milestone" and/or
"feature" in the "roadmap" view definition) or create a new dedicated view that
lists those prefixes, and alternatively update documentation to note they are
domain-only if you intend them to be hidden; look for the "prefixes: [phase,
milestone, task, feature]" declaration and the "roadmap" and "backlog" view
definitions to make the change.
In `@src/cli/commands.js`:
- Line 817: The function extensionList is declared async but does not use await
(it only calls listExtensions() which is synchronous and console.log), so remove
the unnecessary async keyword from the extensionList function declaration to
avoid creating an extra microtask; alternatively, if you intentionally want to
keep the async signature for consistency with other command handlers, add an
ESLint directive (e.g., a single-line // eslint-disable-next-line require-await)
immediately above the extensionList declaration. Ensure you update the
declaration for extensionList and keep calls to listExtensions() unchanged.
In `@src/extension.js`:
- Around line 177-184: The code mutates record.builtin after loadExtension
returns (in registerBuiltinExtensions), which can fail if the record is
sealed/frozen and bypasses _normalize's builtin handling; instead avoid mutating
the returned object and ensure the builtin flag is applied at
normalization/registration time — e.g., in registerBuiltinExtensions call
registerExtension with a new object that merges record plus builtin: true (or
pass a builtin parameter into the existing _normalize/registerExtension flow),
so the builtin property is set without mutating the original record from
loadExtension.
- Around line 177-184: The code mutates the object returned by loadExtension
(record.builtin = true), which can fail if loadExtension begins returning
sealed/frozen objects; instead avoid mutation by creating a new record with the
builtin flag and passing that to registerExtension (e.g. construct a new object
via spread: { ...record, builtin: true }) or call the existing _normalize path
with the builtin parameter so registerExtension receives an explicit
builtin=true; update registerBuiltinExtensions to use the non-mutating newRecord
when calling registerExtension and ensure loadExtension/_normalize signatures
are used accordingly.
In `@src/index.js`:
- Around line 47-50: The export of internal lifecycle helpers captureBuiltIns
and resetExtensions makes them part of the public API; either remove them from
the named export list in src/index.js so only intended symbols (loadExtension,
registerExtension, listExtensions, getExtension, validateExtension,
registerBuiltinExtensions) are exported, or mark them as internal by renaming to
_captureBuiltIns and _resetExtensions and adding an `@internal` JSDoc to their
declarations (and update references in registerBuiltinExtensions accordingly) so
callers know they are not stable.
In `@test/extension.integration.test.js`:
- Around line 128-148: Add assertions that built-in views are registered after
calling registerBuiltinExtensions: after invoking registerBuiltinExtensions() in
the existing tests, call listViews() and assert that 'roadmap', 'backlog' and
'architecture' are present (e.g., verify view names or getView results). Update
the tests in the registerBuiltinExtensions suite to include these listViews()
checks so the invariant implied by registerExtension (skipping declareView when
builtin: true) is explicitly verified.
In `@test/extension.test.js`:
- Around line 230-233: The test titled "listExtensions returns empty array when
nothing registered" only checks that listExtensions() returns an array but
doesn't assert it's empty; update the test to assert emptiness by adding a
length check (e.g., expect(listExtensions()).toHaveLength(0)) so the behavior
matches the description, referencing the existing test that calls
listExtensions() (and relies on resetExtensions()/afterEach cleanup).
- Around line 250-268: captureBuiltIns mutates the module-level builtInDefs so
tests become order-sensitive; fix by ensuring tests clear that captured state
after each run: either export a resetBuiltIns() from src/extension.js that
clears builtInDefs and call it from the test suite's afterEach
(alongside/resetExtensions()), or call vi.resetModules() and re-import the
extension module in afterEach to force a fresh module state; modify the test
file to invoke the chosen approach (resetBuiltIns() or
vi.resetModules()+re-import) so captureBuiltIns no longer persists across tests.
- Fix args[2] manifest path bug when --json precedes path in validate/add - Fix captureBuiltIns test isolation (export _resetBuiltInsForTest) - Lazy registerBuiltinExtensions — only on extension commands - Use spread instead of mutation in registerBuiltinExtensions - Remove captureBuiltIns from public API, mark @internal - Strengthen empty-array assertion in extension tests - Add view availability assertion to integration tests - Remove unnecessary async from extensionList - Add domain prefix comments to extension manifests
Owner
Author
|
@coderabbitai review please |
Contributor
Rate Limit Exceeded
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/extension.integration.test.js`:
- Around line 26-29: Add a call to _resetBuiltInsForTest() inside the existing
afterEach cleanup so builtin extension state is cleared between tests;
specifically, update the afterEach that currently calls resetExtensions() and
resetViews() to also invoke _resetBuiltInsForTest() to undo any
captureBuiltIns()/registerBuiltinExtensions() effects on builtInDefs and ensure
shuffle-safe isolation for the registerBuiltinExtensions tests.
Owner
Author
|
@coderabbitai review please 🥕 |
Contributor
|
This looks like a substantial addition with the extension runtime, built-in manifests, CLI commands, and comprehensive test coverage. Let me analyze the implementation. 🧠 Learnings used✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/extension.integration.test.js`:
- Around line 83-104: The "architecture manifest content" test suite is missing
an assertion that the manifest declares the architecture view; update the suite
that uses validateExtension(ARCHITECTURE_MANIFEST) / record to include an
expectation that record.views contains the 'architecture' view (mirror the
pattern used in the "roadmap manifest content" suite which asserts roadmap and
backlog views) so the content suites remain symmetric and new manifests follow
the same template.
- Around line 54-55: Replace the per-test manifest reload by moving the
validateExtension(ROADMAP_MANIFEST) call from the beforeEach block into a
beforeAll so the variable record is loaded once for the test suite; specifically
change the setup that currently uses beforeEach(async () => { record = (await
validateExtension(ROADMAP_MANIFEST)).record; }) to use beforeAll(async () => {
record = (await validateExtension(ROADMAP_MANIFEST)).record; }), leaving
afterEach (which resets extension/view registry state) intact and ensuring tests
still reference the same top-level record variable.
---
Duplicate comments:
In `@test/extension.integration.test.js`:
- Around line 27-31: The test teardown ordering is correct; keep the afterEach
cleanup as _resetBuiltInsForTest() followed by resetExtensions() and then
resetViews() to ensure built-in definitions are cleared before draining the
extensions registry and finally removing non-built-in views—no code changes
needed to _resetBuiltInsForTest, resetExtensions, or resetViews.
- Around line 130-158: The review approves the added tests as covering required
invariants; no code changes needed—keep the four tests that call
registerBuiltinExtensions(), then assert via listExtensions()/names,
getExtension(...).builtin, resetExtensions() followed by getExtension(...), and
listViews() assertions (views pre-registered by views.js) since they validate
loading, builtin flag, persistence across resetExtensions(), and view
availability; ensure these test helper functions (registerBuiltinExtensions,
listExtensions, getExtension, resetExtensions, listViews) remain
imported/accessible so the suite runs.
…assertion (#252) - Switch beforeEach → beforeAll for read-only manifest records (avoids redundant I/O per test) - Add symmetric 'declares architecture view' assertion matching the roadmap content suite pattern
Owner
Author
|
@coderabbitai review please 🥕 |
Contributor
|
This is a comprehensive implementation of the extension runtime system with built-in manifests, CLI commands, and thorough test coverage. Let me analyze all the changes. 🧠 Learnings used✅ Actions performedReview triggered.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ajvfrom devDependencies → dependencies (needed at runtime for manifest validation)extensions/roadmap/extension.yamlandextensions/architecture/extension.yaml— declarative YAML manifests for the two built-in domainssrc/extension.js— full extension runtime mirroring thelens.jsregistry pattern (loadExtension,registerExtension,validateExtension,resetExtensions,captureBuiltIns,registerBuiltinExtensions)git mind extension list|validate|addCLI subcommand; extension API exported fromsrc/index.js@git-stunts/git-warpfrom v10.3.2 to v11.3.3Problem Statement
ADR Compliance (Required)
Relevant ADR(s)
Compliance Declaration
Architecture Laws Checklist (Hard Gates)
Canonical Truth & Context
--at,--observer,--trust) or deterministically defaulted.Determinism & Provenance
Artifact Hygiene
Contracts & Compatibility
Extension/Effects Safety (if applicable)
Scope Control
Backward Compatibility
git mind extensionsubcommand added (additive only).extensioncommands only) rather than at startup. No change to existing command behavior.Test Plan (Required)
Unit
npx vitest run test/extension.test.js # 20 testsIntegration
npx vitest run test/extension.integration.test.js # 16 testsDeterminism
npx vitest run # 528 tests passing across 28 filesContract/Schema
Policy Gates
git commit # pre-commit hook runs mechanical gatesSecurity / Trust Impact
Performance Impact
extensionsubcommands.Observability / Debuggability
Operational Notes
Linked Issues / Milestones
Summary by CodeRabbit
New Features
Tests