Improve Claude PTY handling & keychain test hooks#320
Conversation
Add TaskLocal-based test overrides and refine keychain cache test hooks; introduce per-test service override and TestStoreKey so test stores are keyed by service+account. Update KeychainAccessGate and KeychainCacheStore with withTaskOverrideForTesting/withServiceOverrideForTesting and a global service fallback. Adjust tests to use the new task-local overrides. Harden Claude CLI PTY session handling: normalize scanned text (strip ANSI + whitespace) for reliable needle matching, add command-palette-specific auto-confirm mappings, increase initial wait for TUI readiness, periodically send Enter with configurable cadence, and centralize idle/timeout checks. Replace raw handle writes with writeAllToPrimary that retries non-blocking writes and surfaces IO errors. Respect CODEXBAR_DISABLE_CLAUDE_WATCHDOG env to skip bundled watchdog helper. Improve Claude status probing/parsing: retry /usage capture if output looks like startup, trim PTY captures to the latest Usage panel to avoid status-bar context contaminating percent scraping, add heuristics to ignore status-line context meters, tighten fallback percent collection logic, and extend stop substrings for usage capture. Tests updated/added to reflect parsing and keychain behavior changes.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 264d231252
ℹ️ 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".
| } | ||
| usleep(5000) | ||
| continue | ||
| } | ||
| throw SessionError.ioFailed("write to PTY failed: \(String(cString: strerror(err)))") |
There was a problem hiding this comment.
The new low-level PTY write path treats any error other than EAGAIN/EWOULDBLOCK as fatal. write(2) can return EINTR when a signal (e.g., SIGCHLD from the spawned CLI process) interrupts the syscall, which is common under PTYs. In that case this code now throws ioFailed and aborts the capture even though a retry would succeed. Consider handling EINTR the same way as EAGAIN to avoid intermittent failures when signals arrive while sending /usage or /status.
Useful? React with 👍 / 👎.
|
Addressed: Claude PTY write loop now retries EINTR (in addition to EAGAIN/EWOULDBLOCK) to avoid spurious ioFailed errors when signals interrupt write() while sending /usage or /status. Change in Sources/CodexBarCore/Providers/Claude/ClaudeCLISession.swift. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ead8e42cb
ℹ️ 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".
| var stoppedEarly = false | ||
| let effectiveEnterEvery: TimeInterval? = sendEnterEvery ?? (isSlashCommand ? 0.8 : nil) | ||
|
|
There was a problem hiding this comment.
The new default sendEnterEvery ?? (isSlashCommand ? 0.8 : nil) forces periodic Enter even when callers intentionally pass nil (e.g., /status uses idleTimeout: 3.0 and sendEnterEvery: nil). If the CLI echoes output on each Enter, lastOutputAt keeps updating and the idle timeout never fires, so /status can run until the full timeout and return noisy output or time out. Consider only defaulting to 0.8 when the caller didn’t opt out (or make /status pass an explicit value) so the idle timeout can still stop the capture.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b5e5a2959
ℹ️ 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".
| buffer.append(newData) | ||
| lastOutputAt = Date() | ||
| if let chunk = String(bytes: newData, encoding: .utf8) { | ||
| scanTailText.append(chunk) | ||
| } |
There was a problem hiding this comment.
The new prompt/stop detection relies on scanTailText, but it is only updated when String(bytes:encoding:) succeeds. PTY reads often split multibyte UTF‑8 sequences, and String(bytes:encoding:) returns nil for any chunk with an incomplete sequence, so those bytes are silently discarded. That means prompts or stop substrings can be missed and the session can stall or time out even though the CLI output was present. Consider using String(decoding: newData, as: UTF8.self) or an incremental decoder so partial UTF‑8 still contributes to the rolling text.
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Summary
Testing