fix(ci): gate version bump on CI, atomic releases, PTY session tests - #9
Merged
Conversation
…add PTY session unit tests Three gaps identified after today's incidents (a broken version-bump.yml syntax error, and a false-positive major-version bump from the bot's own commit message): - version-bump.yml raced CI instead of waiting for it (both fired on the same push). A commit that failed to compile could still get bumped, tagged, and dispatched to release.yml, which would only then fail — after the tag was already public. Switched its trigger from `push: branches: [main]` to `workflow_run: workflows: ["CI"]`, gated on `conclusion == 'success'`, and pinned every git operation to workflow_run.head_sha instead of whatever main's tip happens to be. Added a concurrency group so overlapping runs queue instead of racing. - release.yml published each platform's installers to a live, public release as soon as that platform's job finished — if e.g. Windows and Linux succeeded but macOS notarization failed, users could already be downloading (and the updater pointing at) a release missing a platform entirely. Set releaseDraft: true and added a `publish` job that only flips the release public once every matrix leg succeeded (needs.release.result == 'success'); on any failure the draft is left as-is for inspection, never deleted, never partially published. - Added 4 Rust unit tests for the PTY session bookkeeping introduced in the stability-audit PR (kill_pty on an unknown id, kill_pty actually killing/removing a real session, insert_session reaping a replaced session, and the session_is_current/session_is_superseded generation checks). Extracted the logic those commands relied on into small testable functions (kill_pty_impl, insert_session, session_is_current/superseded) with no behavior change. Two real PTYs overlapping in one test process measured 80s+ on this Windows host (ConPTY/conhost teardown serializing) — the replace-session test keeps only one side real and uses a fake Child/MasterPty for the other, which also required adding anyhow as a direct dev-dependency (portable-pty's Child/MasterPty traits return anyhow::Error but don't re-export it publicly; it's already resolved transitively). cargo test was already wired into the existing CI job/matrix — no separate workflow needed. All 14 tests (10 pre-existing + 4 new) pass; cargo check --locked and npm run build both clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PioEvgnoP4fpxDrSAYTBdF
…TY spawning from tests entirely CI (ubuntu/macos) failed the previous commit: the #[cfg(unix)] mock methods referenced libc::pid_t, matching portable-pty's own trait signature, but libc — like anyhow before it — is only a transitive dependency, not directly nameable without its own Cargo.toml entry. While fixing that, re-ran the suite locally and hit real, host-dependent flakiness: a single real PTY session's Drop (closing its ConPTY/conhost handle) took anywhere from under a second to 60+ seconds on this machine, on different tests each run. That's the same class of "synchronous close can block for a while" risk `reap()` exists to keep off the async runtime for a session's child — but here it was the master/writer half, undeferred, blocking the test thread itself. Rather than fight that timing, dropped the real-PTY test helper entirely; all 4 pty_session_tests now use only the fake Child/MasterPty, making them fast (0.00s) and deterministic. Left a comment flagging the master/writer-drop-can-block-the-async-runtime pattern as a candidate for a future stability pass — out of scope for this PR. All 14 tests pass locally; cargo check --locked clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PioEvgnoP4fpxDrSAYTBdF
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.
Summary
Implements the 3-item CI hardening goal from this session:
version-bump.ymlused to fire onpush: branches: [main], the same event asCI, racing it instead of waiting — a commit that failed to compile could still get bumped, tagged, and dispatched torelease.yml, which would only then fail, after the tag was already public. Switched the trigger toworkflow_run: workflows: ["CI"], gated onconclusion == 'success'andhead_branch == 'main', and pinned every git operation toworkflow_run.head_sha. Added a concurrency group so overlapping runs queue instead of racing.release.ymlpublished each platform's installers live as soon as that leg finished. If one platform failed (e.g. macOS notarization) after the others succeeded, users could already be downloading a release missing a platform. SetreleaseDraft: trueand added apublishjob gated onneeds.release.result == 'success'that flips the release public only once every leg has succeeded; on any failure the draft is left in place for inspection, never deleted or partially published.kill_ptyon an unknown id,kill_ptyactually killing/removing a real session, replacing a reused id reaps the old child, and the current/superseded generation checks). Extracted the logic into small testable functions with no behavior change.cargo testwas already wired into the existingCIjob/matrix — nothing new to add there.Notable finding during implementation
Spawning two real PTYs in one test process measured 80+ seconds on this Windows host (ConPTY/conhost teardown apparently serializes) — the "replace a session" test now keeps only one side real and uses a fake
Child/MasterPtyfor the other. This required addinganyhowas a directdev-dependency(portable-pty's traits returnanyhow::Errorbut don't re-export it publicly; it's already resolved transitively, so no new crate enters the tree).Test plan
cargo test --lockedcargo check --lockedclean, no new warningsnpm run buildcleanversion-bump.yml's first real run this sessionfix(ci): ...) should itself trigger a real patch bump via the newworkflow_run-gated path — watching for that after merge🤖 Generated with Claude Code
https://claude.ai/code/session_01PioEvgnoP4fpxDrSAYTBdF