Release 0.2.0: reviewed reliability and cron improvements - #40
Open
alan-botts wants to merge 36 commits into
Open
Release 0.2.0: reviewed reliability and cron improvements#40alan-botts wants to merge 36 commits into
alan-botts wants to merge 36 commits into
Conversation
Support .txt, .md, .json, .yaml, .yml, and .log files in the Telegram connector's attachment whitelist, matching the text-handling already present in the Slack connector. Adds isTextLikeMIME and isTextLikeExt helpers for content-type validation of text-based uploads. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
A wedged outbound send could lock out the whole gateway. When Telegram's
API became unreachable, the send path had no timeout anywhere in the chain:
tgbotapi's HTTP client had no timeout, the daemon socket handler passed an
unbounded context, and the goat send_user_message client had no read
deadline. The send blocked forever -> the runtime that spawned it blocked
forever -> the session never freed -> new messages queued behind it
indefinitely. The watchdog only checked `kill -0`, so it saw the live (but
wedged) daemon as healthy and did nothing. The failure was unrecoverable
remotely.
Three layers of defense:
1. Bound the actual send.
- telegram: give the bot a 60s HTTP client timeout (above the 30s
getUpdates long-poll, below the client deadline). bot.Send ignores
context, so this transport-level cap is the real fix.
- slack: switch outbound PostMessage -> PostMessageContext so it honors
the daemon's deadline.
- daemon: wrap each send in a 45s context.WithTimeout.
2. Client can't hang either. send_user_message / send_user_file set a 90s
conn.SetDeadline, so a wedged handler can't hang the helper (or the
runtime blocked on it). Layered: 45s send < 60s transport < 90s client.
3. Readiness, not just liveness.
- new `goated daemon status --probe` does a bounded socket round-trip;
exits non-zero if the daemon is down or alive-but-wedged.
- watchdog probes every run and force-restarts on an unresponsive socket,
reaps send_user_message/send_user_file helpers older than 10 min, and
takes a lock so overlapping runs don't fight during a restart.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
When the runtime session is wedged or busy, the owner had no way to recover
remotely — every path ran through the stuck session. This adds owner-only
"/admin ..." commands that the Telegram connector handles directly, before a
message is routed to the runtime, so they work even when the session is stuck.
- internal/adminctl: connector-agnostic command core. Parse recognizes
"/admin <sub>"; Execute runs it and returns a reply plus an optional deferred
action (so the reply is delivered before a self-terminating restart):
/admin status — daemon pid, host, stuck-helper count
/admin reap — SIGKILL stuck send_user_message/file helpers (via pgrep)
/admin restart — detached `goated daemon restart`
/admin help — list commands
- telegram: decouple the receive loop from the (blocking) runtime handler with
a single worker goroutine — mirroring the existing Slack design — so the
receive goroutine stays free to service /admin commands while the worker is
blocked on a busy/wedged runtime. Commands are honored only from the
configured AdminChatID (GOAT_ADMIN_CHAT_ID); other traffic falls through to
normal processing.
- wire cfg.AdminChatID into the Telegram connector.
Slack already decouples via msgQueue; adding the same interception there is a
straightforward follow-up. Disabled by default when GOAT_ADMIN_CHAT_ID is unset.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
When a claude -p run is interrupted before an assistant response is written, the stored session JSONL ends on a user-turn entry (UUID format). On the next --resume, Claude Code passes that UUID as diagnostics.previous_message_id, which the API rejects with a 400 because it must start with msg_. Detect this specific error in the background goroutine, drop the stored session_id so the retry runs without --resume, and register the pattern in DetectRetryableError so the message is automatically retried. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Removes the reap command and the pgrep-based helper-process machinery (including the stuck-helper count in /admin status). The watchdog already reaps stuck send_user_message/file helpers, so this was redundant. Leaves a general escape hatch: status, restart, help. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add an optional per-cron model override so each cron job can run with a specific model, e.g.: ./goat cron add --chat <id> --schedule "0 8 * * *" --prompt "..." --model claude-haiku-4-5 An empty/unset model preserves current behavior (daemon default model). - db: add CronJob.Model field, thread it through AddCronWithNotifications, and add SetCronModel for editing existing jobs - cli: add --model flag to `cron add`, a `cron set-model` command, and show model=<id> in `cron list` output when set - runtime: add HeadlessRequest.Model; the cron runner passes job.Model through, and each headless runtime (claude, claudetui, codex, codextui, pi) applies the per-run override, falling back to its configured default - docs: document --model / set-model in workspace/GOATED_CLI_README.md Verified: gofmt, go build ./..., go vet ./..., ./build.sh, codex/codextui unit tests, and a manual cron add/list/set-model smoke test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The TUI health check greps the last 20 rendered pane lines for auth-error strings. That text is transcript history, not current state: after an auth incident resolves (token refreshed), the old 401/"Please run /login" output stays on screen indefinitely, so GetHealth kept reporting a non-recoverable "login expired" — crash-looping the daemon via runtime validation even though credentials were valid the whole time. Now, when auth-error text matches, GetHealth and GetSessionState consult the on-disk OAuth token ($CLAUDE_CONFIG_DIR/.credentials.json expiresAt). An unexpired token refutes the pane text: the failure is downgraded to recoverable so callers restart the session — clearing the stale pane while --resume preserves the conversation — instead of demanding a manual /login. Missing, unreadable, or expired credentials (macOS Keychain, API-key auth) leave behavior exactly as before.
Review found two gaps in the credentials-file cross-check: a token can be revoked server-side while still unexpired on disk, in which case (a) the recoverable downgrade caused indefinite restart churn — a freshly restarted pane is clean until traffic hits it, so ensureHealthySession always declared recovery after one restart and the admin escalation never fired — and (b) GetSessionState fell through to dispatch, losing user messages silently since the retry detector only matches 5xx errors. Auth-error text plus an unexpired token now triggers a definitive check: a minimal headless claude -p request (same invocation conventions as internal/subagent). Probe success proves the pane text is stale — report healthy and let normal traffic scroll it away; no restart needed. Probe auth failure restores the original non-recoverable escalation. Verdicts are cached (5 min positive, 1 min negative, inconclusive never) and invalidated on session restart; GetSessionState only ever reads the cache, since it runs in 2-second polling loops.
Review demonstrated by mutation that the credentials and probe wiring was untested: hardcoding credentialsUnknown at GetHealth's call site — an exact revert of the incident fix — passed the entire suite, as did inverting the BlockedAuth gate in GetSessionState. Extract healthFromSnapshot and classifySessionState so both classifications are testable without a live tmux server, driving the real on-disk credentials lookup via CLAUDE_CONFIG_DIR. Both mutations now fail the new tests.
Round-2 review found the probe could wedge the whole daemon: claude spawns children that inherit the output pipes, so after the 25s context kill of the direct process, CombinedOutput blocked indefinitely on pipe EOF — and probeMu was held across the probe, so GetHealth, GetSessionState polling, RestartSession, and ResetConversation all queued behind the hang with no escape. The probe now runs in its own process group, kills the group on cancellation, and caps pipe drain with WaitDelay; probe execution is serialized on a dedicated probeRunMu while the verdict cache keeps its own mutex that is never held across a probe, keeping cachedAuthState and invalidateAuthProbe wait-free (covered by a liveness test). Also raise the positive verdict TTL to 15 minutes: at 5 it exactly matched the gateway's post-send polling window, so a verdict could expire mid-wait of a long-running task with stale auth text still on screen, flipping classifySessionState to a false BlockedAuth mid-dispatch. New mutation-verified tests pin the two remaining wiring contracts: both session-recycle paths must invalidate the cache, and GetHealth's probe must land in the same cache GetSessionState reads.
Round-3 review showed the TTL raise alone did not establish the no-expiry-mid-dispatch invariant: verdict lifetime runs from probe time and cache hits never extend it, so a dispatch admitted at minute 11 of a 15-min verdict could still watch it lapse inside the gateway's 5-min post-send polling window — reproducing the false BlockedAuth this branch exists to prevent. GetHealth's probing path now treats a positive verdict with under 8 minutes of life as a miss and re-probes, so any admitted dispatch holds a verdict that outlives the polling window plus dispatch overheads. Expiry is never slid on reads — a verdict stays at most 15 minutes old, keeping the revoked-token window bounded. Negative verdicts renew naturally via their 1-minute TTL. A fake-clock seam (nowFn) pins all three timing rules; mutations shrinking the positive TTL, dropping negative-verdict caching, or removing the renewal all fail the new test.
Round-4 review showed the renewal window only covers one gateway polling window, but sendWithRetry chains up to three 5-minute windows after a single GetHealth admission — and the worst case exceeds the whole verdict TTL, so no renewal constant can guarantee a verdict outlives a retried dispatch. Close it at the consumption point instead: when WaitForAwaitingInput sees BlockedAuth, it re-checks via confirmBlockedAuth — unexpired on-disk token plus a passing probe (cached or fresh) refutes the block and re-arms the cache, so the next poll classifies normally; a failed or inconclusive verification lets the block stand. This bounds the false-escalation window regardless of dispatch length while keeping classifySessionState itself probe-free.
Round-5 review found the safety branch untested by mutation: weakening the comparison so an inconclusive probe refutes the block survived the suite — under which a genuine outage whose re-verification probe times out would be swallowed instead of surfacing login-expired. New subtests cover the inconclusive-probe and unknown-credentials cases; the mutation now fails.
This was referenced Aug 23, 2026
Review feedback on Endgame-Labs#33 (alan-botts): the accepted MIME scope was wider than the user-facing promise, and the new text path had no test coverage. isTextLikeMIME accepted any "text/" prefix, so text/html, text/x-python and text/x-shellscript were all admitted even though the supported-uploads message only offers "images, PDF, CSV/TSV, DOCX, XLSX, and text files (TXT, MD, JSON, YAML, LOG)". Replace the prefix with an explicit allowlist of those formats plus the aliases clients realistically send for them (text/markdown, text/x-markdown, text/json, text/yaml, text/x-yaml, application/x-yaml). Drop application/javascript, application/typescript, application/toml, application/xml and application/x-ndjson: nothing in the repo depends on them and none appear in the promise. The accepted set and that sentence now agree, so the sentence is left as is. Also drop .xml and .toml from textLikeExts. Neither is in allowedAttachmentExts, so both were unreachable once the MIME prefix went away. Tighten the sniffed-content branch for text uploads to require a text/ type. Real text always sniffs as some text/ type, so accepting application/octet-stream there let an executable renamed to notes.txt through the safeguard. The CSV/TSV branch keeps its octet-stream fallback untouched — Excel exports UTF-16 without a BOM and would otherwise regress. Tests cover accepted TXT/MD/JSON/YAML/LOG, a text extension whose bytes sniff as PDF or as an executable, ZIP and executable uploads, and declared types now outside the promise. They exercise isAllowedByMetadata and isAllowedByContent directly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Contributor
6 tasks
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
5 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.
Reviewed integration release for #32 through #39.
Included:
Final integrated verification: go test ./..., go test -race ./..., go vet ./..., ./build.sh, git diff --check, bash -n scripts/watchdog.sh, and shellcheck scripts/watchdog.sh all pass.