fix(addie): run wg-slack-context hourly with daily dedup so business-hours gate cannot starve it#5831
Conversation
…ess-hours gate cannot starve it The scheduler's business-hours gate silently skips ticks and setInterval anchors tick time to boot time, so a 24h-interval job that boots outside 9-17 ET never runs (tonight's secret-set restart anchored it at 23:45 ET). Hourly interval + one-refresh-per-day dedup keyed on the Generated date already stamped in the digest file, checked on both the PR branch and main. Same shape wg-digest uses. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ef60014 to
5757882
Compare
There was a problem hiding this comment.
Correct fix for a real starvation bug. Hourly tick + daily dedup is the right shape: it decouples the run cadence from setInterval's boot-anchored tick so the silent business-hours gate can no longer permanently skip a 24h job.
The root-cause story checks out — code-reviewer confirmed against scheduler.ts that runJob skips out-of-window ticks without rescheduling and that the interval anchors to boot time, so a 24h job booted at 23:45 ET never re-enters the 9–17 window. wg-digest's hourly-tick-with-internal-cadence pattern is the established remedy; this follows it.
Things I checked
- Dedup is race-safe against the real cases.
wg-slack-context.ts:419-431reads today'sGenerated:date fromPR_BRANCHthenmain; first in-window tick writes it, every later tick that day short-circuits before any Slack or LLM call. No same-day double-run: the dedup date (L419, UTC) and the written date (L387, UTC) are consistent, and the 9–17 ET window never straddles UTC midnight in EST or EDT, so the UTC calendar date is stable across the whole run window. - No cross-day starvation of an unmerged PR. Yesterday's open PR carries yesterday's date on
PR_BRANCH, so today's tick proceeds, force-resets the branch to base head (github-pr.ts:112-131), re-commits with today's date, and reuses the open PR via the list-then-create path (github-pr.ts:162-173). Daily refresh survives an open PR. - Fail-closed on privacy.
wg-slack-context.ts:447-451sweeps a channel only whengetChannelInforeturnsis_private === false;null/unknown/private are all excluded.security-reviewerconfirmedgetChannelInforeturnsnullon any API error, so an unresolvable channel is never swept. No private-channel leak into the public repo. - Injection containment holds. Untrusted Slack text → LLM →
.agents/wg/slack-context.mdnever reaches an approving desk without a human. The digest PR trips the.agents/*sensitive-path gate;security-reviewerverified Argus posts a comment and does not run the LLM review on the digest, and desks read it only from the mergedmaincheckout.cleanSlackText(L310) andstripResidualMentions(L324) are secondary hygiene — the load-bearing control is the human-merge gate. - Credentials.
GITHUB_TOKEN/ANTHROPIC_API_KEYare env-only, token sent asBearerand never logged;refisencodeURIComponent-wrapped,pathis the hardcodedCONTEXT_FILE_PATH, host is alwaysapi.github.com. No SSRF, no secret in the error-body logs. - Test-plan honesty. The two new unit tests are behavior tests, not implementation tests: the same-day case asserts the short-circuit fires before
getChannelHistory/messagesCreate(wg-slack-context-job.test.ts:754-756), the prior-day case asserts it proceeds to a PR. The unchecked prod checkbox ("first digest PR appears during today's window") is inherently post-deploy — it can't be validated pre-merge, so leaving it unchecked is honest, not a skipped verification. No changeset needed: Addie server, not published wire.
Follow-ups (non-blocking — file as issues)
- Pin
CONTEXT_FILE_PATHunder.agents/.security-reviewerLow 1: the whole injection-containment guarantee rests on the digest file matching the.agents/*sensitive-path glob inai-review.yml. A future refactor that moved it (e.g. todocs/) would silently let Argus auto-review Slack-derived content. Add a unit assertion thatCONTEXT_FILE_PATH.startsWith('.agents/')so the invariant fails loudly, not silently. no-material-changecompares againstmainonly (wg-slack-context.ts:507), notPR_BRANCH. Narrow edge: content equal to main but an open PR carrying a stale body skips and leaves that PR open. The daily force-reset normally supersedes it; a one-line comment on the asymmetry would help.
Minor nits (non-blocking)
- Silent per-ref catch in the dedup loop.
wg-slack-context.ts:427-430swallows all read errors and falls through to a full Slack+LLM run. On a transient GitHub 5xx during the window that means a full sweep every hour until GitHub recovers, invisibly. Alogger.debugin the catch would make starving-vs-thrashing distinguishable in prod. - Scheduler re-entrancy (not this PR).
job.executingis set but never checked inrunJob, so the 14-minuteinitialDelaysetTimeoutand the hourly interval can firerunJobconcurrently. The daily dedup makes a concurrent second run mostly a no-op and the PR-list reuse prevents a duplicate PR, so worst case is one redundant sweep. Flag for the scheduler owner.
Interesting choice to have the job re-anchor via daily dedup rather than fix setInterval's boot anchoring at the scheduler — but given wg-digest already lives with the same trap, matching the established pattern is the smaller change that closes the loop.
Approving on the strength of both expert verdicts (code-reviewer: sound-with-caveats; security-reviewer: ship, no High/Medium) plus the fail-closed injection and privacy design. Follow-ups noted.
There was a problem hiding this comment.
Clean fix for a real starvation bug. Right shape: hourly tick with internal cadence logic is the pattern wg-digest already uses, and it defeats the boot-time setInterval anchoring that the 24h+window registration couldn't survive.
Things I checked
- Premise holds.
getFileContent(github-pr.ts:63-82) returnsnullon 404 and throws only on non-404 non-ok — so thetry/catchatwg-slack-context.ts:172-186correctly falls through on a genuine outage, and theexisting?.includes(...)guard is null-safe on a missing file. Fail-open here is acceptable: theno-material-changegate at L243-246 still prevents duplicate PR content. - Gate ordering. Dedup guard sits after the
missing-envcheck (L163-167), so it only fires GitHub reads onceGITHUB_TOKENis confirmed present. Correct. - UTC-date vs the window.
Generated:usestoISOString().slice(0,10)(UTC). The UTC date rolls at ~19:00-20:00 ET, after the 9-17 ET window closes — so the date is constant across a single business-hours window and the guard can't split a business day. Right call. - No runaway PRs. Hourly ticks reuse the open PR / force-reset the branch (
github-pr.ts), so the interval bump doesn't spawn duplicate PRs. - Tests: same-day dedup short-circuits before
getChannelHistory/messages.create; prior-day digest proceeds toupsertFilePr.already-ran-todayadded to theskippedunion (L44). 16/16 pass per the description. code-reviewer: sound-with-caveats, no blockers.
Follow-ups (non-blocking — file as issues)
- Quiet-day LLM/Slack re-run. The guard's comment claims it dedups "before any Slack or LLM call," but that guarantee only holds on days that produce a PR.
Generated:is written solely insidecomposeFile, which runs only when a PR commits. On a genuinely-quiet day the body equals main's and the job returnsno-material-change(L243-246) without persisting today's date — so every subsequent hourly tick re-runs the full Slack sweep + Anthropicmessages.create(~8×/quiet day). Not runaway PRs, but runaway cost. Persist a per-day marker independent of content change, or document the quiet-day cost. (code-reviewerflagged this; verified against L234-246.) - Add a test for the guard ×
no-material-changeinteraction — the untested path is the one most likely to bite.
Minor nits (non-blocking)
- Log the swallowed error. The
catch {}atwg-slack-context.ts:181is safe by design, but alogger.debuginside it would surface a real GitHub auth/outage that silently forces a re-run.
Test plan
The unchecked box — first prod digest PR during today's 9-17 ET window — is inherently post-deploy and can't be verified pre-merge; the unit tests cover the code path this PR changes. Follow-up, not a hold.
No changeset needed: Addie server work, no static/schemas/source/** or wire surface touched.
LGTM. Follow-ups noted below.
Summary
Follow-up to #5811, found while verifying the first prod run after the
GITHUB_TOKENupgrade. The scheduler's business-hours gate silently skips ticks (scheduler.tsrunJob), andsetIntervalanchors tick time to whenever the app booted. A 24h-interval job registered with a 9–17 ET window therefore never runs if the machines boot outside the window — which is exactly what happened: tonight's secret-set restart anchored the daily tick at ~23:45 ET, permanently outside the gate. (Existing 24h+window jobs likeprofile-completion-nudgesurvive this only because frequent deploys keep re-anchoring their ticks during working hours.)Fix
wg-digestuses: hourly tick, internal cadence logic).Generated:date already exists on the PR branch or onmain, the tick is a no-op (skipped: 'already-ran-today') before any Slack or LLM call.Test plan
tsc --noEmitclean; precommit on commit.addie/wg-slack-contextduring today's 9–17 ET window after this deploys.No changeset — Addie server work.
🤖 Generated with Claude Code