fix(jobs): honor notifyChannel in job frontmatter for Discord routing - #254
fix(jobs): honor notifyChannel in job frontmatter for Discord routing#254BCDel89 wants to merge 2 commits into
Conversation
TerrysPOV
left a comment
There was a problem hiding this comment.
Nice, clean feature. The parsing is consistent with the other frontmatter fields, the routing correctly falls back to the DM-to-allowedUserIds path when notifyChannel is unset, it respects the existing notify: false / error gate, and discordSendToChannel is set up and torn down symmetrically. I confirmed sendMessage is exported from discord.ts, so the dynamic-import destructure resolves. And thanks for adding parse tests.
Three things before merge:
- The channel-routing behavior itself isn't tested — the tests cover
jobs.tsparsing (present / absent), but not theforwardToDiscord→discordSendToChannelpath, which is the actual feature. Please either add a test that exercises the routing (inject a fakediscordSendToChanneland assert anotifyChanneljob forwards to the channel, and that an unset job still DMsallowedUserIds), or demonstrate it live — a job withnotifyChannelset posting its output to the target channel on a real daemon. This is the same bar applied to #253: an output-routing feature should be shown working, not inferred from mirroring the DM path.
claudeclaw/src/commands/start.ts
Lines 642 to 651 in fcaac43
-
Document the allowlist-bypass semantics.
notifyChanneloutput is sent to the named channel and is not gated bydiscord.allowedUserIds— everyone who can read that channel sees the job output. That's the intended behaviour, but it's a sharp edge (an operator could route sensitive job output to a public channel), so it's worth a line in the field's doc comment / the jobs docs so it's a deliberate choice, not a surprise. -
Version is stale — this targets 1.0.41 but master is 1.0.43 (the only thing making the PR conflict;
jobs.ts/start.tsmerge clean). Rebase and re-bump both.claude-plugin/*.jsonto 1.0.44.
Job files could set notifyChannel in frontmatter, but the field was never parsed or read anywhere — forwardToDiscord() always DM'd every allowed user regardless. Parse notifyChannel in parseJobFile and route the completion notification to that channel instead of DMing when set.
Channel-routed output isn't gated by discord.allowedUserIds — anyone with access to the channel sees the job's output. Call that out in the Job interface doc comment and commands/jobs.md so it's a deliberate choice per job, not a surprise.
fcaac43 to
0ad7bea
Compare
|
Addressed: #3 (version stale) — rebased onto current master (1.0.44), re-bumped to 1.0.45. #2 (allowlist-bypass semantics) — documented in the #1 (untested routing) — going with the "demonstrate it live" option rather than a mocked unit test, since the fix has been running in production since the original PR merged and the completion-notification path (not just the agent's own in-job posts) has fired repeatedly:
All landed in the |
Summary
notifyChannelfield intended to route completion notifications to a specific Discord channel.Jobhad nonotifyChannelproperty, andforwardToDiscord()unconditionally DM'd every user indiscord.allowedUserIds, regardless of what the job file said.notifyChannelinparseJobFile(), wire a channel-based Discord sender alongside the existing DM sender instart.ts, and routeforwardToDiscord()to the channel whenjob.notifyChannelis set (falls back to existing DM-all-allowed-users behavior otherwise — no change for jobs that don't set it).Test plan
bun test— 125 pass, 3 pre-existing/unrelated failures (env-dependentsession-files.test.tscases, confirmed unrelated onmaster)src/__tests__/jobs.test.tscoveringnotifyChannelparsing (present + absent)bunx tsc --noEmit— no new errors in touched filesnotifyChannelset and confirm the completion message lands in that channel, not a DM