Reply: allow authorized group command sessions to receive tool updates#64182
Reply: allow authorized group command sessions to receive tool updates#64182hongzexin wants to merge 1 commit into
Conversation
Greptile SummaryThis PR allows inline tool summaries to be delivered in Feishu/Lark group sessions when
Confidence Score: 4/5Merge after resolving the architecture boundary violation — the functional intent is correct but the implementation approach is explicitly prohibited by project rules. A P1 finding (hardcoded provider names in core) matches a rule that CLAUDE.md flags as a refactor trigger, and there is an asymmetric behavior gap in shouldSendToolStartStatuses. Both need to be addressed before landing. src/auto-reply/reply/dispatch-from-config.ts — the shouldSendToolSummaries / shouldSendToolStartStatuses block around line 570 Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/auto-reply/reply/dispatch-from-config.ts
Line: 570-574
Comment:
**Hardcoded provider names in core violate the architecture boundary**
The CLAUDE.md/AGENTS.md project rules explicitly state: *"do not add hardcoded bundled extension/provider/channel/capability id lists, maps, or named special cases in core when a manifest, capability, registry, or plugin-owned contract can express the same behavior."* Embedding `"feishu"` and `"lark"` string literals directly into `dispatch-from-config.ts` is exactly this pattern — it bakes Feishu-owned behavior into the generic dispatch layer, making it harder to add similar behavior for other channels without further core edits.
The correct approach is for the Feishu plugin to express this opt-in through a manifest capability flag or a plugin-owned channel contract hook (e.g. `allowToolSummariesWhenAuthorized: true`), letting the generic dispatch code read that flag without knowing which provider it belongs to.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/auto-reply/reply/dispatch-from-config.ts
Line: 575
Comment:
**`shouldSendToolStartStatuses` is not updated symmetrically**
`shouldSendToolSummaries` now allows authorized Feishu group sessions through, but `shouldSendToolStartStatuses` on the very next line keeps the original `ChatType !== "group" || IsForum` check, so tool *start* statuses remain suppressed even for these same sessions. If the intent is to treat authorized Feishu groups like an explicitly-invoked command session (as the PR description says), the two flags should behave consistently.
```suggestion
const shouldSendToolStartStatuses =
ctx.ChatType !== "group" ||
ctx.IsForum === true ||
((normalizedSurface === "feishu" || normalizedSurface === "lark") && ctx.CommandAuthorized);
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/auto-reply/reply/dispatch-from-config.ts
Line: 570
Comment:
**Use the already-imported `normalizeLowercaseStringOrEmpty` helper**
The file already imports `normalizeLowercaseStringOrEmpty` from `../../shared/string-coerce.js` (line 40). Rolling a separate inline `(... ?? "").trim().toLowerCase()` chain duplicates that utility.
```suggestion
const normalizedSurface = normalizeLowercaseStringOrEmpty(ctx.Surface ?? ctx.Provider);
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "Feishu: allow tool summaries in authoriz..." | Re-trigger Greptile |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f5488edd23
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
f5488ed to
f2109c7
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f2109c7544
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
6b47117 to
dd895b4
Compare
|
Thanks for the context here. I did a careful shell check against current Close as mostly implemented on main: the useful group tool/progress visibility problem is now covered by the shipped shared So I’m closing this older PR as already covered on Review detailsBest possible solution: Keep the shipped #85488 Do we have a high-confidence way to reproduce the issue? Not applicable as a feature/policy PR; source inspection shows the historical group suppression path, and current main plus Is this the best way to solve the issue? Yes for closing: #85488 is the better maintainer solution because it uses one shared verbose-state gate, preserves quiet defaults, and avoids adding a separate command-authorization bypass. Security review: Security review needs attention: If merged, the branch would expand group-visible progress output and needs privacy review; closing in favor of #85488 avoids that merge risk.
AGENTS.md: found and applied where relevant. What I checked:
Likely related people:
Codex review notes: model internal, reasoning high; reviewed against c78f9376d929; fix evidence: release v2026.6.6, commit 45fbf2d81a0e. |
|
Codex review: needs maintainer review before merge. What this changes: The PR updates command detection and reply dispatch so authorized non-forum group command sessions can receive text tool summaries and tool-start statuses, with tests for authorized/unauthorized groups, mention-prefixed commands, and path-like slash text. Maintainer follow-up before merge: This is an open implementation PR with a narrow and valid goal, but it changes group-visible progress behavior and needs maintainer review/rebase against the current shared verbose-progress gate rather than an automatic replacement fix lane. Review detailsBest possible solution: Update and review this PR against current main by adding a provider-agnostic authorized group command predicate at the shared verbose-progress gate, applying it consistently to tool summaries and tool-start statuses, preserving Slack non-direct suppression, normal group-chatter suppression, unauthorized suppression, and forum-topic behavior, and covering mention-prefixed commands plus path-like slash inputs in tests. Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against acae48b790fa. Re-review progress:
|
dd895b4 to
14ea864
Compare
14ea864 to
b680dfd
Compare
b680dfd to
08ee7c5
Compare
08ee7c5 to
4639b6d
Compare
4639b6d to
cba538f
Compare
cba538f to
0e35f8a
Compare
0e35f8a to
ac949b2
Compare
ac949b2 to
9a625fd
Compare
|
ClawSweeper PR egg 🔥 Warming up: real-behavior proof passed; findings, security review, or rank-up moves are still in progress. Hatch commandComment Hatchability rules:
What is this egg doing here?
|
|
This pull request has been automatically marked as stale due to inactivity. |
|
ClawSweeper applied the proposed close for this PR.
|
Why
Authorized group command sessions can be an explicitly-invoked operator flow, but the current dispatch logic suppresses both tool summaries and tool-start statuses for all non-forum groups.
That makes command-driven group sessions less observable than direct messages, even when the current message is a real control command and the sender has already passed command authorization.
What changed
CommandAuthorized === true/tmp/..., which should remain suppressedTests
node scripts/run-vitest.mjs run src/auto-reply/reply/dispatch-from-config.test.ts src/auto-reply/command-control.test.ts extensions/feishu/src/bot.test.ts(3 files, 217 tests)pnpm exec oxlint src/auto-reply/command-detection.ts src/auto-reply/command-control.test.ts src/auto-reply/reply/dispatch-from-config.ts src/auto-reply/reply/dispatch-from-config.test.tspnpm exec oxfmt --check --threads=1 src/auto-reply/command-detection.ts src/auto-reply/command-control.test.ts src/auto-reply/reply/dispatch-from-config.ts src/auto-reply/reply/dispatch-from-config.test.ts CHANGELOG.mdgit diff --check origin/main..HEADReal behavior proof
origin/mainatb680360fde68a129e72b5ed41803bf788f4b8c3e, running the repaired Reply: allow authorized group command sessions to receive tool updates #64182 code at9a625fd6a70a3efe39dacdf1a16c5afdbc5c190a.node --import tsx --input-type=module - <<'EOF' ... EOFfrom the OpenClaw repo to exercise the real shared command detector and the same verbose-gate predicate shape used by reply dispatch./statusis recognized as a real control command,@OpenClaw /tmp/openclaw.logis not, only an authorized group control command opens the verbose progress gate, and Slack channel commands remain suppressed by the Slack non-direct guard.Out of scope