mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-29 12:59:13 +01:00
fix(runner): preserve OpenCode bash process exit codes (#18076)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Native Runner stores structured tool events so people can inspect actual work. > - OpenCode puts a completed bash command's process exit in `state.metadata.exit`. > - The canonical event mapper only read `state.exit`, so a real nonzero exit became unknown. > - This pull request preserves the documented builtin bash exit and rejects conflicting evidence. > - The benefit is an accurate process exit without changing tool completion or native run status. ## Linked Issues or Issue Description **What happened?** A completed OpenCode bash part with `state.metadata.exit: 37` produced `exitCode: null`. The process result was present, but the canonical tool event lost it. The pinned upstream [shell result](https://github.com/anomalyco/opencode/blob/v1.18.34/packages/opencode/src/tool/shell.ts#L544-L552) and [completed part](https://github.com/anomalyco/opencode/blob/v1.18.34/packages/opencode/src/session/processor.ts#L160-L170) establish the field path. **Expected behavior** The canonical event retains the numeric process exit from a completed builtin bash part. A completed tool call can have a nonzero command exit. Missing or invalid evidence must not become a successful command. **Steps to reproduce** 1. Pass a `type: "tool"`, `tool: "bash"` part with completed state and numeric `metadata.exit` to `canonicalProviderEventsFromOpenCodePart`. 2. Inspect `tool.execution.completed.payload.exitCode`. 3. Before this fix, the value is null. After this fix, it matches the process exit. **Paperclip version or commit** Reproduced on master `f4f02880cae4f5fa54d47d39565ca99b6b72446d`. Duplicate searches found no matching fix. Related PR #15715 concerns OpenCode V2 transport and version support; it does not change this mapper. ## What Changed - Read numeric metadata exits only from completed builtin bash parts. - Keep legacy numeric top-level exits. Return unknown when supplied legacy evidence conflicts. - Cover zero and nonzero exits, invalid values, foreign tools, incomplete calls and conflicting receipts. - Document the run-log field and the distinction between tool completion and command success. ## Verification - The regression suite first failed five assertions on the unchanged mapper. All 53 provider-event tests now pass. - A focused no-emit TypeScript check covers the changed mapper, its tests and imported protocol validators. - Tests use synthetic structured parts and real protocol validation. No local database, provider, SDK build or live scenario was run. - All 48 jobs in the normal PR workflow passed, including the required `ci / verify` and `ci / e2e` gates. CI also executed all 53 provider-event tests. - Greptile reviewed this exact head at 5/5 with no actionable findings. The separate hosted quality-gate review also completed successfully. ## Risks - Conflicting legacy and metadata exits now remain unknown for completed builtin bash calls. This avoids selecting one inconsistent value as proof. - Other tools retain their existing legacy behavior. Output prose never supplies an exit code. - The change adds no schema field and changes no provider version, budget, accounting, fixture or native run outcome. ## Model Used OpenAI `gpt-6.1-sol` through Codex, with reasoning and terminal tools. The session does not expose the context-window size. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge Co-authored-by: Paperclip <noreply@paperclip.ing>
This commit is contained in:
1 parent
ee67d2ece9
commit
743698e700
3 files changed
+69
-1
No files matched your search
@@ -92,6 +92,14 @@ They also become native Observability spans only with an operator OTLP endpoint.
|
||||
|
||||
## Native PRP Run-Log Events
|
||||
|
||||
Completed OpenCode builtin `bash` parts preserve the numeric process exit from
|
||||
`state.metadata.exit` in `tool.execution.completed.payload.exitCode`. The tool
|
||||
status stays `completed` even when the command exits with a nonzero code. This
|
||||
status describes completion of the tool call, not success of the command or the
|
||||
native run. Metadata adds an exit only when it is an integer. Conflicting supplied
|
||||
legacy exits stay unknown. Existing numeric `state.exit` values remain supported;
|
||||
metadata on other tools cannot add process-exit proof.
|
||||
|
||||
`auth.credentials.updated` is a runner-produced PRP event for a change to a
|
||||
supported credential store, including account identity changes and later token
|
||||
refreshes on the new account. The bundled transport awaits a host commit callback
|
||||
|
||||
@@ -484,6 +484,53 @@ describe("provider-neutral events", () => {
|
||||
).toEqual([]);
|
||||
});
|
||||
|
||||
it.each([0, 37, -1])("preserves OpenCode bash metadata exit %i without changing tool completion", (exit) => {
|
||||
const event = canonicalProviderEventsFromOpenCodePart({
|
||||
id: "bash-part", type: "tool", tool: "bash", callID: "bash-call",
|
||||
state: { status: "completed", input: { command: "node verify.mjs" },
|
||||
output: "Work validation finished", metadata: { exit, truncated: false }, time: { start: 1, end: 2 } },
|
||||
})[0]!;
|
||||
expect(event).toMatchObject({ eventType: "tool.execution.completed", itemId: "bash-part",
|
||||
payload: { executionId: "bash-part", name: "bash", status: "completed", exitCode: exit } });
|
||||
expect(validatePrpEvent(envelope(event)).ok).toBe(true);
|
||||
});
|
||||
|
||||
it.each([undefined, null, "37", 1.5, Number.NaN, Number.POSITIVE_INFINITY, Number.MAX_SAFE_INTEGER + 1])("does not infer a bash exit from invalid metadata %s", (exit) => {
|
||||
const event = canonicalProviderEventsFromOpenCodePart({ id: "bash-invalid", type: "tool", tool: "bash",
|
||||
state: { status: "completed", output: "exitCode=37", metadata: { exit } } })[0]!;
|
||||
expect(event.payload.exitCode).toBeNull();
|
||||
});
|
||||
|
||||
it.each([
|
||||
{ type: "tool", tool: "read", state: { status: "completed" } },
|
||||
{ type: "tool", tool: "mcp_bash", state: { status: "completed" } },
|
||||
{ type: "tool", tool: "unknown", callID: "functions.bash:0", state: { status: "completed" } },
|
||||
{ type: "bash", state: { status: "completed" } },
|
||||
{ type: "tool", tool: "bash", state: { status: "running" } },
|
||||
{ type: "tool", tool: "bash", state: { status: "error", error: "Tool execution aborted" } },
|
||||
])("rejects metadata exits outside a completed builtin bash part %#", (part) => {
|
||||
const event = canonicalProviderEventsFromOpenCodePart({ id: "not-bash-complete", ...part,
|
||||
state: { ...part.state, metadata: { exit: 37 } } })[0]!;
|
||||
expect(event.payload.exitCode).toBeNull();
|
||||
});
|
||||
|
||||
it.each([0, 37, "37", 1.5])("rejects conflicting legacy and metadata exits %s", (exit) => {
|
||||
const event = canonicalProviderEventsFromOpenCodePart({ id: "bash-conflict", type: "tool", tool: "bash",
|
||||
state: { status: "completed", exit, metadata: { exit: 1 } } })[0]!;
|
||||
expect(event.payload.exitCode).toBeNull();
|
||||
});
|
||||
|
||||
it.each([
|
||||
{ tool: "bash", exit: 37, metadata: undefined },
|
||||
{ tool: "bash", exit: 37, metadata: { exit: 37 } },
|
||||
{ tool: "bash", exit: 37, metadata: { exit: "37" } },
|
||||
{ tool: "read", exit: 37, metadata: { exit: 1 } },
|
||||
])("preserves an existing legacy numeric exit %#", ({ tool, exit, metadata }) => {
|
||||
const event = canonicalProviderEventsFromOpenCodePart({ id: "legacy-exit", type: "tool", tool,
|
||||
state: { status: "completed", exit, metadata } })[0]!;
|
||||
expect(event.payload.exitCode).toBe(exit);
|
||||
});
|
||||
|
||||
it("maps OpenCode pending dynamic tools to a valid builtin execution start", () => {
|
||||
const event = canonicalProviderEventsFromOpenCodePart({
|
||||
id: "prt_02c35c313001X42cqyvQrpXOFv",
|
||||
|
||||
@@ -1002,6 +1002,19 @@ export function canonicalProviderEventsFromCodex(
|
||||
return [];
|
||||
}
|
||||
|
||||
function openCodeToolExitCode(
|
||||
part: Record<string, unknown>,
|
||||
state: Record<string, unknown>,
|
||||
): number | null {
|
||||
const legacy = integer(state.exit);
|
||||
if (part.type !== "tool" || part.tool !== "bash" || state.status !== "completed") {
|
||||
return legacy;
|
||||
}
|
||||
const exit = integer(record(state.metadata).exit);
|
||||
if (exit === null) return legacy;
|
||||
return state.exit != null && legacy !== exit ? null : exit;
|
||||
}
|
||||
|
||||
/** Maps documented OpenCode structured message parts. Unrecognized parts are deliberately unsupported. */
|
||||
export function canonicalProviderEventsFromOpenCodePart(
|
||||
partValue: unknown,
|
||||
@@ -1051,7 +1064,7 @@ export function canonicalProviderEventsFromOpenCodePart(
|
||||
readOnly: /read|search|grep|find|list|glob/i.test(tool),
|
||||
status,
|
||||
durationMs: integer(state.time),
|
||||
exitCode: integer(state.exit),
|
||||
exitCode: openCodeToolExitCode(part, state),
|
||||
progress:
|
||||
status === "running" ? text(state.title).slice(0, 4000) || null : null,
|
||||
...boundedOutput(
|
||||
|
||||
Reference in new issue
Block a user