fix: sandbox reliability — push org changes, skip redundant setup#140
Open
sidneyswift wants to merge 8 commits intomainfrom
Open
fix: sandbox reliability — push org changes, skip redundant setup#140sidneyswift wants to merge 8 commits intomainfrom
sidneyswift wants to merge 8 commits intomainfrom
Conversation
pushOrgRepos only checked for .git directories (-d) but org repos cloned as submodules use .git files (gitlinks). This caused "No org repos found" on every run, silently dropping all customer changes to artist directories. Matches the pattern already used in addOrgSubmodules.ts. Made-with: Cursor
📝 WalkthroughWalkthroughThis change refactors the skill installation flow from individual Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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.
🧹 Nitpick comments (1)
src/sandboxes/git/pushOrgRepos.ts (1)
33-33: Optional hardening: quoteworkspaceOrgsand avoidxargsparsing.Current command works, but quoting and using
-exec basenameis safer for odd path characters.♻️ Suggested robustness tweak
- `find ${workspaceOrgs} -mindepth 1 -maxdepth 1 -type d '(' -exec test -d {}/.git ';' -o -exec test -f {}/.git ';' ')' -print 2>/dev/null | xargs -I{} basename {}`, + `find "${workspaceOrgs}" -mindepth 1 -maxdepth 1 -type d '(' -exec test -d {}/.git ';' -o -exec test -f {}/.git ';' ')' -exec basename {} ';' 2>/dev/null`,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sandboxes/git/pushOrgRepos.ts` at line 33, The command string that builds the find invocation in pushOrgRepos.ts should quote the workspaceOrgs expansion and avoid xargs parsing: change the template literal from using ${workspaceOrgs} unquoted and piping to xargs to quoting "${workspaceOrgs}" and using find's -exec basename {} \; (or -exec sh -c 'basename "$1"' _ {} \;) to safely handle paths with spaces/newlines; update the string in the function/variable that contains the backticked command so it uses quoted "${workspaceOrgs}" and -exec basename instead of | xargs -I{} basename {}.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/sandboxes/git/pushOrgRepos.ts`:
- Line 33: The command string that builds the find invocation in pushOrgRepos.ts
should quote the workspaceOrgs expansion and avoid xargs parsing: change the
template literal from using ${workspaceOrgs} unquoted and piping to xargs to
quoting "${workspaceOrgs}" and using find's -exec basename {} \; (or -exec sh -c
'basename "$1"' _ {} \;) to safely handle paths with spaces/newlines; update the
string in the function/variable that contains the backticked command so it uses
quoted "${workspaceOrgs}" and -exec basename instead of | xargs -I{} basename
{}.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 92eb2692-5ea0-46bc-bba1-0c9a8be5db77
📒 Files selected for processing (1)
src/sandboxes/git/pushOrgRepos.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file
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="src/sandboxes/git/pushOrgRepos.ts">
<violation number="1" location="src/sandboxes/git/pushOrgRepos.ts:33">
P2: Custom agent: **Flag AI Slop and Fabricated Changes**
Bug-fix PR with no regression test. The tests already inspect the `find` command string (see "uses resolved home dir" test), so asserting that the command includes `test -f {}/.git` would be a one-line addition that directly prevents this gitlink-detection bug from recurring.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
| args: [ | ||
| "-c", | ||
| `find ${workspaceOrgs} -mindepth 1 -maxdepth 1 -type d -exec test -d {}/.git \\; -print 2>/dev/null | xargs -I{} basename {}`, | ||
| `find ${workspaceOrgs} -mindepth 1 -maxdepth 1 -type d '(' -exec test -d {}/.git ';' -o -exec test -f {}/.git ';' ')' -print 2>/dev/null | xargs -I{} basename {}`, |
There was a problem hiding this comment.
P2: Custom agent: Flag AI Slop and Fabricated Changes
Bug-fix PR with no regression test. The tests already inspect the find command string (see "uses resolved home dir" test), so asserting that the command includes test -f {}/.git would be a one-line addition that directly prevents this gitlink-detection bug from recurring.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/sandboxes/git/pushOrgRepos.ts, line 33:
<comment>Bug-fix PR with no regression test. The tests already inspect the `find` command string (see "uses resolved home dir" test), so asserting that the command includes `test -f {}/.git` would be a one-line addition that directly prevents this gitlink-detection bug from recurring.</comment>
<file context>
@@ -30,7 +30,7 @@ export async function pushOrgRepos(
args: [
"-c",
- `find ${workspaceOrgs} -mindepth 1 -maxdepth 1 -type d -exec test -d {}/.git \\; -print 2>/dev/null | xargs -I{} basename {}`,
+ `find ${workspaceOrgs} -mindepth 1 -maxdepth 1 -type d '(' -exec test -d {}/.git ';' -o -exec test -f {}/.git ';' ')' -print 2>/dev/null | xargs -I{} basename {}`,
],
});
</file context>
Asserts the find command checks for .git files (test -f), not just .git directories, preventing this bug from recurring. Made-with: Cursor
- Add RECOUP.md check to ensureSetupSandbox — skips skills install and agent runs when sandbox is already set up (~3-7 min saved per run) - Remove runSetupArtistSkill (skill renamed to artist-workspace, and the setup-sandbox skill already handles the handoff) - Replace 3 individual installSkill calls with single installSkills from recoupable/skills repo - Delete old installSkill.ts Made-with: Cursor
sidneyswift
changed the title
Made-with: Cursor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/sandboxes/__tests__/pushOrgRepos.test.ts`:
- Around line 193-198: The test currently only asserts that the generated shell
args include "test -f {}/.git" so it would miss regressions that remove the
"test -d {}/.git" probe; update the assertion that inspects
sandbox.runCommand.mock.calls (the findCall variable) to assert that
findCall[0].args[1] contains both "test -f {}/.git" and "test -d {}/.git" (e.g.,
two expect(...).toContain(...) checks or a single check that both substrings are
present) so both file and directory probes are enforced.
In `@src/sandboxes/ensureSetupSandbox.ts`:
- Around line 18-26: The current skip logic in ensureSetupSandbox.ts uses
sandbox.runCommand with a glob check for any RECOUP.md (variable check) which is
too coarse and can falsely skip setup for partial or stale snapshots; replace
this with a dedicated sandbox-level completion marker/version (e.g.,
".sandbox_setup_complete" or ".sandbox_setup_v1") and change the early-return
logic to check that marker (read/inspect the file via the sandbox API) and,
optionally, validate the expected org/artist set before skipping; update the
logic around sandbox.runCommand/check and logStep to only return early when the
marker exists and is the expected version (or when a full validation of
orgs/artists passes).
In `@src/sandboxes/installSkills.ts`:
- Around line 34-57: The code currently logs and returns when skill installation
or copying fails (checking install.exitCode and copy.exitCode after
sandbox.runCommand), which lets ensureSetupSandbox continue and later fail;
instead, fail fast by throwing a descriptive Error (including source and the
captured stderr like installStderr or copyStderr) when install.exitCode !== 0 or
copy.exitCode !== 0 so the higher-level flow aborts immediately; update the
branches around install.exitCode and copy.exitCode (and remove the current
logger.warn-only behavior) to throw errors that include context from the failing
sandbox.runCommand calls.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7646bdae-dddd-44cd-a4ea-94f90b85055c
📒 Files selected for processing (6)
src/sandboxes/__tests__/installSkills.test.tssrc/sandboxes/__tests__/pushOrgRepos.test.tssrc/sandboxes/ensureSetupSandbox.tssrc/sandboxes/installSkill.tssrc/sandboxes/installSkills.tssrc/sandboxes/runSetupArtistSkill.ts
💤 Files with no reviewable changes (2)
- src/sandboxes/runSetupArtistSkill.ts
- src/sandboxes/installSkill.ts
Comment on lines
+193
to
+198
| const findCall = sandbox.runCommand.mock.calls.find( | ||
| (call: any[]) => | ||
| call[0]?.cmd === "sh" && call[0]?.args?.[1]?.includes("find") | ||
| ); | ||
| expect(findCall![0].args[1]).toContain("test -f {}/.git"); | ||
| }); |
There was a problem hiding this comment.
Strengthen this regression by asserting both .git directory and .git file probes.
This test only guards test -f {}/.git; it would still pass if test -d {}/.git were accidentally removed.
Proposed test hardening
const findCall = sandbox.runCommand.mock.calls.find(
(call: any[]) =>
call[0]?.cmd === "sh" && call[0]?.args?.[1]?.includes("find")
);
- expect(findCall![0].args[1]).toContain("test -f {}/.git");
+ expect(findCall).toBeDefined();
+ const findCmd = findCall![0].args[1];
+ expect(findCmd).toContain("test -d {}/.git");
+ expect(findCmd).toContain("test -f {}/.git");
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const findCall = sandbox.runCommand.mock.calls.find( | |
| (call: any[]) => | |
| call[0]?.cmd === "sh" && call[0]?.args?.[1]?.includes("find") | |
| ); | |
| expect(findCall![0].args[1]).toContain("test -f {}/.git"); | |
| }); | |
| const findCall = sandbox.runCommand.mock.calls.find( | |
| (call: any[]) => | |
| call[0]?.cmd === "sh" && call[0]?.args?.[1]?.includes("find") | |
| ); | |
| expect(findCall).toBeDefined(); | |
| const findCmd = findCall![0].args[1]; | |
| expect(findCmd).toContain("test -d {}/.git"); | |
| expect(findCmd).toContain("test -f {}/.git"); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/sandboxes/__tests__/pushOrgRepos.test.ts` around lines 193 - 198, The
test currently only asserts that the generated shell args include "test -f
{}/.git" so it would miss regressions that remove the "test -d {}/.git" probe;
update the assertion that inspects sandbox.runCommand.mock.calls (the findCall
variable) to assert that findCall[0].args[1] contains both "test -f {}/.git" and
"test -d {}/.git" (e.g., two expect(...).toContain(...) checks or a single check
that both substrings are present) so both file and directory probes are
enforced.
Comment on lines
+18
to
+26
| const check = await sandbox.runCommand({ | ||
| cmd: "sh", | ||
| args: ["-c", "ls orgs/*/artists/*/RECOUP.md 2>/dev/null | head -1"], | ||
| }); | ||
|
|
||
| if (check.exitCode === 0 && ((await check.stdout()) || "").trim()) { | ||
| logStep("Sandbox already set up, skipping", false); | ||
| return; | ||
| } |
There was a problem hiding this comment.
The skip check is too coarse for partial or stale snapshots.
A single RECOUP.md only proves that some artist was set up once. If the snapshot is partial, or the account has gained new orgs/artists since the snapshot was created, this returns early and leaves the missing folders uncreated. Consider using a dedicated sandbox-level completion marker/version, or validating the expected org/artist set before skipping.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/sandboxes/ensureSetupSandbox.ts` around lines 18 - 26, The current skip
logic in ensureSetupSandbox.ts uses sandbox.runCommand with a glob check for any
RECOUP.md (variable check) which is too coarse and can falsely skip setup for
partial or stale snapshots; replace this with a dedicated sandbox-level
completion marker/version (e.g., ".sandbox_setup_complete" or
".sandbox_setup_v1") and change the early-return logic to check that marker
(read/inspect the file via the sandbox API) and, optionally, validate the
expected org/artist set before skipping; update the logic around
sandbox.runCommand/check and logStep to only return early when the marker exists
and is the expected version (or when a full validation of orgs/artists passes).
Comment on lines
+34
to
+57
| if (install.exitCode !== 0) { | ||
| logger.warn("Skills install failed, continuing without them", { | ||
| source, | ||
| stderr: installStderr, | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| const copy = await sandbox.runCommand({ | ||
| cmd: "sh", | ||
| args: [ | ||
| "-c", | ||
| "mkdir -p ~/.openclaw/workspace/skills && cp -r .agents/skills/* ~/.openclaw/workspace/skills/", | ||
| ], | ||
| }); | ||
|
|
||
| const copyStderr = (await copy.stderr()) || ""; | ||
|
|
||
| if (copy.exitCode !== 0) { | ||
| logger.warn("Failed to copy skills to OpenClaw workspace, continuing without them", { | ||
| source, | ||
| stderr: copyStderr, | ||
| }); | ||
| return; |
There was a problem hiding this comment.
Fail fast when required skill setup fails.
In the current ensureSetupSandbox flow, returning here means we still try to run /setup-sandbox without the skills being available in OpenClaw. That turns an install/copy failure into a later agent failure and hides the real cause.
Proposed fix
if (install.exitCode !== 0) {
- logger.warn("Skills install failed, continuing without them", {
+ logger.error("Skills install failed", {
source,
stderr: installStderr,
});
- return;
+ throw new Error(`Failed to install skills from ${source}`);
}
@@
if (copy.exitCode !== 0) {
- logger.warn("Failed to copy skills to OpenClaw workspace, continuing without them", {
+ logger.error("Failed to copy skills to OpenClaw workspace", {
source,
stderr: copyStderr,
});
- return;
+ throw new Error("Failed to copy skills to OpenClaw workspace");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (install.exitCode !== 0) { | |
| logger.warn("Skills install failed, continuing without them", { | |
| source, | |
| stderr: installStderr, | |
| }); | |
| return; | |
| } | |
| const copy = await sandbox.runCommand({ | |
| cmd: "sh", | |
| args: [ | |
| "-c", | |
| "mkdir -p ~/.openclaw/workspace/skills && cp -r .agents/skills/* ~/.openclaw/workspace/skills/", | |
| ], | |
| }); | |
| const copyStderr = (await copy.stderr()) || ""; | |
| if (copy.exitCode !== 0) { | |
| logger.warn("Failed to copy skills to OpenClaw workspace, continuing without them", { | |
| source, | |
| stderr: copyStderr, | |
| }); | |
| return; | |
| if (install.exitCode !== 0) { | |
| logger.error("Skills install failed", { | |
| source, | |
| stderr: installStderr, | |
| }); | |
| throw new Error(`Failed to install skills from ${source}`); | |
| } | |
| const copy = await sandbox.runCommand({ | |
| cmd: "sh", | |
| args: [ | |
| "-c", | |
| "mkdir -p ~/.openclaw/workspace/skills && cp -r .agents/skills/* ~/.openclaw/workspace/skills/", | |
| ], | |
| }); | |
| const copyStderr = (await copy.stderr()) || ""; | |
| if (copy.exitCode !== 0) { | |
| logger.error("Failed to copy skills to OpenClaw workspace", { | |
| source, | |
| stderr: copyStderr, | |
| }); | |
| throw new Error("Failed to copy skills to OpenClaw workspace"); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/sandboxes/installSkills.ts` around lines 34 - 57, The code currently logs
and returns when skill installation or copying fails (checking install.exitCode
and copy.exitCode after sandbox.runCommand), which lets ensureSetupSandbox
continue and later fail; instead, fail fast by throwing a descriptive Error
(including source and the captured stderr like installStderr or copyStderr) when
install.exitCode !== 0 or copy.exitCode !== 0 so the higher-level flow aborts
immediately; update the branches around install.exitCode and copy.exitCode (and
remove the current logger.warn-only behavior) to throw errors that include
context from the failing sandbox.runCommand calls.
The RECOUP.md check used a relative `orgs/*/artists/*/RECOUP.md` glob which only works if the sandbox cwd is the OpenClaw workspace. The sandbox cwd is /vercel/sandbox, so the check never matched and setup re-ran every time (~7 min wasted). Use getSandboxHomeDir to build the correct absolute path. Made-with: Cursor
Staging test shows org repos still not found even with gitlink fix. Adding debug output to see what's actually on disk at the workspace orgs path — ls -la, find .git entries, and resolved HOME/CWD. Made-with: Cursor
The OpenClaw agent was told to clone org repos but didn't reliably handle directories that exist without .git entries (common after snapshot restore). Replaced the AI-delegated clone with deterministic sandbox.runCommand calls that check for .git, pull if present, or rm + clone fresh if missing. Also removed debug logging from pushOrgRepos. Made-with: Cursor
Revert git/org repo fixes (gitlink detection, deterministic clone, setup skip check) to keep this PR focused on updating skills in task runs. Remaining changes: - installSkill → installSkills (one-shot from recoupable/skills) - Remove runSetupArtistSkill (setup-sandbox skill handles it) - ensureSetupSandbox uses installSkills, drops setup-artist call Made-with: Cursor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/sandboxes/ensureSetupSandbox.ts (1)
17-31:⚠️ Potential issue | 🟠 MajorAdd an idempotent setup guard before rerunning sandbox setup.
Line 17 through Line 31 now execute setup every run. This removes the snapshot fast-path and can repeatedly spend setup time on already-provisioned sandboxes.
💡 Proposed fix (marker-based skip + mark-on-success)
export async function ensureSetupSandbox( sandbox: Sandbox, accountId: string ): Promise<void> { + const setupMarker = await sandbox.runCommand({ + cmd: "sh", + args: ["-c", `test -f ~/.openclaw/workspace/.sandbox_setup_${accountId}_v1`], + }); + + if (setupMarker.exitCode === 0) { + logStep("Sandbox already set up, skipping", false); + return; + } + + if (!process.env.RECOUP_API_KEY) { + throw new Error("Missing RECOUP_API_KEY environment variable"); + } + logStep("Installing skills"); await installSkills(sandbox, "recoupable/skills"); - if (!process.env.RECOUP_API_KEY) { - throw new Error("Missing RECOUP_API_KEY environment variable"); - } - const env = { RECOUP_API_KEY: process.env.RECOUP_API_KEY, RECOUP_ACCOUNT_ID: accountId, }; logStep("Running setup-sandbox skill"); await runSetupSandboxSkill(sandbox, env); + await sandbox.runCommand({ + cmd: "sh", + args: ["-c", `touch ~/.openclaw/workspace/.sandbox_setup_${accountId}_v1`], + }); logStep("Setup-sandbox complete", false); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sandboxes/ensureSetupSandbox.ts` around lines 17 - 31, Add an idempotent guard around the setup steps so setup is skipped if already done: before calling installSkills and runSetupSandboxSkill, check for a persistent marker on the sandbox (e.g., a marker file, metadata flag, or sandbox.getMetadata()/readFile call) and return early if present; if setup proceeds and completes successfully, write the same marker (e.g., createMarkerOnSandbox or sandbox.writeFile/metadata update) so subsequent runs detect it. Update the block that uses installSkills(sandbox, "recoupable/skills") and runSetupSandboxSkill(sandbox, env) to implement this check-and-mark pattern and ensure errors still surface if marker creation fails.
🧹 Nitpick comments (1)
src/sandboxes/ensureSetupSandbox.ts (1)
17-21: Fail fast before running skills installation.Line 18 runs a potentially expensive command before Line 20 validates
RECOUP_API_KEY. Move the env check ahead of install to avoid unnecessary setup work on invalid runs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sandboxes/ensureSetupSandbox.ts` around lines 17 - 21, The RECOUP_API_KEY environment check should run before performing any expensive setup; move the validation that throws the Error("Missing RECOUP_API_KEY environment variable") to precede the call to installSkills(sandbox, "recoupable/skills") in ensureSetupSandbox so the function fails fast. In practice, update the ensureSetupSandbox flow to call the RECOUP_API_KEY guard first (throwing the same error if missing) and only then execute logStep("Installing skills") and await installSkills(...).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/sandboxes/ensureSetupSandbox.ts`:
- Around line 17-31: Add an idempotent guard around the setup steps so setup is
skipped if already done: before calling installSkills and runSetupSandboxSkill,
check for a persistent marker on the sandbox (e.g., a marker file, metadata
flag, or sandbox.getMetadata()/readFile call) and return early if present; if
setup proceeds and completes successfully, write the same marker (e.g.,
createMarkerOnSandbox or sandbox.writeFile/metadata update) so subsequent runs
detect it. Update the block that uses installSkills(sandbox,
"recoupable/skills") and runSetupSandboxSkill(sandbox, env) to implement this
check-and-mark pattern and ensure errors still surface if marker creation fails.
---
Nitpick comments:
In `@src/sandboxes/ensureSetupSandbox.ts`:
- Around line 17-21: The RECOUP_API_KEY environment check should run before
performing any expensive setup; move the validation that throws the
Error("Missing RECOUP_API_KEY environment variable") to precede the call to
installSkills(sandbox, "recoupable/skills") in ensureSetupSandbox so the
function fails fast. In practice, update the ensureSetupSandbox flow to call the
RECOUP_API_KEY guard first (throwing the same error if missing) and only then
execute logStep("Installing skills") and await installSkills(...).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0cfa8bff-582a-46f5-a6e7-d3f5f12378f0
📒 Files selected for processing (1)
src/sandboxes/ensureSetupSandbox.ts
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
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="src/sandboxes/git/pushOrgRepos.ts">
<violation number="1">
P0: Custom agent: **Flag AI Slop and Fabricated Changes**
The PR claims this change adds `.git` file (gitlink) detection to align with `addOrgSubmodules.ts`, but the code does the **opposite**: it removes the existing `-f {}/.git` check. The old line already matched `addOrgSubmodules.ts` exactly (checking both `-d` and `-f`). The new line drops the `-o -exec test -f {}/.git` clause, meaning gitlink-based org repos will no longer be found — the very regression the PR narrative says it fixes.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
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="src/sandboxes/ensureOrgRepos.ts">
<violation number="1">
P2: Missing error handling for `git pull`. Unlike the `git clone` path below which checks `exitCode` and logs on failure, pull failures are silently ignored. A failed pull (merge conflict, network error, diverged history) would leave the repo in an unexpected state with no indication in the logs.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Three issues affecting customer experience in sandbox runs:
Org repo changes silently dropped —
pushOrgReposonly checked for.gitdirectories but org repos use.gitfiles (gitlinks). Every run logged "No org repos found" and skipped pushing customer work. Confirmed from production run logs — customer updated a release timeline, waited 10 min, changes never reached GitHub.Redundant setup on every run (~3-7 min wasted) —
ensureSetupSandboxre-ran skills install + setup agents every time even when the sandbox was already provisioned from a snapshot. No skip check was implemented despite the JSDoc claiming one existed.Old setup-artist skill still running — skill was renamed to
artist-workspaceand the old standalone repo archived, butrunSetupArtistSkillwas still being called.Fixes
.gitdirectories and.gitfiles (gitlinks), matching the pattern already inaddOrgSubmodules.ts. Added regression test.RECOUP.mdfiles in artist dirs before running setup. If they exist, skip skills install + agent runs entirely.setup-sandboxskill already handles the handoff toartist-workspace.recoupable/skillsin one shot instead of 3 individual archived repos.How to verify
npx trigger.dev@4.3.3 deploy --env stagingrun-sandbox-commandfor an account with existing org reposEvidence
run_cmnz65esncdmo0inahle27pw8(customer): 10m 37s total, 23s of actual work, "No org repos found" in push logsrecoupable/org-rostrum-pacific-*: no commits after Apr 8 despite customer activity on Apr 14Summary by CodeRabbit
Refactor
Tests