feat(schema): add Git-backed remote schema sources - #1444
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Git-backed remote schema sources with explicit synchronization, immutable lockfiles, integrity-verified caching, offline fail-closed resolution, remote-aware discovery, schema-root propagation, 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: Return 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
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
The current head still aborts schema which --all on one unsynchronized remote, concurrent named syncs can lose lockfile updates, remote-strict path validation still changes legacy local schemas, and schema sync still assumes the current directory rather than the selected OpenSpec root. Please resolve the authority/root contract and add per-entry diagnostics, project-scoped interprocess locking, and the compatibility/concurrency regressions before merge.
Code review — verdict: do not merge as-is; needs a maintainer product decision firstLet me lead with the important part: this is good work. It's careful, well-tested, security-conscious, and the problem it solves is real. My objection is not to the quality — it's that the change is far larger in architectural scope than can be approved on engineering review alone, and the feature it introduces hasn't been agreed to yet. I reviewed it against four criteria. It passes three and fails the fourth decisively. 1. Real need — yes, but nobody has agreed to solve it this way#1131 is a legitimate report: teams copy a shared schema across repos and it drifts. You describe the same pain from your own multi-repo setup, and that's credible. But #1131 is titled "Feature Request / Discussion: Best practices for sharing a centralized team-schema", it's open, unlabeled, and has no maintainer response at all. The only replies are from community members, one of whom suggests an existing external answer (skillshare's
I think that's exactly right, and it's the reason I can't sign this off. The public contract here — a new config key, a new committed lockfile, a new cache location, a new command — hasn't been agreed to. Merging it is the agreement. 2. Does it work — as far as I can tell, yes
One item CodeRabbit raised that is not yet addressed: 3. Breaking changes — none that I could find, and I did lookThis was my biggest worry given the change to schema resolution order, so I verified it directly rather than reasoning about it:
So on the "must not break a user" bar specifically: it passes. 4. Scope — this is where it failsThe criterion is that a PR should be surgical and fix or enhance a small aspect, not expand a core design. This PR introduces:
That's not an enhancement to schemas; it's OpenSpec taking ownership of schema distribution as a product capability. Lockfiles in particular are a long-term commitment — once users commit There's also a prior question the issue raises and this PR answers by fiat: should OpenSpec own this at all? #1131 floats git submodules and CI-injection as workarounds, and a commenter points at skillshare's RecommendationPark this PR; don't close it. Concretely:
I'd also note the PR commits To be explicit, since I know this represents a lot of effort: I am not saying this is bad code or a bad idea. I'm saying it's a roadmap decision wearing a pull request's clothes, and it needs the maintainers to make that call before an engineering review means anything. |
|
Maintainer product call: please advance this as a native OpenSpec capability. External tooling is a workable stopgap, but OpenSpec owns schema semantics and resolution, and the repeated team use case needs deterministic, reviewable cross-repo distribution. Keep V1 narrow: complete Git-backed schema bundles, explicit sync, a committed immutable lock, and an integrity-checked offline cache; no runtime fetching, inheritance, registry, or general plugin packaging. For the public contract, the consumer repository owns The PR should remain blocked until the current changes-requested items are covered by regressions: per-entry unavailable diagnostics, project-scoped interprocess locking, isolation of remote-strict validation from legacy local schemas, and correct project-root resolution. Also preserve any existing |
|
Addressed the remaining review blockers in commit
Final verification after rebasing onto the updated PR branch:
|
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 (2)
src/commands/workflow/templates.ts (1)
41-45: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReport templates from remote schemas as
remote.A declared remote resolves to its cache directory, which is outside both project and user schema directories; the fallback therefore reports it as
packagein human and JSON output. Add a remote classification branch and extend the source type accordingly.🤖 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/workflow/templates.ts` around lines 41 - 45, Update the schema source classification used by the report templates around schemaDir and the resolved schema path so declared remote schemas are identified as remote when their cache directory falls outside project and user schema directories. Extend the source type and both human-readable and JSON output handling to support the new remote classification, while preserving existing package and local classifications.src/utils/change-utils.ts (1)
136-145: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep generated configuration at the consumer schema root.
When
schemaRoot !== projectRoot, this resolves the schema from the consumer but still scaffoldsprojectRoot/openspec/config.yaml. Thusopenspec new change --store ...can create a store-owned config, violating consumer-owned schema configuration. Keep planning directories underprojectRoot, but initialize config underschemaRoot(or skip initialization when they differ), and assert the store config is not created.Suggested direction
- const openspecDir = path.join(projectRoot, 'openspec'); + const planningOpenSpecDir = path.join(projectRoot, 'openspec'); + const configOpenSpecDir = path.join(schemaRoot, 'openspec'); - const configPath = path.join(openspecDir, 'config.yaml'); + await FileSystemUtils.createDirectory(configOpenSpecDir); + const configPath = path.join(configOpenSpecDir, 'config.yaml');As per PR objectives, V1 requires consumer-owned configuration and lockfiles.
Also applies to: 190-190
🤖 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/utils/change-utils.ts` around lines 136 - 145, Update configuration initialization in the change-generation flow around schemaRoot, projectRoot, and readProjectConfig so generated config.yaml is created under schemaRoot rather than projectRoot when they differ, or initialization is skipped in that case. Keep planning directories rooted at projectRoot, preserve schema resolution behavior, and ensure the store-owned projectRoot/openspec/config.yaml is never created.
🧹 Nitpick comments (7)
test/cli-e2e/remote-schema-concurrency.test.ts (2)
100-123: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSurface CLI stderr on failure and give this e2e case an explicit timeout.
Two CLI processes plus two Git fetches under a contended lock can exceed the default Vitest timeout on slow CI, and a bare
expect(alpha.exitCode).toBe(0)gives no clue why.♻️ Suggested change
- expect(alpha.exitCode).toBe(0); - expect(beta.exitCode).toBe(0); + expect(alpha.exitCode, alpha.stderr).toBe(0); + expect(beta.exitCode, beta.stderr).toBe(0);And widen the case timeout:
- it('reclaims an abandoned ticket while concurrent named sync processes preserve both entries', async () => { + it('reclaims an abandoned ticket while concurrent named sync processes preserve both entries', async () => { + // ... + }, 60_000);🤖 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/cli-e2e/remote-schema-concurrency.test.ts` around lines 100 - 123, Update the concurrent CLI test around the alpha and beta runCLI results to include each process’s stderr in failure diagnostics when asserting exitCode 0. Give this e2e test an explicit, sufficiently generous timeout to accommodate concurrent Git fetches and lock contention.
10-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winIsolate the test repos from the developer's global Git config.
gitEnvinheritsHOME, so a contributor withcommit.gpgsign=true, acore.hooksPath, or templates configured globally will seegit commitfail here for reasons unrelated to the change. Pinning the config sources makes the fixture hermetic.💚 Suggested change
const gitEnv = { ...process.env, + GIT_CONFIG_GLOBAL: '/dev/null', + GIT_CONFIG_SYSTEM: '/dev/null', GIT_AUTHOR_NAME: 'OpenSpec Test', GIT_AUTHOR_EMAIL: 'openspec@example.test', GIT_COMMITTER_NAME: 'OpenSpec Test', GIT_COMMITTER_EMAIL: 'openspec@example.test', }; function git(cwd: string, ...args: string[]): void { - execFileSync('git', args, { + execFileSync('git', ['-c', 'commit.gpgsign=false', ...args], { cwd, env: gitEnv, stdio: ['ignore', 'pipe', 'pipe'], }); }On Windows
/dev/nullisn't valid; usepath.join(tempDir, 'gitconfig')(a nonexistent file) if this suite runs cross-platform.🤖 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/cli-e2e/remote-schema-concurrency.test.ts` around lines 10 - 24, Update the gitEnv and git helper setup to isolate fixture repositories from global Git configuration by disabling the global/system config sources and pointing Git to a temporary, nonexistent config file using a cross-platform path such as path.join(tempDir, 'gitconfig'). Ensure the temporary directory is available to this setup and preserve the existing author, committer, and stdio settings.src/core/artifact-graph/schema-directory.ts (1)
49-59: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard against an empty
issueslist whenschemais missing.If an inspector ever returns
schema === undefinedwith no issues, the thrown error carries an emptyissuesarray.schemaSyncFailureStatusinsrc/commands/schema.tsmapserror.issuesone-to-one, so the CLI would fail with zero reported error entries. Synthesizing a fallback issue keeps the failure diagnosable.♻️ Suggested hardening
function validatedResult( inspection: InspectedSchemaDirectory ): ValidatedSchemaDirectory { if (!inspection.schema || inspection.issues.length > 0) { - throw new SchemaDirectoryValidationError(inspection.issues); + throw new SchemaDirectoryValidationError( + inspection.issues.length > 0 + ? inspection.issues + : [{ path: 'schema.yaml', message: 'Schema could not be parsed' }] + ); }🤖 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 49 - 59, Update validatedResult so that when inspection.schema is missing and inspection.issues is empty, it throws SchemaDirectoryValidationError with a synthesized issue; preserve the existing issues when present and keep the successful return path unchanged.test/core/remote-schema/sync-lock.test.ts (1)
134-144: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd coverage for an unparseable ticket file.
The reclaim test only exercises a well-formed ticket with a dead pid. A truncated/corrupt
*.ticket.json(crash mid-write) takes a different branch —readParticipantreturnsnull, so it is neither reclaimed nor skipped — and currently wedges acquisition permanently. A test asserting the intended behavior here would lock in the fix suggested onsrc/core/remote-schema/sync-lock.ts.💚 Suggested test
it('recovers from a corrupt ticket file', async () => { const lockPath = getSchemaSyncLockPath(projectRoot); fs.mkdirSync(lockPath, { recursive: true }); fs.writeFileSync(path.join(lockPath, 'claim-corrupt.ticket.json'), '{"token"'); await expect( withSchemaSyncLock(projectRoot, async () => 'acquired', { timeoutMs: 200, retryDelayMs: 5, }) ).resolves.toBe('acquired'); });🤖 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/sync-lock.test.ts` around lines 134 - 144, Extend the sync-lock test coverage with a corrupt-ticket scenario near “reclaims an abandoned same-host lock”: create the lock directory, write truncated JSON to a *.ticket.json file, and assert withSchemaSyncLock resolves successfully. Update the corresponding acquisition logic in readParticipant handling so an unparseable ticket is recovered rather than skipped indefinitely.test/core/artifact-graph/schema-directory.test.ts (1)
104-118: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssertion flow relies on the sentinel error falling into the same
catch.If validation unexpectedly succeeds, the sentinel
Erroris caught by this block and only fails on thetoBeInstanceOfassertion — readable enough, butexpect(...).toThrowError(SchemaDirectoryValidationError)plus a captured-error inspection (orexpect.hasAssertions()) is clearer.🤖 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 104 - 118, Update the validation test around validateRemoteSchemaDirectory to use an assertion flow that explicitly expects SchemaDirectoryValidationError and then inspects the captured error’s issues, rather than throwing a sentinel Error inside the same catch block. Preserve the existing three issue-message and length assertions, and ensure the test cannot pass without making the expected validation-error assertions.src/core/remote-schema/bundle.ts (1)
113-125: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueSize race is correctly closed by the post-read re-check.
statSyncpre-check avoids reading obviously oversized files, and the second check oncontent.lengthrejects files that grew betweenstatandreadbefore they are accepted intofiles/totalBytes. Good.One optional consideration: contents of the whole bundle are buffered in memory (bounded by
maxBytes) before hashing. If you later raiseMAX_SCHEMA_BUNDLE_BYTES, consider a two-pass approach (collect sorted relative paths first, then stream each file into the hash) so peak memory stays flat.🤖 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 - 125, No code change is required; the existing stat pre-check and post-read content-length validation correctly close the file-size race. If increasing the bundle size limit later, refactor the bundling flow to collect sorted paths first and stream file contents into hashing rather than buffering the entire bundle in memory.test/commands/schema-sync.test.ts (1)
390-459: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPath handling here is guideline-compliant (realpath canonicalization +
path.join).Consider also adding a symlink/alias-path regression case for this consumer-root resolution logic, since this PR's core surface is path identity for remote sync.
As per path instructions: "Add an alias-path regression test when touching path identity logic. If preserving user-typed path spelling is intentional, assert it separately from identity comparisons."
🤖 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 390 - 459, Add a regression test alongside the nested-directory test that runs schema synchronization from a symlink or alias path to the consumer root. Assert that path identity uses the canonical realpath and writes the lockfile under the resolved consumer root, while separately preserving any intentionally user-typed path spelling in output if applicable.Source: Path instructions
🤖 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 `@src/core/remote-schema/git.ts`:
- Around line 51-62: Update buildNonInteractiveGitEnvironment to preserve any
user-provided StrictHostKeyChecking value, including stricter settings, instead
of removing and replacing it with accept-new; only append -o
StrictHostKeyChecking=accept-new when no such option is already present.
Continue removing and enforcing BatchMode through removeSshOption, and update
the affected tests to expect explicit host-key policies to remain unchanged.
In `@src/core/remote-schema/sync-lock.ts`:
- Around line 157-164: Update readParticipant validation so ticket files must
contain a defined number before they reach precedes; reject or ignore
participants with missing number, while preserving existing handling for valid
tickets and ensuring every locally written ticket remains valid.
- Around line 130-151: Update createParticipantFile to write participant data
through a uniquely named staging file that does not end in .ticket.json or
.choosing.json, then atomically rename it to filePath after the write completes
while preserving exclusive creation semantics. Update the lock
acquisition/reclamation logic, including readParticipant, isAbandonedSameHost,
and acquireSchemaSyncLock, to treat unparseable participant files as reclaimable
after the acquisition timeout so corrupt entries cannot permanently block
synchronization.
- Around line 33-35: Add the runtime lock directory pattern for
openspec/.schemas.lock to the repository’s ignore configuration, including all
nested .ticket.json files, so stale lock claims and tickets remain untracked.
Use getSchemaSyncLockPath as the reference for the directory and preserve
existing ignore rules.
In `@test/core/artifact-graph/schema-directory.test.ts`:
- Around line 67-82: Update the assertion in the `preserves legacy project-local
template lookup outside templates/` test to canonicalize the expected
`sharedTemplate` path with `fs.realpathSync.native()` before comparing it to
`validateLocalSchemaDirectory(schemaDir).templatePaths.proposal`; keep the
production path unchanged and preserve the existing cleanup.
---
Outside diff comments:
In `@src/commands/workflow/templates.ts`:
- Around line 41-45: Update the schema source classification used by the report
templates around schemaDir and the resolved schema path so declared remote
schemas are identified as remote when their cache directory falls outside
project and user schema directories. Extend the source type and both
human-readable and JSON output handling to support the new remote
classification, while preserving existing package and local classifications.
In `@src/utils/change-utils.ts`:
- Around line 136-145: Update configuration initialization in the
change-generation flow around schemaRoot, projectRoot, and readProjectConfig so
generated config.yaml is created under schemaRoot rather than projectRoot when
they differ, or initialization is skipped in that case. Keep planning
directories rooted at projectRoot, preserve schema resolution behavior, and
ensure the store-owned projectRoot/openspec/config.yaml is never created.
---
Nitpick comments:
In `@src/core/artifact-graph/schema-directory.ts`:
- Around line 49-59: Update validatedResult so that when inspection.schema is
missing and inspection.issues is empty, it throws SchemaDirectoryValidationError
with a synthesized issue; preserve the existing issues when present and keep the
successful return path unchanged.
In `@src/core/remote-schema/bundle.ts`:
- Around line 113-125: No code change is required; the existing stat pre-check
and post-read content-length validation correctly close the file-size race. If
increasing the bundle size limit later, refactor the bundling flow to collect
sorted paths first and stream file contents into hashing rather than buffering
the entire bundle in memory.
In `@test/cli-e2e/remote-schema-concurrency.test.ts`:
- Around line 100-123: Update the concurrent CLI test around the alpha and beta
runCLI results to include each process’s stderr in failure diagnostics when
asserting exitCode 0. Give this e2e test an explicit, sufficiently generous
timeout to accommodate concurrent Git fetches and lock contention.
- Around line 10-24: Update the gitEnv and git helper setup to isolate fixture
repositories from global Git configuration by disabling the global/system config
sources and pointing Git to a temporary, nonexistent config file using a
cross-platform path such as path.join(tempDir, 'gitconfig'). Ensure the
temporary directory is available to this setup and preserve the existing author,
committer, and stdio settings.
In `@test/commands/schema-sync.test.ts`:
- Around line 390-459: Add a regression test alongside the nested-directory test
that runs schema synchronization from a symlink or alias path to the consumer
root. Assert that path identity uses the canonical realpath and writes the
lockfile under the resolved consumer root, while separately preserving any
intentionally user-typed path spelling in output if applicable.
In `@test/core/artifact-graph/schema-directory.test.ts`:
- Around line 104-118: Update the validation test around
validateRemoteSchemaDirectory to use an assertion flow that explicitly expects
SchemaDirectoryValidationError and then inspects the captured error’s issues,
rather than throwing a sentinel Error inside the same catch block. Preserve the
existing three issue-message and length assertions, and ensure the test cannot
pass without making the expected validation-error assertions.
In `@test/core/remote-schema/sync-lock.test.ts`:
- Around line 134-144: Extend the sync-lock test coverage with a corrupt-ticket
scenario near “reclaims an abandoned same-host lock”: create the lock directory,
write truncated JSON to a *.ticket.json file, and assert withSchemaSyncLock
resolves successfully. Update the corresponding acquisition logic in
readParticipant handling so an unparseable ticket is recovered rather than
skipped indefinitely.
🪄 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: 1a914327-8c32-46e9-8738-042016f2e54d
📒 Files selected for processing (43)
docs/cli.mddocs/customization.mdopenspec/changes/add-remote-schema-sources/design.mdopenspec/changes/add-remote-schema-sources/proposal.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/cli/index.tssrc/commands/schema.tssrc/commands/validate.tssrc/commands/workflow/instructions.tssrc/commands/workflow/new-change.tssrc/commands/workflow/schemas.tssrc/commands/workflow/status.tssrc/commands/workflow/templates.tssrc/core/archive.tssrc/core/artifact-graph/instruction-loader.tssrc/core/artifact-graph/resolver.tssrc/core/artifact-graph/schema-directory.tssrc/core/completions/completion-provider.tssrc/core/list.tssrc/core/remote-schema/authority.tssrc/core/remote-schema/bundle.tssrc/core/remote-schema/consumer-root.tssrc/core/remote-schema/git.tssrc/core/remote-schema/sync-lock.tssrc/core/remote-schema/sync.tssrc/core/root-selection.tssrc/core/validation/validator.tssrc/utils/change-metadata.tssrc/utils/change-utils.tssrc/utils/task-progress.tstest/cli-e2e/remote-schema-concurrency.test.tstest/commands/schema-sync.test.tstest/commands/store-root-selection.test.tstest/core/artifact-graph/remote-resolver.test.tstest/core/artifact-graph/schema-directory.test.tstest/core/remote-schema/bundle-size-race.test.tstest/core/remote-schema/bundle.test.tstest/core/remote-schema/git.test.tstest/core/remote-schema/sync-lock.test.tstest/core/remote-schema/sync.test.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- openspec/changes/add-remote-schema-sources/design.md
- openspec/changes/add-remote-schema-sources/proposal.md
- openspec/changes/add-remote-schema-sources/specs/schema-resolution/spec.md
- docs/customization.md
- test/core/artifact-graph/remote-resolver.test.ts
- docs/cli.md
- src/core/remote-schema/sync.ts
- openspec/changes/add-remote-schema-sources/specs/remote-schema-sources/spec.md
- src/core/artifact-graph/resolver.ts
|
Thanks @Patodo — this is a lot of careful work (git resolution, content-addressed cache, lockfile, path-safety hardening, real integration tests). @TabishB flagging for your call. TL;DR: What it does: lets a project point Why we are parking it for now: it is ~5,800 lines, and beyond the new feature it introduces a Not a rejection — it needs a design decision from you on whether |
90a082d to
29f2a73
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/core/remote-schema/sync-lock.test.ts (1)
163-178: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTight timing margin may flake under CI load.
Asserting
Date.now() - startedAt >= 35against a 40ms configured timeout leaves only a 5ms buffer for scheduling/polling jitter (retryDelayMs: 5). Consider widening the margin (e.g., lowering the threshold further or increasingtimeoutMs) to reduce flakiness on slower CI runners.🤖 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/sync-lock.test.ts` around lines 163 - 178, The timing assertion in the “waits the full timeout before reclaiming a fresh unparseable participant” test is too close to the configured 40ms timeout and may flake under CI load. Widen the margin by lowering the elapsed-time threshold or increasing timeoutMs, while preserving verification that the corrupt participant is reclaimed only after the timeout.
🤖 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 `@src/core/remote-schema/sync-lock.ts`:
- Around line 192-220: The createParticipantFile retry loop can run forever and
retries all ENOENT errors, potentially hanging acquireSchemaSyncLock. Restrict
retries to the directory-deletion race, bound them by the lock acquisition
deadline/timeout, and add an fs.renameSync fallback when fs.linkSync is
unsupported or persistently fails, while preserving staging cleanup.
---
Nitpick comments:
In `@test/core/remote-schema/sync-lock.test.ts`:
- Around line 163-178: The timing assertion in the “waits the full timeout
before reclaiming a fresh unparseable participant” test is too close to the
configured 40ms timeout and may flake under CI load. Widen the margin by
lowering the elapsed-time threshold or increasing timeoutMs, while preserving
verification that the corrupt participant is reclaimed only after the timeout.
🪄 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: c785b9d0-9a5e-43c7-b6f9-33f3edb115c8
📒 Files selected for processing (17)
docs/cli.mddocs/customization.mdopenspec/changes/add-remote-schema-sources/design.mdopenspec/changes/add-remote-schema-sources/proposal.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/tasks.mdsrc/commands/workflow/templates.tssrc/core/remote-schema/git.tssrc/core/remote-schema/sync-lock.tssrc/utils/change-utils.tstest/cli-e2e/remote-schema-concurrency.test.tstest/commands/schema-sync.test.tstest/core/artifact-graph/schema-directory.test.tstest/core/remote-schema/git.test.tstest/core/remote-schema/sync-lock.test.tstest/utils/change-utils.test.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- openspec/changes/add-remote-schema-sources/design.md
- openspec/changes/add-remote-schema-sources/specs/schema-resolution/spec.md
- docs/cli.md
- docs/customization.md
- test/core/artifact-graph/schema-directory.test.ts
- openspec/changes/add-remote-schema-sources/specs/remote-schema-sources/spec.md
- openspec/changes/add-remote-schema-sources/proposal.md
| function createParticipantFile( | ||
| lockPath: string, | ||
| fileName: string, | ||
| participant: SchemaSyncParticipant | ||
| ): string { | ||
| const filePath = path.join(lockPath, fileName); | ||
| while (true) { | ||
| ensureSelfIgnoredLockDirectory(lockPath); | ||
| const stagingPath = path.join( | ||
| lockPath, | ||
| `.participant-${randomUUID()}.tmp` | ||
| ); | ||
| try { | ||
| fs.writeFileSync(stagingPath, JSON.stringify(participant), { | ||
| encoding: 'utf8', | ||
| flag: 'wx', | ||
| }); | ||
| fs.linkSync(stagingPath, filePath); | ||
| return filePath; | ||
| } catch (error) { | ||
| if ((error as NodeJS.ErrnoException).code === 'ENOENT') { | ||
| continue; | ||
| } | ||
| throw error; | ||
| } finally { | ||
| fs.rmSync(stagingPath, { force: true }); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
git ls-files | grep -F 'src/core/remote-schema/sync-lock.ts' || true
echo "== relevant source =="
if [ -f src/core/remote-schema/sync-lock.ts ]; then
wc -l src/core/remote-schema/sync-lock.ts
sed -n '1,340p' src/core/remote-schema/sync-lock.ts | cat -n
fi
echo "== timeout/error uses =="
rg -n "SchemaSyncLockError|timeoutMs|createParticipantFile|linkSync|renameSync|retry" src/core/remote-schema/sync-lock.ts src -g '*.ts' || true
echo "== test files around sync lock =="
git ls-files | grep -i 'sync-lock\|sync.*lock\|remote-schema' | head -100Repository: Fission-AI/OpenSpec
Length of output: 17972
🌐 Web query:
Node.js fs.linkSync ENOENT Container devcontainer overlay filesystem persistent ENOENT
💡 Result:
The ENOENT error during fs.linkSync in a Dev Container or Docker environment—especially when using overlay filesystems or bind mounts—typically occurs because the operation encounters filesystem limitations or kernel behavior that Node.js does not natively handle as a graceful fallback [1][2]. Even when the source file and destination directory exist, fs.linkSync may fail with ENOENT in containerized environments due to the following reasons: 1. Cross-Layer/Cross-Device Limitations: In OverlayFS (often used by Docker), hard linking between the read-only lower layer and the writable upper layer is restricted [1]. While this is technically a cross-device constraint (which should trigger EXDEV), some containerized filesystem configurations or kernel versions may return ENOENT instead, which many Node.js applications or libraries are not programmed to handle as a signal to switch strategies [1][3]. 2. Filesystem Synchronization/Visibility: When bind-mounting directories (common in VS Code Dev Containers), there can be latency or consistency issues between the host and container filesystem drivers (e.g., virtiofs on macOS/Windows Docker Desktop) [4][5][2]. Tools like pnpm or yarn that rely on hard links often assume atomic filesystem behavior that may not be fully supported by the underlying mount, leading to "file not found" errors during linking operations [1][4][2]. Recommended Solutions: - Configure Package Manager Fallbacks: If you are using pnpm or yarn, you can often bypass this by changing the linking method. For pnpm, setting package-import-method=clone-or-copy in your.npmrc file forces the use of copies instead of hard links, which is more resilient to container filesystem constraints [2]. - Adjust Store Locations: If using a persistent store, ensure the pnpm/yarn store is located on the same filesystem mount as your project, or specifically place it in a location (such as the container's home directory) that avoids cross-device or permission issues [2]. - Implement Robust File Operations: If you are writing custom Node.js code that uses fs.linkSync, wrap the call in a try/catch block [1][3]. If the operation catches an error (such as ENOENT or EXDEV), implement a fallback that uses fs.copyFileSync to replicate the file instead of attempting to link it [1][3]. These patterns ensure that your application remains functional even when the underlying container filesystem does not permit standard hard-link operations [1][3].
Citations:
- 1:
linkSyncfails with ENOENT in Dev Container environments even when source and destination exist pnpm/pnpm#10217 - 2: ENOENT: no such file or directory, copyfile '/project/.pnpm-store/... -> '/project/node_modules/... pnpm/pnpm#5803
- 3: Fix: Robust Cross-Platform Fallback for Git Hook Linking in [link-hooks.mjs](cci:7://file:///d:/Github/TypeScript/scripts/link-hooks.mjs:0:0-0:0) microsoft/TypeScript#61858
- 4: [Bug?]: "ENOENT: no such file or directory, link ..." when configured for
nodeLinker: pnpmand running in Docker with mounted filesystem yarnpkg/berry#6453 - 5: https://stackoverflow.com/questions/78405684/pnpm-cannot-create-files-err-pnpm-enoent-enoent-no-such-file-or-directory
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== inspect relevant tests for timeout/failure behavior =="
wc -l test/core/remote-schema/sync-lock.test.ts test/cli-e2e/remote-schema-concurrency.test.ts || true
sed -n '1,260p' test/core/remote-schema/sync-lock.test.ts | cat -n
echo "== timeout/deadline references in tests =="
rg -n "timeoutMs|SchemaSyncLockError|acquireSchemaSyncLock|withSchemaSyncLock|createParticipantFile|ENOENT|linkSync" test src/core/remote-schema/sync.ts src/commands/schema.ts || trueRepository: Fission-AI/OpenSpec
Length of output: 16678
Bound the participant publish retry and handle unsupported link errors.
In the unbounded while (true) loop, an ENOENT from fs.linkSync causes every later retry to recreate .schemas.lock/.gitignore as empty and continue forever, so acquireSchemaSyncLock can hang before its timeoutMs deadline. Treat only the directory-deletion race as retryable, bound the retry window by the lock deadline, and consider a fs.renameSync(stagingPath, filePath) fallback where fs.linkSync is unsupported or persistently fails.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 204-207: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFileSync(stagingPath, JSON.stringify(participant), {
encoding: 'utf8',
flag: 'wx',
})
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
🤖 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-lock.ts` around lines 192 - 220, The
createParticipantFile retry loop can run forever and retries all ENOENT errors,
potentially hanging acquireSchemaSyncLock. Restrict retries to the
directory-deletion race, bound them by the lock acquisition deadline/timeout,
and add an fs.renameSync fallback when fs.linkSync is unsupported or
persistently fails, while preserving staging cleanup.
|
Thanks @clay-good. After stepping back and reconsidering the architecture, I think this review identified the key issue correctly. I focused too quickly on schema distribution, reproducible pinning, and offline resolution, and overlooked that OpenSpec already has a Git-backed sharing abstraction: Stores. As a result, #1444 introduced a parallel distribution system—sync commands, lockfiles, caches, and Git/network handling—when the existing Store model can cover the main collaboration use case more naturally. A Store-based model treats planning and schemas as two independent roles. For example, a department could maintain:
A consumer team could configure: store: department-planning
schema: qeda-sdd
schemaStore:
id: department-schemas
schemas:
- qeda-sdd
- frontend-sddEach developer clones and registers those Stores once: git clone git@github.com:acme/department-planning.git
openspec store register ./department-planning --id department-planning
git clone git@github.com:acme/department-schemas.git
openspec store register ./department-schemas --id department-schemasGit remains responsible for collaboration and synchronization. Teams update the registered checkout with normal The same schema Store can serve multiple teams while each consumer controls visibility. A frontend team can expose only This does give up per-project commit pinning, but I now think that should be a separate product decision rather than something bundled into the initial schema-sharing feature. I’ve opened a narrower Store-based exploration in #1473. I consider that direction a better fit with OpenSpec’s existing model, and #1444’s remote sync/cache/lockfile approach should be treated as superseded rather than merged as-is. |
3 participants
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
schemaSources, withopenspec schema syncsupporting update and deterministic--lockedrestore, plus structured--jsonreporting.