fix: announcement ordering, UTF-16-safe label cap, base prompt spend list - #3747
Open
Christian-Sidak wants to merge 1 commit into
Open
fix: announcement ordering, UTF-16-safe label cap, base prompt spend list#3747Christian-Sidak wants to merge 1 commit into
Christian-Sidak wants to merge 1 commit into
Conversation
…list Three latent issues from the MCPJam#3696 review: - announcementFor: check kind before resource URL so cancel operations with a resource URL announce "Cancelled" rather than "Approved - follow it here". URL is now only used inside the start branch and as a fallback for unrecognised kinds (newer server). - buildProposalBlocks: replace .slice(0, MAX_BUTTON_LABEL) with capChars() so the button label is capped on code-point boundaries. An emoji at the boundary would yield an unpaired surrogate that can make Slack reject the whole message. - AGENT_API_BASE_PROMPT_LINES: add "setting a schedule" and "running a third-party tool" to the spend enumeration so the model is told about the two gated ops added after the original prompt was written. Snapshot in agent-op-registry.test.ts updated to match. Fixes MCPJam#3705 Signed-off-by: Christian Sidak <christian@sentineltech.eu> Signed-off-by: Christian-Sidak <61099993+Christian-Sidak@users.noreply.github.com>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
dosubot
Bot
added
size:S
Contributor
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Contributor
WalkthroughThe agent approval guidance now includes schedule changes and third-party tool execution as proposal-only actions. Slack proposal announcements prioritize cancellation messages and use resource URLs for applicable outcomes. Proposal button labels now use Unicode-safe truncation with an ellipsis when they exceed the character limit. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
slack-app/listeners/views/proposal-builder.js (1)
214-221: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an emoji-boundary regression test.
The current coverage uses only ASCII text. Add a label with an emoji at the 75-character boundary. Assert that the rendered label remains within 75 code points and contains no unpaired surrogate. This verifies the
capChars()behavior instead of only the existing ASCII limit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@slack-app/listeners/views/proposal-builder.js` around lines 214 - 221, Add a regression test for the label-building flow using an emoji-containing label at the 75-character boundary. Assert the rendered label is at most 75 Unicode code points and contains no unpaired surrogate characters, exercising capChars() rather than only ASCII truncation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mcpjam-inspector/server/routes/v1/agent.ts`:
- Line 651: Correct the shared base prompt wording near the
action-classification text in mcpjam-inspector/server/routes/v1/agent.ts:651-651
so only enabling a schedule is described as recurring quota or credit spending,
while call_server_tool is described as an external action requiring approval;
preserve the instruction that these tools propose actions rather than execute
them. Update the corresponding frozen prompt literal in
mcpjam-inspector/server/routes/v1/__tests__/agent-op-registry.test.ts:562-562 to
exactly match the production wording.
---
Nitpick comments:
In `@slack-app/listeners/views/proposal-builder.js`:
- Around line 214-221: Add a regression test for the label-building flow using
an emoji-containing label at the 75-character boundary. Assert the rendered
label is at most 75 Unicode code points and contains no unpaired surrogate
characters, exercising capChars() rather than only ASCII truncation.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2f61c868-5746-43fc-8d1f-95da64842fc1
📒 Files selected for processing (4)
mcpjam-inspector/server/routes/v1/__tests__/agent-op-registry.test.tsmcpjam-inspector/server/routes/v1/agent.tsslack-app/listeners/actions/proposal-button.jsslack-app/listeners/views/proposal-builder.js
| "- Author cases as `steps` arrays; prefer a `prompt` step plus `toolCalledWith`-style assertions on the tools the conversation showed. Set `expectedOutput` when the user stated one.", | ||
| `- When creating a suite, set the suite \`model\` explicitly to \`${DEFAULT_SUITE_MODEL}\` unless the user asks for a different model.`, | ||
| "- Some actions SPEND the user's quota or credits (running a suite or a case, generating cases, cancelling a run). Calling those tools does NOT perform them: it PROPOSES the action and returns an approval id, and a person must click to confirm. Say that you've proposed it and what it will do. NEVER say it has started, is running, or has been cancelled.", | ||
| "- Some actions SPEND the user's quota or credits (running a suite or a case, generating cases, cancelling a run, setting a schedule, running a third-party tool). Calling those tools does NOT perform them: it PROPOSES the action and returns an approval id, and a person must click to confirm. Say that you've proposed it and what it will do. NEVER say it has started, is running, or has been cancelled.", |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the shared base-prompt classification.
The prompt currently presents call_server_tool as quota spending and treats all schedule changes as spending. The registry shows that call_server_tool is gated for external side effects, while disabling a schedule does not spend quota.
mcpjam-inspector/server/routes/v1/agent.ts#L651-L651: Describe only enabling a schedule as recurring spend, and describecall_server_toolas an external action that still requires approval.mcpjam-inspector/server/routes/v1/__tests__/agent-op-registry.test.ts#L562-L562: Update the frozen prompt literal to match the corrected production wording.
📍 Affects 2 files
mcpjam-inspector/server/routes/v1/agent.ts#L651-L651(this comment)mcpjam-inspector/server/routes/v1/__tests__/agent-op-registry.test.ts#L562-L562
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mcpjam-inspector/server/routes/v1/agent.ts` at line 651, Correct the shared
base prompt wording near the action-classification text in
mcpjam-inspector/server/routes/v1/agent.ts:651-651 so only enabling a schedule
is described as recurring quota or credit spending, while call_server_tool is
described as an external action requiring approval; preserve the instruction
that these tools propose actions rather than execute them. Update the
corresponding frozen prompt literal in
mcpjam-inspector/server/routes/v1/__tests__/agent-op-registry.test.ts:562-562 to
exactly match the production wording.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="slack-app/listeners/actions/proposal-button.js">
<violation number="1" location="slack-app/listeners/actions/proposal-button.js:64">
P3: Reordering kind-vs-URL to fix the cancel-with-resource case is correct, but it also silently drops the server-built "follow it here" link for the other known kinds. Before this change the URL was consulted before the switch, so a `generate`, `schedule`, or `external` outcome that carries a resource URL surfaced the link; now the URL is used only for `start` and unknown kinds, so those outcomes lose the deep link even when the server supplied one. Two concrete knock-ons: the JSDoc just above this function still states "A URL wins over the copy when there is one," which no longer matches the implementation, and the characterization-test comment "A URL outranks the copy, whatever the kind" encodes the old semantics. If URL-scoped-to-start is intended, update the doc comment (and re-check whether generate/schedule/external ops are expected to return resource URLs whose links should not regress); otherwise restore the URL handling for those kinds.</violation>
</file>
<file name="mcpjam-inspector/server/routes/v1/__tests__/agent-op-registry.test.ts">
<violation number="1" location="mcpjam-inspector/server/routes/v1/__tests__/agent-op-registry.test.ts:562">
P3: The updated base prompt line now lists 'running a third-party tool' alongside quota-spending actions and describes schedule changes generically as spend. If call_server_tool is actually gated for external side effects rather than quota consumption, and only enabling a schedule (not disabling one) creates recurring spend, this wording could mislead the agent/user about what each action actually costs. Consider splitting the description so call_server_tool is described as an external action requiring approval, and only 'enabling' a schedule is called out as spend.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| case 'external': | ||
| return `:white_check_mark: Approved by <@${userId}> — the tool ran.`; | ||
| case 'start': | ||
| if (url) return `:white_check_mark: Approved by <@${userId}> — <${url}|follow it here>.`; |
There was a problem hiding this comment.
P3: Reordering kind-vs-URL to fix the cancel-with-resource case is correct, but it also silently drops the server-built "follow it here" link for the other known kinds. Before this change the URL was consulted before the switch, so a generate, schedule, or external outcome that carries a resource URL surfaced the link; now the URL is used only for start and unknown kinds, so those outcomes lose the deep link even when the server supplied one. Two concrete knock-ons: the JSDoc just above this function still states "A URL wins over the copy when there is one," which no longer matches the implementation, and the characterization-test comment "A URL outranks the copy, whatever the kind" encodes the old semantics. If URL-scoped-to-start is intended, update the doc comment (and re-check whether generate/schedule/external ops are expected to return resource URLs whose links should not regress); otherwise restore the URL handling for those kinds.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At slack-app/listeners/actions/proposal-button.js, line 64:
<comment>Reordering kind-vs-URL to fix the cancel-with-resource case is correct, but it also silently drops the server-built "follow it here" link for the other known kinds. Before this change the URL was consulted before the switch, so a `generate`, `schedule`, or `external` outcome that carries a resource URL surfaced the link; now the URL is used only for `start` and unknown kinds, so those outcomes lose the deep link even when the server supplied one. Two concrete knock-ons: the JSDoc just above this function still states "A URL wins over the copy when there is one," which no longer matches the implementation, and the characterization-test comment "A URL outranks the copy, whatever the kind" encodes the old semantics. If URL-scoped-to-start is intended, update the doc comment (and re-check whether generate/schedule/external ops are expected to return resource URLs whose links should not regress); otherwise restore the URL handling for those kinds.</comment>
<file context>
@@ -60,11 +61,15 @@ export function announcementFor(outcome, userId) {
case 'external':
return `:white_check_mark: Approved by <@${userId}> — the tool ran.`;
case 'start':
+ if (url) return `:white_check_mark: Approved by <@${userId}> — <${url}|follow it here>.`;
return `:white_check_mark: Approved by <@${userId}>, and it's away.`;
default:
</file context>
| "- Author cases as `steps` arrays; prefer a `prompt` step plus `toolCalledWith`-style assertions on the tools the conversation showed. Set `expectedOutput` when the user stated one.", | ||
| "- When creating a suite, set the suite `model` explicitly to `anthropic/claude-haiku-4.5` unless the user asks for a different model.", | ||
| "- Some actions SPEND the user's quota or credits (running a suite or a case, generating cases, cancelling a run). Calling those tools does NOT perform them: it PROPOSES the action and returns an approval id, and a person must click to confirm. Say that you've proposed it and what it will do. NEVER say it has started, is running, or has been cancelled.", | ||
| "- Some actions SPEND the user's quota or credits (running a suite or a case, generating cases, cancelling a run, setting a schedule, running a third-party tool). Calling those tools does NOT perform them: it PROPOSES the action and returns an approval id, and a person must click to confirm. Say that you've proposed it and what it will do. NEVER say it has started, is running, or has been cancelled.", |
There was a problem hiding this comment.
P3: The updated base prompt line now lists 'running a third-party tool' alongside quota-spending actions and describes schedule changes generically as spend. If call_server_tool is actually gated for external side effects rather than quota consumption, and only enabling a schedule (not disabling one) creates recurring spend, this wording could mislead the agent/user about what each action actually costs. Consider splitting the description so call_server_tool is described as an external action requiring approval, and only 'enabling' a schedule is called out as spend.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mcpjam-inspector/server/routes/v1/__tests__/agent-op-registry.test.ts, line 562:
<comment>The updated base prompt line now lists 'running a third-party tool' alongside quota-spending actions and describes schedule changes generically as spend. If call_server_tool is actually gated for external side effects rather than quota consumption, and only enabling a schedule (not disabling one) creates recurring spend, this wording could mislead the agent/user about what each action actually costs. Consider splitting the description so call_server_tool is described as an external action requiring approval, and only 'enabling' a schedule is called out as spend.</comment>
<file context>
@@ -559,7 +559,7 @@ const PROMPT_BEFORE_REGISTRY = [
"- Author cases as `steps` arrays; prefer a `prompt` step plus `toolCalledWith`-style assertions on the tools the conversation showed. Set `expectedOutput` when the user stated one.",
"- When creating a suite, set the suite `model` explicitly to `anthropic/claude-haiku-4.5` unless the user asks for a different model.",
- "- Some actions SPEND the user's quota or credits (running a suite or a case, generating cases, cancelling a run). Calling those tools does NOT perform them: it PROPOSES the action and returns an approval id, and a person must click to confirm. Say that you've proposed it and what it will do. NEVER say it has started, is running, or has been cancelled.",
+ "- Some actions SPEND the user's quota or credits (running a suite or a case, generating cases, cancelling a run, setting a schedule, running a third-party tool). Calling those tools does NOT perform them: it PROPOSES the action and returns an approval id, and a person must click to confirm. Say that you've proposed it and what it will do. NEVER say it has started, is running, or has been cancelled.",
"- If a proposal tool is not available to you, you cannot run anything at all. Say so plainly and report the ids the user needs — do not imply you started something.",
"- Always report the ids of anything you created.",
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three latent issues from the #3696 review, batched:
announcementForordering (slack-app/listeners/actions/proposal-button.js): the resource-URL check ran before thekindswitch, so a futurecancel-kind op that also returns a resource would announce "Approved - follow it here" instead of "Cancelled". Fixed by checkingkindfirst; URL is now used only inside thestartbranch and as a fallback for unrecognised kinds from newer servers.UTF-16-safe button label cap (
slack-app/listeners/views/proposal-builder.js):.slice(0, 75)sliced on UTF-16 code units, leaving an unpaired surrogate when an emoji sits at the boundary and causing Slack to reject the whole message. Replaced withcapChars(), the code-point-safe helper already in the same file.Base prompt spend enumeration (
mcpjam-inspector/server/routes/v1/agent.ts): the frozen list of gated ops omitted the two added after original authoring:setEvalSuiteSchedule("setting a schedule") andcallServerTool("running a third-party tool"). Updated the literal and the snapshot-pinned copy inagent-op-registry.test.ts.Test plan
announcementForunit tests still pass (no cancel+url test existed; bug was latent)agent-op-registry.test.tssnapshot updated to match new prompt textFixes #3705
🤖 Generated with Claude Code
Summary by cubic
Fixes cancel announcement ordering, makes Slack button labels code‑point safe, and expands the base prompt spend list to include schedules and third‑party tools; prevents wrong “Approved” messages, avoids Slack 400s on emoji boundaries, and aligns guidance with all gated actions. Fixes #3705.
announcementFor: checkkindbefore resource URLs; cancel proposals now announce “Cancelled”,startstill links to the run, and unknown kinds fall back to the URL.proposal-builder: usecapCharsinstead of.sliceto cap labels on code points; prevents surrogate splits that cause Slack rejections.agentprompt: add “setting a schedule” and “running a third‑party tool” to the spend list; updated test snapshot to match.Written for commit e90a923. Summary will update on new commits.