Skip to content

feat(config): add canonical group-policy scope-tree resolver#106819

Closed
steipete wants to merge 2 commits into
mainfrom
claude/chanref-group-scope-core
Closed

feat(config): add canonical group-policy scope-tree resolver#106819
steipete wants to merge 2 commits into
mainfrom
claude/chanref-group-scope-core

Conversation

@steipete

@steipete steipete commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

16 channel plugins register a groups adapter answering the same questions — effective requireMention, tool policy (including sender-scoped tools), group intro hints — but only 9 delegate to the shared resolver in src/config/group-policy.ts. The rest ship private walkers over channel-specific config nests (discord guild→channel, telegram group→topic, msteams team→channel with cross-team scan, slack/matrix/zalouser candidate matching): same decision, many implementations, measurable drift (e.g. adapter paths that can never match channel:-prefixed toolsBySender keys while the core fallback can).

Why This Change Was Made

Stage 1 of the group-policy scope normalization design (load-time canonical model, zero user-visible config change): a ScopeTree of flat keyed scope nodes plus channel-ordered scope paths, with core-owned resolvers for requireMention (most-specific-wins, override-order semantics identical to resolveChannelGroupRequireMention), tool policy (per-level toolsBySender → tools, whole-object override, reusing resolveToolsBySender verbatim as the sender engine), and intro hints. Flat keys instead of a nested tree are deliberate: telegram's real precedence lets a wildcard-group topic node outrank an exact-group scalar, which a nested walk cannot express; channels encode precedence in the path they emit. Runtime overrides stay resolver parameters, never tree data. Follow-up PRs migrate channels in parity-tested batches and delete the per-channel walkers (~600 LOC across 11 files).

User Impact

None yet — no callers are wired in this PR; behavior is unchanged. The new resolvers are exported via openclaw/plugin-sdk/channel-policy (surface budgets bumped by the exact delta with a citation comment) so channel migrations can adopt them.

Evidence

  • Testbox (Blacksmith): pnpm test src/config/group-scope-tree.test.ts src/config/group-policy.test.ts src/plugin-sdk green; pnpm check:changed green through all lanes with the regenerated Plugin SDK API baseline hash committed.
  • Tests: table-driven precedence walks (skip-missing keys, defaults last, final default true), the telegram-shaped ordering case, msteams-shaped two-level cascade, typed-sender key integration incl. messageProvider gating, before-/after-config override parity with group-policy.ts, configuredScopeDefaultsToNoMention, and narrowest-first intro hints.
  • Autoreview (codex gpt-5.6-sol, xhigh): clean, "patch is correct (0.88)", no actionable findings.
  • Design + parity groundwork: semantics table extracted from all 16 adapter registrations (scope levels, per-field precedence, merge-vs-override, sender scoping, drift list) drives the batch order for the migration PRs.

@openclaw-barnacle openclaw-barnacle Bot added docs Improvements or additions to documentation scripts Repository scripts size: M maintainer Maintainer-authored PR labels Jul 13, 2026
chatgpt-codex-connector[bot]
chatgpt-codex-connector Bot reviewed Jul 13, 2026
View reviewed changes

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d8a7307cee

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/config/group-scope-tree.ts
senderName: params.senderName,
senderUsername: params.senderUsername,
senderE164: params.senderE164,
messageProvider: params.messageProvider,

@chatgpt-codex-connector chatgpt-codex-connector Bot Jul 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Add a channel fallback for sender policy

When a channel migrates its groups.resolveToolPolicy adapter to this new public resolver and passes the existing ChannelGroupContext fields, channel-scoped toolsBySender entries like channel:discord:<senderId> or channel:whatsapp:<senderId> never match because ChannelGroupContext has no messageProvider field and resolveToolsBySender only checks channel: buckets when that value is present. The existing resolveChannelGroupToolsPolicy avoids this by defaulting messageProvider to params.channel, so this resolver should accept a channel/provider parameter and apply the same fallback; otherwise it silently falls through to generic id: or wildcard policies during the advertised channel migrations.

Useful? React with 👍 / 👎.

@steipete steipete mentioned this pull request Jul 13, 2026
Closed
@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. labels Jul 13, 2026
@clawsweeper

clawsweeper Bot commented Jul 13, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed July 13, 2026, 7:22 PM ET / 23:22 UTC.

Summary
Adds a flat ScopeTree resolver for mention policy, sender-scoped tool policy, and intro hints, exports six channel-policy SDK symbols, and adds precedence tests plus SDK baseline updates.

PR surface: Source +117, Tests +296, Generated 0, Other +1. Total +414 across 5 files.

Reproducibility: not applicable. This PR introduces a new canonical resolver and public SDK surface rather than fixing a demonstrated current-main failure.

Review metrics: 2 noteworthy metrics.

  • Plugin SDK surface: 6 exports added to channel-policy, including 3 functions; mirrored in compat. These additions become an external compatibility contract once shipped and should be reviewed together with their first production callers.
  • Runtime adoption: 0 production callers changed. The implementation cannot yet demonstrate that real channel identity, precedence, and tool restrictions survive migration.

Root-cause cluster
Relationship: superseded
Canonical: #106846
Summary: The consolidated PR explicitly subsumes this core slice and the first migration batch, but it is not yet a safe close target because it remains open and lacks positive real behavior proof.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🦪 silver shellfish
Result: blocked until real behavior proof from a real setup is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P2] Make provider identity mandatory or preserve the existing channel fallback and add focused regression coverage.
  • Attach redacted after-change proof from a real migrated channel showing channel-scoped sender tool policy selection.

Proof guidance:

  • [P1] Needs real behavior proof before merge: Only tests, checks, and autoreview are supplied, while the PR explicitly wires no runtime caller; add a redacted live run, terminal output, or logs from a dependent channel migration, update the PR body, and ask a maintainer for @clawsweeper re-review if automatic review does not run.

Risk before merge

  • [P1] A future plugin or channel caller can omit messageProvider, causing channel:<provider>:<senderId> policy entries not to match and potentially selecting a broader generic or wildcard tool policy.
  • [P1] Publishing six new SDK exports before a production migration proves the contract makes later corrections compatibility-sensitive for external plugins.
  • [P1] No real channel run currently proves that the new resolver preserves sender-scoped tool visibility and mention precedence outside tests.
  • [P1] The intended consolidated PR is still open, behind main, and proof-deficient, so merging both branches independently could duplicate API and migration work.

Maintainer options:

  1. Harden the consolidated landing (recommended)
    Make provider identity required or preserve channel fallback, add live channel proof, and land the corrected API with the migrations in refactor(config): add canonical group-policy scope-tree resolver and migrate the first six channels #106846.
  2. Repair this standalone slice
    Keep this PR separate but fix the sender-policy contract and demonstrate it through a dependent migration before exposing the SDK API.
  3. Pause the standalone PR
    Leave this PR unmerged until the consolidated replacement becomes proof-positive and maintainers confirm it as the canonical landing path.

Next step before merge

  • [P1] A protected maintainer/API-contract decision and missing contributor-owned runtime proof prevent automated repair or merge handling.

Maintainer decision needed

  • Question: Should the canonical public ScopeTree tool-policy API require provider identity, preserve an explicit channel fallback, or remain internal until channel migrations establish the contract?
  • Rationale: The choice fixes a security-sensitive sender-policy invariant and determines a shipped plugin SDK contract; tests cannot decide which public API and consolidation boundary maintainers intend.
  • Likely owner: steipete — They merged the underlying sender-policy feature and are carrying both this normalization design and its consolidated migration PR.
  • Options:
    • Consolidate and harden (recommended): Fix the provider-identity contract and prove it through the migrated channels in refactor(config): add canonical group-policy scope-tree resolver and migrate the first six channels #106846, then close the inner stacked PRs.
    • Land core first: Keep this standalone PR, but require provider-safe typing or fallback behavior plus real migration proof before merging its public exports.
    • Keep the resolver internal: Remove the public SDK exports until enough bundled channels use the resolver to establish a stable external contract.

Security
Needs attention: The proposed public resolver can silently misresolve sender-scoped tool restrictions when provider identity is omitted.

Review findings

  • [P1] Require provider identity for sender-scoped tools — src/config/group-scope-tree.ts:22
Review details

Best possible solution:

Use one canonical landing path, preferably #106846, make provider identity mandatory or preserve the existing channel fallback, keep the SDK surface no broader than required, and attach redacted live migration proof before closing the stacked PRs.

Do we have a high-confidence way to reproduce the issue?

Not applicable: this PR introduces a new canonical resolver and public SDK surface rather than fixing a demonstrated current-main failure.

Is this the best way to solve the issue?

Unclear: centralizing duplicated policy resolution is directionally sound, but the best landing is one provider-safe canonical change with real channel migrations rather than an unproven standalone public API slice.

Full review comments:

  • [P1] Require provider identity for sender-scoped tools — src/config/group-scope-tree.ts:22
    The public sender type still inherits optional messageProvider, unlike the existing channel resolver's messageProvider ?? channel fallback. A migrated plugin can therefore omit it, fail to match channel:<provider>:<senderId>, and silently fall through to a generic or wildcard policy with different tool access; make provider identity required or preserve an explicit channel fallback.
    Confidence: 0.97

Overall correctness: patch is incorrect
Overall confidence: 0.95

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 8da737858d62.

Label changes

Label justifications:

  • P2: This is a bounded foundational improvement with security and compatibility implications but no current runtime behavior change because no caller is wired.
  • merge-risk: 🚨 compatibility: The PR adds six public plugin SDK exports before their contract has been proven through production channel migrations.
  • merge-risk: 🚨 security-boundary: Omitting provider identity can bypass channel-scoped sender-tool matches and select a different, potentially broader policy.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦪 silver shellfish.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: Only tests, checks, and autoreview are supplied, while the PR explicitly wires no runtime caller; add a redacted live run, terminal output, or logs from a dependent channel migration, update the PR body, and ask a maintainer for @clawsweeper re-review if automatic review does not run.
Evidence reviewed

PR surface:

Source +117, Tests +296, Generated 0, Other +1. Total +414 across 5 files.

View PR surface stats
Area Files Added Removed Net
Source 2 117 0 +117
Tests 1 296 0 +296
Docs 0 0 0 0
Config 0 0 0 0
Generated 1 2 2 0
Other 1 6 5 +1
Total 5 421 7 +414

Security concerns:

  • [high] Optional provider can bypass channel-scoped policy — src/config/group-scope-tree.ts:22
    Channel-scoped sender keys are checked only when messageProvider is present; exposing a resolver that permits omission can select a generic or wildcard policy and change tool visibility for that sender.
    Confidence: 0.96

What I checked:

Likely related people:

  • steipete: Merged the existing channel-aware sender-policy implementation and is carrying the present scope-tree normalization and consolidated follow-up. (role: merger and current refactor owner; confidence: high; commits: 530b892f0684, e5236611f882; files: src/config/group-policy.ts, src/config/group-scope-tree.ts, src/plugin-sdk/channel-policy.ts)
  • JerranC: Authored the merged toolsBySender feature that established channel-first matching and the messageProvider contract being reused here. (role: introduced behavior; confidence: high; commits: 530b892f0684; files: src/config/group-policy.ts, src/config/types.tools.ts, src/agents/sender-tool-policy.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.
Review history (3 earlier review cycles)
  • reviewed 2026-07-13T21:46:39.577Z sha d8a7307 :: needs real behavior proof before merge. :: [P1] Make provider identity mandatory for sender policy
  • reviewed 2026-07-13T22:19:07.099Z sha 2ccd5bf :: needs real behavior proof before merge. :: [P1] Require provider identity for sender policy
  • reviewed 2026-07-13T22:31:19.630Z sha 2ccd5bf :: needs real behavior proof before merge. :: [P1] Require provider identity for sender policy

@steipete steipete mentioned this pull request Jul 13, 2026
Merged
@steipete
steipete force-pushed the claude/chanref-group-scope-core branch from d8a7307 to 2ccd5bf Compare July 13, 2026 22:13
@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Jul 13, 2026
@steipete
steipete force-pushed the claude/chanref-group-scope-core branch from 2ccd5bf to d38f5b4 Compare July 13, 2026 23:10
@steipete

steipete commented Jul 14, 2026

Copy link
Copy Markdown
Contributor Author

Landed via #106846 (squash b9c4815) — the stack was consolidated into one landing because each stacked landing pays a full CI + generated-baseline-hash cycle against a fast-moving main. Content is patch-identical: the ScopeTree core resolver commits are the first two of that squash's lineage.

@steipete steipete closed this Jul 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chatgpt-codex-connector chatgpt-codex-connector[bot] chatgpt-codex-connector[bot] left review comments

Assignees

No one assigned

Labels

docs Improvements or additions to documentation maintainer Maintainer-authored PR merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. P2 Normal backlog priority with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. scripts Repository scripts size: M status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@steipete