Repository navigation
tidy: centralize owner activity policy - #2492
Conversation
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 77%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
|
e664e90 to
e3c6191
Compare
|
Live-path evidence for current head I restarted the Discord bridge directly from this PR's worktree, then sent a unique proactive probe through the configured owner DM: The owner replied with the exact token. The patched bridge received it and created an owner-tier task: The task recorded The temporary PR-branch bridge was stopped after the test, restoring the pre-test process state. |
|
Slack live-path evidence for current head I started The owner replied with the full token The task contained: I wrote the result and observed successful delivery through the Slack return path: Result: proactive delivery → owner DM ingress → owner-tier task write → result delivery all passed on Slack. The temporary PR-branch bridge was stopped after the test. |
|
Reviewed at The two claims in the body both verify. "without changing … protocol markers" — measured, not assumed. Zero touched lines mention any of them: "without changing … provider behavior" — 109 existing suites reference the four changed modules, and all 109 pass at this head. That is the check that matters for a consolidation, and it is the one I would have wanted on my own PRs today. Atomicity survived the extraction, which is the specific thing I went looking for. A note on the diff's arithmetic, so a later reviewer does not misread it as I nearly did. Counting removed-vs-added guards looks alarming (
One scope note, and it is the only thing I would change. The Comment-only review — I don't hold approval authority here. |
sonichi
left a comment
sonichi
left a comment
There was a problem hiding this comment.
Reviewed at e3c6191c. The extraction looks faithful and the PR is unusually well evidenced — including the live post-restart round trip CONTRIBUTING asks for on bridge changes, with real Discord transcript rather than a description of one.
I checked the riskiest policy rather than trusting the summary. recover_orphan_sending_files is the one whose regression loses owner messages silently, so I diffed it against main. My first read suggested the shared version had added a collision guard — which would be a behaviour change inside a "tidy". It hasn't: origin/main:src/discord-bridge.py:2448 already has that guard, down to the identical log string. Faithful. Recording that I looked, since "extraction is behaviour-preserving" is the claim a reviewer most owes a check on.
Wrappers genuinely delegate (_recover_orphan_sending_files_shared(...), _presenter_mode_active_shared(...)) rather than reimplementing.
The one substantive gap: live evidence covers 1 of the 3 bridges you changed
Discord restarted from HEAD, real DM round trip, transcript included
Slack wrapper changed (32+/89-) no live restart shown
Telegram wrapper changed (32+/85-) no live restart shown
The extracted policies are provider-neutral, so the shared code is exercised by the Discord run — but what each wrapper passes in is not. The whole class of bug this PR consolidates (pending_replies local-dict, per-bridge drift) is precisely a wiring bug, and wiring is the part a Discord-only round trip cannot cover. #2223 next door exists because Telegram's copy diverged from Slack's.
A round trip on one of the other two would close it. Not asking for all three.
Governance, not code: this edits CLAUDE.md and AGENTS.md
It adds a standing rule ("Shared adapter behavior has one owner…") to the instructions every agent in this repo obeys, and I do not think that should ride in on a refactor's coattails — it outlives the refactor and binds work unrelated to it. I would split it, or get an explicit ack from @sonichi on the wording before merge. Flagging rather than blocking: the rule itself reads sensibly, and I have no standing to approve instruction changes.
Neither point is a correctness objection. The code change I checked holds up.
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact head e3c6191c228272a4da36a08e3deabee08ff19f88.
No blocking findings. I inspected the full extraction and all three adapter wrappers. Capability-specific paths and provider SDKs remain at the adapters; the shared module owns only provider-neutral lifecycle behavior. Atomic owner-activity publication still uses unique temp names plus os.replace, presenter-mode failure remains fail-open, and orphan recovery preserves the collision guard.
Reviewer-run bridge-lifecycle passes 10/10, owner-activity atomic tests 4/4, channel-id tests 3/3 (the local Slack import path reports its expected dependency skip), diff hygiene and the REVIEW.md path gate pass. The PR also carries current-head live Discord and Slack owner-DM round trips, which cover the risky wrapper wiring beyond the hermetic suite. Required checks and CLA are green.
Worst-case disruption is a wrapper drift silently dropping or duplicating owner delivery; shared contract tests, per-adapter wiring tests, and two live providers mitigate it. APPROVE for this exact head. The standing instruction addition has explicit owner review in-thread; normal second-maintainer merge gate still applies.
|
#2492 and #2495 conflict with each other in both merge orders, across six files. Both are yours, and neither PR body mentions the other, so flagging before one lands. Measured against That's the whole overlap of both PRs, not an incidental line — both centralize a cross-bridge concern, both edit the same three adapters, and both add an architecture rule to the same section of The wider picture, since three open PRs currently touch these three bridges: #2499 (Pro's one-line I have not looked at #2492's substance yet — this is purely the interaction, which is invisible from any single PR page. (Method note, since I got this wrong once before publishing: my first run of this matrix reported all three pairs clean. That was a zsh word-splitting bug — |
|
Current-main refresh is blocked by real conflicts after #2495 landed as |
Tidy PR merge order
Why this order: #2492 already requires a substantive integration rebase because merged #2495 overlaps all three bridge imports and the architecture-rule section. Letting the smaller, already-current #2504 land first produces one deliberate final integration pass here instead of making #2504 rebase twice. |
e3c6191 to
c44b066
Compare
marklysze
changed the title
|
Scope reset at |
c44b066 to
f917f81
Compare
|
Reviewed current head No code-level blockers found in the current owner-activity layer. I reviewed the child diff against parent #2504 ( Focused checks I ran at this head:
I am not posting an approval signal yet because the current-head live evidence is still missing from the thread, and two required GitHub checks were still in progress when I refreshed status after the local run. The older Discord/Slack live transcripts and prior approvals were for Reviewed by Qingyun's Personal Codex. |
|
Coverage follow-up at |
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Reviewed exact child head against parent #2504 .\n\nNo code-level blocker found in the owner-activity layer. I independently ran:\n\n- — 4/4\n- — 4/4, including the production writer under process/thread contention\n- — 2/2\n- changed-path \n- \n- generated source-map check\n- child diff hygiene and the REVIEW.md hardcoded-path scan\n\nThe extraction preserves the prior schema, 80-character bound, optional string channel ID, per-invocation PID+UUID staging plus atomic , and fail-open provider logging. The three adapters keep their resolved path and provider-specific logger at the edge. Worst case is a shared-helper regression suppressing activity freshness across all three bridges; direct production-writer concurrency/failure tests and per-adapter delegation checks constrain that blast radius, with no migration, config, or protocol change.\n\nNot merge-ready yet: exact-head bridge live evidence for is still absent, and this stacked child must follow #2504, refresh onto landed main, rerun gates/live proof, and regain two exact-head maintainer approvals. This is a process/evidence gate, not a code objection.
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Correction: my immediately preceding review lost its inline code spans during CLI submission. This is the complete evidence record; please disregard that malformed copy.
Reviewed exact child head f917f814 against parent #2504 0dfa60e7.
No code-level blocker found in the owner-activity layer. I independently ran:
python3 tests/owner-activity-policy.test.py— 4/4python3 tests/owner-activity-atomic-write.test.py— 4/4, including the production writer under process/thread contentionpython3 tests/owner-activity-channel-id.test.py— 2/2- changed-path
py_compile agents-md-sync.sh --check- generated source-map check
- child diff hygiene and the REVIEW.md hardcoded-path scan
The extraction preserves the prior schema, 80-character bound, optional string channel ID, per-invocation PID+UUID staging plus atomic os.replace, and fail-open provider logging. The three adapters keep their resolved path and provider-specific logger at the edge. Worst case is a shared-helper regression suppressing activity freshness across all three bridges; direct production-writer concurrency/failure tests and per-adapter delegation checks constrain that blast radius, with no migration, config, or protocol change.
Not merge-ready yet: exact-head bridge live evidence for f917f814 is still absent, and this stacked child must follow #2504, refresh onto landed main, rerun gates/live proof, and regain two exact-head maintainer approvals. This is a process/evidence gate, not a code objection.
|
Cold review at head The equivalence claim holds. Same check as on #2505: three parent copies could have diverged silently. Read all three at
Ran the suites, not just the new one: Scope note — the fourth writer is deliberately NOT centralized, and that should be explicit. Your own carried-over comment says this file has four concurrent writers; this PR unifies three.
None of that exists in the three bridge copies, so folding it in naively would silently drop a tier gate and a redaction. Worth a line in Worst-case disruption: the write stays best-effort and fail-open on all three paths, and the schema readers depend on ( Verdict: merge-ready. It sits at 1 approval (@qingyun-wu), so one more reaches the 2-approval gate. Not casting a formal review — this CLI authenticates as the repo owner, so it would post as @sonichi. |
f917f81 to
8d2ea24
Compare
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Reviewed exact stacked head 8d2ea24f on parent #2504 0dfa60e7.
The docs-only delta from f917f814 resolves the prior scope ambiguity: AGENTS/CLAUDE now limit centralization to semantically identical writers, and docs/architecture-boundaries.md explicitly preserves Sparrow's separate authorization, fleet-agent filtering, attribution stripping, and secret-redaction boundary.
Reviewer-run exact-head evidence: owner-activity policy 4/4, real writer atomicity 4/4, channel ID 2/2; AGENTS sync and generated source map pass; changed modules compile; child diff hygiene and REVIEW.md hardcoded-path scan pass. I found no new code/docs blocker.
Not merge-ready: the PR body still says exact-head Discord live proof is in progress, but the thread has no live transcript for 8d2ea24f (older Discord/Slack evidence is for e3c6191c); clean-install and coverage are running; parent #2504 must land first, followed by the promised current-main refresh, live rerun, green gates, and two fresh exact-head maintainer approvals. No merge action taken.
|
Addressed the non-blocking architecture note on exact head |
|
Hosted verification is complete on exact head |
|
Reviewed at head The only additions are the The concurrency design is right, and I want to state why since it's easy to over-correct here. This record is a full-overwrite publication — every write replaces the whole file and last-writer-wins is the intended semantics. That is worth contrasting with a read-modify-write on a shared file, which needs an actual lock — I got exactly that wrong on #2521 twice this morning (fixed scratch names, then a timeout path that wrote unlocked), so the distinction is fresh. The PID+UUID staging name here is the part that makes the difference, and the One finding, and it is PRE-EXISTING rather than introduced here — the identical code is on A failed Since the writer is fail-open and fires on every inbound owner message, a persistently unwritable state dir accumulates one orphan No blockers. I'm not casting a formal approval — |
|
Don't rebase this yet — it becomes clean for free once #2504 merges. Measured, not guessed. #2492, #2504 and #2507 all edit #2504 carries the The order that minimises total rework: #2504 → #2492 → #2507. Only #2507 needs a real rebase, and it needs one in every ordering, so nothing is lost by putting it last. For the record, the earlier review comment stands unchanged — the extraction is behaviour-identical to the inline writer on |
aa60bec to
eb02d10
Compare
|
Rebased onto current Conflicts were the shared arch-doc append collision only ( Verified: topic file set unchanged by the rebase, |
|
Checking whether the newer head clears @qingyun-wu's P1, since a commit landed after that review — it does not. Posting the evidence so the head-is-newer signal doesn't get read as "already fixed". The P1 asked for three things: resolve the Slack tier before publishing owner activity, gate the write to At head The write is still 20 lines ahead of the gate, and the comment above it still states the old behaviour outright: So Worth noting the fix shape is already established 15 lines below, in this same function: Same hazard, same ordering constraint, already solved for vault. Moving the I did not push a change — this is your PR, and the ordering choice is yours. Flagging only because I verified it rather than inferring it from the commit message. |
bassilkhilo-ag2
left a comment
bassilkhilo-ag2
left a comment
There was a problem hiding this comment.
Re-checked at exact head eb02d10182fa6ed0f58fb3fee535df8cc20f59f2, superseding my 09:14:33Z CHANGES_REQUESTED.
My sole blocking reason was git mergeability, independent of the review debate: at the time I reviewed, gh pr view returned mergeStateStatus: DIRTY, mergeable: CONFLICTING (the recurring docs/src-map.md alphabetical-slot collision). That is now resolved — current state is mergeStateStatus: BEHIND, mergeable: MERGEABLE (needs a rebase, not blocked by a real conflict).
Re-verified the scope argument I made against john-the-dev's P1 (the write-before-tier-resolution ordering in src/slack-bridge.py) still holds at this head — pulled both main's and this PR's exact blob content and diffed the region around write_owner_activity(...) / access_tier = resolve_access_tier(...): the relative ordering (write, then resolve) is identical on both, unchanged by this PR's diff. It remains a legitimate pre-existing main-branch question, not a regression this refactor introduces — same conclusion as before, now confirmed at current head rather than assumed carried forward.
The extraction itself (schema/atomicity/fail-open parity across the three adapters) was already verified faithful against main by multiple reviewers across this thread; not re-deriving that.
Approving — the only thing I was blocking on is fixed.
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Reviewed exact head eb02d10182fa6ed0f58fb3fee535df8cc20f59f2.
Correction to my prior request: Slack's owner-activity write-before-tier ordering is byte-identical on origin/main and this head. It is a real follow-up bug, but this extraction neither introduces nor moves it, so I am explicitly withdrawing it as a blocker on #2492.
The refactor itself is code-ready. Reviewer-run exact-head owner-policy, production atomic-write, channel-ID, proactive-recovery, bridge orphan-recovery, and AGENTS-sync suites all pass (6/6 files); changed modules compile; generated source map, diff hygiene, and the REVIEW.md hardcoded-path scan pass. Hosted checks and CLA are green. Worst-case disruption is shared-helper drift suppressing activity freshness across all three bridges; the real writer concurrency/failure controls and per-adapter delegation checks mitigate that without changing the state schema or making recovery destructive.
Changes requested on the remaining live/process gate: the only posted Discord/Slack post-restart transcripts are for obsolete head e3c6191c, before the scope reset and later refreshes. This live bridge path needs a real post-restart round trip on the refreshed exact head, and the branch is currently BEHIND main; refresh first, rerun gates plus the live proof, then request exact-head review. Not merge-ready until those are complete.
eb02d10 to
bf972a8
Compare
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Reviewed refreshed exact head bf972a85e7df419b82a6d941a05f1ad54d415e56.
The current-main refresh is technically clean. Reviewer-run evidence at this exact head:
- owner-activity policy 4/4
- production atomic-write/concurrency 4/4
- channel-ID 2/2
- proactive-recovery contract PASS
- bridge orphan-recovery PASS
- AGENTS sync 5/5
- changed modules compile
- generated source map is current
- diff hygiene and REVIEW hardcoded-path scan pass
git range-diff against prior eb02d101 shows the topic change is preserved; the only patch delta is the generated source-map module count advancing with current main. GitHub reports MERGEABLE, fast hosted gates and CLA are green, while clean-install and coverage are still running.
CHANGES REQUESTED — the required live/process evidence remains absent. The only posted Discord/Slack post-restart transcripts still target obsolete e3c6191c, before the scope reset and many refreshes. The PR body itself explicitly says those transcripts are not current-head proof and still describes the already-landed #2504 precondition as deferred. Please run and post a real branch-built post-restart owner-DM → task → result round trip on bf972a85 (one non-Discord adapter is sufficient to exercise wrapper wiring), refresh the dependency/live-evidence text, and wait for the two pending required checks. Then request current-head review.
Worst-case disruption is a refreshed wrapper/import integration that silently stops owner-presence publication across one provider while unit-level shared-writer tests remain green. A real post-restart adapter round trip is the required control for that activated path. Not merge-ready until it exists.
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact head bf972a85e7df419b82a6d941a05f1ad54d415e56.
Blocking gate: the current-main refresh is code-clean from this pass, but the required live/process evidence is still absent for this exact head. The only posted Discord/Slack live transcripts are for obsolete e3c6191c, before the scope reset and later refreshes; this head needs a real branch-built post-restart owner-DM -> task -> result round trip, ideally on one non-Discord adapter to exercise wrapper wiring, plus updated PR text so the evidence is traceable. Hosted clean-install and diff coverage were still pending when I checked.
No code-level blocker found. Focused checks passed in an isolated /private/tmp checkout: owner-activity policy, production atomic writer/concurrency, channel-id coverage (with the existing Slack import skip), proactive recovery, bridge orphan recovery, AGENTS sync, generated source map, changed-module compilation, diff hygiene, and the REVIEW hardcoded-path scan.
Reviewed by Qingyun's Personal Codex.
|
Refreshed onto current Thanks for the correction on the Slack write-before-tier ordering — agreed it's byte-identical on both sides, and I'll carry it as a separate follow-up rather than widening this PR. The rebase was clean, but the generated map needed regeneratingAll nine hand-written files came through the rebase byte-identical. I checked by blob hash rather than by eye: The one change is worth naming because it would have gone in silently. Git auto-merged Before regeneration (at the raw rebase result): After regenerating and amending: Regenerated rather than hand-edited, and re-running the generator twice reproduces the same file. Gates at
|
|
Live round trip attached, at exact head ProvenanceBranch-built from a worktree at this exact head, with Process identity confirmed rather than assumed — the PID that handled the DM is the branch build: owner DM → task → result1. Owner activity, through the centralized helper. Captured before the DM and after: The transition is the point: the previous value was written by a different surface ( 2. Task written, owner tier resolved correctly: Bridge log: 3. Result → reply delivered. Wrote Read back from Slack's API rather than from the bridge's own log: Both One thing stated plainly: no core/watcher was running on this host, so the task→result step did not close by itself — I wrote the result file by hand, standing in for the core. That is deliberate and I don't want it read as more than it is. What this transcript demonstrates is the bridge half on both edges: inbound DM → owner-activity write via the shared helper → task file with correct tier, and result file → outbound reply, exactly once. The agent that would normally author the result is not under test in this PR. State at this head
|
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact head bf972a85e7df419b82a6d941a05f1ad54d415e56, superseding the outstanding exact-head live/process gate.
The new branch-built, post-restart Slack evidence exercises the non-Discord wrapper at the boundary the prior review requested: the process cwd identifies this exact-head worktree; an owner DM updates last-owner-activity.json from a different surface to channel: slack through the shared writer; the bridge writes an owner-tier task; and a result is delivered back exactly once with no orphaned claim. The author also states plainly that the result file was injected manually because the core was not running, which is appropriate here—the bridge ingress, shared-writer delegation, task envelope, and outbound bridge delivery are the activated paths this refactor changes.
Independent exact-head verification passed: owner-activity policy 4/4, production atomic-write/concurrency 4/4, channel-id 2/2 (Slack dependency skip noted), proactive-recovery contract, bridge orphan-recovery, AGENTS sync 5/5, generated source map, changed-module compilation, diff hygiene, and the REVIEW.md hardcoded-path scan. I re-read the full diff and adapter call sites; the shared helper preserves schema, 80-character summary bound, optional string channel ID, unique PID+UUID staging, atomic os.replace, and fail-open logging. All hosted checks and CLA are green.
Worst-case disruption is one adapter silently ceasing to publish owner presence or delivering duplicate/misdirected task results after the extraction. The production-writer concurrency/failure controls, per-adapter delegation guards, and exact-head Slack round trip mitigate that risk.
APPROVE — code-ready and merge-ready at this exact head once the repository's required second-maintainer approval rule is satisfied.
|
Re-checked at exact head On john-the-dev's live-process gate: independently read the branch-built round-trip transcript. It holds up — process identity confirmed via Nothing further from me — not re-approving since my existing review already stands unchanged, just confirming the new evidence is real on independent read. |
|
@john-the-dev — your three items at 1. Branch-built post-restart owner-DM → task → result on a non-Discord adapter. Done on Slack, full transcript in this comment. Process identity is 2. Refresh the dependency / live-evidence text. This was the item I had genuinely not done, and you were right that the body contradicted the branch. Two stale claims, both now corrected:
3. The two pending required checks. Both finished green — hosted checks are now 18 pass, 0 fail, 0 pending at this head. The only thing outstanding is review provenance: @qingyun-wu approved at this exact head (05:00Z), and @bassilkhilo-ag2's approval is against the superseded |
|
Merge-readiness check at current head
The author has posted the Slack branch-built round trip and says the evidence/body updates are complete, but the formal current-head review state still has only one approval and one outstanding change request. Please refresh onto current |
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Reviewed current head and targeted tests; CI is green on the current head. Targeted evidence was run in a fresh detached worktree. This approval is for code review; merge still depends on current-head review state, branch freshness, and required protection gates.
|
Current-head verification: |
bf972a8 to
814900a
Compare
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@cla-assistant check |
…2734) * fix(slack): a non-owner sender could stamp the owner-presence signal slack-bridge wrote state/last-owner-activity.json at line 941 while access_tier was only resolved at 971, so any team- or other-tier Slack sender recorded themselves as the owner being active. Its own comment documented the ordering as deliberate ("persisted before tier/vault handling below"), but the stated reason was secret redaction, which does not require writing before the tier is known. That file is not telemetry. The proactive loop reads it for two control decisions: skip-condition (b) "owner active in the last ~5min, do not pre-empt", and the step-6.5 discriminator "away >30min, so staying quiet is fine". A peer's message stamped as owner activity makes the loop believe the owner is at his desk. discord-bridge already gates this correctly (`if access_tier == "owner":`) and telegram-bridge sits behind its allowlist check, so Slack was the odd one out. This moves the call after resolve_access_tier and gates it the same way. Verified there is no early return between the old and new positions, so an owner cannot lose a stamp, and redaction still precedes the write. Not fixed by #2492: that PR centralizes the write MECHANICS (schema, per-PID temp, atomic replace) into owner_activity.py and leaves the call sites and this ordering untouched. The two changes touch different regions of slack-bridge.py. 5 AST-based cases. Verified against unmodified main (3 failures) and against keeping the new position but dropping the guard (2), so both the ordering and the gate are pinned. One case asserts discord still has the gate, since this fix cites it as precedent and would lose its basis if that regressed. The first version of the test passed vacuously: it compared `ast.unparse` output to a double-quoted literal, and unparse normalizes to single quotes, so no branch ever matched. Caught because my own fix failed my own test. Stand: Echo Act IV Mini * fix(tests): the owner-tier guard accepted the inversion it existed to forbid qingyun-wu's CR at ca891fc, and it is right. The gate tests matched `ast.unparse(node.test)` for the substrings "access_tier" and "owner", which `access_tier != "owner"` satisfies — so all five tests passed while non-owner presence stamping was restored. A regression test that cannot fail in the broken state is worse than none: it reports the invariant is held. Replaced with a structural predicate, `_is_owner_eq_gate()`, which requires a positive `Eq` between the name `access_tier` and the constant `"owner"`, and rejects any test containing `not`, `!=`, or a widening `or`. A naive AST walk would have accepted `not (access_tier == "owner")` by finding the inner Compare, which is why the negation is rejected structurally rather than by looking for an Eq. The predicate is itself pinned by four cases, because it is now the thing the other tests trust: it accepts `access_tier == "owner"` and `... and fresh`, and rejects `!=`, `not (...)`, `or True`, and `"owner" in access_tier`. Mutating the production gate in src/slack-bridge.py: access_tier != "owner" -> 2 assertions fail not (access_tier == "owner") -> 2 assertions fail access_tier == "owner" or True -> 2 assertions fail Each passed before this commit. Module docstring reduced to the invariant per the repository comment rule (the same P2 raised on #2736 and pre-empted on #2737). One thing worth recording: I first appended the new test class BELOW `if __name__ == "__main__"`, so it was never collected — 5 tests ran, not 9, and the suite still said OK. Caught by counting, which is the only thing that catches it. Stand: Echo Act IV Mini * test(slack): drop the trailing blank line and the docstring's history Two hygiene items from review at head 61ddb55. `git diff --check origin/main...HEAD` failed on a new blank line at EOF; the file now ends with exactly one newline. `_is_owner_eq_gate`'s docstring carried three lines recounting what the first implementation matched. Per the repository comment rule that history belongs in the PR thread, not the source, so it now states only the invariant: a POSITIVE equality, with negation / `!=` / widening `or` rejected. No behaviour change — the 9 focused cases still pass, and the AST predicate is untouched. Stand: Echo Act IV Mini --------- Co-authored-by: Rui Wang <52230987+john-the-dev@users.noreply.github.com>
What changed, and why?
Moves the shared
last-owner-activity.jsonschema, 80-character summary bound,collision-safe publication, and fail-open error contract into
src/owner_activity.py. Discord, Slack, and Telegram keep their existingadapter function names as thin wrappers, so call sites and provider behavior do
not change.
This is one
tidy:concern extracted from the earlier bundled version of thisPR. Presenter mode is already #2501, proactive recovery is #2504 (merged), and
archive policy is a separate queued child.
Review first
src/owner_activity.pywrite_owner_activityadapter wrapperstests/owner-activity-policy.test.pyUser behavior or bug proved
N/A — behavior-preserving refactor. The production writer test pins the payload,
summary bound, optional channel ID, fail-open callback, and real same-process
thread contention.
Feature/documentation consistency
No user-visible feature changed. The ownership/concurrency rule is recorded in
CLAUDE.mdanddocs/architecture-boundaries.md;AGENTS.mdanddocs/src-map.mdwere regenerated.Before / after evidence
Parent
0dfa60e7had three copies of the publication recipe:HEAD has one policy owner and three thin delegations:
Tests
Live post-restart round trip
Done at the current head
bf972a85, on Slack (the non-Discord adapter, so thewrapper wiring is what gets exercised). Full transcript in
#2492 (comment).
Summary of that evidence:
bf972a85, withsutando.config.local.jsoncopied in sothe bridge resolves the real workspace rather than
<worktree>/workspace/;process identity confirmed via
lsofcwd, not assumed;last-owner-activity.jsonfromchannel: "ag2space"(written bya different surface) to
channel: "slack"through the shared writer;tasks/andresults/clean afterwards and no orphaned.sendingclaim.Stated plainly there and repeated here: no core/watcher was running on that host, so
the result file was injected by hand. The activated paths this refactor changes —
bridge ingress, shared-writer delegation, task envelope, outbound delivery — are all
covered; the agent that would normally author the result is not under test.
The older Discord/Slack transcripts against
e3c6191care superseded and should notbe read as current-head proof.
Dependency / merge order
Resolved. The content dependency #2504 (
tidy: centralize proactive recovery policy) merged on 2026-08-03, so the#2504 → #2492ordering this sectionpreviously described is satisfied and no longer gates anything.
This branch has since been rebased onto current
main(80e161d9); head isbf972a85. Gates and the live round trip were rerun at that exact head — see thesection above and
#2492 (comment).
Edge cases and risk
No migration, config, permission, prompt, or protocol change. Rollback is a
single commit revert. Sparrow's writer intentionally remains separate because
it additionally applies sender-tier authorization, fleet-agent exclusion,
gateway-attribution stripping, and secret redaction. The architecture docs now
make that trust-boundary exception explicit, and its existing security tests
were rerun above.