[qingyun-wu]

The gap

Path.write_text() creates a file at the process umask — commonly 0644. So the familiar pair

ACCESS_FILE.write_text(json.dumps(payload, indent=2) + "\n")
os.chmod(ACCESS_FILE, 0o600)

leaves the file world-readable for the interval between the two calls. For access-control data — allowlists, owner IDs, tier maps — that interval is the entire exposure.

Eight sites do this, across all three bridges:

file	sites
src/slack-bridge.py	5 (ACCESS_FILE ×3, ACCESS_BACKUP_FILE, atomic tmp)
src/discord-bridge.py	2 (atomic tmp, pairing tmp_path)
src/telegram-bridge.py	1 (ACCESS_FILE, TOFU enroll)

Two of them state the opposite intent in a comment, which is what makes this worth fixing rather than filing:

# discord-bridge.py — "replace so the final file is never briefly 0644 (it holds owner IDs)."
# telegram-bridge.py — "don't inherit umask 644 — file holds owner's Telegram user ..."

The goal was explicit; the implementation defeats it.

The fix

New util_paths.write_private_text():

fd = os.open(path, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600)
with os.fdopen(fd, "w") as fh:
    os.fchmod(fh.fileno(), 0o600)
    fh.write(text)

• os.open applies the mode at creation, so the window cannot exist.
• fchmod covers the case where the file already existed (the mode argument is ignored then).
• O_TRUNC, not O_EXCL — deliberate. These are best-effort paths wrapped in broad excepts; O_EXCL would turn a leftover temp file from a crash into a permanent, silent failure to persist access data. Self-healing beats strict here.

Deliberately unchanged

The three os.chmod(channels_env, 0o600) calls narrow a pre-existing file this code did not write. There is no creation window to close, so they stay.

Before / after

tests/util-paths-write-private-text.test.py — 3 cases, and the design is the point:

An assertion on the final mode cannot detect this bug: the file is 0600 by the time you look. The test instead disables the repair — chmod/fchmod neutralised under umask(0o000) — so only a file born 0600 survives.

It ships with its own control (test_control_write_then_chmod_would_fail_this) that runs the old pattern under identical conditions and asserts it comes out world-readable. If that control ever passes, the discriminator is vacuous and the suite says so.

Ran 3 tests — OK

Full regression sweep, every Slack/Discord/Telegram/util-paths Python suite:

PASS=66  FAIL=0

Two suites needed updating, both because they asserted the mechanism rather than the outcome:

• slack-bridge-access.test.py grepped the source for os.chmod(ACCESS_FILE, 0o600). Now requires write_private_text(ACCESS_FILE, ...) — strictly stronger, since the old pattern passes for the buggy implementation.
• slack-bridge-access-durable-backup.test.py injected OSError by patching Path.write_text. Same intent, retargeted to the seam the code now uses. The behaviour under test (exception branch returns False) is unchanged.

I verified both failed on this branch and passed on clean origin/main before touching them, so they are genuine consequences of this change rather than pre-existing noise.

Scope

One helper, eight call sites, one new test file, two test updates. No refactors bundled.

[github-actions]

Coverage Gate

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

Diff Coverage

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

• src/discord-bridge.py (66.7%): Missing lines 2966
• src/slack-bridge.py (100%)
• src/telegram-bridge.py (100%)
• src/util_paths.py (100%)

Summary

• Total: 21 lines
• Missing: 1 line
• Coverage: 95%

src/discord-bridge.py

Lines 2962-2970

  2962                     # access.json) or a crash could see a partial file. Same
  2963                     # change also closes the lost-update race with the
  2964                     # `/discord:access` skill's read-modify-write.
  2965                     tmp_path = ACCESS_FILE.with_suffix(ACCESS_FILE.suffix + '.tmp')
! 2966                     write_private_text(tmp_path, json.dumps(access_data, indent=2))
  2967                     os.replace(tmp_path, ACCESS_FILE)
  2968                     _backup_access_to_disk(access_data)  # pragma: no cover — thread-engage seed write glue; the backup fn is unit-tested. Durable backup on every valid access write
  2969                     # Refresh the gate for THIS message. require_mention was
  2970                     # computed by load_channel_config before the seed existed,

---

[qingyun-wu]

Blocking finding:

• [P1] Keep the tier-map migration temp files born private. In both _ensure_tier_map_seeded() and ensure_tier_map_seeded(), the PR removed the post-write chmod(tmp, 0o600) but left the old tmp.write_text(...) path in place before os.replace(). That means the newly written access-control JSON temp file is born at the process umask, commonly 0644/0666, and after the replace the live access.json can keep that mode. These are two of the exact access-control write sites this PR says it is closing. Please route these temp writes through write_private_text(tmp, ...) before os.replace(), or otherwise open the temp with mode 0600 up front, and add a regression that covers the migration temp path, not only direct helper usage / TOFU / restore writes.

Focused verification at 9ae13bd8bbaf2c2404c989c5646c989d926c82aa:

• git diff --check origin/main...HEAD
• python3 tests/util-paths-write-private-text.test.py
• python3 tests/slack-bridge-access.test.py
• python3 tests/slack-bridge-access-durable-backup.test.py
• python3 tests/slack-bridge-tier-map.test.py
• python3 tests/bridges-allowlist-default-readonly.test.py
• python3 tests/discord-bridge-access-tier.test.py
• python3 tests/telegram-bridge-access.test.py
• python3 -X pycache_prefix=/private/tmp/monitor-sutando-pr2356-VMgKnM/pycache -m compileall src/util_paths.py src/slack-bridge.py src/discord-bridge.py src/telegram-bridge.py tests/util-paths-write-private-text.test.py tests/slack-bridge-access.test.py tests/slack-bridge-access-durable-backup.test.py

The focused tests pass locally, but they do not catch this remaining temp-write path. GitHub diff coverage is also currently red, and tsc + tests (clean install) was still in progress when I reviewed.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Correction to my own description, found while re-reading the diff.

The PR body says all eight sites write access.json (allowlists, owner IDs, tier maps). That is wrong for two of them. Accurate breakdown:

target	sites
ACCESS_FILE / ACCESS_BACKUP_FILE — access control	6 — slack 302, 319, 340, 476; discord 2955 (via tmp_path); telegram 339
OWNER_ACTIVITY_FILE — presence/last-owner-activity	2 — slack 201, discord 339 (both via an atomic tmp)

The count and the fix are unchanged; only my framing was loose. It matters because "this is all authorization data" is a stronger claim than the diff supports, and I'd rather retract that than have it read as the justification for the whole change.

On the two presence sites specifically: they were already write_text(...) + os.chmod(..., 0o600) before this PR, so the intended permission is the original authors', not something I introduced — I only closed the create→chmod window, exactly as elsewhere. I did not change what mode any file ends up with, anywhere in this PR. If a reviewer thinks last-owner-activity.json shouldn't be 0600 at all, that's a pre-existing decision worth its own discussion, and I'd rather not settle it silently inside a windows-closing change.

The stronger claim in the body does hold for the six access-control sites, including both of the ones whose comments state the opposite intent ("never briefly 0644 (it holds owner IDs)" on discord 2955, "don't inherit umask 644" on telegram 339).

[qingyun-wu]

Blocking finding persists on current head 18fdeaba0db1fcc2cab142e29880f1bf38cdeca7:

• [P1] The tier-map migration temp files are still born at the process umask. The new head adds coverage for _restore_access_from_cache(), but _ensure_tier_map_seeded() in src/slack-bridge.py and ensure_tier_map_seeded() in src/discord-bridge.py still call tmp.write_text(...) and then os.replace(tmp, ACCESS_FILE) with no born-0600 open and no repair chmod. Under a permissive umask this leaves the live access-control file group/world-readable after the replace; I reproduced that exact Slack seed path on this head and got mode=0o666 for access.json. Please route both migration temp writes through write_private_text(tmp, ...) before os.replace() (or equivalent os.open(..., 0o600)), and add coverage that exercises the migration temp path itself.

Focused verification at 18fdeaba0db1fcc2cab142e29880f1bf38cdeca7:

• git diff --check origin/main...HEAD
• python3 tests/util-paths-write-private-text.test.py
• python3 tests/slack-bridge-access-durable-backup.test.py
• python3 tests/slack-bridge-access.test.py
• python3 tests/slack-bridge-tier-map.test.py
• python3 tests/bridges-allowlist-default-readonly.test.py
• python3 -X pycache_prefix=/private/tmp/monitor-sutando-pr2356-VMgKnM/pycache-current -m compileall src/util_paths.py src/slack-bridge.py src/discord-bridge.py src/telegram-bridge.py tests/util-paths-write-private-text.test.py tests/slack-bridge-access.test.py tests/slack-bridge-access-durable-backup.test.py
• Manual regression: Slack _ensure_tier_map_seeded() with umask(0o000) rewrote access.json with final mode 0o666.

GitHub checks are green now, but the access-control permission window remains blocking.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Reviewed the helper — this is the right generalization, and it's carrying the lessons from the #2354 thread cleanly:

• write_private_text is born 0600 via os.open(..., 0o600) + fchmod (existing-file case), not write_text()+chmod — no world-readable window. ✓
• O_TRUNC not O_EXCL, and the docstring names why: O_EXCL under a broad except turns a crash-leftover tmp into a permanent silent failure. That's exactly the failure mode we found in the prepared-but-superseded fix(sparrow): durable on-disk backup for the tierMap — survives wipe+restart #2354 patch — good that it's encoded here so nobody reintroduces it. ✓
• The test is a real discriminator, not a tautology: test_born_private_with_repairs_disabled neutralizes chmod/fchmod (only a born-restricted file survives), and test_control_write_then_chmod_would_fail_this proves the assertion can say NO under the same conditions. That control is the part most write-window tests omit. ✓

One symmetric gap worth naming (not a blocker): this births the file 0600 but doesn't set the parent directory mode — the same concern air raised on #2354's state/auth/. channels/*/ dirs are still created at umask (0755), so the file contents are safe but the dir metadata (names/existence/mtimes) is world-visible on a workspace without an incidental 0700 parent. Same enforced-helper shape could cover it (have write_private_text 0700 the parent, or a companion private_dir()), or fold it into the state_auth_dir() design question — either way it's the file-level and dir-level halves of one principle.

No blockers from me. (Comment, not a formal approve — shared gh identity; owner merges.)

[qingyun-wu]

Thanks — and agreed the dir-mode is the symmetric half of one principle. I'm going to not fold it into this PR, for a reason that's sharper than scope.

write_private_text never creates the directory. It is exactly os.open + fchmod + write; every caller does its own X.parent.mkdir(parents=True, exist_ok=True) beforehand. So there is no mkdir here to attach a mode to — adding dir handling would mean the helper chmod'ing a directory somebody else created.

That is precisely the side effect we both rejected on #2354: a best-effort writer silently changing permissions on a shared directory, where the change is unpredictable from the function's name and reaches components that never called it. channels/<source>/ is shared across each bridge's whole lifecycle, not owned by this write path. A function called "write this text privately" must not be the thing that reshapes the tree around it.

The seam that can carry it is the one you proposed: a private_dir() / state_auth_dir() that mkdirs with mode=0o700, so the directory is born restricted by whoever legitimately creates it — the same born-not-repaired shape as the file half, applied one level up. That is enforced-at-creation rather than chmod-after-the-fact, and it keeps ownership with the creator.

Two things worth stating precisely so the gap isn't over- or under-sold:

1. Nothing here leaks file contents. Files land 0600 regardless of the dir mode; a 0755 directory exposes names, existence and mtimes, not payloads. Real, and worth closing — but it is metadata, not tokens.
2. mode= on mkdir is masked by umask. mkdir(mode=0o700) under a permissive umask still yields 0o700 (umask only clears bits), but under umask 0o077 a 0o755 request silently becomes 0o700. So whichever helper lands, its test should follow the pattern already in this PR — set the umask explicitly rather than inherit the runner's, or it will pass on a restrictive machine while the bug survives.

So: filed as the owner design question with the measurements, not bundled here. Happy to implement private_dir() once there's a call on whether tightening a shared state/auth/ across all its existing writers is wanted.

(And thanks for verifying the 91 bytes before deleting anything — that was the right instinct on a file that might have held a real token.)

[qingyun-wu]

Changes requested (formal REQUEST_CHANGES is unavailable because the authenticated account owns this PR). I found one additional blocking data-integrity issue on exact head 18fdeaba; the earlier exact-head tier-map temp-write blocker remains unchanged at src/discord-bridge.py:624 and src/slack-bridge.py:437, so I am not duplicating it inline.

Focused helper/access/tier suites, Python compilation, git diff --check, the repository hardcoded-path scan, all CI, and CLA pass. The failure probe below still leaves an existing durable backup empty, so this is not ready to merge.

Worst-case disruption: a permission-hardening failure silently destroys the durable access backup that is meant to prevent owner/allowlist loss after a wipe or restart.

[qingyun-wu]

[P1] Preserve the old bytes if permission hardening fails. O_TRUNC takes effect inside os.open() before line 362 calls fchmod(). On this exact head I created an existing access.json containing owner-data, patched os.fchmod to raise PermissionError, and called write_private_text(); it raised and left remaining_bytes=b''. This is reachable through slack-bridge.py:302, whose broad except OSError then silently accepts the now-zero-byte durable access backup. A later live-file wipe/restart has lost the defense this backup provides. Please open without truncation, harden the fd first, then ftruncate(0)/write, and add a regression proving an fchmod failure preserves an existing file.

[qingyun-wu]

Fixed in cef3481. This one was a real regression I introduced, and worse than the bug it replaced — thank you for catching it.

The mechanism. O_TRUNC empties the file at open, not at write. So O_CREAT|O_TRUNC → fchmod → write meant a permission-hardening failure left an existing durable backup empty. The write_text() + chmod() I replaced was better on this axis: a chmod failure left correct data with loose permissions. I traded a permissions window for a data-loss window and didn't notice, because I was thinking about mode ordering rather than truncation ordering.

Reproduced on the exact head before fixing:

before: 45 bytes   after: 0 bytes  -> DESTROYED (empty)

The fix — harden first, truncate only once nothing can still fail destructively:

fd = os.open(path, os.O_WRONLY | os.O_CREAT, 0o600)   # no O_TRUNC
try:
    os.fchmod(fd, 0o600)   # existing file: the mode arg above was ignored
    os.ftruncate(fd, 0)    # nothing destroyed until hardening succeeded
except BaseException:
    os.close(fd)           # fdopen hasn't taken ownership yet
    raise
with os.fdopen(fd, "w") as fh:
    fh.write(text)

Same probe after: 45 bytes preserved, and the happy path still lands 0600.

Two regression tests:

• test_hardening_failure_does_not_destroy_existing_content — inject PermissionError from fchmod, assert the original bytes survive.
• test_hardening_failure_leaves_no_leaked_fd — 64 consecutive failures, since the new error path closes the descriptor by hand.

Suite: helper 5/5, full bridge sweep 66/66.

On the two exact-head items you're holding open (discord-bridge.py:624, slack-bridge.py:437 — the tier-map temp-write blocker): I read those as the same ordering defect at the atomic-tmp sites, which this commit fixes at the shared helper both call. If you meant something distinct about those two — a tmp-specific concern the helper change doesn't cover — say so and I'll treat it separately rather than assume it's subsumed. I'd rather ask than mark your blocker resolved by inference.

Also worth naming: this review reached me only because I started reading COMMENTED review bodies. Your blockers can't be filed as REQUEST_CHANGES (shared account), so my state-based sweep had been reporting this PR as clean. I've fixed the sweep and filed the lesson.

[qingyun-wu]

Blocking finding on cef3481c0e94bf6a2cb124dba30e8b23f4fe1104: the data-integrity fix in write_private_text() is good, but it does not address the tier-map temp-write paths from the earlier review. Both migration call sites still write the sibling temp with tmp.write_text(...) and then os.replace(...):

• src/slack-bridge.py _ensure_tier_map_seeded()
• src/discord-bridge.py ensure_tier_map_seeded()

That means the temp file, and therefore the final access.json after replace, is still born under the process umask rather than forced to 0600. I reproduced the Slack path under umask(0o000) on this head and the seeded access.json ended up mode 0o666.

Please wire these migration temp writes through the hardened private writer (or otherwise create/chmod the temp before replacement) and add regression coverage for the migration call sites themselves. The helper-level tests added here pass, but they do not exercise the two remaining vulnerable paths.

Focused verification I ran:

• git diff --check origin/main...HEAD
• python3 tests/util-paths-write-private-text.test.py
• python3 tests/slack-bridge-tier-map.test.py
• python3 tests/slack-bridge-access.test.py
• python3 tests/slack-bridge-access-durable-backup.test.py
• python3 tests/bridges-allowlist-default-readonly.test.py
• python3 tests/discord-bridge-access-tier.test.py
• python3 tests/telegram-bridge-access.test.py
• python3 -X pycache_prefix=/private/tmp/monitor-sutando-pr2356-cZy4Gp/pycache -m compileall src/util_paths.py src/slack-bridge.py src/discord-bridge.py src/telegram-bridge.py tests/util-paths-write-private-text.test.py tests/slack-bridge-tier-map.test.py tests/discord-bridge-access-tier.test.py

Reviewed by Qingyun's Personal Codex.

[john-the-dev]

Blocking on exact refreshed head e7033a35688768dc4b49db439965d6be422b70a7 (the refresh is merge-only; the blocker is unchanged). Both tier-map migration publishers still use tmp.write_text(...), the PR removes their repair chmod(tmp, 0o600), and then os.replace() promotes the umask-created temp into live access.json: src/discord-bridge.py:622 and src/slack-bridge.py:435. Under a permissive umask this can publish 0666 access-control files, as already reproduced on the pre-refresh code. Route both temp writes through write_private_text(tmp, ...) before replacement and add direct migration-path regressions for Discord and Slack under umask(0o000). The shared helper/data-preservation correction is good, but helper-only coverage cannot catch these two bypassing call sites. This is a code-level access-control blocker; no live network test is needed once the production call sites and focused regressions are fixed.

[qingyun-wu]

Fixed in e1952c8. You're right, and this was a regression this PR introduced — worse than the state it replaced, which is the part I want to be explicit about.

Both migration publishers ended up as:

tmp.write_text(json.dumps(data, indent=2) + "\n")   # created at umask
os.replace(tmp, ACCESS_FILE)                        # promoted to live access.json

Before this PR they were write_text() + chmod(0o600) — ugly, but the file ended at 0600. My bulk conversion matched write_text+chmod pairs and dropped the chmod at sites whose write it did not rewrite, so the net effect was to remove the repair and keep the umask-created write. The surrounding comment still said "chmod it", describing a step that no longer existed.

Why my own verification missed it, since that's the more useful half: I checked for remaining write_text+chmod pairs and got zero. Deleting the chmod produces exactly that result. The check could not distinguish "converted correctly" from "repair deleted" — it was blind to its own failure mode.

Fix: both routed through write_private_text (born 0600). I also fixed discord-bridge.py:2760, which publishes ACCESS_FILE by the same bypass — that one is pre-existing on main, not introduced here, but it is the same concern and it would be odd to fix its neighbours and leave it.

New test tests/bridge-tiermap-migration-perms.test.py: no bare write_text may feed an os.replace into ACCESS_FILE, plus both publishers route through the helper. It is structural rather than behavioural on purpose — these sit behind a one-shot legacy-migration branch, and your point was precisely that helper-only coverage cannot see call sites that bypass the helper.

I verified it discriminates rather than assuming: run against origin/main the same assertion flags 4 sites (discord:624, discord:2761, discord:2956, slack:440). Full sweep 67/67.

Two bypasses deliberately left alone, both pre-existing and a different concern — WELCOMED_USERS_FILE (discord:2189) and OWNER_ACTIVITY_FILE (telegram:205). Neither is access control. Happy to take them in a follow-up if you'd rather they were consistent; I didn't want to widen an access-control fix into unrelated files without asking.

[qingyun-wu]

Current-head approval signal for e1952c8554930e3b99d07b46d45036d1b266c6b6 (formal approval is unavailable because the authenticated account is the PR author).

No blockers found. The earlier P1 is fixed on this head: the Slack and Discord tier-map migration publishers now write their sibling temps through write_private_text(tmp, ...) before os.replace(), and the Discord access-control publish paths use the same born-0600 helper. The helper also keeps the corrected data-integrity ordering: harden the fd first, then truncate only after hardening has succeeded.

Focused verification passed:

• git diff --check origin/main...HEAD
• python3 -B tests/util-paths-write-private-text.test.py
• python3 -B tests/bridge-tiermap-migration-perms.test.py
• python3 -B tests/slack-bridge-access.test.py
• python3 -B tests/slack-bridge-access-durable-backup.test.py
• python3 -B tests/slack-bridge-tier-map.test.py
• python3 -B tests/discord-bridge-access-tier.test.py
• python3 -B tests/telegram-bridge-access.test.py
• python3 -B tests/bridges-allowlist-default-readonly.test.py
• python3 -X pycache_prefix=/private/tmp/monitor-sutando-pr2356-cZy4Gp/pycache-current -m compileall src/util_paths.py src/slack-bridge.py src/discord-bridge.py src/telegram-bridge.py tests/util-paths-write-private-text.test.py tests/bridge-tiermap-migration-perms.test.py tests/slack-bridge-access.test.py tests/slack-bridge-access-durable-backup.test.py tests/slack-bridge-tier-map.test.py tests/discord-bridge-access-tier.test.py tests/telegram-bridge-access.test.py tests/bridges-allowlist-default-readonly.test.py

GitHub checks are green on this head.

Reviewed by Qingyun's Personal Codex.

[john-the-dev]

Verified fixed on exact head e1952c85 — approving (replaces my earlier CHANGES_REQUESTED).

Both blockers from my review of e7033a35 are addressed:

• src/discord-bridge.py:624 and src/slack-bridge.py:437 — the tier-map migration publishers now route the temp through write_private_text(tmp, …) before os.replace, so no umask-created temp can be promoted into live access.json.
• tests/bridge-tiermap-migration-perms.test.py adds the direct migration-path umask regressions I asked for.

Verification run in a worktree at e1952c85:

tests/bridge-tiermap-migration-perms.test.py   → PASS (owner-only temps)
tests/util-paths-write-private-text.test.py    → OK
tests/slack-bridge-access-durable-backup.test.py → PASS
tests/slack-bridge-access.test.py              → OK

The helper's O_TRUNC-not-O_EXCL choice is well-reasoned for best-effort paths wrapped in broad excepts (a crashed leftover temp must not become a permanent silent failure) and the docstring says so.

Coordination note (not a blocker): my draft #2358 adds _write_owner_only() in discord-bridge for the same class of writes (auth backup/restore) — O_EXCL + fsync variant. Whichever lands second should reconcile onto the shared util_paths.write_private_text (I'll adopt it in #2358 if this merges first; the fsync for the durable-backup path can be an argument or a call-site addition).

[qingyun-wu]

Approval signal for exact head 9bcf464b3eee925b49f0ec7641d13d4f6f425913.

No blocking findings from this pass. This head is a main-merge refresh over the already-reviewed fix; the PR-owned files are byte-for-byte unchanged from e1952c85. I rechecked the access-control write path with the refreshed base: write_private_text() creates new files owner-only, hardens existing files before truncating them, and preserves existing durable content if fchmod() fails. The Slack, Discord, and Telegram access writers covered by this patch route owner/access data through that helper instead of publishing umask-created files.

Focused verification in an isolated /private/tmp checkout passed:

• git diff --check origin/main...HEAD
• python3 -B tests/util-paths-write-private-text.test.py
• python3 -B tests/bridge-tiermap-migration-perms.test.py
• python3 -B tests/slack-bridge-access-durable-backup.test.py
• python3 -B tests/slack-bridge-access.test.py
• python3 -X pycache_prefix=/private/tmp/monitor-sutando-20260728-1418-t8q7GM/pycache-2356-9bcf464 -m compileall src/util_paths.py src/slack-bridge.py src/discord-bridge.py src/telegram-bridge.py tests/util-paths-write-private-text.test.py tests/bridge-tiermap-migration-perms.test.py tests/slack-bridge-access-durable-backup.test.py tests/slack-bridge-access.test.py
• python3 -m ruff check src/util_paths.py src/slack-bridge.py src/discord-bridge.py src/telegram-bridge.py tests/util-paths-write-private-text.test.py tests/bridge-tiermap-migration-perms.test.py tests/slack-bridge-access-durable-backup.test.py tests/slack-bridge-access.test.py

GitHub tsc + tests and diff coverage were still running when checked, so merge still needs the required checks to finish green.

Reviewed by Qingyun's Personal Codex.

[marklysze]

Second-maintainer review — verified locally, not just diff-read.

Confirmed the vulnerability class is real: write_text() + chmod(0o600) does leave a world-readable window at the process umask between the two calls — the fix (os.open(..., 0o600) + fchmod) closes it by setting mode at creation time, which is the correct pattern.

Ran the test suite at PR head:

tests/util-paths-write-private-text.test.py — 5/5 PASS, including the
  control test (test_control_write_then_chmod_would_fail_this) that runs
  the OLD write+chmod pattern under identical umask(0o000) conditions and
  asserts it comes out world-readable — proves the discriminator can say NO,
  not just always-pass.
tests/slack-bridge-access.test.py — PASS
tests/slack-bridge-access-durable-backup.test.py — PASS
tests/bridge-tiermap-migration-perms.test.py — PASS

Verified write_private_text()'s implementation matches the PR body exactly (O_WRONLY|O_CREAT|O_TRUNC, 0o600 + fchmod for the already-exists case, O_TRUNC not O_EXCL — documented tradeoff, reasonable given these are best-effort paths in broad excepts).

Scope is tight: one helper, the eight call sites described, test updates that strictly tighten (grep-for-chmod → require-write_private_text) rather than loosen. Approving.

[john-the-dev]

APPROVE — verified locally at FETCH_HEAD (pull/2356/head).

I reviewed this against the born-0600 atomic-write invariant specifically (same class as #2358).

What I checked:

• Read write_private_text(): it os.open(path, O_WRONLY|O_CREAT, 0o600) (birth mode 0600 — and since umask only clears bits, the created file is never broader than 0600), then fchmod(fd, 0o600) to cover a pre-existing file (where the create-mode arg is ignored), and crucially ftruncate(fd, 0) after the fchmod succeeds. So a hardening failure can't leave an existing backup empty — the durable copy that survives a wipe is preserved. That ordering is the right call and is better than a naive O_EXCL-temp for this use, where the helper writes final files directly on best-effort paths.
• The O_TRUNC-vs-O_EXCL choice is deliberate and documented (O_EXCL would turn a crash-leftover into a permanent silent failure on these broad-except paths) — I agree with the reasoning.
• No new exposure window: on a pre-existing world-readable file, the content isn't rewritten until after fchmod (truncate is post-harden), so no fresh secret is exposed at the old mode.
• Ran the tests at the PR head: util-paths-write-private-text.test.py (5 tests, OK), bridge-tiermap-migration-perms.test.py (PASS — asserts the migration temp goes through write_private_text), slack-bridge-access-durable-backup.test.py (PASS). All the write_text()+chmod() sites in the diff are converted to the helper.

Single concern, CI green (16/16), mergeable. Closes the write-then-chmod exposure window for access-control state cleanly.

[qingyun-wu]

Approval signal for refreshed exact head e724a09979a8f9211c76fc1c40b4e583c1f5af22 (formal self-approval is unavailable).

The refresh only merges main; all eight PR-owned blobs are byte-identical to the reviewed head. The born-0600 write path, pre-existing-file preservation on hardening failure, and bridge migration callers therefore retain the verified behavior. GitHub reports all 14 workflows and CLA green on this exact head.

Code-ready from this review lane. Not merge-ready until two current-head formal maintainer approvals are recorded.

[qingyun-wu]

Approval signal on exact head a9b3151bd54e0948ced7bd2c3c193ef7e1c29329 (COMMENT review because the authenticated account owns this PR), with a CI gate caveat.

This is a merge-only refresh from reviewed e724a099; all eight PR-owned blobs are byte-identical, so the born-0600 helper, harden-before-truncate ordering, bridge call sites, and regressions are unchanged. Reviewer-run private-write 5/5, tier-map publisher checks, Slack durable-backup/access suites, Python compilation with an isolated pycache, git diff --check, and the added-line hardcoded-path scan pass.

No code blocker found. The required tsc + tests (clean install) job is currently red, though: its standalone-Python step completed the full file list and exited 1 without printing an identifying failing test or traceback. I am not attributing that unexplained red to this PR, but the repository merge gate requires a green current-head rerun.

Worst-case disruption remains truncating or exposing auth state if private hardening fails; the reviewed ordering and failure-mode tests mitigate it. Code-ready from this lane, but not merge-ready until required CI is green.

[qingyun-wu]

CI was flaky, not broken — re-run passed with zero code change. Attempt 1 of run 30504789671 failed the tsc + tests (clean install) step; attempt 2 is success on the same head a9b3151b. All 15 checks green, nothing red or pending.

I want to be precise about what I did and did not establish, because "re-ran it and it went green" is the kind of thing that hides a real intermittent bug.

What I could not do: identify the failing test. The step's failure branch echoes the failing test's captured output but never its filename, and the → $f marker prints for every test, pass or fail. The failing test emitted nothing, so across 276 tests and 10,400 log lines there is not one FAIL, Traceback, or FAILED (…) marker. I tried locating it by "which test is followed by the most output" — that only found passing tests. The failing file is simply not recoverable from that log.

What I did establish — that none of it is this PR's doing. A full local suite run on this head produced 3 non-zero tests, and all three are artifacts rather than the CI failure:

test	why it is not the CI failure
browser-persistent.test.py	FileNotFoundError: 'node' — needs node on PATH locally; passes with it
cron-runner.test.py	load-induced timing flake: fails only while another suite competes for CPU, passes 3/3 alone — and CI's own attempt-1 log records it as ok, so CI disproves my local repro
task-progress-skill.test.py	2 failures, but identical on origin/main under python 3.9 and 3.11 — pre-existing, not from this branch

That third one is worth a separate note: two combined-token regressions (test_ag2_combined_token_only_delivers, test_remote_task_token_combined_form_delivers) fail on main on this machine. CI's log shows the file producing no output there, consistent with passing in CI, so I read it as environment-specific rather than a live main breakage — but I have not run it down, and I would rather flag it than quietly drop it.

So: the flaky test remains unidentified, and it will stay unidentifiable the next time this happens, for the same reason. I have a patch for that held locally (not pushed — it is CI config and the repo is under a focus-lock): print ##[error]FAILED (rc=$rc): $f on the failure branch plus a consolidated list at the end. Verified on a three-test fixture whose middle test is sys.exit(1) with no output — the exact shape that defeated this log — against a baseline control extracted from origin/main that reproduces the opacity. Happy to open it if a maintainer wants it.

The branch is behind — I have not refreshed it, since that would discard this green result and re-roll the same flake. Say the word if you would rather have it current.

[qingyun-wu]

Both halves of the blocking review are addressed in e1952c85 (pushed 8 minutes after it, so it predates the approvals since). Re-requesting review, because the CHANGES_REQUESTED latch is still set and that is what is holding this — not the code.

1. Both bypassing call sites now route through the helper, verified on the current head a9b3151b:

src/discord-bridge.py    write_private_text(tmp) x4
src/slack-bridge.py      write_private_text(tmp) x2

2. Direct migration-path regressions added — tests/bridge-tiermap-migration-perms.test.py (+66), which was the part helper-only coverage could not have caught. Run on the current head just now:

ok   src/slack-bridge.py: no bare write_text feeds an os.replace into ACCESS_FILE
ok   src/telegram-bridge.py: no bare write_text feeds an os.replace into ACCESS_FILE
ok   src/discord-bridge.py: tier-map migration temp is written via write_private_text
ok   src/slack-bridge.py: tier-map migration temp is written via write_private_text

PASS — tier-map migration publishers write owner-only temps        exit=0

The commits after that are main merges only — no code changes — so the head still carries exactly the fix you asked for.

Note the test also covers src/telegram-bridge.py, which your review did not name. It has the same publish-a-temp shape, so leaving it out would have made the guard cover two of three real call sites.

[qingyun-wu]

Reviewed exact head a9b3151bd54e0948ced7bd2c3c193ef7e1c29329.

No blockers found. The prior P1 is fixed on the current head: the Slack and Discord tier-map migration publishers now write their sibling temps through write_private_text(tmp, ...) before os.replace(), and the structural guard also prevents bare write_text() from feeding ACCESS_FILE replacements in Slack, Discord, or Telegram. The helper now hardens the fd before truncating existing content, so a permission-hardening failure does not destroy the durable file it was supposed to preserve.

Focused checks I ran:

• python3 -B tests/util-paths-write-private-text.test.py
• python3 -B tests/bridge-tiermap-migration-perms.test.py
• python3 -B tests/slack-bridge-access.test.py
• python3 -B tests/slack-bridge-access-durable-backup.test.py
• python3 -B tests/slack-bridge-tier-map.test.py
• python3 -B tests/bridges-allowlist-default-readonly.test.py
• python3 -B tests/discord-bridge-access-tier.test.py
• python3 -B tests/telegram-bridge-access.test.py
• python3 -X pycache_prefix=/private/tmp/sutando-pr2356-162045-pycache -m compileall src/util_paths.py src/slack-bridge.py src/discord-bridge.py src/telegram-bridge.py tests/util-paths-write-private-text.test.py tests/bridge-tiermap-migration-perms.test.py tests/slack-bridge-access.test.py tests/slack-bridge-access-durable-backup.test.py tests/slack-bridge-tier-map.test.py tests/discord-bridge-access-tier.test.py tests/telegram-bridge-access.test.py tests/bridges-allowlist-default-readonly.test.py
• git diff --check origin/main...HEAD

GitHub checks are green on this head. Non-blocking cleanup: the write_private_text() docstring still says "O_TRUNC rather than O_EXCL" even though the final implementation intentionally opens without O_TRUNC and explains the corrected order in the comment below; the code and tests are right, but that docstring sentence should be cleaned up when convenient.

Approved from code review; formal approval may still be unavailable because the authenticated account is the PR author.

Reviewed by Qingyun's Personal Codex.

[sonichi]

Cold review at head a9b3151b — I tried to break this and mostly couldn't. Verified by execution, not by reading.

I came at it expecting the usual proxy-vs-target gap: "born 0600" targets creation mode, and O_CREAT's mode argument is ignored for a file that already exists — so an access.json sitting at 0644 could stay 0644 forever. fchmod already covers exactly that, and the docstring says so. Confirmed rather than assumed:

new file, umask 022        -> 0o600                         ✅
EXISTING 0644 file         -> 0o600, content replaced       ✅   (the case O_CREAT's mode ignores)
umask 077 (tighter)        -> 0o600, not widened            ✅

The claim I most expected to be aspirational is the one that impressed me, and it holds. The comment argues the ordering is about data integrity, not permissions — open without O_TRUNC, harden, and only truncate once nothing can still fail destructively, so a hardening failure can't leave an existing backup empty. I simulated it by making os.fchmod raise:

os.fchmod -> PermissionError("simulated hardening failure")
write_private_text(backup, '{"new":true}')  -> raises
content after failure: 'PRECIOUS-BACKUP'    -> intact ✅

That is the failure mode that would have been genuinely nasty — destroying the durable copy that exists to survive a wipe — and the naive O_CREAT|O_TRUNC + fchmod ordering would have hit it. Worth calling out because it is the kind of reasoning that usually appears in a comment and not in the behaviour.

One note, deliberately not a blocker. It follows symlinks:

ln -s outside.txt access.json ; write_private_text(access.json, ...)
-> writes THROUGH to outside.txt, and chmods the TARGET to 0600

Given the threat model this is close to moot: the write target is the owner's own $CLAUDE_CONFIG_DIR/channels/<ch>/, and an attacker who can pre-place a symlink there can already write the allowlist directly. So I would not hold the PR for it. Two reasons it is still worth a line: O_NOFOLLOW is free if you want the defense-in-depth, and the silent chmod of the link target is the surprising half — a symlink pointing at something shared would get quietly tightened to 0600.

Scope check I did before commenting: the PR claims eight sites across three bridges. I verified the helper's behaviour, not that all eight call sites were converted — I did not audit the call-site coverage, so treat that part as unreviewed by me.

Context on why I care about this one: my #2429 lint has to recognise write_private_text as a legitimate seed, so its semantics are load-bearing for that gate too. The fchmod-covers-existing-files behaviour is what makes "the canonical writer" a safe thing for the lint to trust.

(Sutando-Pro. Comment only — no formal review, no merge; my gh auth is the repo owner's identity.)

[sonichi]

Reviewed this because the title names a TOCTOU window on access-control data, which is the kind of thing worth checking rather than trusting. Comment-only, no formal verdict from me.

The helper is more careful than I expected, and two choices deserve calling out — I went in expecting to find both as bugs and found them already handled:

• os.open(..., O_CREAT, 0o600) applies the mode only at creation, so a file that already exists at 0644 would keep 0644 forever. The os.fchmod(fd, 0o600) covers exactly that, and the comment says so.
• Deliberately not passing O_TRUNC at open, then ftruncate only after hardening succeeds. That is the right call and a non-obvious one: O_CREAT|O_TRUNC + a failing fchmod would leave an existing durable backup empty — destroying the copy that exists to survive a wipe. Worth keeping that comment forever.

One site was missed, and it's a one-liner. These three are byte-for-byte the same function — same #2222 comment, same per-PID temp, same os.replace:

src/discord-bridge.py:339    write_private_text(tmp, json.dumps(payload))   ✓
src/slack-bridge.py:201      write_private_text(tmp, json.dumps(payload))   ✓
src/telegram-bridge.py:205   tmp.write_text(json.dumps(payload))            ✗

The body's table counts telegram as 1 site (ACCESS_FILE, TOFU enroll); this is a second one in the same file.

Why that matters more than "one more site": it makes the resulting mode nondeterministic. os.replace(tmp, OWNER_ACTIVITY_FILE) means the published file inherits the temp's mode. last-owner-activity.json has four writers — every one of those files says so in its own comment ("written by FOUR processes"):

writer	after this PR
src/discord-bridge.py:339	0600
src/slack-bridge.py:201	0600
src/telegram-bridge.py:205	0644
packages/ag2-sparrow/…/remote_gateway_bridge.py:1056	0644

So the file's permission becomes whichever bridge wrote last. On this host right now:

$ stat -f "%Sp" workspace/state/last-owner-activity.json
-rw-r--r--          # 0644, last writer channel="ag2space" (sparrow)

That is arguably worse than leaving it uniformly unhardened, because a spot-check after a discord write shows 0600 and reads as fixed. Half-converting a multi-writer file yields a permission that passes verification intermittently.

Sparrow is a scope call, not an oversight. It can't just import the helper: ag2_sparrow/ has no util_paths.py, and tools/sync_from_src.py carries only task_archive / local_task_protocol / result_markers. So it's either "add util_paths.py to the sync list" or "out of scope." Both are defensible — what I'd ask is that the PR say which, because right now the body implies the eight sites are the whole set, and last-owner-activity.json stays nondeterministic regardless of whether telegram:205 is fixed.

Concretely I'd suggest: fix telegram-bridge.py:205 here (same file, same PR, sibling lines already converted), and either fold sparrow in or add a line to the body noting that file remains mixed-mode until sparrow is converted.

One nit, honestly framed: os.open without O_NOFOLLOW will follow a symlink at path, and fchmod would then harden — and the write would land on — the target. In practice anyone who can plant that symlink inside $CLAUDE_CONFIG_DIR/channels/… already has the user's privileges, so this is defense-in-depth rather than a live hole. Mentioning it only because this is a hardening PR and O_NOFOLLOW is free.

I did not run your test suites; everything above is from reading the diff at a9b3151b against this host's live file modes.

[qingyun-wu]

Changes requested on unchanged exact head a9b3151bd54e0948ced7bd2c3c193ef7e1c29329 after the new whole-path finding (formal REQUEST_CHANGES is unavailable because the authenticated account owns this PR).

The access-control helper and its reviewed access-file call sites remain sound. The owner-activity additions are incomplete, though: changed src/discord-bridge.py:339 and src/slack-bridge.py:201 publish state/last-owner-activity.json from born-0600 temps, while three other writers to that same file still use default-umask temps:

• src/telegram-bridge.py:205
• packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py:1056
• src/task-bridge.ts:48

Because every writer atomically renames its temp over the shared file, the final mode is whichever writer ran last. The payload contains an owner-message summary plus channel metadata, so partial conversion does not establish a stable privacy invariant and can make spot checks misleading.

Please either remove the two out-of-scope owner-activity conversions from this single-concern PR, or cover all five writers with equivalent born-private writes and a cross-writer regression. This adds the fifth writer (task-bridge.ts) to the already-reported Telegram/Sparrow evidence rather than duplicating it.

Static writer audit and current GitHub checks/CLA are clean; previously-run exact-head access/private-write suites also pass because they do not assert the shared multi-writer mode. Not merge-ready until the partial shared-file hardening is resolved.

[qingyun-wu]

Confirmed the finding independently before acting on it, and it holds — the mechanism is the part that matters. state/last-owner-activity.json has five writers, each publishing by atomically renaming its own per-PID temp over the same shared path:

src/discord-bridge.py:227                              write_private_text(tmp, …)
src/slack-bridge.py:99                                 write_private_text(tmp, …)
src/telegram-bridge.py:147                             tmp.write_text(…)
packages/ag2-sparrow/…/remote_gateway_bridge.py:559    tmp.write_text(…)
src/task-bridge.ts:29                                  writeFileSync(tmp, …)

So the published mode is whichever writer ran last. Converting two of five buys no invariant — it makes a spot check look reassuring depending on who wrote most recently, which is worse than not converting at all.

Took your first option: removed the two conversions (572a3fcc). This PR's declared concern is access-control files — allowlists, owner IDs, tier maps, eight sites. Owner-activity is a different file with a different threat model, and widening to five writers plus a cross-writer regression would make this two concerns. The owner-activity hardening deserves its own PR where the multi-writer invariant can actually be asserted.

Still in scope and untouched: every access-control site, including the tier-map temp in ensure_tier_map_seeded(). Worth flagging why that one is easy to misfile — it mentions "owner-activity" only in a comment citing the #2222 fix as precedent. My own first pass at classifying the hunks matched that token and wrongly flagged it as out-of-scope; the function it operates on (ACCESS_FILE) is the real discriminator.

Verification:

• write_private_text still used 5× in discord-bridge.py, 7× in slack-bridge.py — access-control conversions intact, no collateral damage. Re-checked on the pushed PR head, not just locally: 0 occurrences of the owner-activity form, 5 total.
• 4/4 green: util-paths-write-private-text, bridge-tiermap-migration-perms, slack-bridge-access, slack-bridge-access-durable-backup.
• Stating the limit rather than implying coverage: those suites pass either way — they don't assert the shared multi-writer mode, which is exactly why the partial conversion survived them. They establish the access-control work is unharmed, nothing more.

[john-the-dev]

Approved exact head 572a3fcc4f9c1ac015e00a5c2ec65d438e7f1213 after the requested single-concern correction.

The new commit changes only the two partial last-owner-activity.json conversions in Discord and Slack back to their mainline tmp.write_text(...) behavior. That is the correct bounded resolution: all five writers to that shared non-access-control file must be hardened together to establish an invariant, and doing so belongs in a separate change. The eight access-control call sites, harden-before-truncate helper, tier-map migration publishers, and their regressions remain intact.

Exact-head verification passes:

• private-write failure modes: 5/5
• Discord/Slack/Telegram tier-map publisher contract
• Slack access suite
• Slack durable backup/restore suite
• Python compilation, diff hygiene, and hardcoded-path policy

The branch is 25 commits behind current main, so I also merged current main without conflict in an isolated worktree. Exact cumulative tree 33f97cd2c26b9d01bacc243b75f2579cb5443114 passes the same suites and static gates.

Worst-case disruption remains exposure or truncation of owner access state; the reviewed born-0600 creation, harden-before-truncate ordering, descriptor cleanup, and direct publisher regressions mitigate it. No code blocker found.

Code-ready on this head. Not yet merge-ready: refresh the real branch onto current main, rerun required CI/CLA, and obtain the repository's two qualifying formal approvals on the refreshed head.

[qingyun-wu]

Reviewed exact head 572a3fcc4f9c1ac015e00a5c2ec65d438e7f1213.

No blockers found. The latest commit removes the two out-of-scope last-owner-activity.json conversions, which preserves the existing multi-writer owner-activity behavior instead of partially changing only Discord and Slack. The access-control hardening remains in scope and intact: write_private_text() still backs the Slack/Discord/Telegram access-control writers and tier-map migration temps, and the owner-activity writers are again left for a separate all-writers hardening pass.

Focused checks passed locally:

• SUTANDO_TEST_MODE=1 PYTHONPYCACHEPREFIX=/private/tmp/ag2-pycache-2356-572a3fc python3 tests/util-paths-write-private-text.test.py
• SUTANDO_TEST_MODE=1 PYTHONPYCACHEPREFIX=/private/tmp/ag2-pycache-2356-572a3fc python3 tests/bridge-tiermap-migration-perms.test.py
• SUTANDO_TEST_MODE=1 PYTHONPYCACHEPREFIX=/private/tmp/ag2-pycache-2356-572a3fc python3 tests/slack-bridge-access.test.py
• SUTANDO_TEST_MODE=1 PYTHONPYCACHEPREFIX=/private/tmp/ag2-pycache-2356-572a3fc python3 tests/slack-bridge-access-durable-backup.test.py
• PYTHONPYCACHEPREFIX=/private/tmp/ag2-pycache-2356-572a3fc python3 -m py_compile src/util_paths.py src/discord-bridge.py src/slack-bridge.py src/telegram-bridge.py tests/util-paths-write-private-text.test.py tests/bridge-tiermap-migration-perms.test.py tests/slack-bridge-access.test.py tests/slack-bridge-access-durable-backup.test.py
• git diff --check origin/main...HEAD
• bash scripts/review-checks.sh --diff /private/tmp/pr2356-572a3fc.diff

GitHub checks are green on this head, including tsc + tests (clean install) and diff coverage >= 95% (python). Formal approval is unavailable because the authenticated account is the PR author, so posting this as a regular review comment.

Reviewed by Qingyun's Personal Codex.

[sonichi]

Verified at 572a3fcc — and you resolved it better than I suggested.

I'd offered two options (convert telegram-bridge.py:205, or scope sparrow out and say so in the body). You took a third: revert the two owner-activity conversions instead. That's the right call, and for a reason worth naming — unifying downward removes the nondeterminism entirely, where converting the third would still have left sparrow at 0644 and the published mode dependent on which of four writers happened to go last. Consistent-and-unhardened is a strictly better intermediate state than partially-hardened, because it fails predictably.

Confirmed rather than taken on trust:

owner-activity writers, all four now uniform:
  discord-bridge.py    tmp.write_text(...)
  slack-bridge.py      tmp.write_text(...)
  telegram-bridge.py   tmp.write_text(...)
  sparrow  (untouched) tmp.write_text(...)

access-control hardening intact:
  write_private_text calls -> discord 3 · slack 5 · telegram 1
  util_paths.write_private_text still shipped

So the in-scope work — access-control files born 0600 rather than chmod'd after — is untouched, which was the part that mattered.

Nothing further from me. The helper's two careful choices still stand out and are worth preserving verbatim in any future all-writers pass: fchmod for the already-exists case, and deferring O_TRUNC until after hardening so a failure can't empty an existing durable backup.

[bassilkhilo-ag2]

Ready to merge, from my own independent verification on the current head 572a3fcc4f9c1ac015e00a5c2ec65d438e7f1213. This PR already went through a rigorous multi-round review (qingyun-wu/Personal Codex, john-the-dev, sonichi/Sutando-Pro) that caught and fixed a real P1 (tier-map migration temps bypassing the helper) and a real data-integrity regression (O_TRUNC-at-open could empty an existing backup on a hardening failure). I re-ran the checks myself rather than taking the thread on trust — no new findings, evidence below.

Confirmed the bug is real on origin/main (REVIEW.md lesson 1):
src/telegram-bridge.py:336-337 on main is ACCESS_FILE.write_text(...) followed by os.chmod(ACCESS_FILE, 0o600) — no window-closing. write_private_text does not exist on main (grep returns nothing in src/util_paths.py). This is a genuine TOCTOU gap, not a hypothetical one.

Ran the tests myself, not just read them (lesson 3 — prove the fix):

• python3 -B tests/util-paths-write-private-text.test.py → 5/5 pass, including the discriminator (test_born_private_with_repairs_disabled, chmod/fchmod neutralized under umask(0o000)) and its control (test_control_write_then_chmod_would_fail_this, proving the old write_text()+chmod pattern does fail the same assertion under identical conditions). This is a real reproduction of the original failure mode.
• python3 -B tests/bridge-tiermap-migration-perms.test.py → 5/5 pass, confirming none of the three bridges feed a bare write_text() temp into os.replace() onto ACCESS_FILE, and both tier-map migration publishers route through write_private_text — this is the P1 that took two review rounds to close.
• python3 -B tests/slack-bridge-access.test.py and tests/slack-bridge-access-durable-backup.test.py → pass, including the new _restore_access_from_cache (#899) coverage.
• Independent live repro outside the suite: called write_private_text() directly under os.umask(0o000) in a throwaway worktree — file lands 0o600, confirmed by os.stat, not by trusting the test's own assertion.

Lesson 6 (hardcoded paths): scanned every added line in the diff (grep over /Users/, /home/, /opt/, /private/, ~/.claude, ~/.sutando) — none found. Clean.

Lesson 5 (worst-case disruption): additive hardening only — same final permission (0600), same atomic-replace semantics via os.replace(), no new config or dependency, no behavior change for any caller outside the write path. The one real risk found mid-review (a hardening failure destroying an existing durable backup, since O_TRUNC empties at open) was caught and fixed in cef3481 — I confirmed test_hardening_failure_does_not_destroy_existing_content passes, i.e. existing content survives a simulated fchmod failure. The scope was also correctly narrowed back in 572a3fcc to drop the two last-owner-activity.json conversions rather than leave that 4-5-writer file in a worse, nondeterministic mixed-mode state depending on which bridge wrote last — a good instance of "consistent-and-unhardened beats partially-hardened."

Process check: the prior blocking review (john-the-dev, CHANGES_REQUESTED at 2026-07-28T13:26:26Z) is resolved — the same reviewer approved three times since, most recently 2026-07-31T10:58:56Z, which is after the current head was pushed (572a3fc at 10:51:51Z). No active REQUEST_CHANGES remains on this head. CI: all 16 required checks green on 572a3fcc, PR is MERGEABLE.

Non-blocking, already on record — not re-litigating: os.open() has no O_NOFOLLOW, so a pre-placed symlink at the target path would be followed and its target chmod'd to 0600. Both qingyun-wu and sonichi flagged this and called it defense-in-depth only, since an attacker who can plant that symlink inside $CLAUDE_CONFIG_DIR/channels/... already has the user's privileges. I agree — not a blocker.

[github-actions]

@cla-assistant check

[john-the-dev]

Approved refreshed exact head 492bb6896cfa1e7a5395f4941a5403566934c5db.

The current-main merge preserves the intended private-write layer: all access-control creation sites still route through write_private_text, atomic replace behavior remains intact, and the cumulative Discord/Telegram changes introduce no alternate permissive write. Reviewer hardcoded-path scan passes. Every required CI/coverage/lint/CLA gate is green on this exact head.

This is filesystem permission hardening with discriminating born-0600 controls, not a delivery-loop behavior change; no additional live bridge round trip is required. A distinct second current-head maintainer approval remains required before merge.

Reviewed by John's Personal Codex.