exec.writeFile/addFile: { writable: true } opts knob#1648
Conversation
Adds an opt-in escape hatch for the 0o400 default fill-rule perms set in PR #1628. Passing { writable: true } to exec.builder().addFile, .writeFile (and the plural forms) lands the file as 0o600 in the workdir, forcing the backend to copy from the archive/value cache to a fresh inode instead of taking the hardlink fast path. Same option is mirrored on workdir.builder().addFile / writeFile (plus plural forms) — that's the layer that actually emits the fill-rule permissions field. exec-impl.tpl threads a writableFiles name set through settings and applies it per file. When no caller marks any file writable, the new settings field is omitted, so existing workflows keep the exact same exec CID.
🦋 Changeset detectedLatest commit: 8573c76 The changes in this PR will be included in the next version bump. This PR includes changesets to release 17 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a { writable: true } option for file operations within exec.builder and workdir.builder, allowing files to be landed with 0o600 permissions instead of the default 0o400. This is implemented by updating the underlying fill rules and propagating the option through the Tengo SDK. The reviewer feedback highlights several opportunities to improve the robustness of the code by adding type checks for optional arguments to prevent potential runtime errors. Additionally, it is recommended to add a writeFiles method to exec.builder to ensure API consistency with workdir.builder.
| * @param opts: { writable?: bool, mnz?: ... } - optional flags. | ||
| * `writable: true` lands the file as 0o600 in the workdir. | ||
| */ | ||
| writeFile: func(fileName, data, ...opts) { |
There was a problem hiding this comment.
The exec.builder is missing the writeFiles method, although it is explicitly mentioned in the pull request description and its counterpart addFiles is present. For consistency with workdir.builder and to support the plural form as described, please consider adding it. Since writeFile already handles canonical encoding for maps and arrays, writeFiles can be implemented by iterating over the input map and calling writeFile for each entry.
|
|
||
| fileName = path.canonize(fileName) | ||
| filesToAdd[fileName] = file | ||
| if len(opts) > 0 && !is_undefined(opts[0]) && opts[0].writable == true { |
There was a problem hiding this comment.
Accessing opts[0].writable without verifying that opts[0] is a map can lead to a runtime error if an unexpected type (e.g., a string or boolean) is passed as an optional argument. While the JSDoc specifies a map, adding a check like ll.isMap(opts[0]) would make the implementation more robust.
if len(opts) > 0 && ll.isMap(opts[0]) && opts[0].writable == true {
| fileName = path.canonize(fileName) | ||
|
|
||
| filesToWrite[fileName] = data | ||
| if len(opts) > 0 && !is_undefined(opts[0]) && opts[0].writable == true { |
There was a problem hiding this comment.
Accessing opts[0].writable without verifying that opts[0] is a map can lead to a runtime error if an unexpected type is passed. Adding a check like ll.isMap(opts[0]) ensures the builder remains stable even with incorrect usage.
if len(opts) > 0 && ll.isMap(opts[0]) && opts[0].writable == true {
| files[fileName] = fileResource | ||
| blobs[fileName] = fileResource | ||
| self._addDirs(fileName) | ||
| if len(opts) > 0 && !is_undefined(opts[0]) && opts[0].writable == true { |
There was a problem hiding this comment.
It is safer to verify that opts[0] is a map before attempting to access the writable property, preventing potential runtime errors if an invalid type is passed.
if len(opts) > 0 && ll.isMap(opts[0]) && opts[0].writable == true {
| validation.assertType(fileName, "string") | ||
| values[fileName] = contentRef | ||
| self._addDirs(fileName) | ||
| if len(opts) > 0 && !is_undefined(opts[0]) && opts[0].writable == true { |
There was a problem hiding this comment.
Adding a map type check for opts[0] before property access is recommended to ensure robustness against unexpected input types.
if len(opts) > 0 && ll.isMap(opts[0]) && opts[0].writable == true {
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (0.00%) is below the target coverage (50.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #1648 +/- ##
==========================================
+ Coverage 53.02% 53.04% +0.02%
==========================================
Files 270 270
Lines 15702 15730 +28
Branches 3391 3408 +17
==========================================
+ Hits 8326 8344 +18
- Misses 6260 6270 +10
Partials 1116 1116 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Address PR review: - Add exec.builder.writeFiles (mirrors addFiles) — the changeset and PR description advertised it, but only workdir.builder had it. - Replace `!is_undefined(opts[0])` with `ll.isMap(opts[0])` at all four opts-parsing sites (exec addFile/writeFile, workdir addFile/writeFile) so passing a non-map opts argument fails the guard cleanly instead of panicking on field access.
Tests ----- tests/workflow-tengo/src/exec/writable/pt_overwrite.tpl.tengo + writable.test.ts: parametrized over (mode: write|add, writable: bool). Drives ptabler with a workflow that reads data.tsv and writes back to the same path. RW case must succeed (file lands 0o600 → backend copies to fresh inode); RO case must fail with non-zero exit (file lands 0o400 hardlinked from archive cache → polars hits EACCES). Content roundtrip is intentionally not asserted because pt's write_csv truncates the target before reading the source fully; we only assert success vs error. Version gate ------------ The backend half of this feature lives in milaboratory/pl PR #1830, merged AFTER the v3.5.0 tag was cut. v3.5.0 release silently ignores the requested perm and always lands files at the canonical archive mode (0o400), so the test would spuriously fail. Added `supportsWritableWorkdirFiles` on `LLPlClient` (and re-exposed on `PlClient`) — returns true iff `serverInfo.coreVersion` is strictly after `3.5.0`. Dev builds past the tag like `3.5.0-224-g0ca182` count as after; the bare `3.5.0` release does not. Implemented via a new `isAfterVersion` helper alongside the existing `isVersionAtLeast`, which has different semantics for the exact-triplet case (we need the tagged release excluded; `isVersionAtLeast` includes it). The four test cases self-skip on backends that lack the capability, with a message identifying the backend version. Verified locally: source build (3.5.0-224-g0ca182) runs all 4 → pass. Prebuilt 3.5.0 runs all 4 → skip with descriptive reason.
Temporary: tests require pl container running as non-root user so OS file-mode enforcement is active (writable: false tests). Non-root behavior lives on github-ci v4-beta until promoted. Revert to @v4 once v4-beta is merged into v4.
1 participant
Summary
Opt-in escape hatch for the 0o400 default workdir fill-rule perms set in #1628. Passing
{ writable: true }toexec.builder().addFile,.writeFile(and the plural formsaddFiles/writeFiles) lands the file as 0o600 in the workdir, forcing the backend to copy from the archive/value cache to a fresh inode instead of taking the hardlink fast path.The same option is mirrored on
workdir.builder().addFile/writeFile(plus plural forms) — that's the layer that actually emits the fill-rulepermissionsfield.exec-impl.tplthreads awritableFilesname set throughsettingsand applies it per file.Compatibility
When no caller marks any file writable, the new
writableFilesfield is omitted from the settings JSON resource, so existing workflows produce the exact same exec CID. Old callers also keep working — the schema additions are all,?(optional).Background
The default landed in #1628 to make the backend's
addFileToWorkdirtake the hardlink path: it compares the fill-rule mode againstmifs.ArchiveEntryPerms(0o400) and only hardlinks on exact match. Anything else forces a copy. The changeset there promised an explicit override mechanism — this PR is that knob, surfaced at the public API.Greptile Summary
This PR adds a
{ writable: true }opt-in escape hatch toexec.builder()andworkdir.builder()file-addition methods so callers can land files as0o600(copy-on-use) instead of the default0o400(hardlinked from the archive cache). When no file is marked writable the newwritableFilesfield is omitted from the settings JSON viamaps.clone({removeUndefs: true}), keeping the exec CID identical for all existing workflows.exec/index.lib.tengo: tracks awritableFilesset inaddFile/addFiles/writeFile; serialises it only when non-empty and forwards it to the impl template.exec-impl.tpl.tengo: reconstructs the set at runtime and callswdBuilder.addFile/writeFilewith the per-filewritableflag.workdir/index.lib.tengo: all four methods (addFile,addFiles,writeFile,writeFiles) updated;_getFileFillRuleand_getValueFillRulenow accept awritableparameter and emit 0o400 or 0o600 accordingly.Confidence Score: 4/5
The core implementation is sound; the only gap is a changeset doc that promises a method not present in the code.
The
writableFilestracking, serialisation, and fill-rule emission are consistent across both builder layers. The changeset text promisesexec.builder().writeFiles()accepts{ writable }, but that method was never added toexec/index.lib.tengo— a user following the docs would hit a runtime error. CID stability viaremoveUndefs, the undefined guard in the impl template, andsets.addmutation semantics are all correct.sdk/workflow-tengo/src/exec/index.lib.tengoand.changeset/exec-writable-files.md— either add awriteFilesmethod to the exec builder or correct the changeset description.Important Files Changed
writeFilesplural method; onlyworkdir.builder()has it.writableSetfromsettings.writableFilesand threads it intowdBuilder.addFile/wdBuilder.writeFileper-file — logic is correct, undefined-guard is sound.writableFilesto schema validation and forwards it throughmaps.clone({removeUndefs:true})to settings — CID stability for non-writable workflows preserved.writableFilesset tracking inaddFileandwriteFile;addFilesproperly tunnels opts;slices.fromSetserialization only when non-empty. Missing awriteFilesplural method that the changeset doc promises.addFile,addFiles,writeFile,writeFiles) updated consistently;_getFileFillRuleand_getValueFillRuleaccept awritablebool and emit 0o400/0o600 accordingly.Sequence Diagram
sequenceDiagram participant Caller participant exec.builder participant exec.tpl participant exec-impl.tpl participant workdir.builder Caller->>exec.builder: "addFile(name, ref, {writable: true})" exec.builder->>exec.builder: sets.add(writableFiles, name) Caller->>exec.builder: run() exec.builder->>exec.tpl: "pureExecInputs.writableFiles = slices.fromSet(writableFiles)" exec.tpl->>exec-impl.tpl: "settings = {writableFiles: [...], ...}" exec-impl.tpl->>exec-impl.tpl: build writableSet from settings.writableFiles exec-impl.tpl->>workdir.builder: "addFile(name, ref, {writable: writableSet[name]==true})" workdir.builder->>workdir.builder: sets.add(writableFiles, name) if writable exec-impl.tpl->>workdir.builder: "writeFile(name, ref, {writable: writableSet[name]==true})" workdir.builder->>workdir.builder: "build() emits fill rule with permissions=0o600 or 0o400"Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "exec.writeFile/addFile: { writable: true..." | Re-trigger Greptile