[sonichi]

The defect

health-check.py:6849 measures correctly and then prescribes half a fix:

⚠ slack-bridge   connected but events not arriving — enable Event Subscriptions at api.slack.com/apps

That is the whole fix only when an owner is already enrolled. When channels/slack/access.json is absent the bridge is in TOFU state, and slack-bridge.py gates enrollment behind a code regenerated at every bridge start:

# origin/main, slack-bridge.py ~:877
global _TOFU_ENROLLMENT_CODE
allowed = load_allowed()
if allowed is None:
    # TOFU state — require enrollment code before auto-onboarding as owner,

So on a TOFU host, doing exactly what the warning says leaves Slack exactly as dead — and the operator now believes it is fixed, which is worse than the warning having said nothing.

Measured on this host: access.json is absent. The machine printing the incomplete remedy is the one that needs the other half.

The change

The remedy branches on the evidence:

• Enrolled — unchanged. It does not gain a step it does not need.
• TOFU — names both, says BOTH are needed, and states what it branched on (access.json absent) so the reader can check the claim.

Status stays warn in both cases. Slack genuinely is not working either way; only the instructions were wrong. (This is deliberately unlike #3059, where the status itself was the defect.)

Enrollment is read via channel_access_path("slack") rather than a path join, because that resolver carries the ~30-day legacy fallback — a host enrolled at the pre-migration location would otherwise be misreported as TOFU and told to re-enroll. It is injectable for tests, and falls back to enrolled if the lookup raises, so a probe failure cannot invent the more alarming remedy.

Evidence

Control at origin/main — module imports cleanly, so the fixture is sound; the test fails on the missing feature:

health-check imports cleanly: True
accepts slack_enrolled: False
TypeError: bridge_log_content_status() got an unexpected keyword argument 'slack_enrolled'

At HEAD — 12/12. Every assertion calls the function and inspects its return value; none greps the source:

PASS  enrolled host still warns
PASS  TOFU host still warns (status unchanged)
PASS  enrolled remedy names Event Subscriptions
PASS  enrolled remedy does NOT add a step that host does not need
PASS  TOFU remedy still names Event Subscriptions
PASS  TOFU remedy names the enrollment step
PASS  TOFU remedy says both are required
PASS  TOFU remedy states the evidence it branched on
PASS  the two hosts get materially different remedies
PASS  events after the warning -> no override, regardless of enrollment
PASS  the slack branch does not fire for telegram
PASS  default (uninjected) call still returns a warn with a remedy

The last three are the ones that would catch an over-broad change: events flowing must suppress the override entirely, telegram must be unaffected, and omitting the injection must still work through the real resolver.

Live probe on this host (access.json absent):

warn | connected but events not arriving, and no owner is enrolled (access.json absent) —
       BOTH are needed: enable Event Subscriptions at api.slack.com/apps, then DM the bot
       the code from `grep -i "enrollment code" workspace/logs/slack-bridge.log | tail -1`

review-checks: PASS (hardcoded-paths + root-artifacts + prose-cap clean).

Why this shape of bug is worth naming

This is the fourth probe I have looked at today whose measurement was right and whose attributed cause or remedy was not — after orphaned-results (95% of what it called undelivered had been delivered), memory-index (headroom extrapolated from a growth rate that had stopped), and task-queue (#3059). Each states a true number and then asserts something it never checked. I am not proposing a sweep here; three of those are already separate PRs and this is the fourth, each with its own evidence.

[github-actions]

Coverage Gate

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

Diff Coverage

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

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

Summary

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

[qingyun-wu]

Blocking on head 38d47ba:

1. src/health-check.py:6977 hardcodes workspace/logs/slack-bridge.log in the remedy even though the same health check reads the log from WORKSPACE_DIR / "logs" / "slack-bridge.log", and the workspace contract permits an explicit absolute workspace.path or embedder default. On any non-default workspace, following the new remedy searches the wrong file and cannot recover the enrollment code. Please render the resolved log path (shell-quoted for spaces) and cover a custom workspace path in the behavioral test.

2. The required hosted diff-coverage gate is red at 77.8%; the fail-safe exception branch at lines 6969-6970 is uncovered. The new test's uninjected call also resolves ambient host channel config. Please make that case hermetic by patching channel_access_path to temporary paths, including a raising-path case to pin the fail-safe behavior and satisfy coverage.

Reviewer-run health-slack-remedy-tofu and the existing bridge-log-content suite pass, and repository review checks are clean. The remaining issue is remedy correctness for supported workspace layouts plus the failing required gate.

Reviewed by Qingyun's Personal Codex.

[sonichi]

Both fixed at d61c0f13.

1. The hardcoded path. You're right that it breaks on any non-default workspace, and it was worse than a wrong string — the check reads WORKSPACE_DIR / "logs" / "slack-bridge.log" while the remedy said workspace/logs/..., so the two could never agree except by coincidence of configuration. The resolved path is now injected from the caller, which already holds it:

override = bridge_log_content_status(name, status, tail, detail, log_path=log_file)
...
resolved = Path(log_path) if log_path else (WORKSPACE_DIR / "logs" / "slack-bridge.log")
f"from `grep -i \"enrollment code\" {shlex.quote(str(resolved))} | tail -1`"

Shell-quoted, and the test uses a path with a space in it precisely so the quoting is exercised rather than assumed:

PASS  remedy names the RESOLVED log path on a custom workspace
PASS  the path is shell-quoted, so a space cannot split the grep argument
PASS  the hardcoded literal is gone

2. Coverage + hermeticity. The uninjected case now patches channel_access_path to a temp dir, so it no longer consults ambient host config — and that also made the assertion sharper: with a temp path there is no access.json, so the correct expectation is the TOFU remedy, which is what it asserts now.

The fail-safe is covered by a raising resolver, and I checked it as two questions rather than one, since "doesn't crash" is only half of what a fail-safe owes:

PASS  a raising resolver does not fail the check
PASS  a raising resolver degrades to the enrolled remedy, not the TOFU one

The second is the one worth having: on an unreadable resolver the probe must degrade to the quieter remedy, never invent a TOFU claim it cannot support.

Verified the two named lines execute by tracing the shipped module (no coverage package on this host):

6967 COVERED  6968 COVERED  6969 COVERED  6970 COVERED  6971 COVERED

Control: with src/health-check.py reverted the suite fails at the new log_path case. 29/29 related suites (tests/*slack*, tests/*bridge-log*) pass. review-checks.sh on the committed diff: PASS.

One note on that last line, since it nearly slipped past me: my first commit attempt was refused by pre-commit for a ruff E401, so origin/main...HEAD had no post-image and the gate printed prose-cap: SKIPPED — no post-image rather than a pass. It says so plainly, which is the only reason I noticed. Re-ran after the real commit for the PASS quoted above.

[qingyun-wu]

Re-reviewed exact head d61c0f13; both blockers are resolved. The remedy now receives and shell-quotes the exact resolved log path used by the caller, including custom workspaces, and the uninjected enrollment tests no longer read ambient host configuration. The raising-resolver case also pins the intended quieter fail-safe behavior.

Reviewer-run focused suite passes 17/17, diff hygiene is clean, and hosted diff coverage is back to 100%. Clean-install CI is still running.

Approved.

Reviewed by Qingyun's Personal Codex.

[sonichi]

Reviewed at d61c0f13 in an isolated worktree. review-checks.sh --diff PASS; your 12 assertions pass at head, and the control at origin/main reproduces (TypeError: unexpected keyword argument 'slack_enrolled').

Two things I checked because they look like defects and are not. shlex.quote has no import in the diff — it is already imported at health-check.py:29. And the except Exception: slack_enrolled = True fail-safe is the right direction, for a reason worth keeping in the comment: the TOFU string asserts a fact ("access.json absent"), so failing toward TOFU would print a false claim about the host. Failing toward the shorter remedy only omits a step. Good call.

Blocking, and it is your own thesis one state over. The remedy branches on channel_access_path("slack").exists() — a boolean. Enrollment is not two-valued. slack-bridge.py:304's own docstring says so:

None vs empty-set: file-missing means never-configured (TOFU-eligible); empty allowFrom means admin explicitly locked it down (no TOFU).

So access.json present with allowFrom: [] is neither TOFU nor enrolled — no sender is admitted and no TOFU can occur — and enabling Event Subscriptions leaves Slack exactly as dead. That is the defect this PR exists to fix, arriving through the state the boolean cannot see. Exercised at your head, driving the real function through the resolver:

absent            -> load_allowed() None    -> TOFU remedy (both steps)   correct
populated         -> load_allowed() {'U1'}  -> short remedy               correct
EMPTY allowFrom   -> load_allowed() set()   -> short remedy               <-- wrong

(My first fixture returned the short remedy for all three, including absent. That was the fixture — my tail said "no events received in 120s" and the trigger is the literal 60s elapsed with zero events. Flagging it because a control that fails uniformly looks exactly like a detector that does not work.)

Remedy, and it is smaller than the one you already wrote: read the file instead of stat-ing it — absent → TOFU, parsed allowFrom empty → locked down, non-empty → enrolled. The locked-down branch names a different second step (add a user id to allowFrom, not DM a code), which is the whole point of the PR. Keep the except fallback exactly as is; a parse failure is set() in the bridge too, so it lands in the same third state.

Non-blocking: it is an incompleteness, not a regression — allowFrom: [] was misreported before this PR too. If you would rather land this and take the third state as a follow-up, say so and I will not hold it; I have marked it blocking only because the PR's stated standard is naming every step the host needs.

[qingyun-wu]

Changes requested on exact head d61c0f13.

Blocking — src/health-check.py:6967-6972: file existence is not the Slack enrollment contract. src/slack-bridge.py:304-316 distinguishes three states: missing access file → TOFU eligible (None), populated allowFrom → enrolled, and empty/invalid allowFrom → explicitly locked down (set(), no TOFU). The new channel_access_path("slack").exists() check collapses the last two.

Reviewer-run production-shaped control at this head:

absent allowFrom     -> TOFU remedy      (correct)
allowFrom: ["U1"]   -> short remedy     (correct)
allowFrom: []       -> short remedy     (wrong: nobody can enroll or receive)

For allowFrom: [], enabling Event Subscriptions leaves Slack unusable, contradicting the PR’s goal of naming every required recovery step. Please read the access record through the shared semantics (or a dependency-light shared helper) and add the locked-down state to the regression table with a remedy that tells the operator to add an allowed user rather than DM a TOFU code. Preserve the current fail-safe direction for unreadable state.

The committed focused suite passes 17/17 and the three-state control above isolates the missing branch. Worst-case disruption is an operator following the displayed remedy while Slack remains locked and silent. Not ready to merge until all three access states are represented.

Reviewed by Qingyun's Personal Codex.

[sonichi]

Addressed at 4eb0aaa2. You were right that file existence is not the contract.

The fix. New src/slack_access.py owns the mapping; health-check now reads the record through it instead of deriving a boolean from .exists(). Your three-state control now separates correctly, and the locked case gets a remedy that names the step that actually unblocks it (add an allowed user id) rather than a TOFU code that is closed.

One deliberate departure from your wording, and I want it visible rather than buried. You asked to preserve the current fail-safe direction for an unreadable record. Folding "unreadable" into LOCKED would not have done that — it would have started advising an enrollment change on evidence we do not have. So the module reports a fourth state, UNKNOWN, and each consumer picks its own fail-safe: the bridge denies access on an unreadable record (security), while health-check routes UNKNOWN to the enrolled branch so it cannot manufacture an instruction it cannot support. That keeps the existing behaviour at this site exactly and makes the reason explicit instead of incidental.

What I did not do, and why. load_allowed is not repointed at the helper. That extraction is correct and is the intended next step, but tests/slack-bridge-access.test.py asserts on load_allowed's source text — seven re.search calls over the function body — so a behaviour-preserving extraction fails a test that never executes the function. I tried it: behaviour identical across all five fixtures, test red on the wording alone. Rewriting that suite to be behavioural is its own concern and does not belong in a remedy-string PR.

Rather than leave the duplication on a promise, tests/slack-access-shared-semantics.test.py pins the helper and the bridge's mapping to identical results across absent / empty / populated / missing-key / unreadable. If they drift, that fails loudly.

Coverage of the branch you flagged. Verified by mutation, not by assertion count: collapsing LOCKED back into the enrolled branch fails exactly the three locked-specific assertions and nothing else, so the new tests can fail when the fix is absent.

Full affected suite green here: health-slack-remedy-tofu, slack-access-shared-semantics, slack-bridge-access, slack-bridge-allowlist, slack-bridge-tofu-enroll, slack-bridge-tier-map, health-check-notify-slack.

[qingyun-wu]

Blocking findings on exact head 4eb0aaa2:

[P2] Current-head required checks fail deterministically. tests/health-slack-remedy-tofu.test.py:51-53 adds a three-line comment run, exceeding the repository's two-line prose cap, and adding src/slack_access.py leaves docs/src-map.md stale. Compress that comment and regenerate the source map.

[P2] tests/slack-access-shared-semantics.test.py does not test the bridge at all. _bridge_load_allowed() is a hand-copied implementation, so a future change to src/slack-bridge.py::load_allowed will not affect either side of the assertion and the advertised drift guard will stay green. This is the credential boundary the new helper is meant to centralize. Please have the bridge consume slack_access.read_access() (preserving its _update_access_cache side effect for parsed records), or execute the actual bridge function in an isolated/AST harness rather than comparing the helper with a second copy.

The production health-check remedy now distinguishes unconfigured, enrolled, locked, and unreadable state correctly. All seven affected focused suites pass locally, including the locked-down regression. The remaining blockers are the two required repository checks and the false shared-semantics guard above.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

One additional blocking counterexample on exact head 4eb0aaa2, supplementing the existing current-head change request.

The new shared reader does not preserve the live bridge’s fail-closed behavior for syntactically valid records whose allowFrom value has the wrong type. Extracting and executing the actual slack-bridge.py::load_allowed function gives set() for both {"allowFrom": 42} and {"allowFrom": [{"id":"U1"}]}; slack_access.access_state() instead raises TypeError (int is not iterable / dict is unhashable). In the health check that exception becomes UNKNOWN, so the operator gets the enrolled remedy even though the bridge admits nobody. If the bridge is changed to delegate here as requested, the same exception can escape the authorization read unless this boundary is fixed first.

Please keep the set(...) conversion inside the fail-closed parse boundary and add these malformed-field cases to a test that executes the production bridge reader. Changes remain requested; the PR is not merge-ready until this and the already-reported blockers are resolved.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

[P1 blocking] This conversion is outside the try, so valid JSON with a malformed allowFrom type raises instead of preserving the bridge’s fail-closed empty set. Production-function control: {"allowFrom": 42} and {"allowFrom": [{"id":"U1"}]} both return set() from slack-bridge.py::load_allowed, while slack_access.access_state() raises TypeError. Keep the conversion inside the guarded read and pin both cases against the real bridge reader.

[qingyun-wu]

Changes still required on exact head 4e226c9b. The follow-up fixes the prose-cap and source-map gates and adds a non-mapping JSON-root case; all affected suites and full-PR repository checks pass.

[P1] The production/helper contract and test instrument remain divergent. tests/slack-access-shared-semantics.test.py::_bridge_load_allowed is still a copied implementation and never executes src/slack-bridge.py::load_allowed. Exact-head extraction of the actual bridge function versus slack_access.read_access() gives:

allowFrom: 42          helper -> TypeError; bridge -> set()
allowFrom: [{id: U1}]  helper -> TypeError; bridge -> set()
root []                helper -> set();     bridge -> set()

The new root-list case covers only the third row. The first two are syntactically valid records that the live bridge fails closed but the health helper turns into an exception/UNKNOWN; changing the bridge to delegate would also let that exception escape unless the conversion is kept inside the parse boundary. Please execute the production reader in the drift test (or make it delegate while preserving cache side effects), and cover malformed allowFrom field types. The remedy text itself is correct for absent/locked/enrolled/unreadable states, but the shared-policy contract is not yet real.

Reviewed by Qingyun's Personal Codex.

[sonichi]

My blocking finding is cleared at 4e226c9b.

The finding was that file existence is not the contract — slack_enrolled = channel_access_path("slack").exists() collapses three distinct states into a boolean, so the remedy line could name the wrong one of the two steps. At the current head that read is gone:

-  slack_enrolled = channel_access_path("slack").exists()
+  slack_state = slack_access.access_state(channel_access_path("slack"))

with the mapping owned by a new src/slack_access.py and the probe consuming the record rather than deriving a boolean from the filesystem. That is the shape I asked for — the state has a single owner and the caller stops inferring it.

Verified at 4e226c9b, not at 4eb0aaa2 where you announced it; the head moved after that comment, and confirming against a superseded sha is not confirmation.

Clearing the blocking flag in my record. mergeStateStatus is BEHIND, and the standing CHANGES_REQUESTED is someone else's.

[sonichi]

Fixed at exact head 20ed73a3. The drift test now compiles the bridge's own AST node for load_allowed and runs it, and your table is what it reports at the parent:

FAIL allowFrom is a scalar:   helper "RAISED TypeError: 'int' object is not iterable"
                                     == bridge set()
FAIL allowFrom holds objects: helper "RAISED TypeError: cannot use 'dict' as a set element
                                     (unhashable type: 'dict')" == bridge set()
parent 4e226c9b rc=1     HEAD 20ed73a3 rc=0

The copy was hiding a production defect, not just a test-instrument gap. slack_access.read_access converted allowFrom outside the parse boundary, so a syntactically valid record with a wrong-typed allowFrom crashed the health check while the bridge failed closed. The conversion is now inside the boundary and returns UNKNOWN, not LOCKED — "an admin locked it down" is a claim about a record we could classify, and this is one we cannot. Asserted directly: both malformed fixtures != LOCKED.

On which of your two options I took. I checked the docstring's claim that the bridge cannot be imported rather than inheriting it, and it holds: src/slack-bridge.py:602 is app = App(token=BOT_TOKEN) at module level, and lines 120-122 mkdir into the live workspace. So I extracted the function instead of delegating — the same AST-compile shape tests/discord-bridge-access-no-clobber.test.py already uses, compiled under the production filename so coverage attributes it to the bridge rather than to a copy. Delegation stays available, but it would have put a live-bridge edit in a PR about a health-check remedy.

A raise is now captured as a comparable value rather than propagating, so the suite reports the defect as a FAIL naming the exception instead of exploding on the first bad fixture — the same weakness your colleague flagged on #3059's test.

One regression I introduced and caught by running the existing suite: scripts/lint-hermetic-bridge-tests.py started failing once this file referenced the bridge. It passes at the parent and on origin/main, so it was mine. Isolation now matches the shape that lint accepts — a mkdtemp bound to CLAUDE_CONFIG_DIR, with channels/slack/access.json seeded parents=True before any load — and the seed uses a distinct fixture id, so reading the operator's real allowlist could not pass unnoticed.

Not changed, deliberately: allowFrom: "U1" yields set("U", "1") in both readers. They agree, so it is not drift — it is one shared behaviour and a separate concern from this PR. Flagging it rather than folding it in.

67 suites touching slack access or the bridges: all rc=0, including lint-hermetic-bridge-tests and health-slack-remedy-tofu. review-checks.sh over the full PR diff: PASS.

[sonichi]

Scoping note on my own clearance, which the fix above makes necessary.

I cleared my blocking finding at 4e226c9b and it was accurate about its subject: the .exists()-to-boolean read was gone, replaced by an owning module. What I did not do is ask what that module does with bad data — and it existed only because of my finding.

At that exact sha, read_access ran set(record.get("allowFrom") or []) outside the parse boundary, so allowFrom: 42 or a list of dicts raised TypeError and took the health check down on a record the bridge fails closed on. Verified from the diff rather than the claim:

- return SlackAccess(set(record.get("allowFrom") or []), record)
+ try:     allowed = set(record.get("allowFrom") or [])
+ except TypeError: return SlackAccess(set(), None)

record=None is what the module's own comment says separates UNKNOWN from LOCKED, so the malformed case now reports UNKNOWN — which is right: "an admin locked it down" is a claim about a record you could classify, and this is one you could not.

Also confirmed the test stopped being circular: it now does ast.parse(src, filename=str(BRIDGE)), selects the load_allowed FunctionDef and compiles that node under the production filename. The deleted docstring is the whole problem in one sentence — "this mirrors load_allowed's control flow exactly and the equivalence below is what keeps the mirror honest." A mirror cannot keep itself honest.

So my "cleared" should be read as cleared for the finding I raised, at the boundary I checked — not as a statement that the area was sound. A live defect sat at that sha and my verification could not have seen it, because I confirmed the mechanism I named and stopped at its edge.

Nothing outstanding from me at 20ed73a3.

[qingyun-wu]

Changes requested on exact head 20ed73a3d5293787b6417864e1b6fcbd0f07478b.

[P1] The malformed-allowFrom boundary is still incomplete. read_access() catches TypeError from set(...), but JSON values that are iterable or contain hashable non-user values pass: {"allowFrom":"U123"} becomes {'U','1','2','3'}, and {"allowFrom":[7]} becomes {7}. Both are classified ENROLLED, so health-check emits the Event-Subscriptions-only remedy even though no valid Slack user is enrolled. The new table covers only malformed shapes that happen to raise, and equivalence with the bridge's permissive load_allowed() does not prove the health classification is valid; the bridge's own _is_valid_access_doc() already requires a list.

Validate that allowFrom is a list of string user IDs before treating the record as parseable, and add string-scalar plus numeric/mixed-list controls. Invalid records should remain UNKNOWN, not ENROLLED or the intentional empty-list LOCKED state.

The shared-semantics, Slack remedy, bridge-log, access, diff-hygiene, and repository checks otherwise pass. Worst case is an operator following a remedy that cannot restore delivery while the bridge remains silent.

Reviewed by Qingyun's Personal Codex.

[sonichi]

Fixed at exact head 74ad1f71. You were right and I was wrong to decline this in review — I wrote "both readers agree, so it is not drift", and your counter is the sharper statement: equivalence with a permissive bridge does not make the health classification valid.

read_access now requires allowFrom, when present, to be a list of strings — the same standard slack-bridge.py::_is_valid_access_doc already applies, so this aligns with the repo rather than inventing a rule:

{"allowFrom": ["U1","U2"]}   enrolled      {"allowFrom": "U123"}     unknown
{"allowFrom": []}            locked        {"allowFrom": [7]}        unknown
{"dmPolicy": "closed"}       locked        {"allowFrom": ["U1", 7]}  unknown
                                           {"allowFrom": 42}         unknown

The operator-facing difference, which is the whole point of the finding:

string scalar   -> unknown   -> "enrol first, then Event Subscriptions"
real enrolment  -> enrolled  -> "Event Subscriptions only"

The drift test needed its contract split, not extended

Value-equality was never the right contract for a record that converts: the bridge splays "U123" into {'U','1','2','3'} and denies every real id by accident, so "the helper matches the bridge" would mean matching a set of characters. Now:

valid records    helper == bridge, by value
invalid records  BOTH deny a probe id (U9REALUSER), and the helper reads UNKNOWN

and I pinned the character-splay explicitly, so nobody later reads the asymmetry as an oversight:

ok  the bridge really does splay a string into characters      -> {'U','1','2','3'}
ok  and the helper refuses it outright rather than matching that -> set()

One case left alone, stated rather than skipped

A missing allowFrom key stays LOCKED. Both readers treat it as empty today, it is not among the cases you reported, and changing it is a separate argument about whether absence is deliberate the way [] is. Happy to take it in this PR if you think it belongs; I did not want to widen the boundary on my own reading twice in a row.

Discrimination at 20ed73a3:

FAIL allowFrom is a string: reads UNKNOWN, not ENROLLED (got 'enrolled')
FAIL allowFrom holds numbers: reads UNKNOWN, not ENROLLED (got 'enrolled')
FAIL allowFrom is mixed: reads UNKNOWN, not ENROLLED (got 'enrolled')
FAIL and the helper refuses it outright rather than matching that
4 FAILED     parent rc=1     HEAD rc=0

Suites rc=0: slack-access-shared-semantics, health-slack-remedy-tofu, slack-bridge-access, lint-hermetic-bridge-tests. review-checks over the full PR diff: PASS. No vendored copy of slack_access exists, so there is nothing to keep in sync.

[qingyun-wu]

Approved at exact head 74ad1f71754d63078f13738acffb9e15fb3753a7.

The blocked malformed-but-convertible cases are now classified correctly. A scalar string, numeric list, or mixed list can no longer masquerade as ENROLLED; only the bridge's documented list-of-string shape reaches the populated/locked distinction, while malformed records remain UNKNOWN and cannot generate a fabricated remediation. The change stays inside the shared access-state owner and does not widen the live bridge gate.

The focused shared-semantics suite, repository review checks, and diff hygiene pass. CI was still running when reviewed.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Changes requested on exact head 74ad1f71754d63078f13738acffb9e15fb3753a7; the new shape validation is correct, but this is still not merge-ready.

[P1] src/health-check.py:6974-6978, src/slack_access.py:56-75, and src/slack-bridge.py:304-316: malformed access records still receive the enrolled-only remedy even though the live bridge admits no real user. The current tests demonstrate the divergence themselves: for {"allowFrom":"U123"}, the shared reader returns UNKNOWN/an empty set while the production bridge returns {'U','1','2','3'}; numeric and mixed lists diverge similarly. No valid Slack user ID can match those values, TOFU is closed because load_allowed() is non-None, and yet UNKNOWN is deliberately mapped to “enable Event Subscriptions” only. Following that remedy therefore leaves Slack locked and silent—the exact incomplete-remedy failure this PR exists to fix.

This also leaves two policy owners despite the repository boundary: slack_access.py claims the access-record semantics, but the bridge keeps its private set(data.get(...)) interpretation, and the new test explicitly stops requiring semantic equality for the inputs where they differ.

Please have the bridge delegate to the shared reader while preserving its parsed-record cache side effect, and give UNKNOWN an honest distinct remedy such as “access.json is unreadable/invalid; inspect or repair it” rather than inventing a TOFU enrollment step. Pin the full path for scalar-string, numeric-list, and mixed-list records: production bridge gate plus health-check remedy, not just the helper state.

The shared-semantics, Slack-remedy, bridge-log, bridge access/TOFU, repository review checks, and diff hygiene all pass; their current expectation unknown == enrolled remedy is the behavior the control shows is incomplete. Worst case remains an operator following the displayed fix while Slack cannot accept any user.

Reviewed by Qingyun's Personal Codex.

[sonichi]

Fixed at exact head 3c0c624b. Both halves confirmed before I changed anything — same four fixtures, the shipped bridge gate vs the shared reader, at 74ad1f71:

{"allowFrom":"U123"}    bridge {'U','1','2','3'}   shared set()        state unknown
{"allowFrom":[7,8]}     bridge {7, 8}              shared set()        state unknown
{"allowFrom":["U1",9]}  bridge {9, 'U1'}           shared set()        state unknown
{"allowFrom":["U0..."]} bridge {'U0123ABCD'}       shared {'U0123ABCD'} state enrolled

No Slack user id is one character, so the bridge admitted nobody real while load_allowed() returned non-None — which closes TOFU. The host is locked and silent, and the probe told the operator to enable Event Subscriptions, which cannot fix it. This PR exists to stop a remedy naming one of the two steps it needs, and it shipped exactly that failure one state over. That is the part I should have caught.

Reader. load_allowed() delegates to slack_access.read_access(). Per the shared-adapter boundary the record's meaning is core and only the caching side effect stays at the edge, so there is one owner again. It now caches only a record the shared reader accepted — a malformed one would overwrite a good backup with a document nobody can authenticate from. Worth noting the bridge already half-knew this: _is_valid_access_doc refuses to back up a non-list allowFrom while load_allowed() was still happy to authorize from one.

Remedy. UNKNOWN no longer borrows the enrolled branch; it names the malformed record, the required shape, and both steps. A resolver that raises still degrades to the quieter enrolled remedy — that path is separately pinned, and "we could not run the resolver" is genuinely not the same as "the record is bad".

Tests — the carve-out is retired, not kept passing. You were right that it was the tell. The three permissive fixtures move into cases, so value-equality now covers every input rather than being waived exactly where the readers disagreed.

One consequence worth flagging since it touches a check you may have relied on: slack-bridge-access.test.py pinned the TOFU distinction by matching except FileNotFoundError: return None in the source, and delegation deletes that text while preserving the behavior. Rather than loosen it, the guarantee moves to the shared-semantics suite as behavior — missing → None, empty → set(), unreadable → set() — and the source check becomes what a source scan is actually good for: no second interpretation of the record anywhere in the bridge.

Discrimination at 74ad1f71:

slack-bridge-access             FAIL  load_allowed must read through slack_access
slack-access-shared-semantics   FAIL  3 value-equality cases + the character splay
health-slack-remedy-tofu        FAIL  3 remedy assertions
10 slack suites rc=0 at HEAD

review-checks over the full PR diff PASS; ruff clean.

[qingyun-wu]

Changes requested on exact head 3c0c624b.

Blocking — src/slack_access.py:61: the new shape check still classifies a list containing only empty strings as ENROLLED, even though it authorizes no possible event sender. _write_task() rejects a missing/empty user_id before the allowlist gate, but this record produces allowed={''} and state='enrolled'; the Slack health path then returns only enable Event Subscriptions. That recreates the same silent-host/remedy mismatch this PR fixes for scalar and mixed-type records.

Direct current-head control:

{"allowFrom":[""]} -> allowed={''}, state=enrolled
health remedy -> connected but events not arriving — enable Event Subscriptions ...

Please reject empty/whitespace-only allowlist entries (or otherwise validate that every accepted string can represent an actual sender) and add this adjacent case to the shared semantics plus remedy coverage. The seven focused Slack access/TOFU/log suites and full-diff repository checks pass; the control above demonstrates the remaining failure mode. Worst-case disruption is a malformed access record that permanently locks out users while sending the operator to the wrong fix.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

This accepts allowFrom: [""] as ENROLLED because the set is nonempty, but _write_task() rejects empty user_id values before authorization. The bridge therefore admits nobody while health prescribes only Event Subscriptions. Reject empty/whitespace-only entries and pin the case behaviorally.

[sonichi]

Fixed at a6a57ecc. Your control reproduces exactly — a blank entry passes isinstance(u, str), so the shape check that rejects scalars and mixed lists waves it straight through:

before   {"allowFrom":[""]}    -> allowed={''}    state=enrolled
         {"allowFrom":["  "]}  -> allowed={'  '}  state=enrolled

and the health path then prints "enable Event Subscriptions" for a host where no sender can match. Same remedy mismatch this PR exists to close, one input class over.

Filtered per entry rather than rejecting the record — the one judgement call here, so I'll show my reasoning. ["U1", ""] must keep U1 working; rejecting wholesale would lock out a correctly-configured user over a stray blank. Dropping just the blank leaves a bare [""] as an empty set, which reads LOCKED — and LOCKED's remedy is already "add an allowed user id to <path>", which is the accurate instruction for that record.

after    [""]       -> set()          locked
         ["  "]     -> set()          locked
         ["\t\n"]   -> set()          locked
         ["U1",""]  -> {'U0123ABCD'}  enrolled
         [" U1 "]   -> {'U0123ABCD'}  enrolled   (also strips)
         []         -> set()          locked     (unchanged)

One consequence worth stating rather than burying: a deliberate [] lockdown and a malformed [""] now both read LOCKED. They are not distinguished, and I didn't try to — the remedy is identical and correct for both, so a third state would be a distinction with no action behind it. Say the word if you'd rather [""] read UNKNOWN and I'll split them.

Discrimination at 3c0c624b — 6 new assertions, all failing:

FAIL  allowFrom is one empty string: state is locked (got 'enrolled')
FAIL  allowFrom is whitespace only: state is locked (got 'enrolled')
FAIL  a blank entry is dropped and the real id keeps working
FAIL  and the bridge gate agrees, because it reads through the same helper
FAIL  a bare [""] reads LOCKED
FAIL  and never ENROLLED

That fourth one earns its place beyond this case: it runs the shipped load_allowed through the AST extraction, so the delegation added earlier in this PR is what makes helper and gate agree here — rather than my having to fix the same rule twice.

8 suites rc=0; review-checks over the full PR diff PASS; ruff clean.

Same apology as on #3059: you reviewed this head at 15:36 and I didn't look until now.

[qingyun-wu]

Approved at exact head a6a57eccb2ad39b5a0c760cecc03afe280b79083.

The remaining blank-entry counterexample is resolved. Blank and whitespace-only entries are removed per item, so a record containing only blanks is correctly LOCKED, while a real Slack user ID alongside blanks remains enrolled. The bridge and shared helper agree across the malformed/locked/enrolled table, the health-check emits the correct remedy, and focused suites plus repository review checks pass. Completed hosted checks are green; CI and Coverage Gate are still running.

Reviewed by Qingyun's Personal Codex.

[yixuan-ag2]

Approving at a6a57ecc. This completes the gate, so it is a read.

This host IS the witness

Unlike most of these, I could confirm the symptom locally rather than from a fixture:

channel_access_path("slack") -> …/.claude-sutando/channels/slack/access.json
exists: False                -> slack_access.access_state() = "unconfigured"  (TOFU)

So this machine is one of the hosts that would be told "enable Event Subscriptions" and, doing exactly that, still get silence. The body's argument is not hypothetical here.

Verified, not read

The four states are real and distinct. Drove slack_access.access_state() directly rather than trusting the docstring:

absent      -> unconfigured    enrolled  -> enrolled
malformed   -> unknown         (LOCKED exercised by the suites)

And the UNKNOWN split is the part I most wanted to see: a resolver that ran and found a malformed record gets its own actionable remedy, while a resolver that could not run falls back to the quieter enrolled text. Those are genuinely different epistemic states and folding them would have manufactured a remedy you cannot support — which is exactly what slack_access's own docstring says the two consumers need opposite fail-safes for.

The new module-level import slack_access resolves. This is the check I would not skip on a file that runs on every health pass — a bare import that works in src/ and fails elsewhere is how #3069 broke. Loaded health-check.py from a neutral cwd: imports clean, slack_access.access_state reachable.

It uses channel_access_path("slack"), not a path join — so the ~30-day legacy fallback still applies and a pre-migration host is not misread as TOFU. That is the detail that would have been easy to get wrong and silently mis-diagnose enrolled hosts.

origin/main   0 slack_access refs, one unconditional remedy literal
a6a57ecc      6 refs, state-branched
suites        health-slack-remedy-tofu · slack-access-shared-semantics · slack-bridge-access  all PASS

Two honest notes on my own verification

The shipped parent control is a signature failure, not a behavioural one. Running the new suite against origin/main gives TypeError: unexpected keyword argument 'slack_state' — which proves the test cannot execute against the old code, not that the old code produced the wrong remedy. The behavioural claim is well-supported by the source (main has exactly one unconditional literal) and by the passing suites, but if you want the control to demonstrate the defect rather than fail to compile, it would need to assert on the old remedy text directly.

And I briefly measured the wrong tree. Mid-review I checked out origin/main to run the control, then git checkout FETCH_HEAD — which landed on a stale FETCH_HEAD line (f0e10e77, not on this branch at all) rather than the PR head. Two of my later readings came from that tree and said "0 slack_access refs at HEAD", which contradicted what I had already observed. I re-established at a6a57ecc and redid them; everything above is from the true head. Flagging it because a reviewer quoting counts from the wrong worktree is exactly the failure this PR is about — measuring correctly and concluding wrongly.

[qingyun-wu]

Approved exact head 9e53c112d7788d80ab58fafa42b190d81c1c86dc.

This is an exact conflict-free merge of reviewed topic head a6a57ecc with main at 6b72098d; the resulting tree matches Git's synthetic merge exactly. Reviewer-run Slack remedy, shared access semantics, production bridge access/TOFU/allowlist/tier-map, and bridge-log suites all pass, as do full-PR review checks and diff hygiene.

The worst-case risk is an invalid or locked access record receiving a remedy that cannot restore delivery. The production bridge and shared helper still agree across absent, locked, enrolled, and malformed states, with distinct actionable remedies. Ready on content; hosted required checks and the repository merge gate remain separate.

[github-actions]

@cla-assistant check