-
Notifications
You must be signed in to change notification settings - Fork 0
Add tmux capture hooks to diagnose live codex startup issues #15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Ensure codex in-flight ends within 2.5s of completion, pick active JSONL per process, and make opencode message completion authoritative. Remove wheel passive listener warnings and align docs/tests.
Start the server on the next available port during dev and ignore stale OpenCode status events when determining in-flight state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 428db10fbf
ℹ️ 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".
| if (inFlight && Number.isFinite(staleMs) && staleMs > 0) { | ||
| if (typeof inFlightSignalAt === "number") { | ||
| if (Date.now() - inFlightSignalAt > staleMs) { | ||
| inFlight = false; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apply stale cutoff when pending tool lacks timestamps
With CONSENSUS_OPENCODE_INFLIGHT_STALE_MS set, the stale cutoff only clears in‑flight if inFlightSignalAt is defined. For common OpenCode payloads where a pending tool has no part.time.start (see existing tests that model pending tools without timestamps), inFlightSignalAt stays undefined while latestActivityAt is still set from message timestamps, so this block never clears the stale in‑flight state. Result: ghost in‑flight sessions persist indefinitely even though the stale cutoff is enabled. Consider falling back to latestActivityAt or the assistant message timestamp when inFlightSignalAt is missing so the stale cutoff actually applies to pending-tool cases.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| const repoRoot = cwdRaw ? findRepoRoot(cwdRaw) : null; | ||
| const repoName = repoRoot ? path.basename(repoRoot) : undefined; | ||
|
|
||
| const sessionIdentity = sessionPath ? `codex:${sessionPath}` : `pid:${proc.pid}`; | ||
| const redactedSessionPath = sessionPath ? redactText(sessionPath) || sessionPath : undefined; | ||
| const sessionIdentity = | ||
| redactedSessionPath && !reuseBlocked | ||
| ? `codex:${redactedSessionPath}` | ||
| : `pid:${proc.pid}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 Codex process can reuse another process’s session JSONL for activity even when reuse is blocked, causing cross-process state/metadata leakage
In scanCodexProcesses, when a Codex process resolves to a session JSONL path that was already claimed by another process, the code sets reuseBlocked = true but still keeps and uses that same sessionPath for tailing and activity/metadata derivation.
Actual behavior: a second Codex PID can be shown with a PID-based identity (so it won’t dedupe), but its activity/doing/title/events/state can still be derived from a different process’s JSONL session.
Expected behavior: when reuse is blocked and no alternate session can be found, the process should avoid tailing/using that already-claimed session path (or should have a deterministic, safe fallback), so activity doesn’t “bleed” across processes.
Impact: incorrect UI state (wrong agent shown as active/idle, wrong “doing” text), and potentially exposing another process’s session content/paths/metadata in the wrong tile.
Click to expand
Where it happens
- Session selection detects reuse but keeps the session:
if (initialSessionPath && usedSessionPaths.has(initialSessionPath) && !allowReuse) {
const alternate = cwdSession;
...
if (alternate && alternatePath && alternatePath !== initialSessionPath) {
session = alternate;
} else {
reuseBlocked = true;
}
}src/scan.ts:1376-1385
- Later, the code still tails
sessionPathand uses it for state:
tailTargets.add(sessionPath)for all contextssrc/scan.ts:1422-1434sessionPath = pickBestJsonl(...) || normalizeSessionPath(session?.path)(no check forreuseBlocked)src/scan.ts:1485-1487getSessionMeta(sessionPath)andsummarizeTail(...)drivedoing/events/model/hasError/inFlight/etc.src/scan.ts:1491-1530
- Only the identity is switched to
pid:${proc.pid}whenreuseBlocked, which prevents dedupe but does not prevent the wrong session log from being used:
const sessionIdentity = redactedSessionPath && !reuseBlocked
? `codex:${redactedSessionPath}`
: `pid:${proc.pid}`;src/scan.ts:1564-1568
(Refers to lines 1376-1568)
Recommendation: When reuseBlocked is true, do not use the already-claimed session.path for tailing/metadata/state. Options:
- Set
session = undefined(orsessionPath = undefined) when reuse is blocked and no alternate exists, and fall back to PID-only withno_hook/idle state. - Or allow reuse only when there’s a reliable discriminator (e.g., matching
startMs/threadId) and otherwise drop the association.
Also avoid writingpidSessionCachewith a blocked/reusedsessionPath.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Superseded by fix/activity-state; live harness changes consolidated. |
Summary
Testing
Ensure you read the contribution guide before submitting a commit and follow the exact guidelines.