Repository navigation
Conversation
…hannels/ When channels/ config (bot tokens + allowlists) lives only at the legacy ~/.claude/channels/ home and was never copied into the active CLAUDE_CONFIG_DIR, each bridge resolves a non-existent token path and exits "no token" with nothing loud. This is exactly what took Telegram + Discord offline on 2026-07-24 on a supervisor-launched core (which skips src/startup.sh). Nothing in health-check surfaced the un-migrated state. Add check_stranded_channel_config(): for each channel dir under legacy ~/.claude/channels/, warn when its .env / access.json exists there but is ABSENT under CLAUDE_CONFIG_DIR/channels/<ch>/, with the exact copy-forward command. It deliberately reads the ACTIVE CLAUDE_CONFIG_DIR directly instead of via claude_home_path() — the resolver's legacy reader-fallback (companion PR sonichi#2299) would otherwise mask the very stranding we want to surface. The two are complementary: the fallback keeps the bridge booting, this check makes the un-migrated state visible so it actually gets fixed. Returns None when CLAUDE_CONFIG_DIR is unset (active home IS ~/.claude — nothing can strand). Read-only; never raises. Real-world before/after on the affected host (legacy has ag2space/discord/ telegram; active has discord/telegram only): Before (origin/main): no such check. After: status: warn detail: channel config stranded at legacy ~/.claude/channels/ (absent under CLAUDE_CONFIG_DIR): ag2space/.env — affected bridges exit 'no token' and never come up. Migrate: mkdir -p "$CLAUDE_CONFIG_DIR/channels/ag2space" && cp -p ~/.claude/channels/ag2space/.env "$CLAUDE_CONFIG_DIR/channels/ag2space/.env" (correctly does NOT flag the already-migrated telegram/discord.) tests/health-check-stranded-channel-config.test.py: 9 checks (warns + names file + remedy on stranded; ok when migrated; None when CCD unset; ok when no legacy dir; partial-migration flags only the still-stranded channel). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 62%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
|
The 'diff coverage >= 95%' gate flagged two spots: the main() registration call-site (which unit tests can't reach) and the OSError degrade branch. - Registration: collapse to the repo's pragma'd one-liner form, matching the check_gateway_bridge() call-site convention (a call site exercised by the running health check, not the unit test). - OSError branch: add case6 (legacy channels dir chmod 0 → iterdir raises → degrades to ok 'unreadable'). diff-cover now reports src/health-check.py at 100% (22/22 changed lines). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Blocking correctness finding on current head 158e6935.
[P1] check_stranded_channel_config() returns status: "ok" when legacy_channels.iterdir() raises OSError (src/health-check.py, new exception branch). In that state the check has not inspected the legacy directory, so it cannot establish that config is not stranded; reporting green hides exactly the condition this diagnostic exists to surface. The new case 6 locks in the false-green behavior. Please return warn (or error) with the unreadable detail and update the regression accordingly.
The ordinary stranded/migrated/partial cases and registration wiring otherwise look coherent. Current CI/coverage reruns were still in flight when reviewed; this formal blocker is independent of those checks.
Reviewed at 158e6935; no unrelated hardcoded host paths found in added production lines.
Addresses review P1 on sonichi#2300: check_stranded_channel_config() returned status ok on OSError from iterdir(), a false-green that hides exactly the stranded-config condition the diagnostic exists to surface. Return warn with the unreadable detail; update case6 regression to assert warn. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Addressed the P1 in |
|
@cla-assistant check |
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Re-reviewed exact current head 54eeecce572d17a5b3c52547e6f01fd97b955f6b after my prior REQUEST_CHANGES on 158e6935.
The blocker is fixed: check_stranded_channel_config() now returns warn (not false-green ok) when the legacy channels directory cannot be inspected, and includes the path/error for manual diagnosis. The updated unreadable-directory regression asserts that behavior.
Local verification on the fetched current head:
python3 tests/health-check-stranded-channel-config.test.py→ 10/10 passedpython3 -m py_compile src/health-check.py tests/health-check-stranded-channel-config.test.py→ passedgit show --check --oneline HEAD/git diff --check HEAD^ HEAD→ clean- current-head patch through canonical
scripts/review-checks.sh→ PASS (hardcoded-paths clean)
GitHub currently reports all checks and license/cla successful. PR is mergeable but behind main; approval is for this exact head.
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Changes requested on exact head 54eeecce572d17a5b3c52547e6f01fd97b955f6b after rechecking the conflict-free merge onto current origin/main (043a60bf75ed0ff498d80ce17ab8e73c79979ba2).
Blocker — the current required review check rejects three added production lines. REVIEW.md now forbids added ~/.claude literals; this PR adds them in src/health-check.py (the function docstring, generated copy command, and warning text). Exact cumulative evidence:
$ git merge --no-commit --no-ff 54eeecce572d17a5b3c52547e6f01fd97b955f6b
Automatic merge went well; stopped before committing as requested
$ git diff --cached | bash scripts/review-checks.sh
review-checks: FAIL — hardcoded-paths:
src/health-check.py:1157: hardcoded path (~/.claude/channels/)
src/health-check.py:1206: hardcoded path (~/.claude/channels/{s})
src/health-check.py:1213: hardcoded path (~/.claude/channels/)
review-checks: 3 violation(s).
Please derive displayed/source paths from the already-computed legacy_channels value (and shell-quote any generated copy command), or add a narrowly justified scoped allowance to REVIEW.md if this diagnostic truly must name the legacy token literally. Then update onto current main and rerun the current check.
The functional regression test itself passes (10/10 checks passed) and python3 -m py_compile src/health-check.py passes on the cumulative tree. Worst-case disruption as written is merge-gating failure on current main; the health check remains read-only at runtime.
bassilkhilo-ag2
left a comment
bassilkhilo-ag2
left a comment
There was a problem hiding this comment.
Independently reproduced the existing CHANGES_REQUESTED (2026-07-31) blocker — agree, changes requested.
Ran scripts/review-checks.sh from current origin/main against gh pr diff 2300 directly:
review-checks: FAIL — hardcoded-paths:
src/health-check.py:516: hardcoded path (~/.claude/channels/): """Detect channel config that lives ONLY at the legacy ~/.claude/channels/
src/health-check.py:565: hardcoded path (~/.claude/channels/{s}): f'&& cp -p ~/.claude/channels/{s} "$CLAUDE_CONFIG_DIR/channels/{s}"'
src/health-check.py:572: hardcoded path (~/.claude/channels/): f"channel config stranded at legacy ~/.claude/channels/ (absent under "
review-checks: 3 violation(s).
Confirms the blocker is real and current, not a stale finding: REVIEW.md's hardcoded-path gate (and the CI "Review checks (REVIEW.md)" step, PR #2281) landed 2026-07-25 — one day after this PR's last CI run (2026-07-24 23:56-00:01, commit 54eeecc). So the green statusCheckRollup predates the gate entirely; it never actually evaluated against it. A push that re-triggers CI on the current head will hit this required check and fail.
Two of the three violations (the cp -p remedy string and the warning detail) look mechanically fixable by interpolating the already-computed legacy_channels Path instead of the literal ~/.claude/channels/.... The docstring occurrence would still need either a reword or a scoped REVIEW.md allowance, since this check's whole purpose is naming that legacy path.
No other issues found beyond what's already flagged: the check is read-only, opt-in (returns None when CLAUDE_CONFIG_DIR is unset), and the OSError fail-closed fix (warn, not false-green ok) looks correct and is covered by case 6 of the test file.
Not merge-ready until the hardcoded-path gate passes on current main.
|
Re-verified at current I hold the only blocking review here, so the current state is my job to keep accurate. This PR still merges cleanly ( Two things changed since 2026-07-31. The line numbers moved (1157/1206/1213 → 3094/3143/3150) — main has grown, so the numbers in my original review are stale and would have sent you to the wrong place. And prose-cap is new: that check landed after my review, so those three comment blocks were not a violation when you wrote them. Not your fault; you inherited it by sitting behind main. Nothing else has appeared — no new findings from me, and the substantive content of the PR is unchanged and still good. The remedy is the same as before: route those three strings through the repo's config helpers rather than literals (or add a scoped Leaving the change-request in place because it still reproduces — but it is a mechanical fix now, with an exact list. |
|
Re-verified my own month-old block rather than leaving it to sit. It still fails — and here is the current output so you have something concrete to act on. You have not pushed since 2026-07-24 and my CHANGES_REQUESTED has been latched since 07-31, so the fair question is whether my objection is still true or has quietly gone moot while Neither happened. One difference from my July comment, so the numbers do not look inconsistent: I quoted lines 1157/1206/1213 then, because that evidence came from a merge diff against The shape of the fix is unchanged and small: derive the displayed and generated paths from the already-computed Worth saying plainly: the feature itself is good. Detecting channel config stranded at the legacy location is exactly the kind of silent misconfiguration that costs an afternoon, and CI here is 16/16 green. This is a lint boundary, not a design objection. Note |
|
@randombet — re-measured my standing The function already computes I want to be fair about the history here: I approved this PR on 2026-07-30 and re-blocked it on 07-31. That was not a reversal on the code — you fixed the original false-green If you judge that this diagnostic genuinely must name the legacy token literally — it is a reasonable position, since the string is the thing being detected — then the alternative is a narrowly scoped allow in One more thing you will hit: the PR is Ping me when it is pushed and I will re-review the same day. |
|
Re-verified my standing CHANGES_REQUESTED at current The remedy for the first group is unchanged: derive the displayed and source paths from the already-computed The branch also reads |
|
Re-checked my CHANGES_REQUESTED today rather than leaving it to age — a blocking flag records what one reviewer believed at one commit and never re-evaluates itself, so it deserves periodic proof that it is still real. It is still real, and unchanged. Ran the current required check against this PR's current diff: Same three lines as on 2026-07-31 (line numbers shifted with main; the violations did not). So the block is not stale — it is simply unaddressed, and nothing has changed on either side. Worth saying plainly, because it may be why this stalled: the literal is not incidental here, it is the feature — a check that detects config stranded at the legacy path has to name the legacy path somewhere. I don't think you can satisfy the checker by deriving all three from So of the two remedies my review offered, the scoped allowance in Happy to re-review the moment it moves. If you would rather this PR be closed than carried, say so and I will dismiss my review so it is not sitting on you. |
|
@randombet — re-checked today (2026-09-03) rather than leaving my July review sitting. The blocker still reproduces, and I can now tell you why CI has been telling you nothing. Why every check on this PR is greenThe check set on this head last ran 2026-07-24T23:56Z. The It reproduces at the current headThree sites, not the eight the naive substring count suggests — the gate scans added lines and the other five are in test fixtures and doc prose it does not police. The fix, per siteYour code already computes the value at the top of the probe: legacy_channels = Path.home() / ".claude" / "channels"That construction is correct and matches what
If you think the diagnostic genuinely must print the canonical token, What I am not claimingI have re-verified only this blocker. The functional test passed 10/10 when I last ran it and the probe is read-only at runtime, but that was July and the tree has moved a long way since; treat the rest of my review as unmeasured rather than as standing evidence. Once these three sites are addressed and the branch is updated onto current main so the gate actually executes, ping me and I will re-review the whole thing at that head. |
|
Re-checked my standing request at current Same procedure as before: merge the head onto current main, pipe the cumulative diff through The three Nothing about the change itself is wrong, and I have no new objection to the behaviour. The fix is unchanged from what I asked for: derive the displayed paths from the already-computed Happy to re-review as soon as there is a new head. |
… missing qingyun-wu's review is right and the failure is worse than incomplete — it is fail-OPEN in exactly the direction the guard exists to prevent. GitHub returns the OLDEST page first, so an unpaginated read computes the "trailing" run from a stale end. Demonstrated against a live thread: gh api --paginate ...comments?per_page=10 -> 39 records, last 2026-09-03T19:46 gh api ...comments?per_page=10 -> 10 records, last 2026-08-27T09:33 The second is not a shorter view of the same tail; it is a different, older window, and a thread whose first 100 records end in peer traffic then reads as "safe to post" while its actual last three events are all the caller's. Adds --paginate to _gh_json (gh merges array pages into one JSON array, so the existing json.loads is unchanged) and four cases: - _gh_json passes --paginate (the argv assertion; mutation-sensitive) - >100 records, my three newest -> run == 3 (the control asked for) - >100 records, a peer newest -> run == 0 (its discriminating twin) - the same 150 records truncated at 100 -> (full, truncated) == (3, 0) The fourth is the one that states the bug rather than merely covering it: the same thread answers REFUSE whole and safe truncated. Mutation verified: dropping --paginate fails the argv assertion with "unpaginated: only the OLDEST 100 records are read". 25/25 restored, and the live smoke still REFUSEs sonichi#2300 at 7 events over 14.3d. Also de-coupled TestFetchLayer's path assertion from argv POSITION — it read calls[i][2] and the new flag shifted the path to index 3, so it failed on a correct change. It now searches the whole argv.
…#3878) * feat(proactive-loop): refuse to post into a PR thread that is only me Re-verifying a standing review is right; posting that re-verification into silence is noise, and each repeat makes the next one less likely to be read. Nothing measured whether anyone was still listening. Measured across 36 open PRs on one host: 5 threads where every recent event was mine and unanswered. #2300 stood at 7 events over 14.3 days with the author never replying once, and an eighth was being composed when this was written. #3672 and #2061 each had 6. pr-monologue-check.py counts the TRAILING run of consecutive events by one login and refuses at a threshold (default 3). Two details that decide whether it is right: - it merges BOTH surfaces (issue comments + reviews), so a thread answered only by a review does not read as silence - it drops `*[bot]` logins. This was a live false SAFE, not a hypothetical: github-actions[bot] commenting after my two unanswered comments on #2406 reset the run to 0 and cleared exactly the post the guard exists to stop. --count-bots reproduces the old answer, so the fix ships with a control rather than an assertion. A fetch failure exits 2 (cannot answer), never 0 — a guard that reads an API error as "safe to post" is worse than no guard. Wired into SKILL.md as step 9.5 so it runs before a post rather than being a script nobody calls. 17 tests; 4 mutations verified red (is_bot always false, counting mine anywhere instead of trailing, dropping reviews from the timeline, and a fetch failure returning 0). * test(proactive-loop): cover the gh layer the other tests inject around The 17 existing tests replace `fetch` wholesale, which is right for the logic but leaves `_gh_json` and `fetch` themselves unexecuted — and those are exactly the lines that decide between REFUSE and a false "safe to post". Traced against the same technique that diagnosed #2406's coverage failure: before: countable=72 hit=65 missing=7 (~90%, under the 95% bar) after: countable=72 hit=72 missing=0 (100%) The 7 were `_gh_json` (subprocess call, non-zero check, raise, parse) and `fetch` (both endpoint calls and the pair it returns). Docstrings and the __main__ block are excluded, matching .coveragerc. Adds TestFetchLayer (4): a successful parse; a non-zero gh exit raising with the path named; fetch reading BOTH surfaces (a mutation dropping the reviews call is otherwise invisible); and one end-to-end case proving a gh failure reaches main as exit 2 rather than 0. 2 mutations verified red with __pycache__ cleared between runs: swallowing the gh failure instead of raising (1 error), and fetch skipping the reviews surface (1 failure). 21/21 green. * fix(proactive-loop): paginate both surfaces, or the newest events are missing qingyun-wu's review is right and the failure is worse than incomplete — it is fail-OPEN in exactly the direction the guard exists to prevent. GitHub returns the OLDEST page first, so an unpaginated read computes the "trailing" run from a stale end. Demonstrated against a live thread: gh api --paginate ...comments?per_page=10 -> 39 records, last 2026-09-03T19:46 gh api ...comments?per_page=10 -> 10 records, last 2026-08-27T09:33 The second is not a shorter view of the same tail; it is a different, older window, and a thread whose first 100 records end in peer traffic then reads as "safe to post" while its actual last three events are all the caller's. Adds --paginate to _gh_json (gh merges array pages into one JSON array, so the existing json.loads is unchanged) and four cases: - _gh_json passes --paginate (the argv assertion; mutation-sensitive) - >100 records, my three newest -> run == 3 (the control asked for) - >100 records, a peer newest -> run == 0 (its discriminating twin) - the same 150 records truncated at 100 -> (full, truncated) == (3, 0) The fourth is the one that states the bug rather than merely covering it: the same thread answers REFUSE whole and safe truncated. Mutation verified: dropping --paginate fails the argv assertion with "unpaginated: only the OLDEST 100 records are read". 25/25 restored, and the live smoke still REFUSEs #2300 at 7 events over 14.3d. Also de-coupled TestFetchLayer's path assertion from argv POSITION — it read calls[i][2] and the new flag shifted the path to index 3, so it failed on a correct change. It now searches the whole argv.
|
Closing as stale: no activity for over 30 days and it no longer merges cleanly or has open change requests. Reopen, or open a fresh PR from current main, if this is still wanted. |
Problem
When
channels/config (bot tokens + allowlists) lives only at the legacy~/.claude/channels/home and was never copied into the activeCLAUDE_CONFIG_DIR, each bridge resolves a non-existent token path and exits"no token"— with nothing loud. This is exactly what took Telegram + Discord offline on 2026-07-24 on a supervisor-launched core (which skipssrc/startup.sh). Nothing inhealth-checksurfaced the un-migrated state, so the failure was invisible until the owner noticed messages going unanswered.Fix
Add
check_stranded_channel_config(): for each channel dir under legacy~/.claude/channels/, warn when its.env/access.jsonexists there but is absent underCLAUDE_CONFIG_DIR/channels/<ch>/, printing the exact copy-forward command.It deliberately reads the active
CLAUDE_CONFIG_DIRdirectly rather than viaclaude_home_path()— the resolver's legacy reader-fallback (companion PR #2299) would otherwise mask the very stranding we want to surface. The two are complementary:Returns
NonewhenCLAUDE_CONFIG_DIRis unset (active home is~/.claude— nothing can strand). Read-only; never raises.Real-world before / after evidence
On the affected host — legacy
~/.claude/channels/hasag2space,discord,telegram; active.claude-sutando/channels/hasdiscord,telegramonly (ag2space never migrated):Before (
origin/main): no such check (grep -c check_stranded_channel_config→0).After (this branch):
It correctly does not flag the already-migrated
telegram/discord.Tests
tests/health-check-stranded-channel-config.test.py(new, 9 checks): warns + names the file + includes the remedy on stranded config;okwhen migrated;Nonewhen CCD unset;okwhen no legacy dir; partial-migration flags only the still-stranded channel.Scope
Single concern: surfacing stranded channel config. Complementary to #2299 (resolver fallback) but independent — either can land first. Bridge auto-restart/supervision remains out of scope (open PR #2068).
🤖 Generated with Claude Code