feat(schema): add Git-backed remote schema sources#1444
Conversation
📝 WalkthroughWalkthroughAdds Git-backed remote schema sources with explicit synchronization, immutable lockfiles, integrity-verified caching, offline fail-closed resolution, remote-aware discovery, and ChangesRemote schema sources
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant Sync
participant Git
participant Cache
participant Lockfile
CLI->>Sync: Run schema sync
Sync->>Git: Fetch and validate bundle
Git-->>Sync: Commit and integrity
Sync->>Cache: Install verified bundle
Sync->>Lockfile: Write resolved metadata
Sync-->>CLI: Return sync result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/commands/schema.ts (1)
82-153: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAvoid index-based shadow computation in
getSchemaResolution.
getRemoteSchemaDircan throw before returning a path, socheckAllLocationscan report a non-existentremotelocation whilegetSchemaDirnever reaches it. The fallbackactivehas no matching entry,activeIndexis-1, and JS slices from0up, reporting every existing location afterprojectas shadows. Also, the fallback dropsrequestedRef,resolvedCommit,bundlePath, andintegrity, which the remote schema JSON spec requires. Use precedence-rank filtering for shadows and carryremotelock metadata through, or merge shadow computation withgetSchemaDirso the active path cannot diverge.🤖 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/commands/schema.ts` around lines 82 - 153, Update getSchemaResolution to avoid relying on activeIndex when the active path is absent from checkAllLocations; compute shadows by filtering existing locations to only lower-precedence entries than the resolved active source/path. Preserve the remote lock metadata (requestedRef, resolvedCommit, bundlePath, and integrity) in the fallback active result by carrying through the remote entry’s metadata, while keeping getSchemaDir resolution behavior unchanged.
🧹 Nitpick comments (11)
src/core/remote-schema/lockfile.ts (1)
16-47: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
.strict()is deprecated in Zod 4.Object schemas in Zod 4 still strip unknown keys by default, but
.passthrough(),.strict(), and.strip()are all deprecated in favor ofz.strictObject()/z.looseObject(). Functionally equivalent today, but worth migrating to avoid future removal.♻️ Optional migration to `z.strictObject()`
-const LockEntrySchema = z - .object({ +const LockEntrySchema = z + .strictObject({ git: z... - }) - .strict(); + });🤖 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/remote-schema/lockfile.ts` around lines 16 - 47, Replace the deprecated .strict() calls in LockEntrySchema and LockSchema with z.strictObject() while preserving all existing fields, validators, and strict unknown-key behavior.src/core/remote-schema/git.ts (1)
179-205: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse bulk blob extraction instead of one
git cat-file blobper file.The 1,000-file bundle limit makes this loop spawn up to 1,000 sequential Git subprocesses, each with its own process-spawn cost and timeout budget. Switch to
git cat-file --batch/--batch-commandto read all blobs through a single long-lived subprocess instead.🤖 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/remote-schema/git.ts` around lines 179 - 205, Replace the per-entry runGit cat-file calls in the blob extraction loop with a single git cat-file --batch or --batch-command subprocess, feeding all entry.objectId values and parsing each returned header/content record. Preserve totalBytes enforcement, blob-to-entry association, timeout handling, and subsequent destination writes while ensuring only one Git subprocess is spawned.src/core/artifact-graph/schema-directory.ts (1)
59-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate candidate expression.
Both branches recompute
path.join(templatesDir, ...normalizedTemplate.split('/')). Hoisting the split and the templates candidate reads better.♻️ Proposed fix
- const candidates = options.requireTemplatesDirectory - ? [path.join(templatesDir, ...normalizedTemplate.split('/'))] - : [ - path.join(templatesDir, ...normalizedTemplate.split('/')), - path.join(schemaDir, ...normalizedTemplate.split('/')), - ]; + const segments = normalizedTemplate.split('/'); + const candidates = [path.join(templatesDir, ...segments)]; + if (!options.requireTemplatesDirectory) { + candidates.push(path.join(schemaDir, ...segments)); + }🤖 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/artifact-graph/schema-directory.ts` around lines 59 - 64, Refactor the candidate construction to compute the normalized template path segments and the templates-directory candidate once before the requireTemplatesDirectory conditional. Reuse that candidate in both branches, while retaining the schema-directory candidate only when templates are not required.test/core/remote-schema/cache.test.ts (1)
51-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
inois not a reliable identity signal on Windows.
fs.Stats.inois0/unstable for directories on Windows, so this assertion degrades to0 === 0there and stops detecting a replaced cache dir. Writing a marker file intoinstalledbefore the second install and asserting it survives is portable.💚 Proposed fix
const installed = installRemoteSchemaCache(source, integrity, dataDir); - const before = fs.statSync(installed).ino; + const marker = path.join(installed, '.marker'); + fs.writeFileSync(marker, 'kept'); expect(installRemoteSchemaCache(source, integrity, dataDir)).toBe(installed); - expect(fs.statSync(installed).ino).toBe(before); + expect(fs.existsSync(marker)).toBe(true);Note this also changes the directory contents, so it must be written after the integrity check — which is the case here.
🤖 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 `@test/core/remote-schema/cache.test.ts` around lines 51 - 61, Replace the platform-dependent fs.statSync(installed).ino identity check in the “does not replace an existing valid cache entry” test with a portable persistence check: write a marker file inside installed after the initial install and assert it still exists after the second install. Keep the existing integrity computation and install calls unchanged.src/core/remote-schema/bundle.ts (2)
128-150: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueLength prefix can desynchronize from hashed content.
contentLengthis written from thestatSyncsize captured during traversal, while the hashed bytes come from a laterreadFileSync. If the file changes in between, the digest embeds a length that doesn't match the content it prefixes. Hashingcontent.lengthfrom the buffer you actually read keeps the framing self-consistent.🐛 Proposed fix
- const contentLength = Buffer.allocUnsafe(8); - contentLength.writeBigUInt64BE(BigInt(file.size)); hash.update(pathLength); hash.update(pathBytes); + const content = fs.readFileSync(file.absolutePath); + const contentLength = Buffer.allocUnsafe(8); + contentLength.writeBigUInt64BE(BigInt(content.length)); hash.update(contentLength); - hash.update(fs.readFileSync(file.absolutePath)); + hash.update(content);🤖 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/remote-schema/bundle.ts` around lines 128 - 150, Update the hashing loop in the bundle integrity calculation to read each file into a buffer before constructing its length prefix, and write contentLength from that buffer’s actual byte length instead of file.size. Continue hashing the path, length prefix, and same content buffer in the existing order.
116-126: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueEnforce file-count/byte limits during traversal, not after.
visit()fully enumerates the tree before the limits are checked, so a hostile or accidentally huge bundle is walked (and itsstatSynccalls performed) in its entirety before rejection. Contents are still read only after the check, so this is bounded-memory, but the walk itself is unbounded. Checking incrementally insidevisit()fails fast.♻️ Sketch
const size = fs.statSync(absolutePath).size; files.push({ relativePath, absolutePath, size }); + if (files.length > limits.maxFiles) { + throw new Error(`Remote schema bundle contains more than ${limits.maxFiles} files`); + } }🤖 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/remote-schema/bundle.ts` around lines 116 - 126, Move the maxFiles and maxBytes enforcement into the recursive visit() traversal so each discovered entry is counted and its size accumulated immediately, throwing as soon as either limit is exceeded. Preserve assertPortableBundleEntries and the existing post-traversal flow, but remove reliance on checking limits only after files has been fully enumerated.src/core/remote-schema/sync.ts (1)
33-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
status: []is a dead field typed as an empty tuple.The type literal
[]permits only the empty array, and the implementation always returns[], so consumers (including the JSON sync output) can never receive anything. Either drop it or give it a real element type now — widening a published field later is a breaking change for the JSON contract.🤖 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/remote-schema/sync.ts` around lines 33 - 39, Update the SyncRemoteSchemasResult interface by removing the dead status field or replacing its empty-tuple type with the intended concrete status element type, and align all returned values and JSON sync output with that decision. Preserve the existing result fields and ensure the published contract can represent actual status values if the field is retained.test/core/remote-schema/bundle.test.ts (1)
100-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest leaks a file outside the temp bundle dir and passes silently when symlinks are unavailable.
outsideis created inos.tmpdir()(the parent ofbundleDir) and never cleaned up, sinceafterEachonly removesbundleDir. Also, the barereturnin the catch makes the test green on platforms wheresymlinkSyncfails;ctx.skip()makes that visible.♻️ Proposed fix
- it('rejects symlinks without reading their targets', () => { - const outside = path.join(path.dirname(bundleDir), 'outside-schema-secret.txt'); - fs.writeFileSync(outside, 'do-not-read'); - const link = path.join(bundleDir, 'templates', 'linked.md'); - try { - fs.symlinkSync(outside, link, 'file'); - } catch { - return; - } - - expect(() => computeBundleIntegrity(bundleDir)).toThrow(/symbolic link/); - }); + it('rejects symlinks without reading their targets', (ctx) => { + const outsideDir = fs.mkdtempSync(path.join(os.tmpdir(), 'openspec-schema-outside-')); + const outside = path.join(outsideDir, 'outside-schema-secret.txt'); + try { + fs.writeFileSync(outside, 'do-not-read'); + const link = path.join(bundleDir, 'templates', 'linked.md'); + try { + fs.symlinkSync(outside, link, 'file'); + } catch { + ctx.skip(); + return; + } + + expect(() => computeBundleIntegrity(bundleDir)).toThrow(/symbolic link/); + } finally { + fs.rmSync(outsideDir, { recursive: true, force: true }); + } + });🤖 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 `@test/core/remote-schema/bundle.test.ts` around lines 100 - 111, Update the symlink test around computeBundleIntegrity to create the outside target within the test’s managed cleanup scope, or explicitly remove it during cleanup, so no file remains in the temporary parent directory. Replace the bare return in the symlinkSync catch with the test framework’s visible skip mechanism, such as ctx.skip(), so unsupported platforms report the test as skipped rather than passing silently.test/core/artifact-graph/schema-directory.test.ts (1)
43-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd nested/backslash template coverage and symlink rejection cases.
Two gaps:
- Every case uses a flat
proposal.md, so thenormalizedTemplate.split('/')→path.join(...)conversion inschema-directory.tsLines 59-64 — the part that is actually separator-sensitive — is never exercised. Anested/proposal.mdpositive case (and atemplates\proposal.mdrejection case) would cover it.- The symlink guards at
schema-directory.tsLines 26 and 68 have no coverage.💚 Suggested additions
+ it('resolves a nested template path across platform separators', () => { + fs.mkdirSync(path.join(schemaDir, 'templates', 'nested'), { recursive: true }); + fs.writeFileSync( + path.join(schemaDir, 'schema.yaml'), + schemaYaml('qeda-sdd', 'nested/proposal.md') + ); + fs.writeFileSync( + path.join(schemaDir, 'templates', 'nested', 'proposal.md'), + '# Proposal\n' + ); + + const result = validateSchemaDirectory(schemaDir, { + expectedName: 'qeda-sdd', + requireTemplatesDirectory: true, + }); + expect(result.templatePaths.proposal).toBe( + path.join(schemaDir, 'templates', 'nested', 'proposal.md') + ); + });And add to the
it.eachtable:+ ['backslash template separator', true, true, String.raw`nested\proposal.md`, /unsafe template path/],As per path instructions: "When touching path behavior, add coverage that would fail on Windows path separators".
🤖 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 `@test/core/artifact-graph/schema-directory.test.ts` around lines 43 - 62, Add nested template coverage and symlink rejection cases to the schema-directory tests. Extend the existing valid-case coverage with a nested template such as nested/proposal.md, add a rejection case using a backslash-separated templates path such as templates\proposal.md, and create symlink cases targeting both the schema file and template file so validateSchemaDirectory rejects each through its symlink guards.Source: Path instructions
test/core/project-config.test.ts (1)
486-519: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPollution assertion is vacuous.
pollutedis never a key in the fixture, so line 509 passes regardless. Assert against a property the fixture could actually have injected via__proto__.💚 Proposed fix
- expect(({} as Record<string, unknown>).polluted).toBeUndefined(); + const probe = {} as Record<string, unknown>; + expect(probe.git).toBeUndefined(); + expect(probe.ref).toBeUndefined(); + expect(probe.path).toBeUndefined();🤖 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 `@test/core/project-config.test.ts` around lines 486 - 519, Update the prototype-pollution assertion in the test “rejects prototype keys explicitly without mutating object prototypes” to check a property that the __proto__ fixture could actually inject, such as the configured git/ref/path data, rather than the absent polluted key. Keep the existing config and warning assertions unchanged.test/commands/schema-sync.test.ts (1)
38-46: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winVerify
XDG_DATA_HOMEreliably isolates the global schema cache across platforms.Based on learnings, this repo has an established, adapter-agnostic pattern for redirecting global writes during tests: "OPENSPEC_GLOBAL_ROOT should unconditionally redirect global writes to the specified directory for testing/CI, regardless of adapter support." This test instead sets
XDG_DATA_HOME. IfgetUserSchemasDir()/the global schema-cache directory doesn't honorXDG_DATA_HOMEconsistently on every CI platform (Windows/macOS), these tests could write into the real global cache instead oftempDir, risking flaky or polluted CI runs — especially relevant given tasks.md calls for Windows CI coverage of this feature.Can you confirm the schema-cache/global-data-dir resolution used here honors
XDG_DATA_HOMEon all supported platforms, or should this test also setOPENSPEC_GLOBAL_ROOTfor reliable isolation?🤖 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 `@test/commands/schema-sync.test.ts` around lines 38 - 46, Update the test setup in beforeEach to set OPENSPEC_GLOBAL_ROOT to the temporary directory using the repository’s established global-write isolation mechanism. Ensure schema-cache/global-data-dir resolution uses this override across platforms, while preserving the existing temporary working directory and environment setup.Source: Learnings
🤖 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/cli.md`:
- Around line 903-919: Update the “Example JSON success” output in docs/cli.md
to include the fields emitted by the schema sync action: add git and
requestedRef to each schemas entry and add the top-level lockfile field,
matching the structure shown in design.md Decision `#8`.
In `@src/commands/schema.ts`:
- Around line 181-204: Update validateSchema and the validateSchemaDirectory
flow to preserve and return every schema validation failure as a separate
ValidationIssue instead of converting only the first thrown error into a
schema.yaml issue. Ensure failures from duplicate artifact IDs, invalid
dependencies, cycles, and broken templates are all surfaced, retaining accurate
paths or context when available.
In `@src/core/remote-schema/cache.ts`:
- Around line 49-114: Add a Windows-compatible test covering
installRemoteSchemaCache replacing an existing stale cache directory: create a
cache entry for a different integrity, invoke installRemoteSchemaCache with the
intended source and integrity, then verify the new cache is valid and no
displaced directory remains. Place the test alongside the existing remote-schema
cache tests and ensure resources or file handles are closed before asserting
replacement behavior.
In `@src/core/remote-schema/git.ts`:
- Around line 38-72: Update the execFile invocation in runGit to force
non-interactive Git authentication by providing an environment that includes
GIT_TERMINAL_PROMPT=0, while preserving the existing process environment and
other execution options.
- Around line 147-166: Update the fetch flow around fetchTarget and
resolvedCommit so options.lockedCommit is not passed directly to git fetch as a
raw SHA. Resolve the locked commit through an advertised ref or another
reachable-history/clone path, then verify the resulting commit matches the
requested lockedCommit; preserve the existing requestedRef fetch behavior and
commit-SHA validation for normal refs.
---
Outside diff comments:
In `@src/commands/schema.ts`:
- Around line 82-153: Update getSchemaResolution to avoid relying on activeIndex
when the active path is absent from checkAllLocations; compute shadows by
filtering existing locations to only lower-precedence entries than the resolved
active source/path. Preserve the remote lock metadata (requestedRef,
resolvedCommit, bundlePath, and integrity) in the fallback active result by
carrying through the remote entry’s metadata, while keeping getSchemaDir
resolution behavior unchanged.
---
Nitpick comments:
In `@src/core/artifact-graph/schema-directory.ts`:
- Around line 59-64: Refactor the candidate construction to compute the
normalized template path segments and the templates-directory candidate once
before the requireTemplatesDirectory conditional. Reuse that candidate in both
branches, while retaining the schema-directory candidate only when templates are
not required.
In `@src/core/remote-schema/bundle.ts`:
- Around line 128-150: Update the hashing loop in the bundle integrity
calculation to read each file into a buffer before constructing its length
prefix, and write contentLength from that buffer’s actual byte length instead of
file.size. Continue hashing the path, length prefix, and same content buffer in
the existing order.
- Around line 116-126: Move the maxFiles and maxBytes enforcement into the
recursive visit() traversal so each discovered entry is counted and its size
accumulated immediately, throwing as soon as either limit is exceeded. Preserve
assertPortableBundleEntries and the existing post-traversal flow, but remove
reliance on checking limits only after files has been fully enumerated.
In `@src/core/remote-schema/git.ts`:
- Around line 179-205: Replace the per-entry runGit cat-file calls in the blob
extraction loop with a single git cat-file --batch or --batch-command
subprocess, feeding all entry.objectId values and parsing each returned
header/content record. Preserve totalBytes enforcement, blob-to-entry
association, timeout handling, and subsequent destination writes while ensuring
only one Git subprocess is spawned.
In `@src/core/remote-schema/lockfile.ts`:
- Around line 16-47: Replace the deprecated .strict() calls in LockEntrySchema
and LockSchema with z.strictObject() while preserving all existing fields,
validators, and strict unknown-key behavior.
In `@src/core/remote-schema/sync.ts`:
- Around line 33-39: Update the SyncRemoteSchemasResult interface by removing
the dead status field or replacing its empty-tuple type with the intended
concrete status element type, and align all returned values and JSON sync output
with that decision. Preserve the existing result fields and ensure the published
contract can represent actual status values if the field is retained.
In `@test/commands/schema-sync.test.ts`:
- Around line 38-46: Update the test setup in beforeEach to set
OPENSPEC_GLOBAL_ROOT to the temporary directory using the repository’s
established global-write isolation mechanism. Ensure
schema-cache/global-data-dir resolution uses this override across platforms,
while preserving the existing temporary working directory and environment setup.
In `@test/core/artifact-graph/schema-directory.test.ts`:
- Around line 43-62: Add nested template coverage and symlink rejection cases to
the schema-directory tests. Extend the existing valid-case coverage with a
nested template such as nested/proposal.md, add a rejection case using a
backslash-separated templates path such as templates\proposal.md, and create
symlink cases targeting both the schema file and template file so
validateSchemaDirectory rejects each through its symlink guards.
In `@test/core/project-config.test.ts`:
- Around line 486-519: Update the prototype-pollution assertion in the test
“rejects prototype keys explicitly without mutating object prototypes” to check
a property that the __proto__ fixture could actually inject, such as the
configured git/ref/path data, rather than the absent polluted key. Keep the
existing config and warning assertions unchanged.
In `@test/core/remote-schema/bundle.test.ts`:
- Around line 100-111: Update the symlink test around computeBundleIntegrity to
create the outside target within the test’s managed cleanup scope, or explicitly
remove it during cleanup, so no file remains in the temporary parent directory.
Replace the bare return in the symlinkSync catch with the test framework’s
visible skip mechanism, such as ctx.skip(), so unsupported platforms report the
test as skipped rather than passing silently.
In `@test/core/remote-schema/cache.test.ts`:
- Around line 51-61: Replace the platform-dependent fs.statSync(installed).ino
identity check in the “does not replace an existing valid cache entry” test with
a portable persistence check: write a marker file inside installed after the
initial install and assert it still exists after the second install. Keep the
existing integrity computation and install calls unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3239137f-b2dd-47e2-b797-300e37ab65f4
📒 Files selected for processing (34)
.changeset/remote-schema-sources.mddocs/cli.mddocs/customization.mdopenspec/changes/add-remote-schema-sources/.openspec.yamlopenspec/changes/add-remote-schema-sources/design.mdopenspec/changes/add-remote-schema-sources/proposal.mdopenspec/changes/add-remote-schema-sources/specs/config-loading/spec.mdopenspec/changes/add-remote-schema-sources/specs/remote-schema-sources/spec.mdopenspec/changes/add-remote-schema-sources/specs/schema-resolution/spec.mdopenspec/changes/add-remote-schema-sources/specs/schema-which-command/spec.mdopenspec/changes/add-remote-schema-sources/tasks.mdsrc/commands/schema.tssrc/commands/workflow/schemas.tssrc/core/artifact-graph/index.tssrc/core/artifact-graph/resolver.tssrc/core/artifact-graph/schema-directory.tssrc/core/completions/command-registry.tssrc/core/project-config.tssrc/core/remote-schema/bundle.tssrc/core/remote-schema/cache.tssrc/core/remote-schema/config.tssrc/core/remote-schema/git.tssrc/core/remote-schema/lockfile.tssrc/core/remote-schema/sync.tssrc/core/remote-schema/types.tstest/commands/schema-sync.test.tstest/core/artifact-graph/remote-resolver.test.tstest/core/artifact-graph/schema-directory.test.tstest/core/project-config.test.tstest/core/remote-schema/bundle.test.tstest/core/remote-schema/cache.test.tstest/core/remote-schema/git.test.tstest/core/remote-schema/lockfile.test.tstest/core/remote-schema/sync.test.ts
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 (1)
src/core/remote-schema/git.ts (1)
46-59: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winMake SSH remote-schema fetches fail non-interactively
GIT_TERMINAL_PROMPT: '0'only affects Git’s own prompts. SSH fetches can still require host-key verification or a key passphrase, which will hang untiltimeoutMsand report as a generic Git fetch failure. Preserve any existing Git SSH command but addBatchMode=yes, then add the host-key policy you want for automation.🤖 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/remote-schema/git.ts` around lines 46 - 59, Update the git execFile invocation to preserve any existing GIT_SSH_COMMAND while adding SSH BatchMode=yes and an explicit non-interactive host-key policy suitable for automation. Ensure remote-schema fetches cannot prompt for host-key verification or key passphrases, while keeping the existing timeout and Git environment behavior unchanged.
🧹 Nitpick comments (5)
src/core/artifact-graph/schema-directory.ts (1)
33-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSurface all issues, not just the first.
inspectSchemaDirectorynow collects multiple diagnostics, but the throwing wrapper discards all butissues[0]. Sync callers (src/core/remote-schema/sync.ts) would benefit from the full list in the error message.♻️ Proposed change
if (!inspection.schema || inspection.issues.length > 0) { - throw new Error(inspection.issues[0]?.message ?? 'Schema validation failed'); + throw new Error( + inspection.issues.map((issue) => issue.message).join('; ') || 'Schema validation failed' + ); }🤖 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/artifact-graph/schema-directory.ts` around lines 33 - 35, Update the throwing validation wrapper in inspectSchemaDirectory to include every diagnostic from inspection.issues in the thrown error message, while preserving the existing fallback message when no issues are available and the current invalid-schema condition.src/core/remote-schema/bundle.ts (1)
113-121: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCheck size before reading the whole file into memory.
fs.readFileSynccompletes before themaxBytescheck, so a single oversized file in a locally-supplied bundle dir is fully buffered before rejection. The Git path pre-checks sizes, but direct callers (e.g. cache verification) don't. AstatSyncpre-check keeps the guard cheap.♻️ Proposed guard
- const content = fs.readFileSync(absolutePath); - files.push({ relativePath, content }); - if (files.length > limits.maxFiles) { + if (files.length + 1 > limits.maxFiles) { throw new Error(`Remote schema bundle contains more than ${limits.maxFiles} files`); } - totalBytes += content.length; - if (totalBytes > limits.maxBytes) { + const size = fs.statSync(absolutePath).size; + if (totalBytes + size > limits.maxBytes) { throw new Error(`Remote schema bundle contains more than ${limits.maxBytes} bytes`); } + const content = fs.readFileSync(absolutePath); + files.push({ relativePath, content }); + totalBytes += content.length;🤖 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/remote-schema/bundle.ts` around lines 113 - 121, Update the bundle file-processing flow around fs.readFileSync to stat each file and validate its size against limits.maxBytes before reading it into memory. Account for the file’s size in totalBytes during this pre-check, while preserving the existing maxFiles validation, file collection, and oversized-bundle error behavior.test/core/remote-schema/git.test.ts (1)
122-142: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest may not actually exercise the
merge-base --is-ancestorrejection path.
git fetch origin mainonly transfers objects reachable frommain;otherCommit(created on the unfetchedotherbranch) likely never lands in the temp repo. That means the earlierrev-parse ${lockedCommit}^{commit}call probably fails with "unknown revision" beforemerge-base --is-ancestorever runs — the assertion on/locked commit verification/ipasses either way, so this test doesn't clearly prove the ancestor check itself rejects a fetched-but-non-ancestor commit.Consider also covering the case where the commit is present locally (e.g., fetch
othertoo, or mergeotherintomainthen resetmainback) but is not an ancestor of the resolved ref, to directly exercisemerge-base --is-ancestor.🤖 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 `@test/core/remote-schema/git.test.ts` around lines 122 - 142, Update the test case around fetchSchemaBundleFromGit so otherCommit is present in the temporary repository while remaining unreachable from requestedRef 'main'; fetch the other branch or otherwise retain its commit object without advancing main. Keep the lockedCommit value set to otherCommit and assert the rejection from the locked commit verification path, ensuring rev-parse succeeds and merge-base --is-ancestor performs the rejection.test/core/artifact-graph/schema-directory.test.ts (1)
84-120: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSymlink-skip test setup duplicates a pattern also used in
bundle.test.ts.The try/create-symlink → catch/cleanup+
ctx.skip()scaffolding here is functionally identical to the one intest/core/remote-schema/bundle.test.ts(lines 114-125). Worth extracting into a shared test helper (e.g.trySymlinkOrSkip(ctx, target, link)) to avoid drift between the two copies. See consolidated comment.🤖 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 `@test/core/artifact-graph/schema-directory.test.ts` around lines 84 - 120, Extract the duplicated symlink setup and skip handling from this test and the corresponding bundle test into a shared helper such as trySymlinkOrSkip(ctx, target, link). Update both tests to use the helper, preserving cleanup and ctx.skip() behavior when symlink creation is unavailable.test/core/remote-schema/bundle.test.ts (1)
100-132: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSolid coverage of traversal byte-limit and symlink rejection. Same symlink-skip scaffolding duplication noted in
schema-directory.test.ts; see consolidated comment for the DRY suggestion.🤖 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 `@test/core/remote-schema/bundle.test.ts` around lines 100 - 132, Deduplicate the repeated symlink setup and cleanup used by “enforces byte limits while traversing the bundle” and “rejects symlinks without reading their targets.” Extract the shared scaffolding into an existing or local test helper, then reuse it here and in schema-directory.test.ts while preserving each test’s distinct assertions and skip behavior.
🤖 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/remote-schema/git.ts`:
- Around line 46-59: Update the git execFile invocation to preserve any existing
GIT_SSH_COMMAND while adding SSH BatchMode=yes and an explicit non-interactive
host-key policy suitable for automation. Ensure remote-schema fetches cannot
prompt for host-key verification or key passphrases, while keeping the existing
timeout and Git environment behavior unchanged.
---
Nitpick comments:
In `@src/core/artifact-graph/schema-directory.ts`:
- Around line 33-35: Update the throwing validation wrapper in
inspectSchemaDirectory to include every diagnostic from inspection.issues in the
thrown error message, while preserving the existing fallback message when no
issues are available and the current invalid-schema condition.
In `@src/core/remote-schema/bundle.ts`:
- Around line 113-121: Update the bundle file-processing flow around
fs.readFileSync to stat each file and validate its size against limits.maxBytes
before reading it into memory. Account for the file’s size in totalBytes during
this pre-check, while preserving the existing maxFiles validation, file
collection, and oversized-bundle error behavior.
In `@test/core/artifact-graph/schema-directory.test.ts`:
- Around line 84-120: Extract the duplicated symlink setup and skip handling
from this test and the corresponding bundle test into a shared helper such as
trySymlinkOrSkip(ctx, target, link). Update both tests to use the helper,
preserving cleanup and ctx.skip() behavior when symlink creation is unavailable.
In `@test/core/remote-schema/bundle.test.ts`:
- Around line 100-132: Deduplicate the repeated symlink setup and cleanup used
by “enforces byte limits while traversing the bundle” and “rejects symlinks
without reading their targets.” Extract the shared scaffolding into an existing
or local test helper, then reuse it here and in schema-directory.test.ts while
preserving each test’s distinct assertions and skip behavior.
In `@test/core/remote-schema/git.test.ts`:
- Around line 122-142: Update the test case around fetchSchemaBundleFromGit so
otherCommit is present in the temporary repository while remaining unreachable
from requestedRef 'main'; fetch the other branch or otherwise retain its commit
object without advancing main. Keep the lockedCommit value set to otherCommit
and assert the rejection from the locked commit verification path, ensuring
rev-parse succeeds and merge-base --is-ancestor performs the rejection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9e060cc2-af5b-40a4-b0e6-14e01071a418
📒 Files selected for processing (14)
docs/cli.mdsrc/commands/schema.tssrc/core/artifact-graph/schema-directory.tssrc/core/artifact-graph/schema.tssrc/core/remote-schema/bundle.tssrc/core/remote-schema/git.tssrc/core/remote-schema/lockfile.tssrc/core/remote-schema/sync.tstest/commands/schema-sync.test.tstest/core/artifact-graph/schema-directory.test.tstest/core/project-config.test.tstest/core/remote-schema/bundle.test.tstest/core/remote-schema/cache.test.tstest/core/remote-schema/git.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- test/core/project-config.test.ts
- src/core/remote-schema/sync.ts
- docs/cli.md
- src/core/remote-schema/lockfile.ts
1 participant
Summary
Add explicit Git-backed remote schema sources so teams can share one schema across repositories without copying bundles or allowing ordinary OpenSpec commands to access the network.
Projects keep the existing scalar schema selection and may declare a source separately:
openspec schema sync [name]resolves the requested branch, tag, or commit to an immutable commit SHA, validates the selected schema bundle, installs it in a content-addressed local cache, and atomically updatesopenspec/schemas.lock.yaml.--lockedrestores or verifies the exact lock state without upgrading it.Relates to #1131.
Behavior and impact
schema: <name>and existing project, user, and package schemasschema syncin shell completion metadataSecurity and reproducibility
Documentation and release tracking
The configuration and lockfile representation remain experimental and can be adjusted based on maintainer feedback without changing the behavioral model.
Validation
pnpm lintpnpm exec tsc --noEmitpnpm run buildpnpm test— 120 test files, 2,328 tests passedpnpm exec openspec validate add-remote-schema-sources --strictgit diff --checkIntegration coverage uses temporary local Git repositories and does not depend on public network access.
Summary by CodeRabbit
--jsonoutput.