Repository navigation
fix: handle empty event logs in exports - #266
dannyward630 wants to merge 1 commit into
Conversation
Assisted-by: Codex (GPT-6)
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a568d84978
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await mkdir(sessionDirectory, { recursive: true }); | ||
| await writeFile( | ||
| join(sessionDirectory, 'session.json'), | ||
| JSON.stringify(createTestSessionRecord({ sessionId })), |
There was a problem hiding this comment.
If an unrelated same-user process has PID 123 or 456, either new empty-log test can kill it during teardown. createTestSessionRecord({ sessionId }) supplies hostPid: 123 and childPid: 456, and the suite's afterEach invokes cleanupHome, which sends SIGKILL to every positive PID in the manifest (test/helpers.ts:185-189). Neither process was created by this fixture. Override both PIDs to null and use a non-running status so this setup respects the cleanup-safety invariant in root AGENTS.md.
AGENTS.md reference: AGENTS.md:L149-L149
Useful? React with 👍 / 👎.
| await writeFile( | ||
| join(sessionDirectory, 'session.json'), |
There was a problem hiding this comment.
This valid-session fixture writes session.json directly with writeFile, contrary to root AGENTS.md's explicit prohibition on ad hoc manifest writes. It creates a separate storage-writing path that bypasses the absolute-path checks, Zod validation, and atomic replacement provided by writeManifest (src/storage/manifests.ts:259-267). Use manifestPath and writeManifest for this fixture rather than serializing the manifest directly.
AGENTS.md reference: AGENTS.md:L150-L151
Useful? React with 👍 / 👎.
|
Thank you for this fix, @dannyward630. #251 is now fixed by #270, which landed in main as 4cc4312. The two PRs agree on asciicast: an empty event log exports at seq -1. They differ on WebM. #270 exports a blank video instead of returning I leave this PR open for a maintainer to close. Generated with |
Fixes #251.
When the event log is empty, asciicast exports now report the pre-event sequence (-1) consistently in the artifact, result schema, and filename. Empty WebM exports return a non-retryable INVALID_INPUT error before artifact creation, with reason empty_event_log and guidance to use asciicast.
Verification
The full CI-equivalent run is limited by this Windows host. Repository-wide formatting check flags about 1,014 files, although every changed file passes formatting. The full unit suite reports 1,634 passed, 57 failed, and 18 skipped; failures are in unrelated Windows-sensitive path, PTY, filesystem, and permission tests. The focused PTY e2e case fails while creating its fixture session on Windows, before reaching export. The project README identifies Windows as Tier 2 and not CI-tested.
AI disclosure: prepared with Codex (GPT-6). The CLI outputs above are from a manual run of the built source against an isolated session.