fix(slack): normalize approval user ids by kevinslin · Pull Request #84671 · openclaw/openclaw

kevinslin · 2026-05-20T18:49:33Z

Summary

Normalize Slack approver ids through one user-id parser before exec and plugin approval authorization.
Preserve rejection for channel-like ids while accepting lowercase raw, user:, slack:, and mention forms.
Add regression coverage for configured-id casing and sender-id casing in exec approvals, plugin approvals, and Slack interaction entrypoints.
Remove stale empty object spread fallbacks in OpenRouter routing to satisfy the current lint gate selected by CI.
Update stale status/secret-file test compatibility so current CI type and infra shards can run cleanly.

Verification

node scripts/run-vitest.mjs extensions/slack/src/exec-approvals.test.ts extensions/slack/src/approval-auth.test.ts
node scripts/run-vitest.mjs extensions/slack/src/monitor/events/interactions.test.ts
node scripts/run-vitest.mjs src/auto-reply/reply/commands-approve.test.ts
node scripts/run-vitest.mjs extensions/slack/src/exec-approvals.test.ts extensions/slack/src/approval-auth.test.ts extensions/slack/src/monitor/events/interactions.test.ts -- --reporter verbose
node scripts/run-vitest.mjs src/commands/status.summary.redaction.test.ts src/infra/secret-file.test.ts -t "redactSensitiveStatusSummary|readSecretFileSync"
node scripts/run-tsgo.mjs -p test/tsconfig/tsconfig.test.src.json --incremental --tsBuildInfoFile .artifacts/tsgo-cache/test-src.tsbuildinfo --pretty false
node scripts/run-oxlint.mjs src/infra/secret-file.ts src/commands/status.summary.redaction.test.ts extensions/openrouter/provider-routing.ts extensions/slack/src/exec-approvals.ts extensions/slack/src/exec-approvals.test.ts extensions/slack/src/approval-auth.test.ts extensions/slack/src/monitor/events/interactions.test.ts
git diff --check

Real behavior proof

Behavior addressed: Slack approvals denied when configured approver ids used lowercase Slack user ids while Slack events delivered uppercase user ids.
Real environment tested: Local OpenClaw Codex worktree on Node/Vitest, exercising Slack approval auth helpers plus the Slack block-action interaction entrypoint used by approval buttons.
Exact steps or command run after this patch: node scripts/run-vitest.mjs extensions/slack/src/exec-approvals.test.ts extensions/slack/src/approval-auth.test.ts; node scripts/run-vitest.mjs extensions/slack/src/monitor/events/interactions.test.ts; node scripts/run-vitest.mjs src/auto-reply/reply/commands-approve.test.ts; node scripts/run-vitest.mjs extensions/slack/src/exec-approvals.test.ts extensions/slack/src/approval-auth.test.ts extensions/slack/src/monitor/events/interactions.test.ts -- --reporter verbose; node scripts/run-oxlint.mjs extensions/openrouter/provider-routing.ts extensions/slack/src/exec-approvals.ts extensions/slack/src/exec-approvals.test.ts extensions/slack/src/approval-auth.test.ts extensions/slack/src/monitor/events/interactions.test.ts; git diff --check.
Evidence after fix: The Slack interaction suite drives approval button payloads through the Slack block-action handler with uppercase Slack event users and lowercase configured approver ids. The exec/plugin auth suites cover lowercase configured approver ids matching uppercase Slack senders, lowercase sender ids matching uppercase configured allow lists, and rejection of C... channel-like ids across raw, prefixed, and mention forms.
Observed result after fix: Targeted Vitest suites passed, including the combined Slack approval proof run with Test Files 3 passed (3) and Tests 56 passed (56); targeted oxlint passed with Found 0 warnings and 0 errors; source-test type graph passed; whitespace check passed.
What was not tested: Cross-workspace Slack OAuth/app installation drift; this proof targets the Slack event/authorization boundary in OpenClaw rather than revalidating Slack's platform delivery.

clawsweeper · 2026-05-20T18:51:13Z

Codex review: needs real behavior proof before merge.

Workflow note: Future ClawSweeper reviews update this same comment in place.

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.

Summary
The PR uppercases and validates Slack approver IDs across exec/plugin approval paths, adds Slack regression coverage, and includes small OpenRouter/secret-file/status-test compatibility updates.

Reproducibility: yes. source-reproducible: current main leaves lowercase Slack approver IDs lowercase and later compares them exactly against normalized Slack sender IDs. I did not run tests because this review is read-only and test commands can create artifacts.

PR rating
Overall: 🦪 silver shellfish
Proof: 🦪 silver shellfish
Patch quality: 🐚 platinum hermit
Summary: The patch looks focused and well tested, but the proof remains mock-only for a Slack authorization path.

Rank-up moves:

Add redacted live Slack workspace proof showing the approval button succeeds for the normalized user ID and channel-like IDs remain unauthorized.

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.

Real behavior proof
Needs real behavior proof before merge: The PR body provides targeted tests and checks only; external PR proof still needs redacted live Slack transcript, recording, terminal output, or logs, with private IDs, tokens, phone numbers, and non-public endpoints redacted before updating the PR body for re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Mantis proof suggestion
A Slack workspace smoke would directly show the approval button path that the tests exercise through mocks. A maintainer can ask Mantis to capture proof by posting a new PR comment that starts with the OpenClaw Mantis account mention, followed by:

slack desktop smoke: verify a Slack approval succeeds when a lowercase configured approver ID receives an uppercase Slack user ID, and C-prefixed channel-like IDs are rejected.

Risk before merge

No redacted live Slack workspace evidence yet shows the signed Slack interaction/button path accepting the intended user and rejecting channel-like IDs.
The diff changes an approval authorization boundary and a secret-file fallback helper, so the protected maintainer/security-boundary review should be explicit before merge.

Maintainer options:

Prove the Slack approval boundary (recommended)
Run or request redacted live Slack proof showing a lowercase configured approver authorizes from an uppercase Slack event user and C-prefixed channel-like IDs remain unauthorized.
Accept unit-level proof intentionally
A maintainer can merge on parser and interaction regression tests alone if they explicitly accept the remaining live Slack auth-boundary proof gap.

Next step before merge
Human handling is needed because the PR has a protected maintainer label and lacks live Slack behavior proof; there is no narrow code repair finding for ClawSweeper to fix.

Security
Cleared: Cleared: the diff touches approval authorization and secret-file behavior but adds no dependency or supply-chain changes, and no definite security regression was found in the read-only pass.

Review details

Best possible solution:

Land the shared Slack ID normalization after maintainer review with redacted live Slack proof, and keep the secret-file compatibility change only if maintainers intentionally want the non-throwing OpenClaw helper contract preserved.

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

Yes, source-reproducible: current main leaves lowercase Slack approver IDs lowercase and later compares them exactly against normalized Slack sender IDs. I did not run tests because this review is read-only and test commands can create artifacts.

Is this the best way to solve the issue?

Yes, the Slack fix uses the existing Slack approver parser as the narrow shared normalization point and covers exec, plugin, target, and interaction paths. The remaining issue is proof and maintainer approval, not a better implementation shape.

Label justifications:

P2: This is a focused Slack approval bug fix with limited blast radius but real authorization impact.
merge-risk: 🚨 security-boundary: The PR changes who is authorized to approve Slack exec/plugin requests and also touches secret-file fallback behavior.
rating: 🦪 silver shellfish: Current PR rating is 🦪 silver shellfish because proof is 🦪 silver shellfish, patch quality is 🐚 platinum hermit, and The patch looks focused and well tested, but the proof remains mock-only for a Slack authorization path.
status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body provides targeted tests and checks only; external PR proof still needs redacted live Slack transcript, recording, terminal output, or logs, with private IDs, tokens, phone numbers, and non-public endpoints redacted before updating the PR body for re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Acceptance criteria:

node scripts/run-vitest.mjs extensions/slack/src/exec-approvals.test.ts extensions/slack/src/approval-auth.test.ts extensions/slack/src/monitor/events/interactions.test.ts
node scripts/run-oxlint.mjs extensions/openrouter/provider-routing.ts extensions/slack/src/exec-approvals.ts extensions/slack/src/exec-approvals.test.ts extensions/slack/src/approval-auth.test.ts extensions/slack/src/monitor/events/interactions.test.ts
redacted live Slack workspace proof for the approval button authorization path

What I checked:

Current main preserves lowercase configured IDs: On current main, normalizeSlackApproverId accepts prefixed, mention, and raw Slack IDs but returns the captured value without canonicalizing case, so lowercase configured IDs remain lowercase. (extensions/slack/src/exec-approvals.ts:11, 1a7669bc63a0)
Current main compares exact normalized strings: Slack plugin approval authorization normalizes the sender and then checks approvers.includes(senderId), so a lowercase configured approver and uppercase Slack event user do not match. (extensions/slack/src/approval-auth.ts:27, 1a7669bc63a0)
PR diff centralizes uppercase U/W normalization: The PR adds normalizeSlackUserLikeId, uppercases accepted IDs, and rejects non-user-like C-prefixed channel IDs for prefixed, mention, and raw forms. (extensions/slack/src/exec-approvals.ts:11, 9d6bb9f987e0)
Slack approval docs define this as an authorization path: The Slack docs say Slack-native approval buttons enforce approver authorization and render through Slack interactivity, which makes the changed path a real auth-boundary behavior rather than a pure parser cleanup. Public docs: docs/channels/slack.md. (docs/channels/slack.md:1286, 1a7669bc63a0)
Real behavior proof is still test-only: The PR body reports targeted Vitest, oxlint, tsgo, and whitespace proof in a local worktree, but does not include a redacted live Slack workspace transcript, recording, terminal output, or logs from an actual Slack approval. (9d6bb9f987e0)
Related history for routing: git blame ties the current Slack approval helpers to commit 357e3ec, OpenRouter routing to ac69776, and the fs-safe dependency update to 1a7669b. (extensions/slack/src/exec-approvals.ts:11, 357e3ecc65e3)

Likely related people:

obviyus: Git blame and GitHub commit metadata tie the current Slack approval normalization/auth files to commit 357e3ec. (role: introduced current Slack approval helpers; confidence: medium; commits: 357e3ecc65e3; files: extensions/slack/src/exec-approvals.ts, extensions/slack/src/approval-auth.ts, extensions/slack/src/monitor/events/interactions.block-actions.ts)
steipete: The current main SHA updates @openclaw/fs-safe to 0.2.7, which is the dependency change this PR's secret-file compatibility patch adapts around. (role: recent dependency updater; confidence: high; commits: 1a7669bc63a0; files: package.json, pnpm-lock.yaml, src/infra/secret-file.ts)
amknight: Git blame and GitHub commit metadata tie the OpenRouter routing file touched by the lint cleanup to commit ac69776. (role: recent OpenRouter area contributor; confidence: medium; commits: ac6977633092; files: extensions/openrouter/provider-routing.ts)

Codex review notes: model gpt-5.5, reasoning high; reviewed against 1a7669bc63a0.

clawsweeper · 2026-05-20T18:56:31Z

ClawSweeper PR egg

🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat.

Where did the egg go?

The egg game starts only after the PR passes the real-behavior proof check.
Before that, no creature or rarity is rolled. The treat waits for real proof.
This is still just collectible flavor: proof affects review readiness, not creature quality.

kevinslin · 2026-05-20T19:07:33Z

@clawsweeper re-review

clawsweeper · 2026-05-20T19:09:35Z

🦞🧹
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.

Re-review progress:

State: Complete
Detail: The targeted re-review finished, the durable review comment was updated, and the synced verdict was routed.
Run: https://github.com/openclaw/clawsweeper/actions/runs/26184092514
Updated: 2026-05-20T19:16:27.490Z

kevinslin · 2026-05-20T19:35:35Z

@clawsweeper re-review

clawsweeper · 2026-05-20T19:37:33Z

🦞🧹
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.

Re-review progress:

State: Complete
Detail: The targeted re-review finished, the durable review comment was updated, and the synced verdict was routed.
Run: https://github.com/openclaw/clawsweeper/actions/runs/26185505427
Updated: 2026-05-20T19:46:35.906Z

* fix(slack): normalize approval user ids * chore(openrouter): satisfy spread fallback lint * fix(ci): unblock status and secret-file checks

fix(slack): normalize approval user ids

d4e33c9

openclaw-barnacle Bot added channel: slack Channel integration: slack size: S maintainer Maintainer-authored PR labels May 20, 2026

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. labels May 20, 2026

chore(openrouter): satisfy spread fallback lint

b5c68dd

openclaw-barnacle Bot added the extensions: openrouter label May 20, 2026

clawsweeper Bot added P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. labels May 20, 2026

fix(ci): unblock status and secret-file checks

9d6bb9f

kevinslin requested a review from a team as a code owner May 20, 2026 19:30

openclaw-barnacle Bot added the commands Command implementations label May 20, 2026

kevinslin merged commit 9e4eca0 into main May 20, 2026
111 of 114 checks passed

kevinslin deleted the codex/slack-approval-userid-normalization branch May 20, 2026 20:40

github-actions Bot mentioned this pull request May 20, 2026

📡 Upstream Digest — 2026-05-20 21:44 UTC curtismercier/openclaw-mods#904

Open

RomneyDa mentioned this pull request May 20, 2026

fix(infra): restore symlink rejection in tryReadSecretFileSync #84711

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(slack): normalize approval user ids#84671

fix(slack): normalize approval user ids#84671
kevinslin merged 3 commits into
mainfrom
codex/slack-approval-userid-normalization

kevinslin commented May 20, 2026 •

edited

Loading

Uh oh!

clawsweeper Bot commented May 20, 2026 •

edited

Loading

Uh oh!

clawsweeper Bot commented May 20, 2026

Uh oh!

kevinslin commented May 20, 2026

Uh oh!

clawsweeper Bot commented May 20, 2026 •

edited

Loading

Uh oh!

kevinslin commented May 20, 2026

Uh oh!

clawsweeper Bot commented May 20, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

kevinslin commented May 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Verification

Real behavior proof

Uh oh!

clawsweeper Bot commented May 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

clawsweeper Bot commented May 20, 2026

Uh oh!

kevinslin commented May 20, 2026

Uh oh!

clawsweeper Bot commented May 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kevinslin commented May 20, 2026

Uh oh!

clawsweeper Bot commented May 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

kevinslin commented May 20, 2026 •

edited

Loading

clawsweeper Bot commented May 20, 2026 •

edited

Loading

clawsweeper Bot commented May 20, 2026 •

edited

Loading

clawsweeper Bot commented May 20, 2026 •

edited

Loading