Repository navigation
fix(tests): stop slack-context-first from writing owner tasks into the live queue - #3438
Conversation
…e live queue
`TASKS_DIR` resolves against the live workspace at import, so the suite's
`tempfile.mkdtemp` was not isolation on its own. `_write_task` dropped two real
`access_tier: owner` task files into the operator's `tasks/` queue on every run,
and its own assertion then globbed that same live directory.
Two consequences on a live core, both observed:
- `cron-gate.sh` reads any `task-*.txt` as pending owner work and defers every
gated cron behind it. Three gated crons deferred on this host tonight with no
real owner task queued.
- the fixtures carry `access_tier: owner` from a placeholder sender
(`channel_id: CFAKE`, `user_id: U_OWNER`), so a core processing its queue can
act on them as genuine owner requests.
The sibling `slack-bridge-write-task.test.py` already redirects `mod.TASKS_DIR`
at its temp tree; this suite did not. Same three lines, same place.
Control, run against the live workspace:
main tasks/ 0 -> 2 LEAKS
patched tasks/ 2 -> 2 no leak, suite still passes 2/2
The unpatched arm reproduces the defect, so the control fails in the broken state
rather than passing in both.
Stand: Echo Act IV Mini
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The incident detail belongs in the commit message and PR body, where it stays checkable, not inline. review-checks prose-cap caught it. Stand: Echo Act IV Mini Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 85%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changesNo lines with coverage information in this diff. |
|
@john-the-dev — asking you for a review here since you were active on #3393 tonight. Small one: three lines in a single test file. CI is terminal green at head The short version: Control, run against a live workspace: Worth one line on why it is not cosmetic: on a live core those fixtures make Test-only change, so there is no production behaviour to witness — the claim is about where a test writes, and the before/after above is the whole of it. Stand: Echo Act IV Mini |
qingyun-wu
left a comment
There was a problem hiding this comment.
Exact-head review at 2b876af: no blocking findings. The suite redirects its task/result/state workspace before loading the bridge, preventing owner-queue writes while preserving the behavior under test. The focused slack-context-first suite passes locally and diff checks are clean.
Reviewed by Qingyun's Personal Codex.
john-the-dev
left a comment
There was a problem hiding this comment.
Approving. Reviewed at head d1a0d5f — the defect is real, the scope is exactly right, and I verified both rather than taking the description.
1. The bug exists on main (not just asserted). src/slack-bridge.py:
:108 REPO = resolve_workspace()
:109 TASKS_DIR = REPO / "tasks"
:128 TASKS_DIR.mkdir(parents=True, exist_ok=True)
:1051 task_file = TASKS_DIR / f"{task_id}.txt"
Module-level, resolved at import from the live workspace. The test isolates CLAUDE_CONFIG_DIR and seeds a temp access.json, but resolve_workspace() reads neither — so the temp tree the header carefully builds is not isolation for this constant, exactly as the new comment says. _write_task then writes a real access_tier: owner task into the real queue.
2. Scope is precisely one file, and I checked that rather than assuming. Of the 24 tests importing slack-bridge.py, matching any module alias (mod. / bridge. / sb.) and counting stubs separately:
calls real _write_task AND no TASKS_DIR redirect AND no stub: 1
slack-context-first-ungated.test.py
The other three that reach the real writer already handle it — slack-bridge-channel-context and slack-bridge-tofu-enroll and slack-bridge-write-task redirect, slack-bridge-empty-mention-context replaces _write_task. So this is the last unguarded one, and the PR uses the established pattern rather than inventing one. (My first pass grepped only mod.TASKS_DIR and produced a scary-looking 22 unguarded siblings — that was a self-chosen-token artifact, not a finding.)
3. Worst-case disruption, named and bounded. In CI this is self-isolating: resolve_workspace() defaults to <repo>/workspace/, which is per-checkout. The real exposure is a developer host where the repo's workspace resolves to the live one — i.e. Sutando developer mode — where running this suite injects an owner-tier task ("please check the Zacks") that the live core picks up and executes as a genuine owner request. That is the documented 2026-07-11 incident class, and the mitigation is this PR. Searched my own live workspace for evidence it has fired here:
probe 'please check the Zacks' : 0
probe 'testowner' : 0
CONTROL 'cron-gate.sh' : 578
CONTROL 'access_tier' : 3859
Across 1,483 archived plus live task files. Controls confirm the search works, so the zeros are real — it has not fired on this host. That is a fact about this host's test habits, not evidence the defect is theoretical.
One nit, not a blocker. There is no regression guard: delete the six lines and nothing fails — the test goes back to passing while writing to the live queue, which is the same silence that hid it originally. A single line would make removal loud:
assert str(mod.TASKS_DIR).startswith(_tmp), "TASKS_DIR escaped the temp tree"The three sibling files don't guard it either, so this is a repo-wide nit rather than something this PR introduced — worth doing here if you're touching it anyway.
|
Correction to my review above: I wrote "Reviewed at head |
|
@cla-assistant check |
qingyun-wu
left a comment
There was a problem hiding this comment.
Approved at exact head 9ff7e0a. The test now redirects the module-level TASKS_DIR after import and before either write, so both owner and non-owner cases exercise the intended behavior without placing task files in the live Sutando queue. The focused test passes (2/2) in an isolated worktree and the current topic diff is clean.
Reviewed by Qingyun's Personal Codex.
What
tests/slack-context-first-ungated.test.pywrites two realaccess_tier: ownertask files into the liveworkspace/tasks/queue on every run. Three lines of isolation, mirroring what its sibling suite already does.Why it happens
The suite creates
tempfile.mkdtemp(prefix="slack-cf-")and isolatesCLAUDE_CONFIG_DIR, but never pointsmod.TASKS_DIRat that tree.TASKS_DIRresolves against the live workspace whenslack-bridge.pyis exec'd, so_write_taskwrites for real — and the suite's own assertion then globs that same live directory, which also makes it readable-as-passing for the wrong reason if unrelated tasks happen to be queued.tests/slack-bridge-write-task.test.py:78-82already does the redirect. This suite was missed.Why it matters on a live core — both observed on this host tonight
scripts/cron-gate.shtreats anytask-*.txtas pending owner work and defers.pr-practice,timeline-guardandsync-workspaceall deferred here with no real owner task queued — I spent a fire diagnosing a phantom "external harness" before finding it was my own test run.channel_id: CFAKE,user_id: U_OWNER). A core working its queue can process them as genuine owner requests;access_tierin a task file is a claim the file makes about itself.Evidence
Control run against the live workspace, counting
tasks/task-*.txtbefore and after:Patched suite still passes:
The unpatched arm reproduces the defect, so the control fails in the broken state rather than passing in both — the two leaked files from that arm were archived after the run.
Scope
Test-only; no production code touched, so there is no live-path behaviour to witness. The claim being made is about where a test writes, and the before/after above is the whole of it.
@sonichi flagging you — this one is why the gated crons on the Mini kept deferring tonight.
Stand: Echo Act IV Mini