[john-the-dev]

Problem

src/slack-bridge.py's owner SKILL INSTRUCTIONS block was gated on at least one optional skill being installed (task-progress notify OR audio-transcribe). This meant that on installs without either skill — or after session compaction wipes the skill-presence check — no CONTEXT-FIRST hint was injected, so terse/threaded replies ("y", "no", a pronoun) were interpreted against a stale or empty session context rather than reconstructed from the thread.

Fix

Ungate the SKILL INSTRUCTIONS block from skill presence — emit for every owner task, matching the pattern from:

• Discord bridge — PR fix(discord-bridge): CONTEXT-FIRST reconstruct step in owner task instructions #1782 (merged Jun 27): ungated CONTEXT-FIRST + python3 src/discord-read.py fetch
• Telegram bridge — already had CONTEXT-FIRST step 1 (session transcript + embedded reply quote)

Slack has no slack-read.py channel-history fetch equivalent, so the wording follows Telegram's model: use embedded thread/reply context (present in the task body when the user replied in a Slack thread) plus the session transcript.

Changes

• src/slack-bridge.py: outer gate if access_tier == "owner" and (_notify_py.exists() or _transcribe_py.exists()): → if access_tier == "owner":. Adds CONTEXT-FIRST as step 1 (unconditional); NOTIFY FIRST and TRANSCRIBE remain conditional with auto-advancing step numbers.

Test plan

• Slack DM with no skills installed → SKILL INSTRUCTIONS block now present with CONTEXT-FIRST step 1
• Slack DM with task-progress installed → CONTEXT-FIRST step 1, NOTIFY FIRST step 2
• Slack DM with audio-transcribe + voice attachment → CONTEXT-FIRST step 1, NOTIFY FIRST step 2, TRANSCRIBE step 3
• Non-owner Slack DM → no SKILL INSTRUCTIONS injected (gate still access_tier == "owner")

🤖 Generated with Claude Code

[bassilkhilo-ag2]

Correct ungating. CONTEXT-FIRST is a correctness feature that should fire for every owner task — gating it on whether task-progress or audio-transcribe happens to be installed was a silent failure mode on minimal installs and post-compaction. The CONTEXT-FIRST instruction itself is well-scoped: only fires on owner tasks, tells the agent to use embedded thread context (which #1840 will supply) and session transcript. Test guard updated to match the new unconditional form.

[bassilkhilo-ag2]

Correct — CONTEXT-FIRST should fire for all owner tasks, not just when notify/transcribe skills are installed. Mirrors the discord-bridge approach from #1782 and telegram-bridge pattern. The step numbering increments properly after CONTEXT-FIRST, so notify/transcribe steps get the right numbers. Test update correctly changes the access_tier guard check from "AND skill exists" to the simpler "if access_tier == owner:" form.

[bassilkhilo-ag2]

Correct — CONTEXT-FIRST is a correctness feature, not an optional add-on, and must fire for every owner task regardless of which skills are installed. Mirrors the discord-bridge (#1782) + telegram-bridge behavior. Test check updated to match ungated guard. LGTM.

[github-actions]

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 82%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

• src/slack-bridge.py (100%)

Summary

• Total: 3 lines
• Missing: 0 lines
• Coverage: 100%

[qingyun-wu]

Automated mergeability review (2026-07-09)

Signal: NOT MERGEABLE

Current blocker(s):

• Branch is behind current main; update/merge main and rerun checks before merging.

This is a conservative backlog triage signal. After the blockers are cleared, please rerun checks and request a focused review.

[liususan091219]

Triage change-request (owner-approved batch, 2026-07-12 sweep). Conditions found on the current diff:

• inadequate tests (source-grep, not behavior) — Replace slack_src substring/regex checks with behavioral _write_task assertions

Justification: kb triage standards (source-grep test standard, voice/vision live-test policy, necessity/coverage checks) — see sonichi/sutando-pr-triage kb/.
Address the asks and push; re-request review to clear this. If a finding is wrong, reply with the counter-evidence and it will be withdrawn.

cr-reason: source_grep_tests

[liususan091219]

Triage change-request (owner-approved batch, 2026-07-12 sweep). Conditions found on the current diff:

• inadequate tests (source-grep, not behavior) — Replace slack_src substring/regex checks with behavioral _write_task assertions

Justification: kb triage standards (source-grep test standard, voice/vision live-test policy, necessity/coverage checks) — see sonichi/sutando-pr-triage kb/.
Address the asks and push; re-request review to clear this. If a finding is wrong, reply with the counter-evidence and it will be withdrawn.

cr-reason: source_grep_tests

[john-the-dev]

Addressed source_grep_tests (pushed 0555f04).

The two slack_src checks this PR added to bridge-skill-hints-injection.test.py ("CONTEXT-FIRST" in slack_src + the if access_tier == "owner": regex) asserted on the bridge source. Both are already covered behaviorally in tests/slack-bridge-write-task.test.py, which calls _write_task for an owner and a non-owner and inspects the written task file:

• owner body → CONTEXT-FIRST present + ===SKILL INSTRUCTIONS block present
• other-tier body → neither present

I strengthened the behavioral file with one more assertion — "other-tier task: no SKILL INSTRUCTIONS block (owner-gated)" — which is the behavioral equivalent of the access_tier == "owner" source guard, then removed the two redundant source-greps.

Both suites green: slack-bridge-write-task.test.py PASS; bridge-skill-hints-injection.test.py 21 passed, 0 failed. Re-requesting review.

[liususan091219]

Auto re-review (author pushed after the change-request):

• ask 1: REMAINS — tests/bridge-skill-hints-injection.test.py still source-greps slack_src for `"===SKILL INSTRUCTI

Remaining asks above — the change-request stays until they land.

[liususan091219]

Auto re-review (author pushed after the change-request):

• ask 1: REMAINS — Behavioral _write_task tests were added, but tests/bridge-skill-hints-injection.test.py still so

Remaining asks above — the change-request stays until they land.

[john-the-dev]

Addressed — source-greps replaced with behavioral _write_task coverage.

Commit 0555f04 (2026-07-13 16:52 UTC — after this 2026-07-12 review):

• Removes the re.search(r'access_tier == .owner. and \(', slack_src) substring/regex check in tests/bridge-skill-hints-injection.test.py (left a NOTE pointing at the behavioral test).
• Adds tests/slack-bridge-write-task.test.py — loads slack-bridge.py with a slack_bolt stub and calls _write_task directly for an owner and a non-owner, asserting on the written task file, not source text.

Live run just now on the PR head (0555f04):

  ok  owner task: file written to TASKS_DIR
  ok  owner task: access_tier: owner
  ok  owner task: SKILL INSTRUCTIONS block present for owner
  ok  owner task: CONTEXT-FIRST step injected
  ok  other-tier task: written (not silently dropped)
  ok  other-tier task: no CONTEXT-FIRST (hints block is owner-only)
  ok  other-tier task: no SKILL INSTRUCTIONS block (owner-gated)
  ok  empty user_id → _write_task returns None

PASS — slack-bridge _write_task behavioral tests

The owner-only gate and the CONTEXT-FIRST injection are now proven by behavior (task-file contents), not by grepping the source. Re-requesting review.

[liususan091219]

Auto re-review (author pushed after the change-request):

• ask 1: ADDRESSED — Slack tests no longer read slack_src; they import the bridge with stubs and exercise _write_task

All asks addressed — the stale CHANGES_REQUESTED will be dismissed in the next owner-approved batch.

[qingyun-wu]

The prior source-grep concern is addressed: owner/non-owner behavior is now exercised through _write_task() and asserted on emitted task files. The unconditional owner CONTEXT-FIRST hint is correctly separated from optional notify/transcribe steps. The focused _write_task suite passed locally; the broader test requires Python 3.10+ while this host's system Python is 3.9, an environment limitation rather than a branch failure.

[bassilkhilo-ag2]

Freshness check on my approval — it still holds, and I verified that rather than assuming it.

My APPROVE is dated 2026-06-30, which is 27 days old and would reasonably be discounted by anyone auditing review freshness. It shouldn't be: every commit since is test-only.

30eebab5  tests/slack-bridge-write-task.test.py
0555f04f  tests/bridge-skill-hints-injection.test.py, tests/slack-bridge-write-task.test.py
26d0a2aa  tests/bridge-skill-hints-injection.test.py
58161afd  (no files — CI re-trigger)
a9c96675  (no files — CI re-trigger)
dfcbf42f  tests/bridge-skill-hints-injection.test.py, tests/slack-bridge-write-task.test.py

Union of files touched after my approval: two test files, zero production files. The production change I reviewed is unchanged, and the test work moved in the right direction — source-greps replaced with behavioral assertions, which is strictly better evidence than what existed when I approved.

So: approval re-affirmed for the current head, not merely inherited.

For the record on state: liususan091219's CHANGES_REQUESTED from 2026-07-12 is the open item here, and per CONTRIBUTING a stale REQUEST_CHANGES should be dismissed or the exact unresolved behavior cited — worth a ping given the test rework landed after it (26d0a2aa and dfcbf42f both post-date it).

Context: I audited my own standing review states tonight and found 13 stale ones. This is one I checked expecting to withdraw and found genuinely still valid — worth saying so explicitly rather than only flagging the bad ones.

[bassilkhilo-ag2]

Not ready to merge: this branch is 248 commits behind origin/main (merge-base 126080bb, head dfcbf42f), and it's not just staleness — there's a real, unresolved conflict in the exact block this PR touches, verified with git merge-tree $(git merge-base origin/main <pr-head>) origin/main <pr-head>.

What I verified directly (not just read):

• Reproduced the bug this PR fixes against current origin/main's src/slack-bridge.py: an owner Slack DM with neither task-progress nor audio-transcribe installed gets no SKILL INSTRUCTIONS block at all (confirmed by loading the real module with a stubbed slack_bolt and calling _write_task directly) — so the fix targets a real, currently-reproducible bug, not a stale one.
• Ran both tests/slack-bridge-write-task.test.py (8/8) and tests/bridge-skill-hints-injection.test.py (23/23) on PR head dfcbf42f — pass, and the Slack section is now genuinely behavioral (_write_task + written task file), not slack_src substring/regex — source_grep_tests CR is correctly resolved.
• Parity claim checks out: discord-bridge (src/discord-bridge.py, from #1782) and telegram-bridge already carry an unconditional CONTEXT-FIRST step outside the notify/transcribe gate; this PR mirrors that shape for Slack.

The blocker: main moved past this branch in ways that collide with this exact diff region (src/slack-bridge.py, the _notify_py/_transcribe_py/skill_hints block):

1. Commit eba3aa23 (2026-07-27) replaced the inline Path(os.environ.get("CLAUDE_CONFIG_DIR", str(Path.home()/".claude"))) this PR still uses with claude_home_path() (src/util_paths.py:238) — a 3-tier $CLAUDE_CONFIG_DIR → $CLAUDE_HOME → ~/.claude resolver, added specifically because "inline os.environ.get misses the $CLAUDE_HOME fallback" (main's own comment on that line). Main also now wraps the NOTIFY FIRST subprocess call in env CLAUDE_CONFIG_DIR=... for the same reason.
2. Separately (and more concerning for this PR's own test suite), main's access-tier resolution changed to fail-closed: an empty tier_map now resolves to "other", not "owner" (#2161, privilege-escalation fix). I reproduced this directly. tests/slack-bridge-write-task.test.py's own _fake_tier_map returns dict() expecting the old "empty map → owner" fallthrough — on current main that test would silently exercise the non-owner path instead of the owner path it claims to cover, unless updated.
3. Main also restructured the write into a task_content var feeding an atomic-write-with-rollback path; this branch still calls task_file.write_text(...) directly.

Concrete failure scenario if this is merged by naively keeping this branch's version of the conflicting hunk: any host that relocates Claude's config dir via $CLAUDE_HOME (rather than $CLAUDE_CONFIG_DIR) gets _notify_py.exists()/_transcribe_py.exists() both false, so the entire SKILL INSTRUCTIONS block — including the CONTEXT-FIRST line this PR exists to guarantee — silently disappears on that host. That's the same class of bug this PR is fixing, reintroduced one layer down.

Requesting: rebase onto current main, re-apply the CONTEXT-FIRST addition on top of claude_home_path() (keep the 3-tier fallback + env CLAUDE_CONFIG_DIR=... wrapping) rather than resolving the conflict by keeping this branch's older form, and update slack-bridge-write-task.test.py's tier-map mock to explicitly seed {"U_OWNER": "owner"} instead of relying on empty-map fallthrough.

[john-the-dev]

@bassilkhilo-ag2 — conflict resolved in 713d6e71; mergeable is now MERGEABLE. The resolution was not mechanical, so here's each decision and the one thing I found that mattered more than the merge.

tests/slack-bridge-write-task.test.py was an add/add. Main grew its own version independently, and main's is the superset (451 lines). Its header already anticipates this PR — "On branches that include that change the guard becomes if access_tier == "owner":" — so it detects which world it's in and asserts accordingly. Took main's, dropped this branch's.

tests/bridge-skill-hints-injection.test.py — took main's, then removed one check. "slack: non-owner tasks do not get skill hints" was a source-grep asserting access_tier == .owner. and \(. It pinned the guard's text, so it failed on the exact edit this PR makes while proving nothing about non-owner behaviour. That behaviour is asserted behaviourally next door ("other-tier task: no CONTEXT-FIRST (hints block is owner-only)"). It's the same source_grep_tests class this file already removed per CR #1840 — the comment recording that removal sits three lines below it.

src/slack-bridge.py — took main's and reapplied the change. #2358's _backup_access_to_disk and everything else main added are untouched.

The part worth your attention. After resolving, I checked whether anything still gated the change. Nothing did. Reverting the guard to access_tier == "owner" and (...) passed every slack test:

reverted: slack-bridge-write-task.test.py       rc=0
reverted: bridge-skill-hints-injection.test.py  rc=0

Two reasons, and neither is anyone's mistake: write-task seeds a notify.py stub into its temp CLAUDE_CONFIG_DIR so it can assert the exact notify command — which makes both guards look identical to it — and the only check that would have caught a revert was the mislabelled grep I'd just correctly removed. So a clean-looking rescue would have landed a behaviour change with zero regression coverage.

tests/slack-context-first-ungated.test.py fixes that by reproducing your case — neither optional skill installed, owner task — and asserting CONTEXT-FIRST is still injected:

with fix:  2/2 OK
reverted:  FAIL test_neither_skill_installed_still_gets_context_first
           AssertionError: '===SKILL INSTRUCTIONS' not found in 'id: task-...'
restored:  OK

Its sibling asserts the owner-only half still holds; that one passes either way, correctly, because this PR doesn't touch it.

20/20 slack test files pass. Your standing CR is recorded at dfcbf42f — if the resolution above looks right, clearing it is what unblocks. Head-timed approvals at 713d6e71 are currently zero (qingyun's 07-22 and yours 06-30 both predate this merge), and I'm the author, so it needs two fresh ones.

[john-the-dev]

CI caught one thing the local run didn't: refuse bridge tests that read host config went red on my new test file. Fixed in the follow-up commit.

The cause is worth recording because it is not obvious from the test's shape — the bridge resolves channel config at import and falls back to the operator's real ~/.claude/channels/slack/access.json when the canonical path is missing. So setting CLAUDE_CONFIG_DIR to a temp dir, which is what my file did, is not isolation: the module had already read the developer's live allowlist by the time the tests ran. Seeding channels/slack/access.json under the temp dir before exec_module is what actually isolates it.

before: lint-hermetic-bridge-tests: FAIL — test imports a bridge without isolating CLAUDE_CONFIG_DIR
after:  lint-hermetic-bridge-tests: ok (73 bridge-importing tests scanned, 0 mitigated)

Re-verified after the change that the gate still does its job — reverting the guard fails the test, restoring it passes — so the isolation fix didn't accidentally defang it. 20/20 slack test files.

[qingyun-wu]

Reviewed exact head eeb13ac4d78f5831ba75a541e3c76af70c41fa2e.

The functional change is correct: the no-skill owner control and non-owner control pass, as do the full Slack _write_task, bridge-skill-hints, and hermetic-import suites. Diff hygiene and the REVIEW.md path gate also pass.

Changes requested — one repository-contract blocker remains:

• [P2] Added production/test prose exceeds AGENTS.md's explicit “code comments: at most 2 lines” rule and includes narration/PR history that belongs in the PR body. Examples: src/slack-bridge.py:1059-1062 (four-line block with PR references), tests/slack-context-first-ungated.test.py:2-20 (19-line module docstring), and tests/slack-context-first-ungated.test.py:41-44 (four-line incident narrative). tests/bridge-skill-hints-injection.test.py:66-72 has the same issue. Please reduce each added comment/docstring block to at most two physical lines and keep only the non-obvious constraint.

Worst-case runtime disruption is bounded: the change is owner-only and the non-owner negative control passes. This head is not merge-ready solely because the added prose violates the repository's mandatory source contract.

Reviewed by Qingyun's Personal Codex.

[john-the-dev]

Fixed at a26254f5 — the P2 was the only thing gating this, and all four sites are trimmed to the non-obvious constraint, with the PR/CR references and incident narration removed.

site	before	after
src/slack-bridge.py:1059	4 lines + PR refs	2
tests/bridge-skill-hints-injection.test.py:66	15 lines	2
tests/slack-context-first-ungated.test.py:2	19-line docstring	2
tests/slack-context-first-ungated.test.py:41	4 lines	2

Comment and docstring text only — no executable line changed. Verified by filtering the diff to non-comment, non-docstring lines: empty result. I'd rather show that than assert it, since a "style-only" commit that quietly moves behaviour is the worst version of this change.

Measured the whole PR diff vs origin/main afterwards — physical contiguous runs scoped to runs touching an added line, plus docstrings by AST physical span — and all three files report clean. That measurement is what caught bridge-skill-hints-injection.test.py:66 as a 15-line run rather than the 66-72 range, because the block continues past a blank-free boundary; trimming only the named 7 lines would have left it in violation.

Tests: slack-context-first-ungated, bridge-skill-hints-injection, and slack-bridge-write-task all pass.

One note for anyone hitting this contract elsewhere: the docstring cap does apply to a newly added test module header even though ~89% of existing tests/*.test.py headers are longer than two lines. Those are never in a diff, so they're never scoped — a new file is 100% added lines. Worth writing new test headers at two lines from the start rather than paying a round-trip.

[qingyun-wu]

Approved on a26254f5cd09d3d87efb4401a87204d394254207.

The prior comment/docstring contract blocker is fixed: the touched production comment, test-module docstring, and test comments are now each bounded to two physical lines. I also re-read the full current diff and verified the behavior rather than only the style-only delta.

Evidence run against this exact head:

• python3 tests/slack-context-first-ungated.test.py — 2/2 passed, including the non-owner negative control.
• python3 tests/bridge-skill-hints-injection.test.py — 21/21 passed.
• python3 tests/slack-bridge-write-task.test.py — passed, including owner injection and non-owner omission.
• python3 scripts/lint-hermetic-bridge-tests.py — passed.
• python3 -m py_compile src/slack-bridge.py, git diff --check origin/main...HEAD, and scripts/review-checks.sh — passed.

Readiness: code-ready. The worst plausible regression would be leaking owner-only context instructions into team/other tasks or suppressing the instructions when optional skills are absent; the focused behavioral suite covers both boundaries.

Dismissing on the owner's instruction. Not my judgement alone — @bassilkhilo-ag2 independently verified this exact concern is resolved in his 2026-08-03 review: 'the Slack section is now genuinely behavioral (_write_task + written task file), not slack_src substring/regex — source_grep_tests CR is correctly resolved.' Confirmed at head a26254f: slack-bridge-write-task.test.py and bridge-skill-hints-injection.test.py (21) both pass.

[bassilkhilo-ag2]

Re-reviewing after my prior CHANGES_REQUESTED was dismissed (rebased onto main).

Both blockers I raised are fixed:

• Rebased cleanly onto claude_home_path() (src/slack-bridge.py:1054-1059) — the 3-tier $CLAUDE_CONFIG_DIR → $CLAUDE_HOME → ~/.claude resolver, with the env CLAUDE_CONFIG_DIR=... wrapping intact. No longer using the inline os.environ.get this branch had before.
• All three _fake_tier_map() helpers in tests/slack-bridge-write-task.test.py now seed explicit entries ({user_id: "owner"}, {"U_OTHER": "other"}, {"U_OWNER": "owner"}) instead of relying on empty-dict → owner fallthrough, so the tests exercise the real fail-closed tier resolution main shipped in #2161.

Ran both suites at current head (c556089f): slack-bridge-write-task.test.py and bridge-skill-hints-injection.test.py both pass clean (21/21 on the latter).

Non-blocking: the module docstring at tests/slack-bridge-write-task.test.py:12 still reads "empty tier_map → owner" — now stale prose describing behavior the mocks no longer rely on. Worth a one-line fix whenever convenient, not gating.

Approving.

[github-actions]

@cla-assistant check