Add agent workspace provisioning helpers (workspace-seeder + workspace-mount)#208
Conversation
Lifts seedWorkspace / seedWorkflowAcls / createWorkspaceIfNeeded / seedAclRules / seedWorkspaceTar and the ensureRelayfileMount lifecycle helper out of @agent-relay/sdk and into @relayfile/sdk where they belong. These have always been RelayFileClient wrappers and relayfile-mount binary lifecycle helpers — they were sitting in agent-relay only because the original consumer (the workflow runner) lived there. Splitting them here lets agent-relay shrink to broker concerns and keeps relayfile-server interactions inside the relayfile SDK. Two new files: src/workspace-seeder.ts ── workspace creation, file/ACL seeding src/workspace-mount.ts ── ensureRelayfileMount + MountConfig/MountHandle Both re-exported from the package root and accessible via the @relayfile/sdk/workspace-seeder and @relayfile/sdk/workspace-mount subpaths. Bumps to 0.8.0 (minor — additive public API surface).
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Warning Review limit reached
More reviews will be available in 52 minutes and 1 second. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR adds workspace seeding and relayfile-mount lifecycle management to the TypeScript SDK. It introduces ChangesWorkspace Provisioning and Mount Features
Sequence Diagram(s)Mount binary installation and lifecycle are covered in the hidden review stack artifact above. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Relayfile Eval ReviewRun: Passed: 4 | Needs human: 0 | Reviewable: 0 | Missing output: 0 | Failed: 0 | Skipped: 0 Human Review CasesNo reviewable human-review cases captured Relayfile output. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 050eb38bd4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| stopped = true; | ||
| await stopMountProcess(mountProc).catch(() => undefined); | ||
| await rm(mountPoint, { recursive: true, force: true }).catch(() => undefined); |
There was a problem hiding this comment.
ensureRelayfileMount always removes mountPoint recursively in stop(), even when the path came from config.mountPoint. If callers pass an existing directory, calling stop() can delete that entire directory tree and cause data loss; cleanup should be limited to internally-created temp mount paths (or be explicitly opt-in).
Useful? React with 👍 / 👎.
| } | ||
| const stat = fs.statSync(resolved); | ||
| if (stat.isDirectory()) { | ||
| collectSeedPaths(rootDir, nextRelative, excludeDirs, output); |
There was a problem hiding this comment.
When a symlink resolves to rootDir (or another ancestor inside it), this branch recurses back into the same tree via collectSeedPaths(rootDir, nextRelative, ...) without any visited-realpath guard. A repo containing a self-referential symlink can therefore cause unbounded recursion/hangs during seeding; directory symlinks should be skipped or tracked by resolved path.
Useful? React with 👍 / 👎.
| ? rawFiles.filter((f) => { | ||
| const segments = f.split('/'); | ||
| if (DEFAULT_EXCLUDED_FILES.has(segments[segments.length - 1])) return false; | ||
| return !segments.some((seg) => excludes.has(seg)); |
There was a problem hiding this comment.
In the git-tracked path, excludes are checked per segment (segments.some(...)), so a configured exclude like "dist/generated" is never matched because neither segment equals that full key. This causes nested excluded paths to still be uploaded in tar mode; filtering should evaluate full relative paths/prefixes, not only individual segments.
Useful? React with 👍 / 👎.
| batch, | ||
| `seed-workspace-${workspace}-${Date.now()}-${batchIndex}` | ||
| ); | ||
| seededCount += result.written; |
There was a problem hiding this comment.
seedWorkspace accumulates result.written but never checks result.errorCount, so partial batch failures are silently treated as success. The same module already treats nonzero errorCount as fatal in seedAclRules; this omission can leave workspaces partially seeded without surfacing an error to callers.
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
6 issues found across 5 files
You’re at about 97% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/sdk/typescript/src/workspace-mount.ts">
<violation number="1" location="packages/sdk/typescript/src/workspace-mount.ts:98">
P2: Add request timeouts to binary/checksum downloads; a stalled HTTPS connection can block workspace provisioning indefinitely.</violation>
</file>
<file name="packages/sdk/typescript/src/workspace-seeder.ts">
<violation number="1" location="packages/sdk/typescript/src/workspace-seeder.ts:360">
P2: File contents are fully materialized before batching, which can cause high memory usage on large workspaces.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| resolve: () => void, | ||
| reject: (error: Error) => void | ||
| ) => { | ||
| const request = https.get(currentUrl, (res) => { |
There was a problem hiding this comment.
P2: Add request timeouts to binary/checksum downloads; a stalled HTTPS connection can block workspace provisioning indefinitely.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/sdk/typescript/src/workspace-mount.ts, line 98:
<comment>Add request timeouts to binary/checksum downloads; a stalled HTTPS connection can block workspace provisioning indefinitely.</comment>
<file context>
@@ -0,0 +1,419 @@
+ resolve: () => void,
+ reject: (error: Error) => void
+ ) => {
+ const request = https.get(currentUrl, (res) => {
+ const status = res.statusCode ?? 0;
+ const location = res.headers.location;
</file context>
| collectSeedPaths(rootDir, '', excludes, seedPaths); | ||
| const allFiles = seedPaths | ||
| .sort((left, right) => left.localeCompare(right)) | ||
| .map((filePath) => buildSeedFilePayload(filePath, rootDir)); |
There was a problem hiding this comment.
P2: File contents are fully materialized before batching, which can cause high memory usage on large workspaces.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/sdk/typescript/src/workspace-seeder.ts, line 360:
<comment>File contents are fully materialized before batching, which can cause high memory usage on large workspaces.</comment>
<file context>
@@ -0,0 +1,571 @@
+ collectSeedPaths(rootDir, '', excludes, seedPaths);
+ const allFiles = seedPaths
+ .sort((left, right) => left.localeCompare(right))
+ .map((filePath) => buildSeedFilePayload(filePath, rootDir));
+
+ let seededCount = 0;
</file context>
The seeder and mount helpers statically pull in node:child_process, node:fs, and node:path. Re-exporting them from the default entry breaks the SDK's import-safety guarantee for browser and edge consumers, so import them from @relayfile/sdk/workspace-seeder and @relayfile/sdk/workspace-mount instead.
The original file used node:test, which vitest does not collect, so the suite reported 'No test suite found' and CI failed.
- workspace-mount: only remove the mount point on shutdown/failure when ensureRelayfileMount created it itself (mkdtemp path). When the caller passes their own mountPoint, leave it alone so we never recursively delete user-owned directories. - workspace-seeder: send the tar upload body as a Uint8Array view (Buffer-backed) so vitest can introspect it and the receiving end still gets bytes, not a raw ArrayBuffer. - workspace-seeder: track visited real paths during the directory walk so a symlinked directory cycle can no longer cause infinite recursion. - workspace-seeder: surface errorCount from bulk-write batches by throwing when any batch reports errors, instead of silently swallowing partial failures. - workspace-seeder: honor nested excludeDirs entries like packages/app/build when filtering git ls-files output, not just single-segment names.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/sdk/typescript/src/workspace-mount.ts (1)
226-233: ⚡ Quick winAvoid shell interpolation for
codesign.
execSync()here still goes through a shell. A quote or shell metacharacter inbinaryPathcan break signing, and this keeps the command-injection warning alive for no real benefit. Prefer an argv-based call instead.Suggested change
-import { execSync, spawn, type ChildProcess } from 'node:child_process'; +import { execFileSync, spawn, type ChildProcess } from 'node:child_process'; function resignBinaryForMacOS(binaryPath: string): void { if (os.platform() !== 'darwin') { return; } try { - execSync(`codesign --force --sign - "${binaryPath}"`, { stdio: 'pipe' }); + execFileSync('codesign', ['--force', '--sign', '-', binaryPath], { stdio: 'pipe' }); } catch { // Ignore best-effort re-sign failures. } }🤖 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 `@packages/sdk/typescript/src/workspace-mount.ts` around lines 226 - 233, In resignBinaryForMacOS replace the shell-interpolated execSync call with an argv-based child process call (e.g., child_process.execFileSync or spawnSync) so the binaryPath is passed as a separate argument instead of interpolated into a shell string; update the call in resignBinaryForMacOS to invoke "codesign" with arguments ["--force","--sign","-","<binaryPath>"] (using the actual binaryPath variable) and keep the same stdio options and try/catch behavior so command injection and shell-escaping issues are eliminated.packages/sdk/typescript/src/workspace-seeder.test.ts (1)
61-323: ⚡ Quick winAdd focused tests for
createWorkspaceIfNeededbehavior matrix.This suite skips the new public provisioning entrypoint. Please add cases for SDK overload attempts, HTTP fallback body variants, and
409as success to protect release-critical workspace creation flows.🤖 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 `@packages/sdk/typescript/src/workspace-seeder.test.ts` around lines 61 - 323, Add new focused tests for createWorkspaceIfNeeded: simulate the SDK path by spying on RelayFileClient.prototype.createWorkspace to throw an overload/error and assert the function falls back to HTTP POST with the correct request body variants (e.g., with and without optional fields) by mocking globalThis.fetch and inspecting its payload; also add a test where the HTTP POST returns status 409 and ensure createWorkspaceIfNeeded treats 409 as success (resolves) rather than throwing. Target the createWorkspaceIfNeeded callsite in your tests, and use the existing patterns (vi.spyOn(RelayFileClient.prototype, 'createWorkspace') and globalThis.fetch mocks) and helpers like parseFetchBody/singleFetchCall to validate 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.
Inline comments:
In `@packages/sdk/typescript/src/workspace-mount.ts`:
- Around line 422-434: The stop() implementation currently flips the stopped
flag and then runs asynchronous cleanup, allowing concurrent callers to return
early; fix by memoizing the in-flight shutdown Promise: add a local variable
(e.g., stoppingPromise) alongside stopped, and when stop() is first invoked
assign stoppingPromise = (async () => { await
stopMountProcess(mountProc).catch(() => undefined); await cleanupMountPoint();
})(); set stopped = true immediately and always return await stoppingPromise for
subsequent callers so all callers await the same shutdown sequence involving
stopMountProcess and cleanupMountPoint.
- Around line 283-312: runCommandCapture currently can hang if the child never
exits; add a one-shot timeout that kills the spawned process and rejects the
promise with a clear timeout error; specifically, inside runCommandCapture after
spawning proc, create a timer (e.g., via setTimeout) that calls proc.kill() and
rejects with a new Error like "command timed out", store the timer id, and
ensure you clearTimeout(timer) in the proc.on('close') and proc.on('error')
handlers so the timer is cleaned up; make sure the rejection on timeout happens
only once (remove/guard listeners or check a settled flag) so you don't end up
calling resolve/reject twice.
---
Nitpick comments:
In `@packages/sdk/typescript/src/workspace-mount.ts`:
- Around line 226-233: In resignBinaryForMacOS replace the shell-interpolated
execSync call with an argv-based child process call (e.g.,
child_process.execFileSync or spawnSync) so the binaryPath is passed as a
separate argument instead of interpolated into a shell string; update the call
in resignBinaryForMacOS to invoke "codesign" with arguments
["--force","--sign","-","<binaryPath>"] (using the actual binaryPath variable)
and keep the same stdio options and try/catch behavior so command injection and
shell-escaping issues are eliminated.
In `@packages/sdk/typescript/src/workspace-seeder.test.ts`:
- Around line 61-323: Add new focused tests for createWorkspaceIfNeeded:
simulate the SDK path by spying on RelayFileClient.prototype.createWorkspace to
throw an overload/error and assert the function falls back to HTTP POST with the
correct request body variants (e.g., with and without optional fields) by
mocking globalThis.fetch and inspecting its payload; also add a test where the
HTTP POST returns status 409 and ensure createWorkspaceIfNeeded treats 409 as
success (resolves) rather than throwing. Target the createWorkspaceIfNeeded
callsite in your tests, and use the existing patterns
(vi.spyOn(RelayFileClient.prototype, 'createWorkspace') and globalThis.fetch
mocks) and helpers like parseFetchBody/singleFetchCall to validate behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0aaf199f-2d58-4de8-b6fb-5ea11ae1a610
📒 Files selected for processing (7)
packages/sdk/typescript/CHANGELOG.mdpackages/sdk/typescript/package.jsonpackages/sdk/typescript/src/index.tspackages/sdk/typescript/src/workspace-mount.tspackages/sdk/typescript/src/workspace-seeder-tar.test.tspackages/sdk/typescript/src/workspace-seeder.test.tspackages/sdk/typescript/src/workspace-seeder.ts
- ensureRelayfileMount.stop() now memoizes the in-flight shutdown promise so concurrent callers all await the same termination and cleanup sequence instead of the second caller returning early while stopMountProcess/cleanupMountPoint are still running. - runCommandCapture enforces a five-minute timeout that kills the spawned child and rejects with a clear error, so a stalled relayfile-mount sync can no longer hang ensureRelayfileMount forever. Settled state is guarded so we never double-resolve. - resignBinaryForMacOS calls codesign via execFileSync with an argv array so shell metacharacters in the cached binary path cannot hijack or break the signing step.
There was a problem hiding this comment.
2 issues found across 6 files (changes from recent commits).
You’re at about 99% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/sdk/typescript/src/workspace-seeder.ts">
<violation number="1" location="packages/sdk/typescript/src/workspace-seeder.ts:134">
P2: The new global `visited` set can skip legitimate directory aliases, making seeded file paths nondeterministic when both a real dir and symlinked path point to the same target.</violation>
</file>
<file name="packages/sdk/typescript/src/workspace-mount.ts">
<violation number="1" location="packages/sdk/typescript/src/workspace-mount.ts:412">
P2: The `stop()` method is not safe for concurrent callers. After setting `stopped = true`, the async cleanup (`stopMountProcess` + `cleanupMountPoint`) is in-flight, but a second `await handle.stop()` will hit the early `return` and resolve before the first call finishes teardown. Memoize the shutdown promise so all callers await the same completion:
```ts
let stopPromise: Promise<void> | undefined;
async stop() {
if (!stopPromise) {
stopPromise = (async () => {
await stopMountProcess(mountProc).catch(() => undefined);
await cleanupMountPoint();
})();
}
await stopPromise;
}
```</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| // Cycle guard: a symlinked directory pointed back to an ancestor. | ||
| return; | ||
| } | ||
| visited.add(realDir); |
There was a problem hiding this comment.
P2: The new global visited set can skip legitimate directory aliases, making seeded file paths nondeterministic when both a real dir and symlinked path point to the same target.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/sdk/typescript/src/workspace-seeder.ts, line 134:
<comment>The new global `visited` set can skip legitimate directory aliases, making seeded file paths nondeterministic when both a real dir and symlinked path point to the same target.</comment>
<file context>
@@ -117,9 +117,22 @@ function collectSeedPaths(
+ // Cycle guard: a symlinked directory pointed back to an ancestor.
+ return;
+ }
+ visited.add(realDir);
+
const entries = fs.readdirSync(absoluteDir, { withFileTypes: true });
</file context>
| if (mountProc) { | ||
| await stopMountProcess(mountProc).catch(() => undefined); | ||
| } | ||
| await cleanupMountPoint(); |
There was a problem hiding this comment.
P2: The stop() method is not safe for concurrent callers. After setting stopped = true, the async cleanup (stopMountProcess + cleanupMountPoint) is in-flight, but a second await handle.stop() will hit the early return and resolve before the first call finishes teardown. Memoize the shutdown promise so all callers await the same completion:
let stopPromise: Promise<void> | undefined;
async stop() {
if (!stopPromise) {
stopPromise = (async () => {
await stopMountProcess(mountProc).catch(() => undefined);
await cleanupMountPoint();
})();
}
await stopPromise;
}Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/sdk/typescript/src/workspace-mount.ts, line 412:
<comment>The `stop()` method is not safe for concurrent callers. After setting `stopped = true`, the async cleanup (`stopMountProcess` + `cleanupMountPoint`) is in-flight, but a second `await handle.stop()` will hit the early `return` and resolve before the first call finishes teardown. Memoize the shutdown promise so all callers await the same completion:
```ts
let stopPromise: Promise<void> | undefined;
async stop() {
if (!stopPromise) {
stopPromise = (async () => {
await stopMountProcess(mountProc).catch(() => undefined);
await cleanupMountPoint();
})();
}
await stopPromise;
}
```</comment>
<file context>
@@ -392,13 +409,13 @@ export async function ensureRelayfileMount(config: MountConfig): Promise<MountHa
await stopMountProcess(mountProc).catch(() => undefined);
}
- await rm(mountPoint, { recursive: true, force: true }).catch(() => undefined);
+ await cleanupMountPoint();
const message = error instanceof Error ? error.message : String(error);
throw new Error(`${startupPhase} failed for ${config.workspace}: ${message}`);
</file context>
|
Pushed a follow-up batch to take the PR from red to green and address the AI reviewer findings:
Remaining open items: the two cubic P2s (HTTPS download timeouts and large-file streaming for the seeder) and the CodeRabbit nitpick to add |
1 participant
Summary
src/workspace-seeder.ts(createWorkspaceIfNeeded, seedAclRules, seedWorkspace, seedWorkflowAcls, seedWorkspaceTar) andsrc/workspace-mount.ts(ensureRelayfileMount + MountConfig/MountHandle).@relayfile/sdk/workspace-seederand@relayfile/sdk/workspace-mountsubpaths.@relayfile/sdkto 0.8.0 (additive public API).Why
These helpers have always been RelayFileClient wrappers + relayfile-mount binary lifecycle helpers. They were sitting in
@agent-relay/sdkonly because the original consumer (the workflow runner) lived there. We're untangling that: workflows move to@relayfile/sdkfor relayfile primitives, agent-relay shrinks to broker concerns, and@relayflows/coreconsumes both.Once this merges and a new
@relayfile/sdk@0.8.0ships, the matching changes in agent-relay (drop the in-package seeder/mount) and relayflows (depend on the new exports) can land.Test plan
npm --prefix packages/sdk/typescript run typecheck✓ (already passes locally)npm --prefix packages/sdk/typescript test(no new tests added — these files were already battle-tested in agent-relay; existing relayfile suites should be unaffected)🤖 Generated with Claude Code