Repository navigation
feat(discord): restrict collaborators by purpose - #2269
john-the-dev wants to merge 6 commits into
Conversation
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 89%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
|
|
Useful addition — "collaborator" currently means "substantive engagement on anything in this channel", and narrowing that is worth having. The input hygiene is the part I'd keep as-is: owner-only source, One thing I'd like the framing to match: this is a guidance-layer control, not an enforcement one.
That is a real improvement over no restriction, and I am not arguing against the approach. But the language around it currently reads stronger than the mechanism:
All of that is true of the instruction; none of it is true of the system. Compare the existing non-owner tiers, where Concrete suggestion, cheap and additive: log the classification. Rather than trying to enforce intent in code (which is genuinely hard — classifying a request is the agent's job), make compliance auditable after the fact. When a purpose restriction is active and the agent proceeds with substantive work, emit one line recording which purpose it matched. Today an owner has no way to answer "was this honoured?" without reading every transcript; with a log line it is a grep. That also keeps the control honest as the instruction competes with a long context — a restriction that is silently dropped and a restriction that is followed currently look identical from outside. Smaller notes:
|
|
Updated the implementation and framing in commit 8c6f2ce and the PR body to state the exact boundary: this is an owner-authored in-band guidance control, not a structural sandbox. The helper now documents its three states: None unrestricted, empty tuple deny-all guidance, populated tuple allowlist. Tests explicitly prove a purpose restriction configured for someone else injects no instruction. I am deferring matched-purpose audit logging because there is not yet a reliable task/result protocol field for the agent classification; adding a pretend bridge-side match would overstate enforcement. That can be a focused follow-up. |
|
Checked 8c6f2ce — this resolves my review. The docstrings now state the boundary exactly ("agent-enforced instruction, not a structural sandbox boundary; team/other tiers remain the hard enforcement layer"), the three-state return is documented where the subtle line lives, and the new assertion proves another sender's restriction injects nothing — which was the silent-over-restriction case I wanted pinned. On deferring the audit log: agreed, and for the right reason. A bridge-side "matched purpose" stamp would be the bridge guessing at a classification only the agent performs — logging it would manufacture exactly the overstated-enforcement problem the reframing just removed. When a task/result protocol field for agent-reported classification exists, the log becomes real; until then, leaving it out is the honest choice. Nothing further from me. |
qingyun-wu
left a comment
There was a problem hiding this comment.
Focused collaborator-tier tests pass. The updated code clearly documents that purpose limits are agent-enforced guidance, preserves the three-state configuration semantics, and prevents another sender’s restriction from leaking across identities/channels.
qingyun-wu
left a comment
There was a problem hiding this comment.
Re-reviewed after the branch merged current main. Range-diff shows both PR commits are unchanged, and the full current collaborator-purpose diff remains consistent with the prior approval. No conflict-resolution regression or new blocker found.
# Conflicts: # tests/discord-bridge-access-tier.test.py
|
Merged current Post-merge verification: Added-line scan found no host-specific |
qingyun-wu
left a comment
There was a problem hiding this comment.
Re-reviewed current head 02ec6269d1cb618b901d06074df242eb54b6e346 after the merge from current main.
No blockers found. The structural-test conflict resolution kept main’s wider access-tier block scan and extended it enough for this PR’s collaborator-purpose additions. The implementation still accurately frames purpose limits as owner-authored, agent-enforced guidance rather than a structural sandbox boundary; malformed configured restrictions fail closed, and sender/channel scoping remains covered.
Focused verification in an isolated /private/tmp worktree:
git diff --check origin/main...HEADpython3 -m py_compile src/discord-bridge.py tests/discord-bridge-collaborator-tier.test.py tests/discord-bridge-access-tier.test.pypython3 tests/discord-bridge-collaborator-tier.test.pypython3 tests/discord-bridge-access-tier.test.pypython3 tests/injection-guard-sweep.test.py— 48/48python3 tests/discord-bridge-mod-judge-integration.test.py
GitHub CI/CLA are green on this head.
Reviewed by Qingyun's Personal Codex.
bassilkhilo-ag2
left a comment
There was a problem hiding this comment.
Reviewed as a maintainer against REVIEW.md. I did not re-run the suites - qingyun-wu already recorded that evidence on head 02ec626. I verified the code paths by reading the deployed bridge copy (not a git checkout, so no git diff from me).
Lesson 1 (gap is real on main): confirmed. resolve_is_collaborator is immediately followed by select_rulebook_key, and tier_instructions["team-collaborator"] grants blanket in-channel engagement with no purpose scoping.
Lesson 6: no host-specific path literals in added lines.
Lesson 5 (worst case): an owner configures a restriction and believes it is enforced while the agent never honors it, or the entry silently does not apply. Residual exposure is limited to substantive in-channel discussion outside the stated purpose - the hard owner-only boundaries (git, sends, credentials, restarts) are untouched, and installs without collaboratorPurposes see byte-identical task files.
Non-blocking, in priority order:
-
_build_task_content()appendspurpose_instructionAFTER the rulebook's===END SUTANDO SYSTEM INSTRUCTIONS===, under a new===SUTANDO PURPOSE RESTRICTION===marker, anddiscord_skill_hints/secret_noticefollow it. CLAUDE.md's Discord section only obliges the agent to follow the SYSTEM INSTRUCTIONS block verbatim;grepfinds no mention of the new marker in CLAUDE.md or docs. Since the block is this feature's only mechanism, emitting it inside the team-collaborator rulebook (before its END line) would make it authoritative for free. -
resolve_collaborator_purposesreturnsNone(unrestricted) when the channel has acollaboratorPurposesmap but the sender key does not match. Correct for the multi-collaborator case, but a typo'd or username-instead-of-ID key yields zero restriction and zero signal. A one-line warning when an engaged collaborator has no entry in a configured map would surface it. -
collaborator_purposes = None # pragma: no coveris an unconditional assignment, not a branch; excluding it slightly overstates the 100% diff-coverage figure. -
SUTANDO_DISCORD_BRIDGEin the test is undocumented. If it leaks into an environment, the suite silently validates a different file and still reports PASS. Worth a docstring line. -
Owner-authored purpose text is not stripped of
===, so a 160-char entry could close the block early. Trusted source, cheap to harden.
Merge-readiness: mergeable as-is - I found nothing that regresses existing users, and the fail-closed handling of malformed values is correct. I would prefer item 1 land first, since it is a small change to the single mechanism this PR depends on.
liususan091219
left a comment
There was a problem hiding this comment.
APPROVE at 02ec6269d — tier suites run green; three-state backward-compatible design, malformed-config denies all, purposes normalized+capped and constrain-never-elevate, owner-before-team ordering.
|
Merge hold despite two approvals: this changes the live Discord authorization/task-ingestion path, and the current “Live runtime smoke” explicitly did not restart the bridge or send a real Discord collaborator request. Under the repository live-path gate, please add a controlled post-restart round trip proving (1) an in-scope restricted collaborator request reaches the sandboxed task with the purpose block, (2) an outside-scope request is declined/routed for owner authorization, and (3) an unrestricted collaborator remains unchanged. I am not running that on the production bridge during this audit because it requires a temporary live access-map mutation and bridge restart, which could alter active collaborator authorization. |
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes requested on current head 02ec6269d.
The code-side purpose restriction remains sound, but the repository's live-path gate is still unmet. This changes the Discord authorization/task-ingestion path, while the PR's current smoke test only calls the resolver, instruction builder, and task writer in-process; it explicitly does not restart the bridge or exercise a real Discord message. The author has now confirmed that gap and placed the PR on merge hold.
Please add exact-head post-restart evidence covering the three runtime cases the author identified: (1) an in-scope restricted collaborator request reaches the sandboxed task with the purpose metadata/instruction, (2) an outside-scope or ambiguous request is declined and routed for owner authorization, and (3) an unrestricted collaborator remains unchanged. This must traverse the real Discord bridge/message-to-task path, not only the helper harness.
I re-inspected the full current diff and surrounding tier/task assembly. Focused checks pass under the host Python: collaborator-tier behavior, access-tier classification, injection guard 48/48, mod-judge integration, Python compilation, git diff --check, and the repository hardcoded-path scan. GitHub CI is green. I found no additional code-level blocker.
Worst-case disruption without the live proof is that the helper policy is correct but the async bridge glue fails to load or serialize it after restart, silently giving a collaborator broader engagement than the owner configured. Not ready to merge until the exact-head Discord round trip is attached.
Reviewed by Qingyun's Personal Codex.
bassilkhilo-ag2
left a comment
There was a problem hiding this comment.
Read the full diff end-to-end (resolve_collaborator_purposes, collaborator_purpose_instruction, and the _handle_discord_message/_build_task_content wiring) — the code itself is sound: sender_id typing matches resolve_is_collaborator's existing string convention, the three-state return (None/()/tuple) is correctly threaded through purpose_line/purpose_instruction, fail-closed paths (malformed list, non-dict configs, exception in access_data.get) all verified by tracing the logic, and CI is green.
Agreeing with @qingyun-wu's existing CHANGES_REQUESTED: this is a live authorization/task-ingestion path change, and the PR's own "Live runtime smoke" section states the bridge was not restarted and no real Discord message was sent through it — that's harness-only proof of the resolver/instruction-builder, not the post-restart round trip this repo's live-path gate requires. Not ready to merge until that's supplied.
|
Status from my own stale-PR triage — 36 days without a commit, so it deserves a position rather than silence. The gap is still real. Two things block it, and only one is mine to fix. Conflicts. GitHub reports DIRTY. I could not compute the conflict set locally — my clone shares no common ancestor with the PR ref ( The blocker @qingyun-wu and @bassilkhilo-ag2 both name. This is a live authorization and task-ingestion path, and the PR's own "Live runtime smoke" section says the bridge was not restarted. Criterion 15 asks for a real post-restart round trip, and that is the right bar here — a change to who may inject tasks is exactly the class where harness proof is insufficient. Why I have not produced it: the round trip requires running patched Leaving it open. The code read is clean per @bassilkhilo-ag2's own trace, and the feature tightens an authorization boundary rather than widening one — closing it would leave the coarser behavior in place indefinitely. |
# Conflicts: # src/discord-bridge.py # tests/discord-bridge-collaborator-tier.test.py
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes requested on exact head 212cfe1.
[P1 blocker] Rebase and resolve the live Discord task-assembly conflict before this can merge. GitHub reports CONFLICTING/DIRTY, and git merge-tree --write-tree origin/main HEAD reproduces the content conflict at src/discord-bridge.py:4378-4379. Current main now falls back to tier_instructions["guest"], while this head still contributes tier_instructions["other"] beside purpose_instruction. The exact-head access-tier suite also still reports the legacy default other. A wrong resolution can either restore the retired tier name or drop the new purpose instruction. Rebase onto current main, retain the second-line trusted access_tier placement and guest fallback, preserve purpose_instruction, and rerun the bridge/tier guards.
The existing live-path blocker also remains. This changes Discord authorization and task ingestion, but the PR evidence explicitly did not restart the bridge or send a real Discord collaborator message. After the rebase, attach a real post-restart round trip for: (1) an in-scope restricted collaborator task carrying the purpose metadata/instruction, (2) an outside-scope or ambiguous request being declined and routed for owner authorization, and (3) an unrestricted collaborator remaining unchanged.
Exact-head evidence: both topic commits are patch-identical to the previously reviewed layer by range-diff. Collaborator-tier and access-tier suites pass; injection guard passes 48/48; mod-judge integration, Python compilation, git diff --check, and repository review checks pass. Hosted checks and CLA are green. The structural conflict and absent live witness, not a focused-suite failure, are the blockers.
Worst case is conflict resolution silently restoring stale authorization semantics or omitting the only purpose-guidance control, while helper tests remain green. Verdict: not merge-ready until the branch is rebased cleanly and the post-restart Discord round trip is supplied.
Reviewed by Qingyun Personal Codex automation.
One conflict, in the tier-instruction fallback. main renamed the default rulebook key `other` -> `guest`; this branch had appended `purpose_instruction` after the same expression. Resolved to main's key plus this branch's line — `tier_instructions['other']` no longer exists at the merged state, so keeping the branch's spelling would have been a KeyError on every unrecognized tier. tests/discord-bridge-access-tier.test.py and tests/discord-bridge-collaborator-tier.test.py both auto-merged and pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@cla-assistant check |
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes requested on exact head 644a7c4.
The rebase/conflict blocker is resolved correctly: the task keeps access_tier as the trusted second line, uses the current guest fallback, and retains collaborator purpose metadata and guidance. Focused collaborator-tier and access-tier tests pass, injection guard passes 48/48, mod-judge integration passes, Python compilation passes with an isolated cache, and the diff check is clean.
The existing live-path blocker remains. This changes the live Discord authorization and task-ingestion path, but there is still no exact-head post-restart Discord round trip proving: an in-scope restricted collaborator reaches the sandboxed task with purpose metadata/instructions; an outside-scope or ambiguous request is declined and routed for owner authorization; and an unrestricted collaborator remains unchanged. Please attach that real bridge witness before merge. I found no additional code-level blocker in the new head.
Reviewed by Qingyun's Personal Codex.
|
Status on the live-path gate — I am not contesting it, and I cannot clear it from here. Naming why, so this stops looking like silence. The witness you are asking for has to be produced on a host running the core off this branch. I checked what that would take on the only machine that could do it: One live core in this workspace, and its engine tree is a packaged install rather than a checkout — so a branch build cannot be swapped in where the core actually runs until Sutando Developer Mode is enabled, which re-attaches that tree to git. That is an owner action, and the supervised restart you asked for is separately an owner action. Both are outside what I can authorise. So the gate stands and the ball is with the owner, not with you and not with the code. I have raised it as a single owner question covering the PRs in this state rather than four separate ones. Specific to this PR. Your ask is a Discord round trip after restart covering three cases: an in-scope restricted collaborator reaching the sandboxed task with its purpose metadata, an out-of-scope or ambiguous request declined and routed for owner authorization, and an unrestricted collaborator unchanged. That is the right shape — the third is the one a narrower test would skip, and it is where a regression would hide. Your note that the rebase/conflict blocker is resolved correctly matches what I see at head; the remaining gate is the live one only. |
|
Closing as stale: no activity for over 30 days and it no longer merges cleanly or has open change requests. Reopen, or open a fresh PR from current main, if this is still wanted. |
What changed and why
Add optional, per-sender purpose restrictions to Discord channel collaborators. Today
collaboratorsgrants substantive engagement for every harmless request in that channel; describing a grant as “for code review” does not enforce that purpose.Owners can now add an optional
collaboratorPurposesmap beside a channel’scollaboratorslist:{ "groups": { "1485653767402553457": { "collaborators": ["786410785197785088"], "collaboratorPurposes": { "786410785197785088": [ "review GitHub pull requests and discuss code-review findings" ] } } } }Absent configuration preserves existing collaborator behavior. When configured, the bridge serializes
access_purposesinto the task and appends a trusted instruction requiring the agent to classify the request before tools/context access. Outside-scope or ambiguous requests are declined and routed to the owner for authorization. User content cannot widen the configured purposes.Malformed, empty, oversized, or non-string configured entries fail closed to deny all substantive collaborator requests.
Review guide
src/discord-bridge.py: purpose resolution, normalization, fail-closed instruction, and task wiringtests/discord-bridge-collaborator-tier.test.py: per-channel/per-sender behavior, backward compatibility, malformed config, and anti-widening guardstests/discord-bridge-access-tier.test.py: widens the structural block bound to accommodate the additive collaborator glueFailing-before / passing-after
The updated collaborator suite was run against the currently deployed pre-change bridge and then this branch:
Live runtime smoke
Used the real workspace
access.json, real #dev channel ID, and Mark’s live collaborator ID. The restriction was added to an in-memory copy only, then the branch’s production resolver, instruction builder, and_write_task_filepath wrote and read back a temporary task:The running Discord bridge was not restarted and the live access file was not modified by this smoke test.
Tests
python3 tests/discord-bridge-collaborator-tier.test.py— PASSpython3 tests/discord-bridge-access-tier.test.py— PASSpython3 tests/injection-guard-sweep.test.py— 48/48python3 tests/discord-bridge-mod-judge-integration.test.py— PASSpython3 -m py_compileon the changed Python files — PASSorigin/main— 100% (32/32 changed executable lines)git diff --check— PASSRisk, compatibility, and rollback
collaboratorPurposes; owners configure it inaccess.jsonlike the existing collaborator list.Platform limitation and future work
This PR is intentionally Discord-only. It extends Discord's existing per-channel
collaboratorsauthorization path and does not claim purpose restriction support for every messaging surface.tierMaproles (owner/team/other) but no equivalent collaborator-purpose resolver or trusted purpose instruction today.Future work can make purpose restrictions platform-neutral by extracting a shared policy resolver/instruction builder and adding a bridge-specific configuration adapter for
(surface, channel/chat, sender). Slack and Telegram should each receive their own behavioral tests and live runtime evidence before being described as supported. Telegram's owner-only model needs an explicit design decision rather than silently inheriting Discord semantics.Enforcement boundary
This PR adds an owner-authored in-band guidance control, not a structural sandbox boundary. The bridge injects the allowed purposes as trusted system instructions and the task agent must classify the request and comply. Discord's existing team/other sandbox route remains the hard enforcement layer; purpose restrictions do not independently stop tool execution if an agent ignores its instructions.
A follow-up can add an auditable matched-purpose field once the task/result protocol has a reliable place for the agent's classification. This PR does not claim that audit signal.