fix: deliver harness prompts off argv#274
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
📝 WalkthroughWalkthroughNon-interactive harness prompts are now delivered through stdin or temporary prompt files instead of embedding task text in argv. Shared process capture handles output, exit codes, timeouts, delivery errors, and cleanup across runtime and CLI execution paths. ChangesNon-interactive harness execution
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PersonaSpec
participant CloudHarnessRunner
participant HarnessProcess
participant ChildHarness
PersonaSpec->>CloudHarnessRunner: return args and prompt delivery mode
CloudHarnessRunner->>HarnessProcess: call spawnNonInteractiveAndCapture
HarnessProcess->>ChildHarness: pass prompt through stdin or prompt file
ChildHarness-->>HarnessProcess: emit output and exit status
HarnessProcess-->>CloudHarnessRunner: return captured result
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![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 refactors the delivery of non-interactive tasks to harness processes by moving them off the command-line arguments (argv) to avoid Linux's 128 KiB single-argument limit. It introduces a NonInteractivePrompt type supporting stdin and file modes, and adds utility functions spawnAndCapture and spawnNonInteractiveAndCapture to handle the process execution and prompt delivery. Feedback on these changes suggests wrapping the temporary directory cleanup in a try-catch block to prevent cleanup errors from suppressing primary execution errors, and wrapping the synchronous spawn call in a try-catch block to handle synchronous spawn failures gracefully.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| } finally { | ||
| await rm(promptDir, { recursive: true, force: true }); | ||
| } |
There was a problem hiding this comment.
In spawnNonInteractiveAndCapture, the finally block attempts to delete the temporary directory promptDir. However, if rm fails (for example, due to file locking on Windows or permission issues), the error will be thrown from the finally block, which will suppress and overwrite the actual process execution result or any primary error thrown in the try block. Wrapping the rm call in a try-catch block ensures that cleanup failures do not interfere with the primary execution outcome.
| } finally { | |
| await rm(promptDir, { recursive: true, force: true }); | |
| } | |
| } finally { | |
| try { | |
| await rm(promptDir, { recursive: true, force: true }); | |
| } catch { | |
| // Best-effort cleanup; do not let cleanup failures suppress the actual process result or primary error. | |
| } | |
| } |
There was a problem hiding this comment.
Keeping cleanup failures loud is intentional here: this file can contain the complete private task, so silently succeeding while leaving it behind is worse than surfacing cleanup failure. force: true handles the normal missing-path case. I agree preserving both causes would be useful if execution and cleanup both throw, but swallowing the cleanup error would weaken the security/cleanup contract, so I am leaving this behavior unchanged.
| const child = spawn(args.bin, args.args, { | ||
| cwd: args.cwd, | ||
| env: args.env, | ||
| stdio: [hasStdin ? 'pipe' : 'ignore', 'pipe', 'pipe'], | ||
| shell: false | ||
| }); |
There was a problem hiding this comment.
The spawn function from node:child_process can throw synchronous errors (for example, if arguments are malformed or if certain synchronous errors occur on specific platforms). If spawn throws synchronously, the Promise executor will reject, causing spawnAndCapture to return a rejected promise instead of a resolved CapturedProcessResult with exitCode: 1 and the error message in stderr. Wrapping the spawn call in a try-catch block ensures that synchronous spawn failures are handled gracefully and consistently with asynchronous error events.
let child;
try {
child = spawn(args.bin, args.args, {
cwd: args.cwd,
env: args.env,
stdio: [hasStdin ? 'pipe' : 'ignore', 'pipe', 'pipe'],
shell: false
});
} catch (err) {
resolve({
output: '',
stderr: `${err instanceof Error ? err.message : String(err)}\n`,
exitCode: 1
});
return;
}There was a problem hiding this comment.
A synchronous throw inside the Promise executor already rejects the returned promise, so it is not lost or silent; spawnNonInteractiveAndCapture also still runs its finally cleanup on that rejection. That loud rejection is intentional for failures before a child process exists, while asynchronous child error events remain normalized into CapturedProcessResult. spawn E2BIG—the production failure this PR fixes—is the concrete case where preserving a distinguishable spawn exception is diagnostically better than flattening it into an ordinary harness { exitCode: 1 }. This also matches the prior callers’ synchronous-spawn behavior, so I am leaving it unchanged.
1 participant
Summary
--prompt-fileBreaking API change
buildNonInteractiveSpec()no longer includes the task inNonInteractiveSpec.args; it returns a requiredpromptdelivery descriptor instead. External consumers that previously spawned onlyspec.binandspec.argsmust now deliverspec.promptthrough stdin or a prompt file as directed. Ignoring the new field produces a prompt-less command. This is also called out under[Unreleased]in the persona-kit changelog.Why
Linux applies two different exec limits:
ARG_MAXcaps argv + envp in aggregate, whileMAX_ARG_STRLENcaps each individual argv element at 32 pages (131,072 bytes on the production sandbox). The prior implementation placed the complete per-run task in one argv element, so a daily-ship prompt containing PR facts could fail withspawn E2BIGeven while the aggregate argv + env block remained belowARG_MAX.On the probe host,
getconf ARG_MAXwas 1,048,576 bytes and the representative environment was about 7.1 KB, so prompt size dominated. macOS does not reproduce Linux's 128 KiB single-element boundary; this PR therefore tests the portable invariant directly: the 200 KB task marker never appears in argv and the remaining per-argument and total argv byte budgets stay below 128 KiB.Empirical CLI probe
These canaries were run on macOS with the locally installed versions below. Each prompt asked the model to return a unique
MAGIC_TOKEN; success means the token appeared in model stdout, not merely that the command exited successfully.--print --output-format textcodex exec ... -opencode run --format default--prompt-file <path>Negative evidence: OpenCode
-f/--fileis an attachment mechanism, not a prompt replacement (You must provide a message or a commandwithout a positional message). Grok bare stdin failed withDevice not configured, so it uses a real temporary prompt file which is deleted infinally.Scope of evidence: the real CLI canaries above used small prompts on macOS and the host versions shown. The automated 200 KB tests use trivial
node -echildren and prove only that our launcher delivers bytes intact, converts an early stdin close to a loud failure, and cleans up files. They do not prove that the production versions of Claude, Codex, or OpenCode consume a 200 KB stdin prompt without early-closing inside the Linux sandbox. That far-side E2E remains for the post-deploy real trigger.Production/version evidence
agentworkforce deployments logs daily-ship --tail 200 --jsonshowed the large-promptspawn E2BIG. Inspection of the affected sandbox showed@agentworkforce/runtimeand persona-kit 4.1.17 on disk even though snapshot metadata saidruntime-4.1.24; the cloud tick was reinstalling the pinned 4.1.17 package. That stale pin also explains the separate OpenCode help dump (the OpenCode flag fix shipped in 4.1.20). This PR addresses the structural argv delivery bug across all four harnesses; deployment/version correction is tracked separately.Validation
origin/main: 117/130 passing, 13 failingNo npm publish is included.