Skip to content

fix: gate Active Memory global toggles [AI]#97841

Merged
pgondhi987 merged 3 commits into
openclaw:mainfrom
pgondhi987:fix/fix-783
Jun 30, 2026
Merged

fix: gate Active Memory global toggles [AI]#97841
pgondhi987 merged 3 commits into
openclaw:mainfrom
pgondhi987:fix/fix-783

Conversation

@pgondhi987

@pgondhi987 pgondhi987 commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

Fixes an issue where command-authorized channel users could change the gateway-wide Active Memory setting when the command arrived without gateway scope metadata.

Why This Change Was Made

The global Active Memory on/off command now requires either an owner sender identity from trusted channel dispatch or operator.admin from gateway clients before it persists config changes. Read-only global status and session-scoped Active Memory controls are unchanged.

AI-assisted change.

User Impact

Operators can expect /active-memory on --global and /active-memory off --global to remain restricted to owner/admin contexts, while authorized non-owner channel users can still use normal session-level controls where they already could.

Evidence

  • node scripts/run-vitest.mjs extensions/active-memory/index.test.ts
  • git diff --check

@openclaw-barnacle openclaw-barnacle Bot added size: S maintainer Maintainer-authored PR labels Jun 29, 2026
@clawsweeper

clawsweeper Bot commented Jun 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed June 30, 2026, 4:57 AM ET / 08:57 UTC.

Summary
The PR changes the Active Memory plugin command to expose trusted sender-owner status and deny global on/off config mutations unless the caller is a channel owner or has operator.admin, with focused regression coverage.

PR surface: Source +12, Tests +29. Total +41 across 2 files.

Reproducibility: yes. Current main source lets absent gatewayClientScopes fall through to the global config mutation branch, and the inspected Mantis baseline shows a non-owner Telegram global off command succeeding before the PR.

Review metrics: 1 noteworthy metric.

  • Global config mutation gate: 1 command guard changed; 2 global write actions gated. The small diff changes who may persist gateway-wide Active Memory on/off state, which needs maintainer attention before merge.

Root-cause cluster
Relationship: canonical
Canonical: #97841
Summary: This PR is the current canonical fix for the remaining Active Memory unscoped-channel global-toggle authorization gap; the earlier merged gateway-scope PR is related but did not cover this path.

Members:

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

Merge readiness
Overall: 🦞 diamond lobster
Proof: 🦞 diamond lobster ✨ media proof bonus
Patch quality: 🦞 diamond lobster
Result: ready for maintainer review.

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

Risk before merge

  • [P1] Existing command-authorized non-owner channel users who could previously run global Active Memory on/off commands will be denied after merge.
  • [P1] The security boundary now treats trusted channel owner identity as sufficient for a persistent gateway-wide Active Memory config mutation when gateway scopes are absent.

Maintainer options:

  1. Accept owner-or-admin hardening (recommended)
    Maintain the current PR shape and land it once maintainers accept denying existing non-owner global toggle use as intended security hardening.
  2. Require gateway admin only
    If channel owner identity should not be enough for gateway-wide Active Memory config writes, change the guard to require operator.admin for global mutations and update tests/proof.
  3. Pause for authorization policy
    Keep the PR paused if the permanent owner/admin boundary for plugin-owned global config mutations needs explicit design review first.

Next step before merge

  • No automated repair is needed; the remaining action is maintainer acceptance of the protected authorization-policy and compatibility change before merge.

Security
Cleared: The diff tightens an authorization-sensitive config mutation path and introduces no dependency, workflow, secret-handling, or code-execution surface.

Review details

Best possible solution:

Land this patch after maintainer acceptance of the owner-or-admin hardening and green required checks, while preserving global status and session-scoped Active Memory controls for non-admin and non-owner callers.

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

Yes. Current main source lets absent gatewayClientScopes fall through to the global config mutation branch, and the inspected Mantis baseline shows a non-owner Telegram global off command succeeding before the PR.

Is this the best way to solve the issue?

Yes, with maintainer acceptance. The fix stays in the Active Memory plugin that owns the global toggle, reuses the existing trusted senderIsOwner command contract, and avoids a broad command-level requiredScopes gate that would also block status and session paths.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 3d4b7cade9cd.

Label changes

Label changes:

  • add rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
  • remove rating: 🐚 platinum hermit: Current PR rating is rating: 🦞 diamond lobster, so this older rating label is no longer current.

Label justifications:

  • P1: The PR addresses an authorization gap that lets command-authorized non-owner channel users mutate a gateway-wide Active Memory setting.
  • merge-risk: 🚨 compatibility: Existing non-owner channel users who could run global Active Memory toggles will be denied after this change.
  • merge-risk: 🚨 security-boundary: The diff changes authorization for a persistent gateway-wide config mutation and relies on trusted channel owner identity when gateway scopes are absent.
  • rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (recording): Mantis Telegram Desktop before/after proof shows the real visible command behavior changing from non-owner global toggle success to an owner/operator.admin denial; screenshots were inspected after prepared video probing failed due missing ffprobe.
  • proof: sufficient: Contributor real behavior proof is sufficient. Mantis Telegram Desktop before/after proof shows the real visible command behavior changing from non-owner global toggle success to an owner/operator.admin denial; screenshots were inspected after prepared video probing failed due missing ffprobe.
  • proof: 🎥 video: Contributor real behavior proof includes video or recording evidence. Mantis Telegram Desktop before/after proof shows the real visible command behavior changing from non-owner global toggle success to an owner/operator.admin denial; screenshots were inspected after prepared video probing failed due missing ffprobe.
  • mantis: telegram-visible-proof: Mantis should capture Telegram visible proof. The changed denial text is visible Telegram chat behavior and has already been demonstrated by the Mantis Telegram Desktop proof.
Evidence reviewed

PR surface:

Source +12, Tests +29. Total +41 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 16 4 +12
Tests 1 30 1 +29
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 46 5 +41

What I checked:

  • Repository policy applied: Root AGENTS.md and extensions/AGENTS.md were read fully; the touched files are under the bundled plugin boundary, and repository policy treats authorization, config mutation, and compatibility changes as maintainer-sensitive review surfaces. (AGENTS.md:1, 3d4b7cade9cd)
  • Current main allows absent-scope global writes: On current main, the Active Memory global mutation guard only rejects explicit non-admin gatewayClientScopes; when gateway scope metadata is absent, global on/off reaches mutateConfigFile. (extensions/active-memory/index.ts:830, 3d4b7cade9cd)
  • PR guard shape: At PR head, the helper rejects explicit non-admin gateway scopes and rejects unscoped callers unless senderIsOwner === true; the command also opts into sender-owner exposure. (extensions/active-memory/index.ts:830, d06380039c90)
  • PR regression coverage: The PR adds handler coverage for non-owner Telegram global on/off denial, preserves owner global writes, keeps non-admin gateway global writes denied, and retains global status/session behavior coverage. (extensions/active-memory/index.test.ts:656, d06380039c90)
  • Trusted owner exposure contract: Bundled plugins can receive trusted owner-status exposure when exposeSenderIsOwner is accepted during command registration, and executePluginCommand only forwards senderIsOwner to commands that satisfy that exposure contract. (src/plugins/command-registration.ts:381, 3d4b7cade9cd)
  • Telegram dispatch contract: Telegram native command authorization resolves senderIsOwner from owner access and passes it into plugin command execution, so the Active Memory handler receives the channel owner bit when the command opts in. (extensions/telegram/src/bot-native-commands.ts:820, 3d4b7cade9cd)

Likely related people:

  • Takhoffman: Authored the merged Active Memory plugin PR and the commit that added the Active Memory global toggle now being gated. (role: feature originator; confidence: high; commits: b83726d13e33, 281e429a31fc; files: extensions/active-memory/index.ts, extensions/active-memory/index.test.ts)
  • pgondhi987: Authored and merged the earlier Active Memory admin-scope hardening PR that this PR extends from gateway-scoped callers to unscoped channel callers. (role: adjacent authorization contributor; confidence: high; commits: 8befc4b0c038, 5852f5d15cdc; files: extensions/active-memory/index.ts, extensions/active-memory/index.test.ts)
  • vincentkoc: Authored the owner-only authorization regression fix that defines the current senderIsOwner semantics used by this PR's trusted owner path. (role: owner-status/auth contributor; confidence: medium; commits: 15a5e39da2b7; files: src/auto-reply/command-auth.ts, src/auto-reply/command-auth.owner-default.test.ts)
  • Pick-cat: Current blame on the Active Memory files points through a recent broad PR that carried the current file snapshot, though the Active Memory-specific ownership trail is stronger for the feature and authorization contributors above. (role: recent area contributor; confidence: low; commits: 9949f6bd8500; files: extensions/active-memory/index.ts, extensions/active-memory/index.test.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.

@pgondhi987

pgondhi987 commented Jun 29, 2026

Copy link
Copy Markdown
Contributor Author

Verification before merge:

Behavior addressed: /active-memory on|off --global now requires owner identity for external channel callers without gateway scopes, or operator.admin for gateway clients, before changing gateway-wide Active Memory config.
Real environment tested: local source checkout and GitHub Actions PR CI on head c611e5c2ce4970d8a2b2cd5addd66b2f93376720; no external live channel environment was used.
Exact steps or command run after this patch:

  • node scripts/run-vitest.mjs extensions/active-memory/index.test.ts
  • git diff --check origin/main...HEAD
  • scripts/pr review-tests 97841 extensions/active-memory/index.test.ts
  • timeout 1800 .agents/skills/autoreview/scripts/autoreview --mode branch --base origin/main
  • auto-pr.sh issue 783 --run-stage ghsa_dry_run
  • auto-pr.sh issue 783 --run-stage ghsa_real_gate
    Evidence after fix:
  • Active Memory focused tests passed: 153 tests.
  • review-pr: passed, READY FOR /prepare-pr, zero findings.
  • autoreview: passed, no accepted/actionable findings.
  • ghsa_dry_run: passed; ghsa_real_gate: passed.
  • GitHub checks: gh pr checks reported 57 pass, 34 skipped, no attention items for head c611e5c2ce4970d8a2b2cd5addd66b2f93376720.
    Observed result after fix: the regression test blocks authorized external non-owner global on/off attempts before mutateConfigFile, while owner/admin global writes, global status, and session-scoped controls remain covered.
    What was not tested: live Telegram/Slack/etc. command dispatch was not run; no known untested critical path in the changed Active Memory command logic.

Regression Risk:
Medium. The affected boundary is owner/admin command authorization for a plugin-owned global config mutation. Auth, sandboxing, storage schema, migrations, provider behavior, channel transport behavior, and plugin SDK shape are unchanged; the behavior change is limited to denying a previously allowed non-owner global mutation path.

Best fix verdict: best. The fix belongs in the Active Memory command because the plugin owns the global toggle semantics, and it reuses the existing trusted bundled-command owner exposure contract instead of adding a new core special case.

User behavior change:
Before, a command-authorized non-owner external channel sender could change global Active Memory when gateway scope metadata was absent. After, /active-memory on --global and /active-memory off --global require owner identity or operator.admin; /active-memory status --global and session-level /active-memory controls remain unchanged.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. mantis: telegram-visible-proof Mantis should capture Telegram visible proof. labels Jun 29, 2026
@clawsweeper
clawsweeper Bot temporarily deployed to qa-live-shared June 29, 2026 15:29 Inactive
@openclaw-mantis

openclaw-mantis Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Mantis Telegram Desktop Proof

Summary: Mantis captured Telegram before/after evidence for the Active Memory global toggle command.

Main screenshot This PR screenshot
Baseline native Telegram Desktop screenshot Candidate native Telegram Desktop screenshot
Main This PR
Baseline native Telegram Desktop proof GIF Candidate native Telegram Desktop proof GIF

Motion-trimmed clips:

Raw QA files: https://artifacts.openclaw.ai/mantis/telegram-desktop/pr-97841/run-28383345433-1/index.json

@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. proof: 🎥 video Contributor real behavior proof includes video or recording evidence. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P1 High-priority user-facing bug, regression, or broken workflow. 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. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jun 30, 2026
@pgondhi987

pgondhi987 commented Jun 30, 2026

Copy link
Copy Markdown
Contributor Author

Current-head behavioral proof for ClawSweeper re-review:

Head verified: d06380039c90f4c51f065f11901396bd7ded781c

Behavior fixed: /active-memory on --global and /active-memory off --global no longer let an authorized external-channel non-owner mutate gateway-wide Active Memory config; external channel callers now need owner identity, and gateway clients still need operator.admin.

Real environment tested: local OpenClaw source checkout plus GitHub Actions PR CI on the exact head above. Existing Mantis Telegram Desktop proof on this PR also shows the before/after visible command behavior for the same security boundary.

Exact current-head evidence:

  • ghsa_dry_run gate: PASS / SOLVES for PR fix: gate Active Memory global toggles [AI] #97841 at d06380039c90f4c51f065f11901396bd7ded781c.
  • scripts/pr review-tests 97841 extensions/active-memory/index.test.ts: passed through the review-pr wrapper.
  • timeout 1800 .agents/skills/autoreview/scripts/autoreview --mode branch --base origin/main: clean, no accepted/actionable findings.
  • git diff --check origin/main...HEAD: passed.
  • node scripts/run-vitest.mjs extensions/active-memory/index.test.ts: 153 tests passed.
  • GitHub Actions PR CI: passed after rerunning QA Smoke CI job 84246670868; rollup at this head was 60 success, 80 skipped, with only cancelled older auto-response/proof attempts remaining.

Before/after fidelity: current main lets absent gatewayClientScopes fall through to the global mutateConfigFile branches. This PR makes the command request trusted owner-status exposure and denies the global write before mutation unless gateway scopes include operator.admin or unscoped channel dispatch supplies senderIsOwner === true.

Known gaps: no new live manual channel run was performed in this orchestrator pass; the PR already has Mantis Telegram Desktop before/after proof, and this pass revalidated the current code head, local tests, review gates, and GitHub CI.

@pgondhi987

pgondhi987 commented Jun 30, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 30, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@clawsweeper clawsweeper Bot added rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. labels Jun 30, 2026
@pgondhi987
pgondhi987 merged commit 738b2be into openclaw:main Jun 30, 2026
86 checks passed
github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Jul 1, 2026
@pgondhi987
fix: gate active memory global toggles (openclaw#97841)
9d84a8d
QiuYuang pushed a commit to QiuYuang/openclaw that referenced this pull request Jul 1, 2026
@pgondhi987 @QiuYuang
fix: gate active memory global toggles (openclaw#97841)
8c14eef
chenyangjun-xy pushed a commit to chenyangjun-xy/openclaw that referenced this pull request Jul 1, 2026
@pgondhi987 chenyangjun-xy
fix: gate active memory global toggles (openclaw#97841)
8713b3d
chenyangjun-xy pushed a commit to chenyangjun-xy/openclaw that referenced this pull request Jul 3, 2026
@pgondhi987 @chenyangjun-xy
fix: gate active memory global toggles (openclaw#97841)
f8afe94
Rorqualx pushed a commit to Rorqualx/cortex that referenced this pull request Jul 6, 2026
@pgondhi987 cron
fix: gate active memory global toggles (openclaw#97841)
9788f58 
(cherry picked from commit 738b2be)
Rorqualx pushed a commit to Rorqualx/cortex that referenced this pull request Jul 8, 2026
@pgondhi987 cron
fix: gate active memory global toggles (openclaw#97841)
9c128bf 
(cherry picked from commit 738b2be)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

maintainer Maintainer-authored PR mantis: telegram-visible-proof Mantis should capture Telegram visible proof. 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. P1 High-priority user-facing bug, regression, or broken workflow. proof: sufficient ClawSweeper judged the real behavior proof convincing. proof: 🎥 video Contributor real behavior proof includes video or recording evidence. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. size: S status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@pgondhi987