fix: create parent directory before moving workflow clone#1520
Open
guzalv wants to merge 3 commits intoambient-code:mainfrom
Open
fix: create parent directory before moving workflow clone#1520guzalv wants to merge 3 commits intoambient-code:mainfrom
guzalv wants to merge 3 commits intoambient-code:mainfrom
Conversation
When loading a custom workflow without a subpath, the /workspace/workflows/ parent directory was never created before the mv/shutil.move call. This caused the move to fail silently (hydrate.sh has set +e, workflow.py catches exceptions), and the finally block then deleted the successfully cloned temp directory. The runner later created an empty directory as the CWD, making it appear the workflow loaded but was empty. The subpath code paths already had the mkdir -p / parent.mkdir() calls; this adds them to the non-subpath paths in both hydrate.sh and workflow.py. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
✅ Deploy Preview for cheerful-kitten-f556a0 canceled.
|
Contributor
📝 WalkthroughWalkthroughAdjusts runtime workflow placement and hydration behavior: both the ambient-runner endpoint and the state-sync script now ensure the destination parent directory exists before placing a cloned or extracted workflow. The Python endpoint also switches to moving the temporary clone into the final path for certain missing-subpath cases, and the shell hydration now only attempts S3 restore when ChangesWorkflow Destination & Hydration
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (7 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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}
Contributor
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)
components/runners/state-sync/hydrate.sh (1)
339-345:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing
mkdir -pbeforemvin the subpath-not-found fallback.The subpath-found branch (line 334) and the no-subpath branch (line 348) both call
mkdir -p "$(dirname "$WORKFLOW_FINAL")"before moving/copying, but the subpath-not-found fallback at line 343 does not. Sinceset +eis active here, themvwill silently fail if/workspace/workflows/doesn't exist yet.🐛 Proposed fix
echo " Using entire repo instead" + mkdir -p "$(dirname "$WORKFLOW_FINAL")" mv "$WORKFLOW_TEMP" "$WORKFLOW_FINAL"🤖 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 `@components/runners/state-sync/hydrate.sh` around lines 339 - 345, The fallback branch that handles a missing subpath moves the entire cloned repo with mv "$WORKFLOW_TEMP" "$WORKFLOW_FINAL" but does not ensure the destination parent exists, so mv can silently fail under set +e; add a mkdir -p "$(dirname "$WORKFLOW_FINAL")" immediately before the mv in that branch (the same pattern used in the subpath-found and no-subpath branches) to guarantee the parent directory for WORKFLOW_FINAL exists before moving WORKFLOW_TEMP into place.
🤖 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 `@components/runners/state-sync/hydrate.sh`:
- Around line 339-345: The fallback branch that handles a missing subpath moves
the entire cloned repo with mv "$WORKFLOW_TEMP" "$WORKFLOW_FINAL" but does not
ensure the destination parent exists, so mv can silently fail under set +e; add
a mkdir -p "$(dirname "$WORKFLOW_FINAL")" immediately before the mv in that
branch (the same pattern used in the subpath-found and no-subpath branches) to
guarantee the parent directory for WORKFLOW_FINAL exists before moving
WORKFLOW_TEMP into place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2420c208-5c06-4cce-90e1-6813a4e8e7fc
📒 Files selected for processing (2)
components/runners/ambient-runner/ambient_runner/endpoints/workflow.pycomponents/runners/state-sync/hydrate.sh
The subpath-fallback branch (when a subpath is specified but not found in the cloned repo) also lacked mkdir -p before mv, same root cause. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The init container previously called exit 0 when S3 credentials were missing, which skipped all git cloning of repos and workflows. Repo and workflow cloning are independent of S3 state persistence and should always run. Replace the early exit with a conditional block around S3-specific operations (rclone setup, state download, repo state restore), while leaving git credential setup, repo cloning, and workflow cloning unconditional. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
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)
components/runners/state-sync/hydrate.sh (1)
469-471:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing
chown/chmodfor/workspace/workflowsafter cloning.
/workspace/workflows(and its contents) is created underset +eby the newmkdir -pcalls, but only/workspace/reposis covered in the final permissions block. The existing comment at lines 87–92 explains exactly why repos needchmod 777(init container = root, runner = uid 1001, no shared fsGroup). The same applies here: if the runner writes anything into the workflow directory at runtime (e.g.__pycache__, PID files, generated config), it will get a permission denial.🔧 Proposed fix
# Set permissions on repos after restore (repos may have been cloned or restored) chown -R 1001:0 /workspace/repos 2>/dev/null || true chmod -R 777 /workspace/repos 2>/dev/null || true + +# Set permissions on workflows (same reasoning as repos — init container runs as root) +chown -R 1001:0 /workspace/workflows 2>/dev/null || true +chmod -R 777 /workspace/workflows 2>/dev/null || 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 `@components/runners/state-sync/hydrate.sh` around lines 469 - 471, The permissions block currently only adjusts /workspace/repos; add the same chown -R 1001:0 and chmod -R 777 commands for /workspace/workflows so the runner (uid 1001) can write runtime files there; update the final permissions section that contains chown -R 1001:0 /workspace/repos and chmod -R 777 /workspace/repos to also perform chown -R 1001:0 /workspace/workflows 2>/dev/null || true and chmod -R 777 /workspace/workflows 2>/dev/null || true.
🤖 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 `@components/runners/state-sync/hydrate.sh`:
- Around line 469-471: The permissions block currently only adjusts
/workspace/repos; add the same chown -R 1001:0 and chmod -R 777 commands for
/workspace/workflows so the runner (uid 1001) can write runtime files there;
update the final permissions section that contains chown -R 1001:0
/workspace/repos and chmod -R 777 /workspace/repos to also perform chown -R
1001:0 /workspace/workflows 2>/dev/null || true and chmod -R 777
/workspace/workflows 2>/dev/null || true.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4ca458f6-93a3-44c7-9122-3d36742b7c8d
📒 Files selected for processing (1)
components/runners/state-sync/hydrate.sh
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.
Summary
Custom workflows loaded via "Load custom" (git URL + branch, no subpath) silently fail — the session starts with an empty workflow directory. None of the workflow's commands, system prompt, or
CLAUDE.mdare available to the agent.Root causes
1. Missing parent directory for workflow clone
The init container (
hydrate.sh) and runtime clone (workflow.py) both attempt to move the cloned workflow to/workspace/workflows/<name>, but the parent directory/workspace/workflows/is never created beforehand. Themkdir -pcall exists in the subpath code path but is missing from the non-subpath path (which is the common case — entire repo = the workflow). The move fails silently (set +ein hydrate.sh,exceptin workflow.py), and the runner later creates an empty directory as the CWD.2. Repo and workflow cloning coupled to S3 availability
hydrate.shcallsexit 0when S3 credentials are missing, which skips everything after line 98 — including all git credential setup, repo cloning, and workflow cloning. These operations are independent of S3 state persistence and should always run. This means any deployment without S3 configured (or with temporarily unavailable S3) gets no repos and no workflows.Who is affected
Changes
hydrate.shmkdir -p "$(dirname "$WORKFLOW_FINAL")"beforemvin the no-subpath and subpath-fallback branchesexit 0when S3 is unconfigured with a conditional block: S3-specific operations (rclone setup, state download, repo state restore) are skipped, but git credential setup, repo cloning, and workflow cloning always runworkflow.pyworkflow_final.parent.mkdir(parents=True, exist_ok=True)beforeshutil.move()in the non-subpath and subpath-fallback branches ofclone_workflow_at_runtime()Test plan
/workspace/workflows/<name>/is populatedmake kind-up LOCAL_IMAGES=truewith local builds of ambient-runner and state-sync🤖 Generated with Claude Code
Summary by CodeRabbit