Repository navigation
fix(sync-workspace): snapshot canonical Claude config - #2336
Conversation
Coverage Gate✅ No Python changes — nothing to gate (bar: 95% on changed lines). |
qingyun-wu
left a comment
There was a problem hiding this comment.
Approval signal for current head 23f286753715db9ba0090a696554da03205a8924.
Reviewed the new sync-workspace snapshot fix. Switching the per-host settings/access snapshot from claude-home-path to claude-sutando-config-dir matches the startup-exported canonical Claude config directory and avoids copying stale legacy ~/.claude access state from launchd/cron environments where CLAUDE_CONFIG_DIR is absent. The snapshot remains intentionally narrow: settings.json and channel access.json only, with channel .env token files still excluded.
Focused verification in an isolated /private/tmp checkout:
git diff --check origin/main...HEAD- PASSbash -n scripts/sync-workspace.sh tests/sync-workspace.test.sh scripts/sutando-config.sh- PASSbash tests/sync-workspace.test.sh- PASS (89checks)bash tests/sutando-config-runtime.test.sh- PASS (11checks)bash tests/sutando-config-claude-home-path.test.sh- PASS (11checks)
GitHub CI is green on this head. No blocking findings from this pass.
Reviewed by Qingyun's Personal Codex.
qingyun-wu
left a comment
There was a problem hiding this comment.
Approval signal for current head 551ada579d619c10d4dcffae8a2dcce9d85a64b1.
Re-reviewed after the main refresh. The PR-owned diff remains the focused sync-workspace snapshot fix: per-host settings/access backups now resolve the canonical Claude config directory from Sutando config instead of falling back to legacy ~/.claude when launchd/cron lacks CLAUDE_CONFIG_DIR. The merge-main refresh brings in #2280 but does not change the snapshot behavior.
Focused verification in an isolated /private/tmp checkout passed: git diff --check origin/main...HEAD; shell syntax for scripts/sync-workspace.sh, tests/sync-workspace.test.sh, and scripts/sutando-config.sh; bash tests/sync-workspace.test.sh (89/89); bash tests/sutando-config-runtime.test.sh (11/11); and bash tests/sutando-config-claude-home-path.test.sh (11/11). GitHub CI is green on this head.
Reviewed by Qingyun's Personal Codex.
qingyun-wu
left a comment
There was a problem hiding this comment.
Current-head re-review on 551ada57.
The main refresh preserves the functional commit exactly (git range-diff is unchanged). The full current diff still resolves the snapshot source through claude-sutando-config-dir, matching startup’s canonical Claude config rather than a launchd/cron process’s stale legacy home, and the regression exercises the missing-ambient-env path.
Exact-head verification passed: tests/sync-workspace.test.sh (89/89), shell syntax, git diff --check, and the repository hardcoded-path review gate. GitHub CI and CLA are green. Worst-case disruption—backing up stale access/tier state and later restoring it over the live owner configuration—is directly covered and mitigated without changing the vault carrier set or making snapshot failures fatal.
Ready to merge from this review.
Reviewed by Qingyun's Personal Codex.
sonichi
left a comment
There was a problem hiding this comment.
Approving on the owner's explicit instruction. Verified the fix is correct + needed on this machine: without CLAUDE_CONFIG_DIR (launchd/cron), the old claude-home-path resolves to legacy ~/.claude, where a stale Jun-8 access.json (2.3KB) sits vs the live 8.3KB canonical one — so the snapshot could back up stale access-control and roll it back on restore. This fix reads the canonical config-dir. Tested (Test 2b).
Extends the mechanism to cover the #2336-class (Chi 2026-07-27): a peer PR the owner can unblock. classify_prs now surfaces peer PRs that are green + REVIEW_REQUIRED + already carry >=1 approval — "your approval unblocks the merge" — into the owner's court alongside his own ready-to-merge PRs. Everything else peer (0 approvals / not green / changes-requested) is still skipped, so it stays a handful, not the ~45-PR REVIEW_REQUIRED firehose. Fetch now includes reviews; new tests cover the surface + the exclusions. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rq18tfPmFcgzHQFgoSpsUN Stand: Echo Act IV Pro
|
@cla-assistant check |
qingyun-wu
left a comment
There was a problem hiding this comment.
Current-head approval signal for 079867ae32e94cca7588b5da2f19e5c89a475e3b.
The main refresh leaves the PR-owned change focused on the canonical Claude config snapshot: sync-workspace.sh resolves claude-sutando-config-dir for the per-host backup instead of falling back to legacy ~/.claude when launchd/cron lacks CLAUDE_CONFIG_DIR. The added regression still covers that exact missing-env path.
Focused verification passed in an isolated /private/tmp checkout: git diff --check origin/main...HEAD, shell syntax for scripts/sync-workspace.sh, tests/sync-workspace.test.sh, and scripts/sutando-config.sh, bash tests/sync-workspace.test.sh (89/89), bash tests/sutando-config-runtime.test.sh (11/11), and bash tests/sutando-config-claude-home-path.test.sh (11/11). GitHub CI is green on this head. No blocking findings.
Reviewed by Qingyun's Personal Codex.
…n PRs (#2343) * feat(pr-flag): structural mechanism to flag the owner for his own open PRs Root cause (Chi 2026-07-27 "fix root cause", after repeated flagging misses): agent commits carry the owner's GH-mapped email, so every PR the agent opens is authored under the owner's identity — GitHub NEVER lists it in his "review requested" queue. Flagging was a *discipline* (remember to ping him) that got missed, mis-targeted (flagged #2342 instead of #2336), and mis-stated (called CHANGES_REQUESTED PRs "green + mergeable, ready to merge" — conflating mergeable=no-conflict with merge-ready). scripts/pr_flag.py makes it a *mechanism*: - scans open PRs, scoped to the owner's OWN authored PRs (the set GitHub hides). Deliberately NOT "any REVIEW_REQUIRED/CHANGES_REQUESTED PR" — that swept in ~40 peer PRs whose changes are their authors' job (a noise-bomb). A peer PR needing the owner's specific approval is routing-driven (a @-mention), not metadata. - classifies each into the OWNER's court (green + mergeable + unblocked → ready for his merge) vs the AGENT's court (changes-requested / CI-failing / conflict / needs-review → mine to finish). A CHANGES_REQUESTED PR is never shown as ready-to-merge — the exact mis-flag this prevents. - emits a digest that flags ONLY the owner-court PRs and honestly says "nothing ready" when his court is empty, summarizing the rest as "on me". - dedups on a state hash so it fires only when the actionable set changes (new PR, CI flips green, changes-requested cleared) — never re-spams a title edit. Fail-open (exit 0 always) so a cron/caller never breaks. Pure core (classify_prs / _ci_state / state_hash / render_digest) unit-tested in tests/pr-flag.test.py incl. the load-bearing "my CHANGES_REQUESTED PR is in my court, never the owner's" case; gh/discord/state I/O is glue. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rq18tfPmFcgzHQFgoSpsUN Stand: Echo Act IV Pro * fix(pr-flag): resolve workspace via loader cwd, not __file__ path-walk The workspace-resolution lint refused the `Path(__file__).resolve().parent.parent` repo-walk. The script runs from the repo root (cron/loop cwd), so resolve the workspace through `scripts/sutando-config.sh` (relative invocation) and let the gh/discord subprocesses inherit cwd — no __file__ walking. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rq18tfPmFcgzHQFgoSpsUN Stand: Echo Act IV Pro * test(pr-flag): cover remaining classifier branches + pragma the I/O glue Addresses qingyun's #2343 review (diff coverage 52.8% < 95%): - mark _fetch_prs() and main() `# pragma: no cover` — subprocess/gh/discord/state I/O glue that unit tests can't exercise (the untested CLI/I/O paths the review flagged); the pure logic stays fully covered. - add tests for the remaining classify_prs branches — mine + CI-failing, mine + REVIEW_REQUIRED, mine + else('open') — so every court/why path in the pure core is executed. (The other finding — the __file__ path-walk — was already fixed in 445b2ca; the workspace-resolution lint is green on head.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rq18tfPmFcgzHQFgoSpsUN Stand: Echo Act IV Pro * feat(pr-flag): also flag peer PRs that are one-approval-from-merge Extends the mechanism to cover the #2336-class (Chi 2026-07-27): a peer PR the owner can unblock. classify_prs now surfaces peer PRs that are green + REVIEW_REQUIRED + already carry >=1 approval — "your approval unblocks the merge" — into the owner's court alongside his own ready-to-merge PRs. Everything else peer (0 approvals / not green / changes-requested) is still skipped, so it stays a handful, not the ~45-PR REVIEW_REQUIRED firehose. Fetch now includes reviews; new tests cover the surface + the exclusions. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rq18tfPmFcgzHQFgoSpsUN Stand: Echo Act IV Pro * fix(pr-flag): hold-list so a PR I flagged issues on is never shown "ready" Chi caught the digest listing #2339 under "Ready for your merge" while I'd told him to HOLD it (2 fail-open bugs) — the metadata-driven mechanism was blind to my content review. Adds a durable hold-list (state/pr-flag-holds.json, str number → reason): a held PR (mine or peer) is classified "held", rendered under "⏸ Held (I flagged issues — don't merge yet)", and never under "ready". So the mechanism can't contradict my own review. Tests cover the hold override + that a held-only set prints no "ready" section. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rq18tfPmFcgzHQFgoSpsUN Stand: Echo Act IV Pro * refactor(pr-flag): script gathers state, agent judges (no scripted verdicts) Chi 2026-07-27 ("are you using a script to do judgement that should be done by an agent?"): yes — the "ready/held/needs-you" verdicts were hardcoded rules, and #2339 proved a script can't judge readiness (green+approved but fail-open bugs it couldn't see). Refactor: pr_flag.py now does ONLY the mechanical part — fetch PRs, emit objective per-PR state (ci/mergeable/review/approvals/is_mine) as JSON, dedup on a state-hash so the agent isn't woken when nothing changed. All judgement (readiness, whose-court, caveats/holds) moves to the AGENT, done live from this raw state each cycle. Drops classify_prs/render_digest/court and the hold-list (unnecessary once judging live). Tests cover raw_state fields + dedup. The cron prompt now hands the agent the state to judge. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rq18tfPmFcgzHQFgoSpsUN Stand: Echo Act IV Pro * fix(pr-flag): gather complete current-head owner state * fix: classify legacy PR status contexts * fix(pr-flag): count effective review states --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: john-the-dev <johnv.the.dev@gmail.com> Co-authored-by: Rui Wang <52230987+john-the-dev@users.noreply.github.com>
…ressed reply Owner-reported 2026-07-27: Chi forwarded a Discord message (a #2336 approval nudge) into a shared channel and asked "did you receive this" — the bridge saw an empty message and nothing was processed. Root cause: a forwarded message carries `message.reference` too (type=forward), with its real payload in `message_snapshots` and empty top-level content. The shared-channel addressee gate computed `is_reply = (message.reference is not None)`, so a forward was classified as a reply. A forward has no `reference.resolved.author`, so `is_addressed_in_shared_channel` saw a "reply not addressed to me" and skipped the message BEFORE the existing forward-handler (which extracts snapshot content) could run. Result: the owner's forward silently vanished. Fix: a forward is not a reply. New pure helper `reference_is_reply()` in `discord_addressee.py` returns True only for `reference.type == default` (matches on the enum member NAME so it is robust to value changes; a missing/None type stays a reply for back-compat). The bridge feeds its result into the addressee gate, so an owner's forward now flows through as addressed and the snapshot-extraction path runs. Pure helper is unit-tested (forward→not-reply, reply→reply, none, None-type back-compat, string names, and end-to-end owner-forward-is-addressed) in tests/discord-bridge-shared-channel-addressee.test.py; the bridge change is object-resolution glue. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rq18tfPmFcgzHQFgoSpsUN Stand: Echo Act IV Pro
…ressed reply Owner-reported 2026-07-27: Chi forwarded a Discord message (a #2336 approval nudge) into a shared channel and asked "did you receive this" — the bridge saw an empty message and nothing was processed. Root cause: a forwarded message carries `message.reference` too (type=forward), with its real payload in `message_snapshots` and empty top-level content. The shared-channel addressee gate computed `is_reply = (message.reference is not None)`, so a forward was classified as a reply. A forward has no `reference.resolved.author`, so `is_addressed_in_shared_channel` saw a "reply not addressed to me" and skipped the message BEFORE the existing forward-handler (which extracts snapshot content) could run. Result: the owner's forward silently vanished. Fix: a forward is not a reply. New pure helper `reference_is_reply()` in `discord_addressee.py` returns True only for `reference.type == default` (matches on the enum member NAME so it is robust to value changes; a missing/None type stays a reply for back-compat). The bridge feeds its result into the addressee gate, so an owner's forward now flows through as addressed and the snapshot-extraction path runs. Pure helper is unit-tested (forward→not-reply, reply→reply, none, None-type back-compat, string names, and end-to-end owner-forward-is-addressed) in tests/discord-bridge-shared-channel-addressee.test.py; the bridge change is object-resolution glue. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rq18tfPmFcgzHQFgoSpsUN Stand: Echo Act IV Pro
…ressed reply (#2342) * fix(discord-bridge): don't skip owner's FORWARDED message as an unaddressed reply Owner-reported 2026-07-27: Chi forwarded a Discord message (a #2336 approval nudge) into a shared channel and asked "did you receive this" — the bridge saw an empty message and nothing was processed. Root cause: a forwarded message carries `message.reference` too (type=forward), with its real payload in `message_snapshots` and empty top-level content. The shared-channel addressee gate computed `is_reply = (message.reference is not None)`, so a forward was classified as a reply. A forward has no `reference.resolved.author`, so `is_addressed_in_shared_channel` saw a "reply not addressed to me" and skipped the message BEFORE the existing forward-handler (which extracts snapshot content) could run. Result: the owner's forward silently vanished. Fix: a forward is not a reply. New pure helper `reference_is_reply()` in `discord_addressee.py` returns True only for `reference.type == default` (matches on the enum member NAME so it is robust to value changes; a missing/None type stays a reply for back-compat). The bridge feeds its result into the addressee gate, so an owner's forward now flows through as addressed and the snapshot-extraction path runs. Pure helper is unit-tested (forward→not-reply, reply→reply, none, None-type back-compat, string names, and end-to-end owner-forward-is-addressed) in tests/discord-bridge-shared-channel-addressee.test.py; the bridge change is object-resolution glue. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rq18tfPmFcgzHQFgoSpsUN Stand: Echo Act IV Pro * docs(addressee): trim added comments to the two-line/no-history rule Clears the P2 half of this PR's review. The three named sites — the forward-vs- reply comment in discord-bridge.py, reference_is_reply's docstring, and the test narrative — carried incident provenance (a date, a PR number, an owner report) and ran past two lines. Rewritten by hand, keeping the constraints and dropping the story: - a forward sets message.reference too, but its payload is in message_snapshots, so classing it as a reply makes the gate skip forwards - the match is on the enum NAME so it survives numeric changes, and a missing type is a reply (the pre-forward default) Topic diff 64 -> 52 added lines across the three files. No behaviour change. Both suites pass, and a mutation control confirms they still discriminate: forcing reference_is_reply to report every forward as a reply fails the addressee suite (exit 1), restoring it passes (exit 0). The live-path blocker is untouched and remains outstanding — it needs a supervised post-restart round trip on the owner's host, which no commit can supply. Stand: Echo Act IV Pro * docs(addressee): finish the two-line trim on the remaining docstring and test block The previous pass measured only `#`-prefixed lines, so the `reference_is_reply` docstring was never in the population it reported on. Re-measured over the AST: 1 added docstring, 0 paragraphs over two lines; 4 added comment runs, 0 over two. Both durable constraints are kept — forwards also carry `reference`, and the enum NAME is matched so renumbering does not break it. Stand: Sutando-Pro (principal) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(addressee): collapse the docstring to two lines TOTAL The cap in AGENTS.md:29 is two lines per comment, not two per paragraph, so a summary line plus a two-line paragraph is three. Both invariants survive the shorter form: forwards also carry `reference`, and the enum NAME is matched so renumbering is safe. Whole-diff measure: 4 comment runs, 1 docstring, 0 over cap. Addressee suite 25/25. Stand: Sutando-Pro (principal) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(addressee): assert the bridge IMPORTS the symbols it calls The three structural checks here match call-site text in discord-bridge.py, so deleting the `reference_is_reply` import leaves all three green while the bridge raises NameError on the first shared-channel forward. Measured: with the import removed the suite exited 0. Adds an AST pass over the bridge collecting module-level import bindings and asserts both addressee symbols are bound. Control: import removed -> exit 1 ("calls reference_is_reply() but never imports it"); restored -> exit 0. Stand: Sutando-Pro (principal) --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Rui Wang <52230987+john-the-dev@users.noreply.github.com>
What changed
sync-workspacenow resolves the per-host settings/access snapshot source withclaude-sutando-config-dir, the same canonical config path that startup exports to the Claude core and channel bridges.Previously
_snapshot_per_host_configusedclaude-home-path. In launchd/cron environments withoutCLAUDE_CONFIG_DIR, that helper falls back to legacy~/.claude, so a stale Discordaccess.jsoncould overwrite the vault backup and later restore an owner without theirownertier.This is the focused corrective follow-up to the drift detector in #2277.
Before / after evidence
Parent
649af1fwith the regression added but before the implementation change:HEAD
23f2867:Additional checks:
Live post-restart round trip
Tested on the affected host after restarting the Discord bridge:
The live access file retained owner ID
1358841611580080168asownerthrough the restart.Scope / risk
Single concern: snapshot source resolution plus its regression. No bridge behavior, prompts, restore semantics, or unrelated sync logic changed. The added lines contain no host-specific paths or inline path fallbacks.