feat: add Slack as a source via Glean, skipped when unavailable - #54
Draft
Bhekani Khumalo (bhekanik) wants to merge 11 commits into
Draft
feat: add Slack as a source via Glean, skipped when unavailable#54Bhekani Khumalo (bhekanik) wants to merge 11 commits into
Bhekani Khumalo (bhekanik) wants to merge 11 commits into
Conversation
Bhekani Khumalo (bhekanik)
force-pushed
the
feat/m3-slack-source
branch
2 times, most recently
from
August 27, 2026 14:29
d2ad7e2 to
8dd210b
Compare
Slack has no API token in this setup and is not reachable directly. The only route is Glean, and the only client that can reach Glean is the Claude Code CLI with its Glean MCP server connected. So the source spawns `claude` with only the Glean tools allowed, asks one focused question, and parses the answer against a zod schema. That makes it slow and non-deterministic, so it is treated as optional: `isAvailable()` gates on `claude` being on PATH and `claude mcp get glean_default` reporting Connected, and every failure path returns an empty batch with a warning instead of throwing. A reply that does not parse gets one stricter retry before the week gives up on Slack. Messages are filed by their own timestamp and keyed by permalink, so adding this source to an old week amends it rather than rewriting it. Out-of-range, duplicate and over-cap messages are trimmed on our side rather than trusted from the model. The process runner is injected so tests never spawn anything. The 240s timeout is measured: a real week of Glean search plus synthesis took 157s, and the 120s first guess cut real answers off.
Each week's work log gains a `## Slack` section listing the user's own public-channel messages grouped by channel, with threads kept together, and a permalink and UTC timestamp per message. The section and its summary row only appear when there are messages, so a run without Slack produces exactly the bytes it produced before. The weekly command checks availability once per run rather than once per week. When Slack is unavailable it prints one `Slack source skipped: <reason>` line and carries on. Slack is fetched separately from the API sources so a Slack failure never costs the week its Jira, Confluence or GitHub data. The brag prompt now says what Slack material is for: coaching context such as decisions made in the open and people unblocked, not achievement evidence unless Jira, GitHub or Confluence corroborates the same work.
Review found that `--allowedTools` pre-approves tools, it does not remove them.
Under `--permission-mode dontAsk` the child still had Bash, Read, WebFetch and
every MCP server the user has configured, so text in a Slack message could have
told it to read local files or send mail.
Verified against Claude Code 2.1.246: `--tools ""` leaves the child with no
built-in tools at all, and `--strict-mcp-config --mcp-config <glean only>`
leaves it with the Glean tools alone. The Glean server is discovered from
`claude mcp get`, so nothing company-specific is hardcoded; when it cannot be
isolated the source reports itself unavailable rather than falling back to a
wider child.
Glean is permission-aware, but the reply is still model output, so authorship
and visibility are now checked locally too. Messages carry a required `author`
and `channelType`, and anything not written by the configured profile or not in
a public channel is dropped with a count in a warning. Permalinks must be https
on slack.com, and each entry is parsed on its own so one bad entry cannot lose
the rest. The docs say plainly that this is a second check on top of Glean, not
a guarantee.
Other fixes from the same review:
- `Status:` is parsed as a word list rather than tested with `includes`, so
"Not Connected" no longer passes the availability gate.
- Structured output (`--json-schema` + `--output-format json`) replaces brace
extraction on the happy path. The fallback scans for a balanced JSON object
with a depth counter instead of taking the outermost braces, so prose like
`Found {one result}: {"messages":[]}` parses.
- One 240s budget now covers the attempt and its retry, the retry gets only the
time left and is skipped below 60s, and `--verbose` prints the Slack seconds.
- `--no-slack` skips the source, and the availability probe, for a run.
- Slack is timed separately from the API fetch in the stats and the summary.
- Markdown link destinations are escaped.
- The byte-equality test freezes the clock, which the `Generated:` header needs.
- The brag prompt says quoted message text is data, never instruction.
- The README says what reaches Claude Code and Glean when Slack is on.
Verified live: the Claude Code CLI validates --json-schema itself and refuses the draft/2020-12 $schema reference zod emits, so every fetch failed within a second. Stripping it makes the structured-output path work end to end.
Text full of unclosed braces yielded nothing, so the match counter never reached its ceiling and the scan re-ran to the end of the reply from every brace. Counting attempts is what actually bounds the work.
runWorklog's new noSlack option is optional so a caller that predates it still compiles, which means the default path checks Slack availability and would spawn the Claude Code CLI from a test. The source is stood in for as unavailable, matching how that file already stands in for the AI and network.
…ack child Removing tools and MCP servers did not stop settings files loading, so a user or project hook still ran inside the child and a PostToolUse hook would have received every Glean tool response. Measured it: a project UserPromptSubmit hook fires in the child, and with `--setting-sources ""` it does not, while the child still authenticates and answers. So the child now gets `--setting-sources ""`, a freshly made empty temporary directory as its working directory (nothing above it to discover as project CLAUDE.md, settings or memory), and an environment built from an allowlist rather than a copy of this process's. The allowlist is what the child needs to start, authenticate and reach the network through a proxy; the three Claude Code nesting markers fall out of it for free. The Glean endpoint is also checked before it is handed to the child. `Scope:` must be the user's own config, because a local or project entry could shadow the real server and a checked-out repository is a place one could come from, and the `URL:` must parse as https. The hostname is deliberately not checked against a list: every company has its own Glean host and this is a public repo. Verified live after the change: availability ok, one week in 197s, three messages kept, nothing rejected, no warnings. The author check already required the whole normalised name to match rather than a prefix; that is now pinned by a test. `docs/setup.md` names the limit it still has, that two people whose names normalise alike would both pass, and why member ids are not used instead.
…eate The cleanup after each child process is an rm -rf on whatever path the directory factory returned. That was safe only because the factory was always mkdtemp, and it is now provably unsafe to rely on: a mutation test that pointed it at process.cwd() deleted this worktree outright. The path is now checked before it is removed: it has to sit directly under the system temp directory and carry this module's prefix. The factory is injectable so the refusal is tested against throwaway directories rather than by stubbing node:fs, which vitest cannot spy on in ESM anyway.
…ailures survivable
Four things, all about the same idea: an optional source may decline to run, but
it may not take the run down with it, and its cleanup may not touch anything it
did not create.
The directory factory is gone from production code. There is no seam a caller
can hand a path to any more: the temp root is resolved with `realpath` before
creation, the child's cwd is exactly what `mkdtemp` returned, and cleanup
records the created directory's dev and ino with `lstat` and refuses to remove
anything whose identity has changed or which is no longer a directory. Tests
redirect `TMPDIR` instead of injecting, and cover a same-prefix decoy and a
symlink retarget.
Neither failure is fatal now. `TMPDIR` pointing nowhere used to throw straight
out through the weekly command; it becomes `{ ok: false }` from `isAvailable`
and an empty batch with a warning from a fetch. A cleanup that cannot run warns,
leaves the directory, and keeps the result; a directory that is already gone
says nothing.
The child environment keeps `CLAUDE_CONFIG_DIR`, `CLAUDE_CODE_OAUTH_TOKEN` and
`XDG_CONFIG_HOME`. All three appear in the installed CLI, and dropping the first
two would log out anyone who has moved their config or authenticates by token.
The three nesting markers stay out.
The docs no longer claim no hooks run. User, project and local settings are not
loaded, but organisation policy settings are, including any hooks they define,
and those would see the Glean tool responses. Turning them off needs `--bare`,
which never reads an OAuth login, so worklog does not use it and says so.
Mutation testing showed removing it changes no test: a symlink has its own inode and already fails the dev/ino comparison. Kept anyway, because this guards an rm -rf and a reader should not have to derive that.
… remove The dev/ino check did not do what it looked like it did. Stat-then-delete is two path operations, and another process running as this user can swap the directory between them; the recursive delete would then follow the swap and take the replacement with it. No amount of checking makes those two calls atomic. The directory only ever holds a working directory for a child that writes nothing, so it does not need a recursive delete at all. `rmdir` refuses a directory with anything in it and never descends, which closes the race by construction rather than by inspection: the worst a swap can achieve now is the removal of an empty directory someone else put there. That takes the dev/ino machinery and the isDirectory guard with it, both of which existed only to make a recursive delete safe. Anything the child does leave behind survives and is reported as a warning instead of being deleted.
Bhekani Khumalo (bhekanik)
force-pushed
the
feat/m3-slack-source
branch
2 times, most recently
from
August 27, 2026 15:05
34ba82a to
0f4870f
Compare
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
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.
Activity is now recorded as timestamped events in a cache-side ledger, sources are plugins behind one interface, and
worklog refreshpicks up what has changed since the last run and rewrites only the weeks it belongs to.The rule this serves
Before this, a week was fetched as "how these items look right now". Regenerating an August week in September narrated August from September's state — a ticket closed last week read as finished work in a week where it was still in progress.
Ledger layout
Why this shape:
refreshwrites only the weeks whose event set changed, which is the property the whole command rests on.source|id), elsesource|kind|itemId|at. Re-fetching a week matches instead of duplicating, files are only written when their contents differ, and a second run is byte-identical in both the vault and the cache — asserted incommands/__tests__/refresh.test.ts.The frozen
Sourceinterfacelib/sdk/sources.ts, matching what M3 (#54) built against, extended only with optional fields:Two questions and nothing else: what happened in this window (the expensive first look) and what has happened since this moment (the cheap one every later run asks).
worklog refreshAsks each source the delta question, files each answer in the week it happened, and rewrites only the weeks whose event set changed. A run that finds nothing makes no AI call and writes no file. A week that does need rewriting has its existing brag book sent back with only the events this run added, and the instruction is to add to the entry rather than replace it; the vault writers from #47 make re-applying what was already applied a no-op.
The closing table shows what each source contributed per week and how long each source took — Slack's Glean path takes minutes where the HTTP sources take seconds, and a user waiting deserves to know which.
Dating what has no date
A description edit has no timestamp of its own. It is dated at the moment it was found and its payload marked
spotted: true; the work log then carries a## Datingsection saying so plainly, so the model is never quietly told a change happened on a day it did not.Prompt
prompts/weekly-brag-prompt.mdno longer tells the model to fetch the current status of a ticket and describe reality at generation time. It now says the week is a closed record, andgeneration_contextstates whether it is the current week or a past one.Also
getWeekEndreturns Sunday at 00:00 — the start of the last day. Window ends are pushed to the last instant of Sunday in one place (weekWindow) rather than in each source.allSources()is one list, read by both commands.Tests
652 passing, 23 files. New:
lib/sdk/__tests__/ledger.test.ts(28),lib/sdk/__tests__/source-adapters.test.ts(30),commands/__tests__/refresh.test.ts(6), plus 8 inmarkdown.test.ts.anti-slop-check --base origin/mainis down to 3 findings, allno-unknown-parameterson the three functions whose job is to be the parsing boundary (keepParsable,writeJsonAtomic,renderable).Review round 1 (all nine findings addressed)
Every CLI flag was verified against the installed
claude --help(2.1.246). The relevanthelp lines:
--allowedToolsalone leaves the child with Bash,Read, WebFetch and every configured MCP server (Gmail, Playwright and others were all
reachable). The invocation is now
--tools ""plus--strict-mcp-config --mcp-config <glean only>plus--allowedToolsfor the three Glean tools. Verified live: the child's tool listdrops to the eight
mcp__glean_default__*tools and nothing else. The Glean server is read outof
claude mcp getat runtime, so no company-specific URL is hardcoded, and the stored OAuthstill works through the inline config. When the server is not http/sse it cannot be isolated,
so the source reports itself unavailable rather than falling back to a wider child. The argv is
asserted in tests.
authorandchannelTypeare required schema fields, the prompttells the model to fill them from Glean metadata, and anything not written by
profile.fullName/displayNameor not in a public channel is dropped locally with a count ina warning. Identity comparison is on letters and digits only, so
@first.lastmatchesFirst Last.docs/setup.mdsays plainly this is best-effort on top of Glean'spermission-aware index, not a guarantee.
--no-slackadded (commander boolean, skips the availability probetoo). One 240s deadline now covers the attempt and its retry; the retry gets only the time
left and is skipped below 60s.
--verboseprints the per-week Slack seconds.httpsonslack.comor a subdomain. Link destinations are escapedbefore rendering.
Status:line is parsed into a word list that must be exactly["connected"].Not ConnectedandNeeds authenticationare both tested and both rejected.Slack is on, and how to turn it off.
--json-schema(derived from the zod schema, one source of truth) plus--output-format json;structured_outputis read directly. The fallback scans for abalanced JSON object with a depth counter, so
Found {one result}: {"messages":[]}parses —that exact case is a test.
WeekTiming, in the stats file, and as aSlack via Gleanline in the summary breakdown. The API fetch timer stops before Slack.vi.setSystemTime.Also from this round: the brag prompt says quoted message text is data and never instruction, and
the fetch prompt says the same to the child.
Live re-verification after the changes
The first run with the hardened argv failed in one second:
--json-schema is not a valid JSON Schema: no schema with key or ref "https://json-schema.org/draft/2020-12/schema". The CLI validates the schema and rejects the$schemadialect ref zod emits, so it is stripped; there is a regression test. The next run wentthrough end to end: 169s, three messages kept,
0 not public, 0 not yours, 0 malformed, 0 outside the range, zero warnings,authorandchannelTypecorrectly filled from Glean metadata.Checks
bunx tsc --noEmitclean;bun run lintclean;bun run test680 passing across 22 files.Thirteen guards mutation-checked, each turning tests red when removed: the channelType check, the
author check, the permalink host check, the https check, the status word-list check, the shared
deadline, the retry's remaining-time budget,
--strict-mcp-config,--tools "", the$schemastrip, the balanced scan, per-item parsing, and link escaping.
One deliberate collision
anti-slop-checkflags thevi.mockadded tocommands/__tests__/worklog.test.ts. That filealready mocks the AI, the network, the vault writer and clack the same way (six pre-existing
findings). The stub is needed because
runWorklogwould otherwise spawn the Claude Code CLI froma test. Left as-is to match the file rather than making it the odd one out.
Review round 2
1. Hooks and settings in the child — fixed, and measured rather than assumed.
claude --help(2.1.246):Measured it rather than trusting the description. A project
.claude/settings.jsonwith aUserPromptSubmithook, child run with that directory as cwd:So the finding was real, and
--setting-sources ""closes it without breaking auth. Three changeswent in together:
--setting-sources ""in the argv, asserted in a test.mkdtempper call and removed afterwards, so there is noproject
CLAUDE.md, settings or memory above it to discover. A test asserts the cwd is underthe system temp dir, is empty, is not
process.cwd(), is shared by the status probe and thefetch, and is gone afterwards.
HOME,PATH, shell/locale basics,ANTHROPIC_*, proxyand CA variables) instead of a copy of this process's with three keys deleted. The three Claude
Code nesting markers fall out of it for free.
2. MCP endpoint validation — fixed.
resolveGleanServernow requiresScope:to be theuser's own config, because a local or project-scoped entry can shadow the real server and a
checked-out repository is somewhere one could come from, and requires the
URL:to parse ashttps:. The hostname is deliberately not checked against a list: every company has its ownGlean host and this is a public repo. Three tests: plain
httprejected, non-user scope rejected,https user-scope accepted (plus a non-URL rejected).
3. Handle normalisation — documented limit. The check already compared whole normalised
strings rather than prefixes; that is now pinned by a test (
Test Usersonandtestare bothrejected against
Test User/testuser).docs/setup.mdnames the remaining limit: it comparesnames, not Slack member ids, so a colleague whose name normalises to the same string would pass,
and Glean does not reliably expose member ids to compare instead.
Live re-verification
Availability ok, one week in 197s, three messages kept,
0 not public, 0 not yours, 0 malformed, 0 outside the range, zero warnings. Payload keys,channelType: "public", and https*.slack.compermalinks all as expected. The tightened environment and settings isolation do not break OAuth.
One more fix, found the hard way
While mutation-testing the new cwd handling, a mutation pointed the directory factory at
process.cwd(). The cleanup is anrm -rf, so the test run deleted this worktree. Nothing waslost (everything was committed) but the lesson stands: that cleanup was safe only because the
factory happened to always be
mkdtemp. It now refuses any path that is not directly under thesystem temp directory with this module's prefix, and the factory is injectable so the refusal is
tested against throwaway directories.
Checks
bunx tsc --noEmitclean;bun run lintclean;bun run test688 passing across 22 files.Eighteen guards now mutation-checked in total; the five new ones (temp directory removed, the
deletion guard itself, the scope check, the https check, whole-name matching) each turn tests red
when removed.
anti-slop-checkstill reports one finding, the samevi.mockincommands/__tests__/worklog.test.tsreported in round 1, kept to match that file's sixpre-existing mocks.
Review round 3
1. Cleanup can delete a directory it did not create — fixed by removing the seam.
The injectable directory factory is gone from production code, so there is no longer any way for
a caller to hand this an arbitrary path.
withEmptyCwdresolves the temp root withrealpathbefore creating anything, uses exactly what
mkdtempreturned as the child cwd, records thatdirectory's
dev/inowithlstatat creation, and checks them again before removing it.A changed identity, or a path that is no longer a directory, means: warn, leave it, carry on.
Tests redirect
TMPDIRat a sandbox instead of injecting, and cover a same-prefix decoy(a sibling
worklog-slack-*directory that must survive) and a symlink retarget (the directoryis swapped for a symlink pointing at a directory that must survive).
2. Managed policy hooks — documentation corrected. No auth redesign. Every absolute claim
about hooks is gone from
README.md,docs/setup.mdand the source comment. They now say thatuser, project and local settings and hooks are not loaded, that organisation-managed policy
settings still apply including any hooks they define, that those hooks would see the Glean tool
responses, and that disabling them needs
--bare, which never reads an OAuth login and would logout every subscription user, so worklog does not use it.
--no-slackis the answer for anyonethat affects.
3. The optional source can no longer crash the run.
TMPDIR=/nonexistentused to throwmkdtemp's error straight out throughcommands/worklog.ts. Setup failure is now{ ok: false, reason }fromisAvailableand an empty batch with a warning from a fetch; acleanup failure warns, leaves the directory and keeps the result; a directory that is already
gone says nothing. Tests for each, including one asserting nothing is spawned when setup fails.
4. Child environment —
CLAUDE_CONFIG_DIR,CLAUDE_CODE_OAUTH_TOKENandXDG_CONFIG_HOMEadded. All three appear in the installed CLI binary.
CLAUDE_CONFIG_DIRis clearly load-bearing(the bundle carries messages about its behaviour and suggests
CLAUDE_CONFIG_DIR=/tmpforephemeral local writes). The
XDG_CONFIG_HOMEhits are around git and editor keybinding pathsrather than Claude's own config, so its relevance is less certain; it is preserved anyway because
passing it through costs nothing and dropping it would break anyone who relies on it. The three
nesting markers stay out.
childEnvironment()is extracted and directly tested: kept keys,dropped nesting markers, dropped unrelated secrets (
GITHUB_TOKEN,ATLASSIAN_API_TOKEN,AWS_SECRET_ACCESS_KEY), and noundefinedvalues passed through.Live re-verification
Availability ok; one week in 184s; 3 messages kept;
0 not public, 0 not yours, 0 malformed, 0 outside the range; zero warnings;channelType: "public", https*.slack.compermalinks.The realpath change, the identity checks and the widened environment allowlist all hold up
against the real CLI.
Checks
bunx tsc --noEmitclean;bun run lintclean;bun run test696 passing across 22 files.anti-slop-checkreports one finding, the samevi.mockcollision reported in rounds 1 and 2.Seven of the eight new guards are mutation-checked. The eighth, the
isDirectory()test on thecleanup path, provably changes no test: a symlink has its own inode and already fails the
dev/ino comparison. It is kept anyway, with a comment saying so, because it guards an
rm -rfand the intent should be readable without deriving it.
Review round 4
Cleanup is
rmdir, never a recursive remove. The dev/ino check did not do what it lookedlike it did: stat-then-delete is two path operations, and a same-user process can swap the
directory between them, at which point the recursive delete follows the swap and takes the
replacement with it. No amount of checking makes those two calls atomic.
The directory only ever holds a working directory for a child that writes nothing, so it never
needed a recursive delete.
rmdirrefuses a directory with anything in it and never descends,which closes the race by construction rather than by inspection: the worst a swap can achieve is
the removal of an empty directory someone else put there. The dev/ino machinery and the
isDirectory()guard are gone with it, since both existed only to make a recursive delete safe.Behaviour change worth knowing: anything the child leaves behind now survives and is reported as
a warning instead of being deleted.
Tests: an empty directory is removed; a non-empty one is left with a warning and its contents
intact; a same-prefix sibling and a directory reachable only through a symlink inside the cwd are
both untouched; the body's result survives a failed cleanup; an already-removed directory is
silent.
Checks
bunx tsc --noEmitclean;bun run lintclean;bun run test704 passing across 22 files.anti-slop-checkreports the same singlevi.mockcollision as rounds 1 to 3.Six mutations, all red, including the one this round is about: restoring the recursive delete
breaks the non-empty-directory test and the touches-nothing-outside test.
No live probe this round — the change is cleanup only and cannot affect what the child returns.