feat: post-M12 extension polish + git-warp v11.5.0#270
Conversation
Establishes that CAS-backed content-on-node is a git-warp substrate responsibility, aligning with Paper I's Atom(p) vertex attachment formalism. git-mind provides CLI/UX layer on top.
Replace plain console.log with formatExtensionList() using chalk for consistent terminal styling across all CLI commands.
Add builtInsLoaded flag to avoid redundant YAML file reads on repeated invocations. Reset flag in _resetBuiltInsForTest() for test isolation.
registerExtension() now checks incoming prefixes against all other registered extensions. Throws a descriptive error on overlap while still allowing idempotent re-registration of the same extension name.
Add milestone, progress, traceability, coverage, and onboarding views to their respective extension manifests so extensionList shows the full picture. Purely declarative — no runtime behavior change since builtin=true skips declareView().
Add removeExtension() to the extension runtime (throws on built-in or non-existent), export from public API, and wire as `git mind extension remove <name>` in the CLI.
Add 4 new CLI output schemas: extension-list, extension-validate, extension-add, and extension-remove. Valid samples added to the contract test harness.
537 tests passing — no breaking changes.
|
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. 📝 WalkthroughWalkthroughThis PR extends git-mind's extension system with removal capability, adds architectural documentation about content attachments, introduces new views for roadmap and architecture extensions, defines JSON schemas for all extension CLI commands, and upgrades the git-warp dependency. It also centralizes extension list formatting and updates tests accordingly. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
extensions/roadmap/extension.yaml (1)
6-8:⚠️ Potential issue | 🟡 MinorStale comment contradicts the views added immediately below.
The comment says
milestoneandprogressare "surfaced via dedicated built-in views … rather than here," but lines 19–24 add exactly those views to this extension'sviewssection. The comment should be updated (or removed) to avoid confusing future maintainers.📝 Suggested update
domain: - # milestone and feature are domain-owned prefixes but surfaced - # via dedicated built-in views (milestone, progress) rather than here prefixes: [phase, milestone, task, feature]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/roadmap/extension.yaml` around lines 6 - 8, The inline comment above the prefixes array is stale and contradicts the extension's views: update or remove the comment so it no longer claims "milestone and progress are surfaced via dedicated built-in views rather than here"; instead either remove that sentence or change it to reflect that this extension's views section now includes "milestone" and "progress" (see the prefixes line prefixes: [phase, milestone, task, feature] and the views entries for milestone and progress) so future maintainers aren't confused.bin/git-mind.js (1)
90-99:⚠️ Potential issue | 🟡 MinorMissing
removesubcommand inprintUsage()help text.The usage block documents
list,validate, andadd(lines 91–96), but the newly addedremovesubcommand is absent. The error fallback on line 399 correctly references<list|validate|add|remove>, creating an inconsistency with the help output.Proposed fix
Add the
removeentry after theaddblock:add <manifest> Load and register an extension --json Output as JSON + remove <name> Remove a registered extension + --json Output as JSON Edge types: implements, augments, relates-to, blocks, belongs-to,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/git-mind.js` around lines 90 - 99, The usage/help block printed by printUsage (the multi-line string that lists extension subcommands) is missing the newly added "remove" subcommand; update that help text to include a line for "remove <manifest> Remove a registered extension" (matching the style of the existing "list", "validate", and "add" entries) so the printed usage is consistent with the error fallback that references "<list|validate|add|remove>" (see the usage string in bin/git-mind.js and the error branch that builds the allowed subcommands).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/adr/ADR-0004.md`:
- Line 3: Update the ADR status from "Proposed" to "Accepted" in ADR-0004.md to
reflect that the design has been implemented in this PR; locate the "Status:"
line in the ADR (ADR-0004) and change the value to "Accepted" (or "Accepted —
pending upstream" if you want to note dependency) so the document matches the
shipped implementation.
- Line 6: The "Related" references in ADR-0004.md point to ADR-0002 and ADR-0003
which are unresolvable because the ADR README index uses placeholder headings
like "ADR-00XX" and "ADR-00XY"; fix by updating the ADR index (README) to use
the real ADR numbers or by changing the Related line in ADR-0004.md to link
directly to the actual ADR files (or their correct numbers) so the
cross-reference resolves; search for the README ADR index headings
"ADR-00XX"/"ADR-00XY" and either rename them to the real identifiers or update
ADR-0004.md’s Related entry to the true filenames/URLs for ADR-0002 and
ADR-0003.
In `@docs/contracts/cli/extension-list.schema.json`:
- Around line 25-36: The nested view item schema (the "items" object defining
properties name, description, prefixes, edgeTypes, requireBothEndpoints) is
missing "additionalProperties": false; update that object (the one with "type":
"object" and "required": ["name"]) to include "additionalProperties": false so
extraneous fields are rejected and match the strictness of the parent extension
item schema.
In `@docs/contracts/cli/extension-validate.schema.json`:
- Around line 17-23: The nested "record" schema currently allows null or object
but lacks constraints: add "additionalProperties": false to the "record" schema
and add "required": ["name", "version"] so that when "record" is an object it
must include both name and version and disallow unknown fields; keep the
existing "type": ["object","null"] to preserve nullability and apply these
constraints within the same "record" definition.
In `@src/cli/format.js`:
- Around line 506-524: The output from formatExtensionList currently
concatenates extension blocks without separators; update formatExtensionList to
insert a blank line between each extension block (e.g., build each extension's
lines as a single block and join blocks with '\n\n' or push an empty string
between blocks) so multi-extension output is readable, and add an empty-state
guard that returns a clear dimmed message (or empty string) when extensions is
empty; reference formatExtensionList and the lines/ext variables to locate where
to group per-extension output and modify the final join.
In `@src/extension.js`:
- Around line 104-116: The prefix-collision check uses incoming =
record.domain?.prefixes ?? [] but reads existing.domain.prefixes directly;
update the overlap calculation to defensively read existing prefixes via
(existing.domain?.prefixes ?? []) so it won't throw if domain or prefixes is
missing—modify the overlap line that currently uses existing.domain.prefixes to
use the optional chained/default form (referencing variables: incoming, record,
registry, existing, existingName) and keep the idempotent re-register guard
unchanged.
In `@test/extension.test.js`:
- Around line 216-221: Update the test "built-in roadmap + architecture have no
prefix collisions" to explicitly assert there are no duplicate prefixes instead
of relying on registerBuiltinExtensions to throw: after calling await
registerBuiltinExtensions() and const exts = listExtensions(), extract each
extension's prefix (e.g., ext.prefix or ext.id depending on how prefixes are
stored) and assert that new Set(prefixes).size === prefixes.length (or otherwise
detect duplicates and fail with a clear message). Reference the test name and
the functions registerBuiltinExtensions and listExtensions so the change is made
in that test block.
- Around line 322-325: The test currently hardcodes the built-in name 'roadmap'
which makes it brittle; update the test that calls registerBuiltinExtensions()
and removeExtension('roadmap') to instead determine a valid built-in name at
runtime (e.g., call registerBuiltinExtensions() and read the first entry from
the built-in registry or use a helper like getBuiltinExtensions() / the builtins
array) and then call removeExtension(builtInName) expecting the /cannot remove
built-in/i error; keep references to registerBuiltinExtensions and
removeExtension to locate the test and ensure the chosen name is definitely
registered as a built-in before asserting the thrown error.
- Around line 311-320: The test lacks verification of view cleanup after
removeExtension: after registering the extension (which calls declareView for
the "widgets" view) add an assertion after removeExtension('test-ext') to
explicitly document expected view behavior — either assert that
getView('widgets') (or the view registry query function used in tests) is
undefined if views should be unregistered (and if so, also update
removeExtension to call the view teardown/registry.delete for that view), or
assert that getView('widgets') is still defined and add a clarifying comment
that views persist after extension removal; reference VALID_YAML,
registerExtension, declareView, removeExtension and getView (or your test helper
that queries the view registry) when making the change.
---
Outside diff comments:
In `@bin/git-mind.js`:
- Around line 90-99: The usage/help block printed by printUsage (the multi-line
string that lists extension subcommands) is missing the newly added "remove"
subcommand; update that help text to include a line for "remove <manifest>
Remove a registered extension" (matching the style of the existing "list",
"validate", and "add" entries) so the printed usage is consistent with the error
fallback that references "<list|validate|add|remove>" (see the usage string in
bin/git-mind.js and the error branch that builds the allowed subcommands).
In `@extensions/roadmap/extension.yaml`:
- Around line 6-8: The inline comment above the prefixes array is stale and
contradicts the extension's views: update or remove the comment so it no longer
claims "milestone and progress are surfaced via dedicated built-in views rather
than here"; instead either remove that sentence or change it to reflect that
this extension's views section now includes "milestone" and "progress" (see the
prefixes line prefixes: [phase, milestone, task, feature] and the views entries
for milestone and progress) so future maintainers aren't confused.
| - **Status:** Proposed | ||
| - **Date:** 2026-02-20 | ||
| - **Deciders:** Git Mind maintainers | ||
| - **Related:** ADR-0002, ADR-0003, WARP Paper I (aion-paper-01), M13A VESSEL-CORE roadmap milestone |
There was a problem hiding this comment.
ADR-0002 and ADR-0003 references are unresolvable from the current index.
The README ADR index uses placeholder headings ADR-00XX and ADR-00XY — there are no entries numbered ADR-0002 or ADR-0003 that a reader can follow. Consider either linking directly to the ADR files or updating the README headings to use their real numbers so cross-references work.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/adr/ADR-0004.md` at line 6, The "Related" references in ADR-0004.md
point to ADR-0002 and ADR-0003 which are unresolvable because the ADR README
index uses placeholder headings like "ADR-00XX" and "ADR-00XY"; fix by updating
the ADR index (README) to use the real ADR numbers or by changing the Related
line in ADR-0004.md to link directly to the actual ADR files (or their correct
numbers) so the cross-reference resolves; search for the README ADR index
headings "ADR-00XX"/"ADR-00XY" and either rename them to the real identifiers or
update ADR-0004.md’s Related entry to the true filenames/URLs for ADR-0002 and
ADR-0003.
| "record": { | ||
| "type": ["object", "null"], | ||
| "properties": { | ||
| "name": { "type": "string" }, | ||
| "version": { "type": "string" } | ||
| } | ||
| } |
There was a problem hiding this comment.
record object is under-constrained — missing additionalProperties and required.
The top-level schema is strict ("additionalProperties": false) but the nested record object has neither additionalProperties: false nor a required array. This means:
- An empty
{}is a valid record (bothnameandversionare implicitly optional). - Unknown extra fields on a non-null record slip through undetected.
🛡️ Suggested tightening
"record": {
"type": ["object", "null"],
+ "required": ["name", "version"],
+ "additionalProperties": false,
"properties": {
"name": { "type": "string" },
"version": { "type": "string" }
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "record": { | |
| "type": ["object", "null"], | |
| "properties": { | |
| "name": { "type": "string" }, | |
| "version": { "type": "string" } | |
| } | |
| } | |
| "record": { | |
| "type": ["object", "null"], | |
| "required": ["name", "version"], | |
| "additionalProperties": false, | |
| "properties": { | |
| "name": { "type": "string" }, | |
| "version": { "type": "string" } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/contracts/cli/extension-validate.schema.json` around lines 17 - 23, The
nested "record" schema currently allows null or object but lacks constraints:
add "additionalProperties": false to the "record" schema and add "required":
["name", "version"] so that when "record" is an object it must include both name
and version and disallow unknown fields; keep the existing "type":
["object","null"] to preserve nullability and apply these constraints within the
same "record" definition.
There was a problem hiding this comment.
@coderabbitai Thanks for the thorough review! This one is intentionally left open.
The extensionValidate command emits the full ExtensionRecord as record (see src/cli/commands.js:853 — outputJson('extension-validate', result) passes through the raw validateExtension() return). The record object contains fields like manifestPath, domain, rules, syncAdapters, materializers, views (as full objects), etc.
Adding additionalProperties: false with only name and version would reject valid CLI output — the exact C1 bug we just fixed in extension-list. The schema is intentionally lenient on the record sub-object so it validates real output without requiring us to enumerate every ExtensionRecord field (which would create a maintenance coupling).
The top-level envelope remains strict (additionalProperties: false), which is the important contract boundary. 🎯
There was a problem hiding this comment.
@flyingrobots, you're absolutely right—thank you for the detailed explanation! I misunderstood the design intent. The lenient record schema makes perfect sense given that it reflects the full ExtensionRecord structure with all its fields (manifestPath, domain, rules, syncAdapters, materializers, views, etc.). Requiring additionalProperties: false with only name and version would indeed break validation of real CLI output.
I appreciate the reference to the C1 bug fix in extension-list—it's a helpful reminder of the maintenance coupling risk when enumerating every internal field in a contract schema. Keeping the top-level envelope strict while allowing the nested record flexibility is the right boundary. 🎯
✏️ Learnings added
Learnt from: flyingrobots
Repo: neuroglyph/git-mind PR: 270
File: docs/contracts/cli/extension-validate.schema.json:17-23
Timestamp: 2026-02-21T12:42:50.449Z
Learning: In `docs/contracts/cli/extension-validate.schema.json`, the nested `record` object intentionally omits `additionalProperties: false` and `required` constraints because it represents the full `ExtensionRecord` structure (which includes fields like `manifestPath`, `domain`, `rules`, `syncAdapters`, `materializers`, `views`, etc.). Making it strict would reject valid CLI output and create maintenance coupling. The top-level schema envelope remains strict (`additionalProperties: false`), which is the important contract boundary.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: flyingrobots
Repo: neuroglyph/git-mind PR: 217
File: src/cli/commands.js:308-317
Timestamp: 2026-02-14T08:13:54.898Z
Learning: In `src/cli/commands.js` and `docs/contracts/cli/at.schema.json`, the `recordedAt` field in the `at` command JSON output is intentionally optional (not in the required list) and nullable because older epoch markers created before the field existed may lack it. The field comes from a `Map.get()` on epoch node properties (src/epoch.js:75), and the schema correctly models backward compatibility by allowing it to be absent or null.
Learnt from: flyingrobots
Repo: neuroglyph/git-mind PR: 169
File: docs/templates/code-review-feedback.md:26-33
Timestamp: 2025-09-16T17:10:27.477Z
Learning: The user (flyingrobots) uses Obsidian to view/edit Markdown files and prefers to keep Obsidian-specific syntax like collapsible callout markers (e.g., `[!NOTE]-` for collapsible callouts) even if they're not GitHub-compatible.
Learnt from: CR
Repo: neuroglyph/git-mind PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-07T14:14:58.924Z
Learning: Conventional commits (type(scope): description) and reference issues; PRs must describe changes, link issues, include test plan, and update docs when applicable
test/extension.test.js
Outdated
| it('removes a custom extension', async () => { | ||
| const path = join(tempDir, 'extension.yaml'); | ||
| await writeFile(path, VALID_YAML); | ||
| const record = await loadExtension(path); | ||
| registerExtension(record); | ||
| expect(getExtension('test-ext')).toBeDefined(); | ||
| const removed = removeExtension('test-ext'); | ||
| expect(removed.name).toBe('test-ext'); | ||
| expect(getExtension('test-ext')).toBeUndefined(); | ||
| }); |
There was a problem hiding this comment.
Missing assertion: view cleanup behavior after removeExtension is untested.
VALID_YAML declares a widgets view; registerExtension calls declareView for it. After removeExtension('test-ext'), the widgets view still exists in the view registry because removeExtension (src/extension.js lines 160–170) only calls registry.delete(name) with no corresponding view teardown.
The test verifies registry removal but leaves the view-cleanup contract entirely unspecified. If removal is expected to unregister views, the implementation has a silent bug. If view persistence is intentional, a negative assertion should document it:
🔍 Suggested additions (choose one path)
Path A — views are cleaned up on remove (requires implementation fix too):
const removed = removeExtension('test-ext');
expect(removed.name).toBe('test-ext');
expect(getExtension('test-ext')).toBeUndefined();
+ expect(listViews()).not.toContain('widgets'); // views deregistered on removalPath B — views intentionally persist after remove:
const removed = removeExtension('test-ext');
expect(removed.name).toBe('test-ext');
expect(getExtension('test-ext')).toBeUndefined();
+ // Views declared by a removed extension are NOT automatically unregistered.
+ expect(listViews()).toContain('widgets');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/extension.test.js` around lines 311 - 320, The test lacks verification
of view cleanup after removeExtension: after registering the extension (which
calls declareView for the "widgets" view) add an assertion after
removeExtension('test-ext') to explicitly document expected view behavior —
either assert that getView('widgets') (or the view registry query function used
in tests) is undefined if views should be unregistered (and if so, also update
removeExtension to call the view teardown/registry.delete for that view), or
assert that getView('widgets') is still defined and add a clarifying comment
that views persist after extension removal; reference VALID_YAML,
registerExtension, declareView, removeExtension and getView (or your test helper
that queries the view registry) when making the change.
…p text (#270) - C1: extensionList --json now cherry-picks only schema-declared fields (name, version, description, builtin, views, lenses), matching the extension-list.schema.json contract with additionalProperties: false - M1: add `remove` subcommand to extension help text in printUsage() - M2: add @throws JSDoc for prefix collision error in registerExtension() - L2: use Set for O(1) prefix collision lookup in registerExtension()
- ADR-0004: status Proposed → Accepted (ships with implementation) - ADR README: fix placeholder headings (ADR-00XX/00XY → ADR-0002/0003) - ADR README: sync ADR-0004 status to Accepted - extension-list schema: add additionalProperties: false to view items - roadmap extension.yaml: remove stale comment contradicting views section - formatExtensionList: add blank-line separators + empty-state guard - registerExtension: defensive optional chaining on existing.domain?.prefixes - Tests: explicit prefix uniqueness assertion, view persistence docs, dynamic built-in name lookup in removal test
|
@coderabbitai review please 🥕 |
|
This looks like a comprehensive polish pass with well-structured atomic commits, good test coverage (537 tests!), and thoughtful deferral decisions documented in the ROADMAP. I'll take a closer look at the extension system enhancements, the new ADR, JSON schemas, and the git-warp upgrade. 🧠 Learnings used✅ Actions performedReview triggered.
|
Update ADR-00XX → ADR-0002 and ADR-00XY → ADR-0003 to match the actual ADR index headings fixed in the same PR.
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 `@bin/git-mind.js`:
- Around line 394-401: The call to the synchronous helper extensionList is
incorrectly invoked with await (while extensionRemove is called synchronously);
remove the unnecessary await when calling extensionList so both
extensionList(...) and extensionRemove(...) are invoked consistently and
synchronously (update the call site that currently uses await extensionList(...)
to just extensionList(...) and keep passing the same arguments such as cwd and {
json: extFlags.json ?? false }).
---
Duplicate comments:
In `@src/extension.js`:
- Around line 108-111: The loop over registry reads existing.domain.prefixes
without optional chaining, which can throw if domain is undefined; change the
creation of existingPrefixes to use optional chaining/default (e.g., build the
Set from existing.domain?.prefixes || []) so it mirrors the guard used for
incoming and avoids latent errors in the registry iteration where
existing.domain may be absent.
|
@coderabbitai review please 🥕 |
|
This is a substantial polish pass with six implemented issues, thoughtful deferrals, and excellent test coverage. I'll examine the extension system enhancements, new ADR-0004, JSON schemas, and the git-warp upgrade. 🧠 Learnings used✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/adr/README.md`:
- Around line 91-104: Add explicit relative markdown links for ADR files in the
ADR index so readers can click through (e.g., replace plain titles with link
text like ADR-0002 pointing to ./ADR-0002.md); update the ADR index generation
in README.md (targets: the ADR listing area containing "ADR-0004 — Content
Attachments Belong in git-warp") to use linked references for each ADR entry
(ADR-0001, ADR-0002, etc.) ensuring anchors point to the correct filenames and
not header anchors.
In `@src/extension.js`:
- Around line 227-236: The current memoization in registerBuiltinExtensions uses
the boolean builtInsLoaded and sets it only after async work, so concurrent
callers can race and double-register; make it race-safe by replacing
builtInsLoaded with a stored Promise (e.g., builtInsLoadingPromise) that is set
immediately when registerBuiltinExtensions starts, awaited by callers, and
resolved after captureBuiltIns completes, update registerBuiltinExtensions to
set that Promise before the loop and await it for subsequent callers instead of
using the boolean, and update the test helper _resetBuiltInsForTest to
clear/reset the stored Promise so tests can reinitialize built-ins.
In `@test/extension.test.js`:
- Around line 216-222: Remove the semantically incorrect promise matcher and
simply await the registration call: replace the assertion await
expect(registerBuiltinExtensions()).resolves.not.toThrow() with a plain await
registerBuiltinExtensions(); so the test still fails on rejection and proceeds
to the existing uniqueness checks (listExtensions, exts, allPrefixes, and the
Set size assertion) without applying toThrow() to an undefined resolved value.
---
Duplicate comments:
In `@docs/contracts/cli/extension-list.schema.json`:
- Around line 1-46: No code changes required; the JSON Schema object for "views"
now correctly includes "additionalProperties": false and properly closes the
nested object — verify the schemaVersion/command/constants and that the "views"
item schema (properties: name, description, prefixes, edgeTypes,
requireBothEndpoints) and top-level "additionalProperties": false are present
and then approve the change.
In `@src/cli/format.js`:
- Around line 506-527: The reviewer approved the changes; no code fixes
required—keep the current implementation of formatExtensionList (function
formatExtensionList) as-is, which correctly handles empty-state via the early
return and uses blocks.join('\n\n') to separate extension blocks and display
ext.name, ext.version, ext.description, ext.views, and ext.lenses appropriately.
In `@src/extension.js`:
- Around line 105-118: No change required: the prefix collision check in
register logic already symmetrically guards both sides by using incoming =
record.domain?.prefixes ?? [] and creating existingPrefixes = new
Set(existing.domain?.prefixes ?? []), so leave the loop in register (the for
(const [existingName, existing] of registry) block that computes overlap and
throws the Error) as-is; no additional null/undefined chaining or checks are
necessary.
In `@test/extension.test.js`:
- Around line 325-330: Test is fine as-is; the dynamic lookup using
listExtensions().find(e => e.builtin)?.name plus the toBeDefined() guard
correctly ensures a built-in extension name is found before calling
removeExtension; no code changes are required to the it('throws when removing a
built-in extension') test referencing listExtensions and removeExtension.
| export async function registerBuiltinExtensions() { | ||
| if (builtInsLoaded) return; | ||
| for (const url of BUILTIN_MANIFESTS) { | ||
| const manifestPath = fileURLToPath(url); | ||
| const record = await loadExtension(manifestPath); | ||
| registerExtension({ ...record, builtin: true }); | ||
| } | ||
| captureBuiltIns(); | ||
| builtInsLoaded = true; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Memoization ordering is correct; consider hardening against concurrent callers.
Setting builtInsLoaded = true only after captureBuiltIns() is the right ordering. However, if registerBuiltinExtensions() is ever called concurrently (e.g., two test workers sharing the same module instance, or future parallelism), both calls will pass the guard and attempt to double-register the same built-ins, producing a prefix-collision error on the second registration.
For a CLI tool with sequential startup this is benign today, but the memoization pattern could be made race-proof with a stored Promise:
♻️ Optional: race-safe memoization
-let builtInsLoaded = false;
+/** `@type` {Promise<void> | null} */
+let builtInsPromise = null;
export async function registerBuiltinExtensions() {
- if (builtInsLoaded) return;
+ if (builtInsPromise) return builtInsPromise;
+ builtInsPromise = _doRegisterBuiltins();
+ return builtInsPromise;
+}
+
+async function _doRegisterBuiltins() {
for (const url of BUILTIN_MANIFESTS) {
const manifestPath = fileURLToPath(url);
const record = await loadExtension(manifestPath);
registerExtension({ ...record, builtin: true });
}
captureBuiltIns();
- builtInsLoaded = true;
}And reset in _resetBuiltInsForTest:
export function _resetBuiltInsForTest() {
builtInDefs.clear();
- builtInsLoaded = false;
+ builtInsPromise = null;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/extension.js` around lines 227 - 236, The current memoization in
registerBuiltinExtensions uses the boolean builtInsLoaded and sets it only after
async work, so concurrent callers can race and double-register; make it
race-safe by replacing builtInsLoaded with a stored Promise (e.g.,
builtInsLoadingPromise) that is set immediately when registerBuiltinExtensions
starts, awaited by callers, and resolved after captureBuiltIns completes, update
registerBuiltinExtensions to set that Promise before the loop and await it for
subsequent callers instead of using the boolean, and update the test helper
_resetBuiltInsForTest to clear/reset the stored Promise so tests can
reinitialize built-ins.
- Replace semantically incorrect .resolves.not.toThrow() with plain await (registerBuiltinExtensions returns void, toThrow on undefined is meaningless) - Add relative markdown links to ADR files in the index README
|
@coderabbitai review please 🥕 |
|
This is a well-structured PR with six implemented extension polish issues, thoughtful deferrals documented in the ROADMAP, and solid test coverage (537 tests). I'll examine the extension system enhancements, ADR-0004, JSON schemas, and the git-warp upgrade. 🧠 Learnings used✅ Actions performedReview triggered.
|
1 participant
Summary
git mind extension removesubcommand #263, [EXT] Detect and warn on prefix collisions between extensions #264, [EXT]extensionListoutput lacks chalk formatting (inconsistent with other commands) #265, [EXT]registerBuiltinExtensions()runs on every CLI invocation — consider lazy loading #266, [EXT] Extension manifestdomain.prefixesis undeclared for the built-in imperative views #268)--extension <path>flag: load an extension for a single command invocation #269)@git-stunts/git-warpupgraded from v11.3.3 to v11.5.0Implemented
extension listoutputregisterBuiltinExtensions()— skip redundant YAML readsregisterExtension()git mind extension remove <name>subcommand +removeExtension()APIDeferred
--extension <path>flag: load an extension for a single command invocation #269 (--extensionflag) — workaround for [EXT] Extension registration is ephemeral — not persisted across invocations #261, cleaner after persistence existsProblem Statement
Post-M12 extension polish: the extension runtime shipped in M12 lacked CLI output formatting, schema contracts, prefix collision safety, memoization, and a removal workflow. This PR closes those gaps so the extension system is production-ready before H2.
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
extension removesubcommand (additive). New JSON schemas for extension CLI output (additive).extension list --jsonoutput now strips internal fields to match schema (fix, not break).extension listnow has visual separators between extensions.extension removeis new.Test Plan (Required)
Unit
npm test -- test/extension.test.jsIntegration
npm test -- test/extension.integration.test.js test/contracts.integration.test.jsDeterminism
npm test -- test/extension.test.jsContract/Schema
npm test -- test/contracts.test.js test/contracts.integration.test.jsPolicy Gates
npm testSecurity / Trust Impact
removeExtensionthrows on built-in or non-existent (documented)Performance Impact
Observability / Debuggability
Operational Notes
Linked Issues / Milestones
git mind extension removesubcommand #263, [EXT] Detect and warn on prefix collisions between extensions #264, [EXT]extensionListoutput lacks chalk formatting (inconsistent with other commands) #265, [EXT]registerBuiltinExtensions()runs on every CLI invocation — consider lazy loading #266, [EXT] Extension manifestdomain.prefixesis undeclared for the built-in imperative views #268Summary by CodeRabbit
New Features
Documentation