You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Watch for connector requests with a one-shot background watcher, not Monitor - #902
Replace the expiring Monitor watch with a one-shot background request watcher. Each invocation reads complete connector-output lines, returns unhandled requests once, and exits. The session reads its output, records the ids, starts the next watcher, and handles the requests.
Replays the output from the start, so requests arriving between watcher invocations are not skipped.
Ignores partial lines, deduplicates repeated ids, handles literal backslashes in paths, and refuses unusable inputs.
Supports --once to drain the output when the connector stops.
Updates the skill and post-compaction wording to describe the watcher. The existing skill-size checks remain enforced.
Verification
Full local bin/ci passes after rebasing onto current main.
14 focused regressions are committed to the existing BATS suite: usage, missing/empty inputs, handled-file creation, handled ids and exact matching, duplicate lines, other event types, partial lines, escaped content, literal paths, unusable handled paths, loss of the handled file between startup and scanning (both modes), and delivery after a new line arrives.
Mutation checks reject removing the complete-line guard, removing per-run deduplication, and passing a literal handled path through awk -v.
Existing skill layout and command-drift checks pass. No new testing harness or server is introduced.
Review boundary
The session must restart the watcher after each completion; it is not an independent supervisor for an unavailable session. Recording handled ids before dispatch also retains the documented interruption trade-off. No fresh real-session, macOS-runtime, laptop-sleep, or expired-login recovery claim is made by these script tests.
The newer alternative in #958 addresses sleep/expired-login recovery with a recurring heartbeat and monitor re-arming. These are alternative approaches and should not both land unchanged. This PR is being verified on its own scope, not presented as covering that broader recovery scenario.
Replaces expiring Monitor-based connector request handling with a one-shot polling watcher.
Changes:
Adds request filtering, deduplication, and drain support.
Updates connector skill lifecycle instructions.
Renames the post-compaction monitor reference.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Watcher loops forever when task file becomes unreadable
skills/basecamp-connect/watch-requests.sh:28
The connector output is checked only before polling starts. If that background-task file is later removed or becomes unreadable, this redirection fails and the arithmetic expansion yields zero, so the watcher stays alive and sleeps forever instead of reporting failure. That also defeats the compaction notice's liveness check because the task still appears running while it can no longer observe requests. Revalidate the file on each poll and exit 2 when it cannot be read.
Shell-quote expanded watcher paths to handle apostrophes
skills/basecamp-connect/SKILL.md:84
The expanded skill directory and connector output path are inserted between literal single quotes. If either valid path contains an apostrophe, this command is syntactically invalid and the watcher never starts (for example, Bash reports an unmatched quote for a skill path such as skill'copy). Instruct the session to shell-quote each expanded path, including escaping embedded apostrophes, rather than wrapping raw placeholders in fixed quotes.
This issue also appears on line 553 of the same file.
An output file that vanishes mid-poll left the watcher sleeping forever. This was in the review body on b42a8df, and e1d4116 fixes it: each poll checks the file and exits 2 if it can't be read. Checked with a new local case, which the previous script fails.
Declined on merit: an apostrophe in the expanded skill or output path breaking the single-quoted commands (review body on e1d4116, SKILL.md:84 and :553).
The skill's quoting rule already says to write a quote inside a value as '\''.
The pattern predates this PR: tail -f -n +1 '<connector-output-file>' and `${CLAUDE_SKILL_DIR}/subagent.md`.
Both paths come from the harness, which sanitizes the session directory name.
A fix here would also belong in every other command in the skill, so it's a separate change if anyone wants it.
…Monitor
The skill armed the Monitor tool with `persistent: true`. Claude Code 2.1.271
removed that option: every watch now expires within 30 minutes, so a session
has to re-arm it. Re-arming and skipping handled ids with `grep -v` loses
requests in a different way. In Claude Code's Bash, `grep` is the embedded
ugrep, and `ugrep -v` holds each line on a stream until the next one arrives,
`--line-buffered` or not. The newest request then waits for the next one.
watch-requests.sh reads the connector's output from the start, prints the
request lines whose event_id the handled file lacks, and exits. The session
records each id, starts the watcher again, and handles the lines. The notification
is the exit itself, so no stream filter sits between a line and the session,
and reading from the start leaves no gap between watchers. The watcher reads only
complete lines, and the handled file lives outside the run, so a line
reprinted after a crash is still skipped.
…t, and fail closed on the handled file
A request printed just before the connector exits could sit in the old
output file after the watcher moved to the new one. watch-requests.sh --once
checks the old file a last time, without needing timeout(1), which macOS
lacks.
The watcher's completion notice names its output file rather than carrying
the lines, so the skill says to read them from there.
awk -v decodes backslashes, so the handled path reaches awk through ENVIRON.
A handled file the watcher can't create or read now exits 2 instead of
reporting every request as new.
The post-compaction notice said the session's monitor kept running. It now
names the request watcher.
…he watcher
A directory or a read-only file passed touch and -r, so the watcher ran
against a handled file the session could never record into, and every
restart replayed the same requests. It now requires a regular file it can
read and write.
The post-compaction notice said everything kept running. The watcher exits
on each request, so it may have exited during the compaction. The notice now
says to check it, and to read its output and start it again if it has
exited.
The output file was checked once, before polling. If it went away later,
wc failed, the line count read as zero, and the watcher slept forever while
looking alive. Each poll now checks the file and exits 2 if it can't be read.
Rebased and added committed coverage to the existing BATS suite. All 14 watcher tests and full local CI pass. A handled-file disappearance between startup and scanning now fails closed in both polling and --once mode rather than replaying work. The skill still fits the existing compaction/layout limits. Current head: 20449f0. The alternative in #958 remains a separate review choice; this PR does not claim that broader session-recovery behavior.
Ready for human review on 20449f0: local and hosted CI are green and both remaining script/test findings are fixed and resolved. Copilot reported on this exact head; it asks for human validation of the Claude Code task-completion/re-arm lifecycle, not another script fix. Cubic could not provide a fresh pass because its monthly quota is exhausted. The remaining decision is this one-shot approach versus the heartbeat/re-arm alternative in #958. The BATS regressions do not claim a fresh real-session, sleep, or expired-login test. This is ready to review, not an instruction to merge both alternatives.
This branch has not been deployed
No deployments
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
commandsCLI command implementationsskillsAgent skillstestsTests (unit and e2e)
2 participants
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.
github-actions
Bot
added
commands
Summary
Replace the expiring Monitor watch with a one-shot background request watcher. Each invocation reads complete connector-output lines, returns unhandled requests once, and exits. The session reads its output, records the ids, starts the next watcher, and handles the requests.
--onceto drain the output when the connector stops.Verification
bin/cipasses after rebasing onto current main.awk -v.Review boundary
The session must restart the watcher after each completion; it is not an independent supervisor for an unavailable session. Recording handled ids before dispatch also retains the documented interruption trade-off. No fresh real-session, macOS-runtime, laptop-sleep, or expired-login recovery claim is made by these script tests.
The newer alternative in #958 addresses sleep/expired-login recovery with a recurring heartbeat and monitor re-arming. These are alternative approaches and should not both land unchanged. This PR is being verified on its own scope, not presented as covering that broader recovery scenario.