Enforce add_comment constraints at MCP server invocation#16011
Enforce add_comment constraints at MCP server invocation#16011
Conversation
Implement MCE1 (Early Validation) from Safe Outputs Specification: - Add addCommentHandler in safe_outputs_handlers.cjs - Validate comment constraints before recording to NDJSON - Return JSON-RPC error -32602 with actionable messages - Register handler in safe_outputs_tools_loader.cjs - Add comprehensive test suite with 16 test cases Per Requirement MCE4, dual enforcement remains: - MCP server validates during tool invocation (Phase 4) - Safe output processor validates before API calls (Phase 6) Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Document implementation of MCE1 (Early Validation): - Increment version to 1.12.0 - Add changelog entry describing implementation - Note dual enforcement now operational - Document JSON-RPC error format compliance Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧪 Smoke Project is now testing project operations... |
|
🌑 The shadows whisper... Smoke Codex failed. The oracle requires further meditation... |
|
💫 TO BE CONTINUED... Smoke Claude failed! Our hero faces unexpected challenges... |
|
📰 BREAKING: Smoke Copilot is now investigating this pull request. Sources say the story is developing... |
|
❌ Smoke Copilot SDK failed. Please review the logs for details. |
|
🧪 Smoke Temporary ID is now testing temporary ID functionality... |
|
❌ Smoke Project encountered failures. Check the logs for details. |
|
❌ Smoke Temporary ID encountered failures. Check the logs for details. |
|
📰 DEVELOPING STORY: Smoke Copilot reports failed. Our correspondents are investigating the incident... |
| const { getCurrentBranch } = require("./get_current_branch.cjs"); | ||
| const { getBaseBranch } = require("./get_base_branch.cjs"); | ||
| const { generateGitPatch } = require("./generate_git_patch.cjs"); | ||
| const { enforceCommentLimits } = require("./add_comment.cjs"); |
There was a problem hiding this comment.
@copilot refactor limit checkers in separate file.
Update setup.sh to add limit helper to safe outputs file list
There was a problem hiding this comment.
Refactored limit validation into comment_limit_helpers.cjs module and updated setup.sh to include it in safe outputs file list (commit 7a311a7).
The new module exports:
MAX_COMMENT_LENGTH,MAX_MENTIONS,MAX_LINKSconstantsenforceCommentLimits()function
Both add_comment.cjs and safe_outputs_handlers.cjs now import from this shared module, ensuring constraint consistency per MCE5.
Extract enforceCommentLimits and constants into comment_limit_helpers.cjs: - Create new comment_limit_helpers.cjs module with MAX_COMMENT_LENGTH, MAX_MENTIONS, MAX_LINKS constants - Move enforceCommentLimits function to new module - Update add_comment.cjs to import from helper module - Update safe_outputs_handlers.cjs to import from helper module - Add comment_limit_helpers.cjs to SAFE_OUTPUTS_FILES in setup.sh - Add comprehensive test suite for helper module (15 tests) This refactoring improves code organization by centralizing comment validation logic in a single reusable module, making it easier to maintain constraint consistency across MCP server and safe output processor per MCE5. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✨ The prophecy is fulfilled... Smoke Codex has completed its mystical journey. The stars align. 🌟 |
|
🎬 THE END — Smoke Claude MISSION: ACCOMPLISHED! The hero saves the day! ✨ |
|
❌ Smoke Copilot SDK failed. Please review the logs for details. |
|
🧪 Smoke Temporary ID is now testing temporary ID functionality... |
|
📰 BREAKING: Smoke Copilot is now investigating this pull request. Sources say the story is developing... |
|
🧪 Smoke Project is now testing project operations... |
|
✅ Smoke Temporary ID completed successfully. Temporary ID validation passed. |
Agent Container Tool Check
Result: 12/12 tools available ✅ All required development tools are present and accessible in the agent container.
|
|
✅ Smoke Project completed successfully. All project operations validated. |
|
PRs: #16014 "[docs] Update documentation for logs command and skip-bots/skip-roles features"; #16010 "Update ai-moderator workflow to use codex engine"
|
Smoke Test ResultsPRs Reviewed:
✅ GitHub MCP | ✅ GH CLI | ✅ Playwright Status: PASS
|
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
💥 Automated smoke test review - all systems nominal! This refactoring successfully consolidates comment validation logic into a reusable module, improving code quality and maintainability.
💥 [THE END] — Illustrated by Smoke Claude for issue #16011
There was a problem hiding this comment.
Pull request overview
This PR implements Safe Outputs Specification requirement MCE1 (Early Validation) by enforcing add_comment tool constraints during MCP server invocation (Phase 4) rather than only during safe output processing (Phase 6). This allows the LLM to receive immediate feedback when constraints are violated and make corrections before operations are recorded.
Changes:
- Created a new
comment_limit_helpers.cjsmodule to centralize validation logic (constants andenforceCommentLimitsfunction) for reuse across MCP server and safe output processor - Added
addCommentHandlerto MCP server that validates constraints before recording to NDJSON and returns JSON-RPC errors (-32602) with actionable messages - Refactored
add_comment.cjsto import constants and validation function from the shared helper module - Added comprehensive test suites for both the helper module (15 tests) and MCP constraint enforcement (16 tests)
- Updated Safe Outputs Specification to version 1.12.0 with changelog documenting the implementation
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/comment_limit_helpers.cjs |
New shared module defining MAX_COMMENT_LENGTH (65536), MAX_MENTIONS (10), MAX_LINKS (50) constants and enforceCommentLimits validation function |
actions/setup/js/comment_limit_helpers.test.cjs |
Test suite for helper module with 15 tests covering all constraints and edge cases |
actions/setup/js/safe_outputs_handlers.cjs |
Added addCommentHandler that validates constraints and throws JSON-RPC errors before recording operations |
actions/setup/js/safe_outputs_tools_loader.cjs |
Registered addCommentHandler in handlerMap to attach it to add_comment tool |
actions/setup/js/safe_outputs_mcp_add_comment_constraints.test.cjs |
Comprehensive MCP server test suite with 16 tests validating constraint enforcement, error format, and dual enforcement |
actions/setup/js/add_comment.cjs |
Refactored to import constants and enforceCommentLimits from shared helper module, maintaining dual enforcement pattern |
actions/setup/setup.sh |
Added comment_limit_helpers.cjs to SAFE_OUTPUTS_FILES array for deployment |
docs/src/content/docs/reference/safe-outputs-specification.md |
Incremented version to 1.12.0 and added changelog entry documenting MCE1 implementation |
.changeset/patch-enforce-add-comment-mcp.md |
Added changeset describing the patch-level change |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,99 @@ | |||
| // @ts-check | |||
| import { describe, it, expect } from "vitest"; | |||
There was a problem hiding this comment.
The test file uses beforeEach on line 8 but it's not imported from vitest on line 2. This will cause a runtime error when the tests are executed. Add beforeEach to the import statement.
| import { describe, it, expect } from "vitest"; | |
| import { describe, it, expect, beforeEach } from "vitest"; |
| * 5. Errors use MCP JSON-RPC error format with code -32602 | ||
| * 6. Error messages are actionable per Requirement MCE3 | ||
| * | ||
| * Related issues: github/gh-aw#<issue_number> |
There was a problem hiding this comment.
The comment contains a placeholder <issue_number> that should be replaced with the actual issue number (15976, based on the PR description mentioning "Fixes #15976").
| * Related issues: github/gh-aw#<issue_number> | |
| * Related issues: github/gh-aw#15976 |
Plan: Add Runtime Checks to Enforce add_comment Limits in MCP Server
Problem
The
add_commenttool enforces limits (max length, mentions, links) only during safe-outputs processing (Phase 6), not during the MCP tool call (Phase 4). This prevents the agent from getting immediate feedback and making corrections.Requirements (from Safe Outputs Specification Section 8.3)
Implementation Summary
add_commentin MCP serverenforceCommentLimitsbefore recording to NDJSONattachHandlersfunctionFiles Modified
actions/setup/js/safe_outputs_handlers.cjs- AddedaddCommentHandleractions/setup/js/safe_outputs_tools_loader.cjs- Registered handleractions/setup/js/safe_outputs_mcp_add_comment_constraints.test.cjs- Added comprehensive MCP testsdocs/src/content/docs/reference/safe-outputs-specification.md- Incremented version to 1.12.0actions/setup/js/comment_limit_helpers.cjs- New helper module with validation logicactions/setup/js/comment_limit_helpers.test.cjs- New test file (15 tests)actions/setup/js/add_comment.cjs- Updated to import from helper moduleactions/setup/setup.sh- Added helper to SAFE_OUTPUTS_FILES listTest Results
Refactoring Benefits
The new
comment_limit_helpers.cjsmodule:Key Design Decisions
enforceCommentLimitsfunction (no duplication)Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.
✨ PR Review Safe Output Test - Run 22046490655
Changeset
✨ PR Review Safe Output Test - Run 22046973195