fix: writeAtomicFile only unlinks the temporary after the write succeeded, and surfaces a non-ENOENT cleanup failure - #15
Merged
Conversation
…and runProve
Bug review of EauDoon/agent-action-stack found:
- MEDIUM: runAct (bin/aas.mjs) short-circuits to childProcessError when result.status !== 0, before parseStageJson is reached. Any JSON crctl wrote to stdout is dropped. The throw also bypasses failedStderr, but the bigger loss is the stdout payload: persistRunBundle only writes a stage artifact when current.raw !== undefined, and stageErrorFields never sets raw, so a `--fault duplicate` failure that surfaces as JSON on stdout is not persisted, does not appear in report.stages.act, and printHuman shows only act_code / act_reason / act_stderr. runDecide and runProve both parse the JSON first and return {ok: false, raw, status, stderr} for nonzero exits; runAct was the asymmetric outlier.
Fix: parse the JSON first, then if status is nonzero return the same {ok: false, raw, status, ...failedStderr(result)} shape that runDecide and runProve use. Throw childProcessError only when the spawn itself failed (result.error) or when the child exited nonzero with no parseable JSON.
Verified: node --test test/stack.test.mjs runs 40 tests, all pass.
Bug review of EauDoon/agent-action-stack found:
- LOW: bin/aas-gui.mjs requestBoundaryFailure returns the same "origin" code whether the caller is missing the Origin header entirely or sent a wrong-origin header. The handler at line 100 returns 403 Forbidden for both, with the body { error: "Forbidden" }. A programmatic local client that forgets to send Origin (curl, a CI step, a non-browser agent) sees a generic 403 and has no way to tell that the only fix is to add the header. A wrong-origin POST is genuinely forbidden; a missing-Origin POST is a misconfiguration that should be 400.
Fix: add a "missing-origin" branch in requestBoundaryFailure and a 400 response with a clear "Origin header required for POST" message in the handler. Wrong-origin POSTs still get 403. Update test/gui.test.mjs to assert the 400 and the new error message.
Verified: node --test test/gui.test.mjs runs 2 tests, all pass. node --test test/stack.test.mjs still runs 40 tests, all pass.
…eded, and surfaces a non-ENOENT cleanup failure Bug review of EauDoon/agent-action-stack found: - LOW: bin/aas.mjs writeAtomicFile had a try/catch that called unlink(temporary) in the catch arm of the write-or-rename try. The unlink ran even when the writeFile itself failed, in which case the temporary was never created and unlink would throw ENOENT that the catch silently swallowed. The two real failure modes (write failed, rename failed) both got the same misleading "cleanup noise ignored" path, and a real cleanup failure (e.g. EBUSY) was indistinguishable from a no-op ENOENT. Fix: track whether the write succeeded with a `written` flag. Only attempt the unlink in a finally block when the temporary actually exists. Distinguish ENOENT (legitimate "nothing to clean up") from any other unlink error; the latter is a real operator-visible problem (the temporary is stuck on disk) so emit a process warning named "WriteAtomicFileCleanup" instead of swallowing it. The original write-or-rename error is still the one that propagates. Verified: node --test test/stack.test.mjs runs 40 tests, all pass.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Comment on lines
+525
to
+530
| if (result.status !== 0) { | ||
| // A nonzero exit with parseable JSON is an unsuccessful act (the CLI | ||
| // surfaces structured errors as JSON on stdout), not a child-process | ||
| // error. Mirror runProve so persistRunBundle and printHuman see the | ||
| // structured failure and the GUI can render the stage artifact. | ||
| return { ok: false, raw: payload, status: result.status, ...failedStderr(result) }; |
There was a problem hiding this comment.
🔴 Failed actions become successful runs
When runAct returns a nonzero result, runDemo ignores ok and records the action as passed. The run can exit successfully after the action failed.
Prompt for agents
Update bin/aas.mjs so runDemo handles the new runAct failure result instead of treating every resolved result as a passed action. A result with ok false must mark the act stage failed, preserve its raw payload and stderr in the bundle and report, set a nonzero run exit code, and avoid presenting the action as passed. Decide explicitly whether proof still runs for this failure shape, while ensuring a successful proof cannot turn the overall run back into success. Add coverage for a nonzero, parseable act result through runDemo, including report status, manifest status, artifact persistence, and exitCode.
Was this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
617
to
+618
| writeFile(temporary, data, { encoding: "utf8", flag: "wx" }); | ||
| written = true; |
There was a problem hiding this comment.
🟡 Partial writes leave temporary files
When writeFile creates a partial temporary file before throwing, written remains false and skips cleanup. Repeated write failures leave orphaned files.
Prompt for agents
Revise writeAtomicFile in bin/aas.mjs so cleanup also handles writes that create or partially write the temporary file before throwing. The current written flag only proves that writeFile returned, not whether the temporary path exists. Preserve the original write or rename error, ignore ENOENT during cleanup, and emit the cleanup warning for other unlink failures. Avoid deleting an unrelated file if exclusive creation itself failed. Add a test whose writeFile implementation creates the temporary path and then throws, and assert that cleanup removes it.
Was this helpful? React with 👍 or 👎 to provide feedback.
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.
Bug review of EauDoon/agent-action-stack found:
Fix: track whether the write succeeded with a
writtenflag. Only attempt the unlink in a finally block when the temporary actually exists. Distinguish ENOENT (legitimate "nothing to clean up") from any other unlink error; the latter is a real operator-visible problem (the temporary is stuck on disk) so emit a process warning named "WriteAtomicFileCleanup" instead of swallowing it. The original write-or-rename error is still the one that propagates.Verified: node --test test/stack.test.mjs runs 40 tests, all pass.