Create parent dir before writing entry-point scripts#377
Draft
jeanschmidt wants to merge 1 commit into
Draft
Conversation
- Add fs.mkdirSync(path.dirname(...), { recursive: true }) before each
of the three writeFileSync calls in prepareJobScript, writeRunScript,
and writeContainerStepScript.
- Add explicit RUNNER_TEMP guard at the top of prepareJobScript and
writeRunScript so a missing env var still fails fast instead of
creating a literal "undefined/" directory.
- Add tests covering all three writers with a non-existent parent dir.
Notes:
fs.writeFileSync throws ENOENT when the target file's parent directory
does not exist. In normal job flows the parent is created early, but
cleanup races, custom RUNNER_TEMP overrides, and fresh container-step
dst directories can leave it missing — surfacing as a confusing,
indirect failure. The mkdirSync call is cheap, idempotent, and removes
the failure mode entirely.
The explicit RUNNER_TEMP guard preserves the prior contract: before
this change, "RUNNER_TEMP unset" failed implicitly via ENOENT on
"undefined/<uuid>.sh". With mkdirSync added that path would silently
create a literal "undefined/" directory in cwd, so the guard is
required to keep the failure visible.
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds defensive directory creation when writing runner/container scripts to ensure missing parent directories don’t cause failures, and introduces tests covering these scenarios.
Changes:
- Ensure
RUNNER_TEMPis validated and its directory exists before writing scripts. - Ensure destination directories exist before writing container step scripts.
- Add filesystem-based tests to verify parent directory creation behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| packages/k8s/src/k8s/utils.ts | Creates required parent directories (and validates RUNNER_TEMP) before writing script files. |
| packages/k8s/tests/k8s-utils-test.ts | Adds tests that assert the above directory-creation behavior using temp directories. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+131
to
+137
| beforeEach(() => { | ||
| tempRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'pr05-')) | ||
| }) | ||
|
|
||
| afterEach(() => { | ||
| fs.rmSync(tempRoot, { recursive: true, force: true }) | ||
| }) |
| it('writeRunScript creates RUNNER_TEMP parent directory when missing', () => { | ||
| const missingDir = path.join(tempRoot, 'nested', 'runner_temp') | ||
| expect(fs.existsSync(missingDir)).toBe(false) | ||
| process.env.RUNNER_TEMP = missingDir |
| it('prepareJobScript creates RUNNER_TEMP parent directory when missing', () => { | ||
| const missingDir = path.join(tempRoot, 'nested', 'runner_temp') | ||
| expect(fs.existsSync(missingDir)).toBe(false) | ||
| process.env.RUNNER_TEMP = missingDir |
Comment on lines
+49
to
+55
| const runnerTemp = process.env.RUNNER_TEMP | ||
| if (!runnerTemp) { | ||
| throw new Error('RUNNER_TEMP is not set') | ||
| } | ||
| const filename = `${uuidv4()}.sh` | ||
| const entryPointPath = `${process.env.RUNNER_TEMP}/${filename}` | ||
| const entryPointPath = `${runnerTemp}/${filename}` | ||
| fs.mkdirSync(path.dirname(entryPointPath), { recursive: true }) |
Comment on lines
93
to
+95
| const filename = `${uuidv4()}.sh` | ||
| const entryPointPath = `${process.env.RUNNER_TEMP}/${filename}` | ||
| const entryPointPath = `${runnerTemp}/${filename}` | ||
| fs.mkdirSync(path.dirname(entryPointPath), { recursive: true }) |
Comment on lines
129
to
+131
| const entryPointPath = `${dst}/${filename}` | ||
| core.debug(`Writing container step script to ${entryPointPath}`) | ||
| fs.mkdirSync(path.dirname(entryPointPath), { recursive: true }) |
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
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.
Create parent dir before writing entry-point scripts
Impact: k8s hook users — eliminates a class of indirect ENOENT failures when writing job/run/step entry-point scripts.
Risk: low
What
Defensively
mkdir -pthe parent directory before eachfs.writeFileSynccall that writes an entry-point script inpackages/k8s/src/k8s/utils.ts, and add an explicitRUNNER_TEMPguard to the two writers that depend on it.Why
fs.writeFileSyncthrowsENOENTwhen the target file's parent directory does not exist. In normal flows the parent is created early in the job, but cleanup races, customRUNNER_TEMPoverrides, and fresh container-stepdstdirectories can leave it missing. The resultingENOENTsurfaces as a confusing, indirect failure far from the real cause.How
fs.mkdirSync(path.dirname(entryPointPath), { recursive: true })immediately before each of the threewriteFileSynccalls. The call is cheap and idempotent.RUNNER_TEMPis unset" relied implicitly onwriteFileSyncrejectingundefined/<uuid>.sh. WithmkdirSyncadded, that path would silently create a literalundefined/directory in the cwd. To preserve the prior failure contract, an explicitif (!runnerTemp) throwguard is added at the top ofprepareJobScriptandwriteRunScript.writeContainerStepScriptneeds no guard becausedstis a required parameter.Changes
packages/k8s/src/k8s/utils.ts:path.prepareJobScript,writeRunScript: explicitRUNNER_TEMPguard,mkdirSync(..., { recursive: true })before write.writeContainerStepScript:mkdirSync(..., { recursive: true })before write.packages/k8s/tests/k8s-utils-test.ts: newdefensive parent-dir creationdescribe block with one test per writer, each pointing at a non-existent nested directory and asserting both the directory and the script file exist after the call.