Skip to content

refactor(core): share job notice formatting and shell output snapshots - #60704

Open
kitlangton wants to merge 1 commit into
devfrom
job-seam-1-notice-snapshot
Open

kitlangton wants to merge 1 commit into
devfrom
job-seam-1-notice-snapshot

Conversation

@kitlangton

Copy link
Copy Markdown
Contributor

Share background job notice formatting and live shell output snapshots ahead of auto-backgrounding.
Startup recovery and live background tools format the same shell and image completion notices, and shell result capture only worked after exit. Extracting both helpers keeps behavior identical today while preparing live job reads and central notice delivery.

❌ Before

  • session/execution/restart.ts owned a private restartMessage formatter for shell and image background notices.
  • Shell.result computed bounded output tails inline only after wait completed, so callers could not read bounded partial output from a still-running command.

✅ After

  • Job.formatNotice formats shell and image notices in one place, and restart.ts calls it with { restarted: true }.
  • Shell.snapshot reads the bounded output tail without waiting for exit, and Shell.result reuses it.

🟰 Behavior change: none.

- Shell.result -> wait(id) -> inline tail slicing
- restart.ts -> private restartMessage(background, recovery)
+ Shell.snapshot(id) -> bounded tail anytime; Shell.result reuses it
+ Job.formatNotice(background, recovery, { restarted: true })

📊 Numbers

File Before After
Production lines (+87 −66) Duplicate notice logic + post-exit-only shell tail Shared Job.formatNotice + live Shell.snapshot

Tests: 41 existing tests pass across job.test.ts and session-execution.test.ts.

👀 Read: packages/core/src/shell.ts around snapshot, ensuring empty running output returns "" while completed empty output still renders "(no output)". The rest is routine.

🔍 Split 1 of 6 from #60495 · no user reports

Checks run
  • bun run --cwd packages/core typecheck
  • bun --cwd packages/core test test/job.test.ts test/session-execution.test.ts: 41 pass

@opencodejr opencodejr Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks sound. With { restarted: true }, Job.formatNotice produces the same restart notices as the removed restartMessage for every shell and image status. Shell.result still renders (no output) for an empty capture, and Shell.snapshot only extracts the existing tail logic. I compared the old and new paths and ran job.test.ts, session-execution.test.ts and shell.test.ts in packages/core (59 pass). Type errors from tsc are the same as on the base, and none are in the changed files.

Public API is unchanged: Job.formatNotice, Job.Notice and Shell.Interface.snapshot are internal to packages/core.

One small inline note: the unknown cast in formatNotice.

Comment thread packages/core/src/job.ts
}

export function formatNotice(
input: Pick<Background, "id" | "status" | "output" | "error"> & { readonly value?: unknown },

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

value?: unknown is then cast to the shell output type on line 86, so the type checker won't catch a wrong value passed in by the live-notice callers in the next splits. Could this take the ShellResult output type directly (exported from shell/result.ts if needed) instead of using unknown plus a cast?

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant