Fix background task liveness probe after session resume - #215
Conversation
![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: d795398521
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
d795398 to
8929fb0
Compare
Fix A - TaskStop completion recognition: list_pending_background_task_ids now treats a TaskStop tool_result (.toolUseResult.message "Successfully stopped task: <id>") as a terminal completion event, because many Claude Code builds do not also emit a task_notification system record for a TaskStop. The id is read from .task_id, falling back to parsing it out of the message text when that field is absent. The message field is coerced with tostring before contains() so unrelated tool results carrying a structured (object/array/number) .message do not error and wipe the whole completed set under pipefail. Fix B - cross-session liveness glob: is_bg_task_alive now searches sibling session dirs under the same project slug when neither the transcript-recorded output path nor the session-derived path exists. This only expands where we look, so a genuinely alive task is never wrongly pruned; a file missing everywhere still fails open as before. Builds on the extract_bg_task_output_path_from_transcript helper landed in PolyArch#215. Tests: 51/51. Renames existing AC-NN cases to descriptive names and adds regression coverage for TaskStop completion, the cross-session glob, and non-string .message poisoning.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8929fb042c
ℹ️ 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".
The path extractor required the trailing 'You will be notified when it completes.' sentence. When a Claude Code launch record omits it, the regex returned no match, so is_bg_task_alive fell back to the derived current-session path and dead/orphaned tasks stayed pending forever on session resume. Bound the match by the closing JSON string quote ([^"]*) instead of the suffix sentence, and strip the optional suffix afterwards. Add AC-25c covering the suffix-omitted launch form. Discussion: PolyArch#215 (comment)
|
@codex review again |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b079f84ea8
ℹ️ 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".
The path extractor used grep -F "$task_id", a substring match. Probing "bash_1" also matched an earlier "bash_10" launch line, so head -n1 selected bash_10's output path; lsof then saw a dead file and pruned the still-running bash_1, letting the stop hook reach Codex before bash_1 finished. Anchor the match on the exact backgroundTaskId JSON value by wrapping the id in literal double quotes: "bash_1" cannot match "bash_10", and the output path never contains a double quote so the boundary cannot fall inside a path value. Add AC-25d (selective lsof mock: bash_1.output alive, others dead; bash_10 launched before bash_1) asserting the alive bash_1 still short-circuits. Discussion: PolyArch#215 (comment)
|
@codex review again |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1034c93ad2
ℹ️ 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".
| match=$(grep -F "\"$task_id\"" "$transcript_path" 2>/dev/null \ | ||
| | grep -oE 'Output is being written to: [^"]*' \ | ||
| | head -n1) || true |
There was a problem hiding this comment.
When the recorded output path contains JSON-escaped characters, such as a project slug with a literal backslash, this pipeline reads the raw JSONL text and returns the escaped spelling (for example /tmp/.../foo\\bar/...) rather than the actual filesystem path (/tmp/.../foo\bar/...). [[ -f "$output_file" ]] then fails and the dead Bash task is treated as still running forever; this also regresses same-session probes that previously used the correctly derived path. Extract the text via jq/JSON decoding before passing the path to lsof.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Root cause: extract_bg_task_output_path_from_transcript read the raw JSONL line and regex-extracted the path with grep -oE 'Output is being written to: [^"]*'. The path lives inside a JSON string value, so any character JSON must escape comes through in its escaped spelling — a literal backslash in the path is serialized as "\". [[ -f "$output_file" ]] then checked the doubled-backslash path, missed the real file, is_bg_task_alive failed open, and the dead task was treated as alive forever. The stop hook never reached Codex, and same-session probes that previously used the correctly derived path regressed too, as you noted.
Fix: keep the quote-anchored grep -F ""$task_id"" that selects the launch record by exact backgroundTaskId (so bash_1 is still not confused with bash_10), then decode that record's text field with jq -r '.message.content[]?.content[]? | select(.type == "text") | .text' so the extracted path matches the filesystem. jq is already a hard dependency of every validator hook; when it is absent the function falls back to the derived tasks_dir path (the pre-transcript-extraction behavior), preserving the fail-open contract.
Regression coverage: added AC-25e — a dead task whose recorded output path contains a literal backslash (tasks/with\backslash/...) is pruned via the decoded path. Confirmed the test has teeth: with the old raw-grep extraction AC-25e fails (escaped path → [[ -f ]] fails → treated alive → stop hook short-circuits and never reaches Codex), and with the fix it passes. Full suite 46/46.
@zevorn could you take another look?
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ 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". |
extract_bg_task_output_path_from_transcript read the raw JSONL text and regex-extracted the path, so JSON-escaped characters came through in their escaped spelling: a literal backslash in the path was doubled to "\\". [[ -f ]] then missed the real file, is_bg_task_alive failed open, and the dead task was treated as alive forever -- the stop hook never reached Codex and same-session probes regressed. Keep the quote-anchored grep that selects the launch record by exact backgroundTaskId, then decode the record's text field with jq -r so the extracted path matches the filesystem. Require jq (already a hard dependency of every validator hook) and fall back to the derived tasks_dir path when it is absent. Regression coverage: AC-25e -- a dead task whose recorded output path contains a literal backslash is pruned via the decoded path. Old raw-grep code fails AC-25e (escaped path -> [[ -f ]] fails -> treated alive); the fix passes. Full suite 46/46.
|
LGTM! Thanks for the contribution. |
Fix A - TaskStop completion recognition: list_pending_background_task_ids now treats a TaskStop tool_result (.toolUseResult.message "Successfully stopped task: <id>") as a terminal completion event, because many Claude Code builds do not also emit a task_notification system record for a TaskStop. The id is read from .task_id, falling back to parsing it out of the message text when that field is absent. The message field is coerced with tostring before contains() so unrelated tool results carrying a structured (object/array/number) .message do not error and wipe the whole completed set under pipefail. Fix B - cross-session liveness glob: is_bg_task_alive now searches sibling session dirs under the same project slug when neither the transcript-recorded output path nor the session-derived path exists. This only expands where we look, so a genuinely alive task is never wrongly pruned; a file missing everywhere still fails open as before. Builds on the extract_bg_task_output_path_from_transcript helper landed in PolyArch#215. Tests: 51/51. Renames existing AC-NN cases to descriptive names and adds regression coverage for TaskStop completion, the cross-session glob, and non-string .message poisoning.
2 participants
Problem
When a Claude Code session is resumed or continued, the current transcript may have a different session id from the session that originally launched a
background task. The task's
.outputfile physically lives under the old session directory, but the stop hook's liveness probe derived the output pathfrom the current transcript's session id. As a result, dead/orphaned tasks were never pruned and the loop was blocked from reaching Codex review.
Fix
Teach
is_bg_task_aliveto look up the real output file path recorded in the transcript's launch message:extract_bg_task_output_path_from_transcript()to parse the "Output is being written to: " line.tasks_dirpath when the transcript is unreadable or the message is missing.transcript_pathargument throughprune_dead_bg_task_idsandlist_pending_background_task_ids.Test
Add regression test AC-25: simulate a session resume with a new-session transcript that records an old-session output path, then assert the dead task is
pruned using the real path rather than the derived new-session path.
42/42 tests pass.