Repository navigation
refactor(core): expose shell exit code, elapsed time, and log path - #63647
Conversation
![opencodejr[bot]](https://avatars.githubusercontent.com/in/5497873?s=60&v=4){kind=small}
There was a problem hiding this comment.
I found no blocking problems. The shell and session-execution test suites pass on Linux (136 passed, 23 skipped, 0 failed) and tsc -b in packages/core is clean. I couldn't confirm it end to end: my run from source stopped before the tool result appeared. I read through the completed, clipped, backgrounded and restart-recovery paths and they match the description.
Public API impact:
- Shell tool structured result (not backwards compatible):
exitbecomesexitCode, which is always present andnullwhen there is no exit code.statusandtruncatedare removed.wallTimeSecondsandoutputPathare added. Anything reading the old fields needs to switch to the new ones or to the tool metadata, which is unchanged. - Shell notification metadata: gains an optional
wallTimeSeconds(backwards compatible). - Stored shell recovery record: gains an optional
wallTimeSeconds(backwards compatible).
One small test comment is inline.
| expect(yield* shell.wait(id)).toMatchObject({ status: "exited", exit: 0 }) | ||
| }), | ||
| true, | ||
| const now = Date.now() + 60_000 |
There was a problem hiding this comment.
This test now sets the test clock 60s ahead of real time. It then checks for an exact wallTimeSeconds worked out from that fake time and the real process start time. It works, but it's hard to follow and isn't what this test is about (explicit background ignoring the timeout). Could you keep the original test and check exitCode: null, outputPath and wallTimeSeconds: expect.any(Number) instead? The foreground-to-background test already checks the elapsed value.
There was a problem hiding this comment.
The test comment from my last review is fixed: the explicit-background test no longer moves the test clock and just checks that wallTimeSeconds is a number. tool-shell.test.ts passes (87 passed, 23 skipped, 0 failed). The new commit only edits that test, so the public API impact is as in my earlier review, and I have no new concerns.
There was a problem hiding this comment.
The new commit makes the change smaller: shell notifications and the stored recovery record no longer get wallTimeSeconds, and the related job.ts/restart edits are gone. I found no new problems. The shell and session-execution tests pass (136 passed, 23 skipped, 0 failed) and tsc -b in packages/core is clean.
Public API impact is now limited to the shell tool's structured result, which is not backwards compatible: exit becomes exitCode (a number, or null), status and truncated are removed, and wallTimeSeconds and outputPath are added. Tool metadata, notification metadata and the recovery record are unchanged.
There was a problem hiding this comment.
The new commit is fine and I found no new problems. timeout and killed move out of the structured result but stay in the tool metadata and in the model's message text. The shell and session-execution tests pass (136 passed, 23 skipped, 0 failed) and tsc -b in packages/core is clean.
Public API impact (shell tool structured result, not backwards compatible): exit becomes exitCode (a number, or null); status, truncated, timeout and killed are removed; wallTimeSeconds and outputPath are added. Callers that need timeout or killed can read them from the tool metadata, which is unchanged.
Issue for this PR
Standalone shell output cleanup.
Type of change
What does this PR do?
The shell tool returns
{ output, exitCode, wallTimeSeconds }for a completed command. A running background job returns a null exit code, its shell ID and the path to its live log. A clipped foreground result also includes the output path. Callers using the oldexit,status,truncated,timeoutorkilledfields need to migrate. Timeout and termination notices appear in the model text; the UI reads status from metadata.Time runs from process launch until completion for a foreground result, or until handoff for a background result.
How did you verify your code works?
Repository checks and all 12 focused background tests passed. The shell and session-execution suites had 146 passes, 11 skips and two failures on macOS; I reproduced both process-substitution failures on the base commit.
Screenshots / recordings
The change is in the structured result.
Checklist
Added by the opencode bot (AI-generated).
Public API: shell tool structured output:
exit→exitCode(number | null);status,truncated,timeoutandkilledremoved;wallTimeSecondsandoutputPathadded (not backwards compatible for consumers of the structured result; metadata unchanged)