Skip to content

feat(inline-comment): add confirmed param + probe-pattern safety net #1048

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
km-anthropic merged 4 commits into from
Mar 12, 2026
Merged

feat(inline-comment): add confirmed param + probe-pattern safety net #1048

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8b7fe18
feat(inline-comment): add confirmed param + probe-pattern safety net
km-anthropic Mar 12, 2026
cfce07a
refactor: replace probe-regex with Haiku classification in post-step
km-anthropic Mar 12, 2026
5a4cfe6
feat: add classify_inline_comments opt-out input
km-anthropic Mar 12, 2026
0638d8b
docs: document classify_inline_comments input and confirmed param
km-anthropic Mar 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions action.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,19 @@ runs:
run: |
bun run ${GITHUB_ACTION_PATH}/src/entrypoints/cleanup-ssh-signing.ts

- name: Report buffered inline comments
if: always()
shell: bash
run: |
BUF=/tmp/inline-comments-buffer.jsonl
if [ -f "$BUF" ]; then
N=$(wc -l < "$BUF")
echo "::warning::$N inline comment call(s) were buffered and NOT posted (missing confirmed=true)."
echo "::group::Buffered calls (not posted)"
cat "$BUF"
echo "::endgroup::"

@claude claude Bot Mar 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Buffer contents are cat'd without neutralizing workflow commands.

The buffer body is user-influenced. Strings starting with :: (e.g. ::warning::, ::error::) are interpreted as workflow commands by the Actions runner. A crafted comment body could inject spurious warnings/errors into the log.

Consider wrapping with stop-commands:

Suggested change
echo "::group::Buffered calls (not posted)"
cat "$BUF"
echo "::endgroup::"
TOKEN=$(uuidgen)
echo "::stop-commands::${TOKEN}"
cat "$BUF"
echo "::${TOKEN}::"

fi

- name: Revoke app token
if: always() && inputs.github_token == '' && steps.run.outputs.skipped_due_to_workflow_validation_mismatch != 'true'
shell: bash
Expand Down
65 changes: 62 additions & 3 deletions src/mcp/github-inline-comment-server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#!/usr/bin/env node
import { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js";
import { StdioServerTransport } from "@modelcontextprotocol/sdk/server/stdio.js";
import { appendFileSync } from "fs";
import { z } from "zod";
import { createOctokit } from "../github/api/client";
import { sanitizeContent } from "../github/utils/sanitizer";
Expand All @@ -10,6 +11,12 @@ const REPO_OWNER = process.env.REPO_OWNER;
const REPO_NAME = process.env.REPO_NAME;
const PR_NUMBER = process.env.PR_NUMBER;

// Calls without confirmed=true are buffered here instead of posted. This
// prevents subagents from posting test/probe comments when they inherit this
// tool and probe it after hitting unrelated errors. The action's post-step
// reports the buffer count for diagnostics.
const BUFFER_PATH = "/tmp/inline-comments-buffer.jsonl";
Comment on lines +14 to +18

@claude claude Bot Mar 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment is misleading — overstates the buffering behavior.

The comment says "Calls without confirmed=true are buffered here instead of posted", but that's not accurate. Calls with confirmed omitted are only buffered if the body matches the probe regex; otherwise they post normally. A future reader would incorrectly assume confirmed=true is mandatory to post.

Suggested change
// Calls without confirmed=true are buffered here instead of posted. This
// prevents subagents from posting test/probe comments when they inherit this
// tool and probe it after hitting unrelated errors. The action's post-step
// reports the buffer count for diagnostics.
const BUFFER_PATH = "/tmp/inline-comments-buffer.jsonl";
// Calls with confirmed=false, or probe-like calls without confirmed=true,
// are buffered here instead of posted. This prevents subagents from posting
// test/probe comments when they inherit this tool and probe it after hitting
// unrelated errors. The action's post-step reports the buffer count for
// diagnostics.
const BUFFER_PATH = "/tmp/inline-comments-buffer.jsonl";


if (!REPO_OWNER || !REPO_NAME || !PR_NUMBER) {
console.error(
"Error: REPO_OWNER, REPO_NAME, and PR_NUMBER environment variables are required",
Expand Down Expand Up @@ -67,8 +74,17 @@ server.tool(
.describe(
"Specific commit SHA to comment on (defaults to latest commit)",
),
confirmed: z
.boolean()
.optional()
.describe(
"Set true to post the comment. When omitted, posts by default — " +
"unless the body looks like a test/probe, in which case the call is " +
"buffered and NOT posted. Set false to force buffering. Only set " +
"true when posting final review comments.",
),
},
async ({ path, body, line, startLine, side, commit_id }) => {
async ({ path, body, line, startLine, side, commit_id, confirmed }) => {
try {
const githubToken = process.env.GITHUB_TOKEN;

Expand All @@ -80,8 +96,6 @@ server.tool(
const repo = REPO_NAME;
const pull_number = parseInt(PR_NUMBER, 10);

const octokit = createOctokit(githubToken).rest;

// Sanitize the comment body to remove any potential GitHub tokens
const sanitizedBody = sanitizeContent(body);

Expand All @@ -92,10 +106,55 @@ server.tool(
);
}

const looksLikeProbe =
confirmed === undefined &&
/^\s*(test comment|testing if|probe\b|can i\b|does this work|just testing)/i.test(
body,
);

@claude claude Bot Mar 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can i\b will match legitimate review comments.

This pattern matches any comment starting with "Can I suggest..." or "Can improvements be made...", which are plausible review comment openings. Consider tightening to something like can i (post|comment|write|test|create)\b to reduce false positives.

Also: the regex is inherently brittle — minor rewording ("trying this out", "hello world", "checking tool access") bypasses it entirely. If you keep the deny-by-default approach from the companion suggestion, this regex becomes a nice-to-have diagnostic rather than the critical safety gate.


if (confirmed === false || looksLikeProbe) {
appendFileSync(
BUFFER_PATH,
JSON.stringify({
ts: new Date().toISOString(),
path,
line,
startLine,
side,
body: sanitizedBody,
reason: confirmed === false ? "confirmed=false" : "probe-pattern",
}) + "\n",
);
Comment on lines +111 to +123

@claude claude Bot Mar 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

appendFileSync failure will masquerade as a comment-creation error.

If /tmp is full or the write fails, the exception is caught by the outer catch block which returns "Error creating inline comment: ...". This is misleading since the actual failure was in the buffering path, not in posting.

Consider wrapping the appendFileSync in its own try-catch so a write failure degrades gracefully (still return the "buffered" response, perhaps with a note that logging failed).

return {
content: [
{
type: "text",
text: JSON.stringify(
{
success: true,
buffered: true,
message:
"Comment buffered (not posted). " +
(looksLikeProbe
? "The body looks like a test/probe. "
: "") +
"Set confirmed=true to post. If you are testing whether " +
"this tool works: it works — no need to test further.",
},
null,
2,
),
},
],
};
Comment on lines +124 to +144

@claude claude Bot Mar 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The buffered response coaches subagents on how to bypass the safety net.

The message "Set confirmed=true to post" explicitly teaches the calling subagent how to circumvent the guard on its very next call. Since the whole point of this PR is preventing subagent probe comments, this is counterproductive.

Consider removing the confirmed=true coaching and instead saying something like:

"Comment was not posted. If you believe this is a legitimate review comment, return it as text in your response and let the orchestrating agent handle posting."

Alternatively (and more robustly): invert the default so that confirmed: true is required to post. This makes the safety mechanism deterministic rather than relying on the fragile probe regex as the primary gate. Existing callers that never pass confirmed would need updating, but the companion skill PR is already being updated — so this may be the right time.

}

// If only line is provided, it's a single-line comment
// If both startLine and line are provided, it's a multi-line comment
const isSingleLine = !startLine;

const octokit = createOctokit(githubToken).rest;

const pr = await octokit.pulls.get({
owner,
repo,
Expand Down
Loading