Skip to content

feat: add temporary ID support to add_comment tool#19737

Merged
pelikhan merged 10 commits intomainfrom
copilot/review-add-comment-implementation
Mar 6, 2026
Merged

feat: add temporary ID support to add_comment tool#19737
pelikhan merged 10 commits intomainfrom
copilot/review-add-comment-implementation

Conversation

Copy link
Contributor

Copilot AI commented Mar 5, 2026

  • Investigate issue [safeoutputs] Improve add_comment tool description: inaccurate "silently discarded" behavior and missing temporary_id guidance #19734 and understand current add_comment temporary ID support
  • Update item_number schema type to accept both numbers and strings (temporary IDs)
  • Add temporary_id field to add_comment tool schema
  • Fix "silently discarded" description inaccuracy → "fail with an error"
  • Update safe_outputs_handlers.cjs to auto-generate and return temporary_id for add_comment
  • Use generateTemporaryId and getOrGenerateTemporaryId helpers from temporary_id.cjs in both createProjectHandler and addCommentHandler (replaces inline crypto.randomBytes)
  • Fix test patterns to match mixed-case alphanumeric IDs from generateTemporaryId()
  • Align all temporary_id schema patterns from {3,8} to {3,12} to match isTemporaryId() validation in temporary_id.cjs
  • Remove unused generateTemporaryId import from safe_outputs_handlers.cjs
  • Guard args access in addCommentHandler to handle missing arguments gracefully
  • Add temporary_id.cjs to SAFE_OUTPUTS_FILES in setup.sh (fixes CI test failure in setup_sh_file_lists.test.cjs)
  • All tests pass (61 targeted + setup file list tests)
Original prompt

Review add_comment implementation to make sure it supports temporary ids as parent and also returns a temporary id as a result.


🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.



✨ PR Review Safe Output Test - Run 22748465797

💥 [THE END] — Illustrated by Smoke Claude ·

@pelikhan
Copy link
Contributor

pelikhan commented Mar 5, 2026

@copilot

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot AI and others added 2 commits March 5, 2026 17:52
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
- Update item_number schema to accept strings (temporary IDs) in addition to numbers
- Add temporary_id field to add_comment tool schema
- Fix 'silently discarded' description inaccuracy -> 'fail with an error'
- Update addCommentHandler to auto-generate and return temporary_id
- Fix randomBytes(6) -> randomBytes(4) to stay within 8-char schema limit
- Add tests for new temporary_id behavior in handlers and MCP constraints

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Mar 5, 2026

@copilot

Addressed issue #19734 in commit 8e8b62b:

  1. item_number now accepts temporary IDs — changed type from "number" to ["number", "string"] in both tool schema files, so agents can pass aw_abc123 from a previously created issue as the comment target

  2. add_comment returns a temporary_id — updated addCommentHandler in safe_outputs_handlers.cjs to auto-generate a temporary_id (using randomBytes(4).toString("hex") to stay within the 8-char schema limit) and return it in the MCP response so agents can reference the comment later

  3. Fixed description inaccuracy — updated "silently discarded" wording to "will fail with an error" to accurately reflect the hard failure behavior

Also fixed a pre-existing bug in createProjectHandler where randomBytes(6) produced 12-char IDs that exceeded the schema's 8-char maximum.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • https://api.github.com/repos/github/gh-aw/contents/.github%2Fworkflows%2Faudit-workflows.md
    • Triggering command: /opt/hostedtoolcache/node/24.14.0/x64/bin/node /opt/hostedtoolcache/node/24.14.0/x64/bin/node --conditions node --conditions development --experimental-import-meta-resolve --require /home/REDACTED/work/gh-aw/gh-aw/actions/setup/js/node_modules/vitest/suppress-warnings.cjs /home/REDACTED/work/gh-aw/gh-aw/actions/setup/js/node_modules/vitest/dist/workers/forks.js main modules/@npmcli/HEAD git add . lure test commit ache/node/24.14.0/x64/lib/node_modules/npm/node_modules/@npmcli/run-script/lib/node-gyp-bin/git -b main tions/setup/nodeHEAD git (http block)

If you need me to access, download, or install something from one of these locations, you can either:

Copilot AI changed the title [WIP] Review add_comment implementation for temporary ID support feat: add temporary ID support to add_comment tool Mar 5, 2026
…dCommentHandler

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
@pelikhan pelikhan marked this pull request as ready for review March 6, 2026 03:00
Copilot AI review requested due to automatic review settings March 6, 2026 03:00
},
"temporary_id": {
"type": "string",
"pattern": "^aw_[A-Za-z0-9]{3,8}$",
Copy link
Contributor

Choose a reason for hiding this comment

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

@copilot review if pattern is compatible with other temporary_id patterns. see how other safe outputs handle it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in commit 578bca7. All temporary_id patterns across both schema files (create_issue, add_comment, create_project, update_project) now use {3,12}, consistent with the isTemporaryId() regex in temporary_id.cjs. Descriptions updated to say "3 to 12 alphanumeric characters" accordingly.

…Id in temporary_id.cjs

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds first-class temporary ID support to the add_comment safe output tool so agents can (a) target newly-created items via temporary IDs and (b) receive a temporary_id back from the MCP handler for later reference.

Changes:

  • Update add_comment.item_number schema to accept number | string (temporary IDs) and clarify failure behavior when auto-targeting is unavailable.
  • Add temporary_id to the add_comment tool schema and implement auto-generation + return of temporary_id in addCommentHandler.
  • Update/extend tests and regenerate workflow lock schemas to reflect the new schema shape.

Reviewed changes

Copilot reviewed 44 out of 44 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
pkg/workflow/js/safe_outputs_tools.json Expands add_comment schema (string item_number, new temporary_id, updated description).
actions/setup/js/safe_outputs_tools.json Mirrors add_comment schema updates for the action runtime.
actions/setup/js/safe_outputs_handlers.cjs Implements addCommentHandler temporary ID generation/validation and returns it in the MCP response.
actions/setup/js/safe_outputs_mcp_add_comment_constraints.test.cjs Adds constraint tests asserting temporary_id is persisted and returned.
actions/setup/js/safe_outputs_handlers.test.cjs Adds unit tests for addCommentHandler response structure and NDJSON recording.
.github/workflows/workflow-health-manager.lock.yml Regenerated tool schema snapshot to reflect new add_comment fields/types.
.github/workflows/unbloat-docs.lock.yml Regenerated tool schema snapshot to reflect new add_comment fields/types.
.github/workflows/technical-doc-writer.lock.yml Regenerated tool schema snapshot to reflect new add_comment fields/types.
.github/workflows/sub-issue-closer.lock.yml Regenerated tool schema snapshot to reflect new add_comment fields/types.
.github/workflows/smoke-workflow-call.lock.yml Regenerated tool schema snapshot to reflect new add_comment fields/types.
.github/workflows/smoke-update-cross-repo-pr.lock.yml Regenerated tool schema snapshot to reflect new add_comment fields/types.
.github/workflows/smoke-test-tools.lock.yml Regenerated tool schema snapshot to reflect new add_comment fields/types.
.github/workflows/smoke-temporary-id.lock.yml Regenerated tool schema snapshot to reflect new add_comment fields/types.
.github/workflows/smoke-project.lock.yml Regenerated tool schema snapshot to reflect new add_comment fields/types.
.github/workflows/smoke-multi-pr.lock.yml Regenerated tool schema snapshot to reflect new add_comment fields/types.
.github/workflows/smoke-gemini.lock.yml Regenerated tool schema snapshot to reflect new add_comment fields/types.
.github/workflows/smoke-create-cross-repo-pr.lock.yml Regenerated tool schema snapshot to reflect new add_comment fields/types.
.github/workflows/smoke-copilot.lock.yml Regenerated tool schema snapshot to reflect new add_comment fields/types.
.github/workflows/smoke-copilot-arm.lock.yml Regenerated tool schema snapshot to reflect new add_comment fields/types.
.github/workflows/smoke-codex.lock.yml Regenerated tool schema snapshot to reflect new add_comment fields/types.
.github/workflows/smoke-claude.lock.yml Regenerated tool schema snapshot to reflect new add_comment fields/types.
.github/workflows/smoke-agent.lock.yml Regenerated tool schema snapshot to reflect new add_comment fields/types.
.github/workflows/security-alert-burndown.campaign.g.lock.yml Regenerated tool schema snapshot to reflect new add_comment fields/types.
.github/workflows/scout.lock.yml Regenerated tool schema snapshot to reflect new add_comment fields/types.
.github/workflows/refiner.lock.yml Regenerated tool schema snapshot to reflect new add_comment fields/types.
.github/workflows/q.lock.yml Regenerated tool schema snapshot to reflect new add_comment fields/types.
.github/workflows/pr-triage-agent.lock.yml Regenerated tool schema snapshot to reflect new add_comment fields/types.
.github/workflows/poem-bot.lock.yml Regenerated tool schema snapshot to reflect new add_comment fields/types.
.github/workflows/pdf-summary.lock.yml Regenerated tool schema snapshot to reflect new add_comment fields/types.
.github/workflows/issue-triage-agent.lock.yml Regenerated tool schema snapshot to reflect new add_comment fields/types.
.github/workflows/issue-monster.lock.yml Regenerated tool schema snapshot to reflect new add_comment fields/types.
.github/workflows/draft-pr-cleanup.lock.yml Regenerated tool schema snapshot to reflect new add_comment fields/types.
.github/workflows/discussion-task-miner.lock.yml Regenerated tool schema snapshot to reflect new add_comment fields/types.
.github/workflows/dev-hawk.lock.yml Regenerated tool schema snapshot to reflect new add_comment fields/types.
.github/workflows/daily-fact.lock.yml Regenerated tool schema snapshot to reflect new add_comment fields/types.
.github/workflows/daily-cli-performance.lock.yml Regenerated tool schema snapshot to reflect new add_comment fields/types.
.github/workflows/daily-assign-issue-to-user.lock.yml Regenerated tool schema snapshot to reflect new add_comment fields/types.
.github/workflows/craft.lock.yml Regenerated tool schema snapshot to reflect new add_comment fields/types.
.github/workflows/contribution-check.lock.yml Regenerated tool schema snapshot to reflect new add_comment fields/types.
.github/workflows/cloclo.lock.yml Regenerated tool schema snapshot to reflect new add_comment fields/types.
.github/workflows/ci-doctor.lock.yml Regenerated tool schema snapshot to reflect new add_comment fields/types.
.github/workflows/brave.lock.yml Regenerated tool schema snapshot to reflect new add_comment fields/types.
.github/workflows/archie.lock.yml Regenerated tool schema snapshot to reflect new add_comment fields/types.
.github/workflows/agent-performance-analyzer.lock.yml Regenerated tool schema snapshot to reflect new add_comment fields/types.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Comment on lines +235 to +238
"temporary_id": {
"type": "string",
"pattern": "^aw_[A-Za-z0-9]{3,12}$",
"description": "Unique temporary identifier for this comment. Format: 'aw_' followed by 3 to 12 alphanumeric characters (e.g., 'aw_abc1', 'aw_Test123'). Auto-generated if not provided. The temporary ID is returned in the tool response so you can reference this comment later."
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

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

Same issue as in pkg/workflow/js/safe_outputs_tools.json: temporary_id pattern/description restricts to 3–8 chars, but the helper validation supports 3–12 and tests expect 3–12. This inconsistency can reject otherwise-valid temporary IDs. Align this schema to 3–12 (and ideally mention lowercase normalization in the description).

Copilot uses AI. Check for mistakes.
const { ERR_CONFIG, ERR_SYSTEM, ERR_VALIDATION } = require("./error_codes.cjs");
const { findRepoCheckout } = require("./find_repo_checkout.cjs");
const { resolveTargetRepoConfig, resolveAndValidateRepo } = require("./repo_helpers.cjs");
const { generateTemporaryId, getOrGenerateTemporaryId } = require("./temporary_id.cjs");
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

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

generateTemporaryId is imported but never used in this file. Either remove it from the require() or use it directly; leaving unused imports makes future refactors noisier and can trigger lint failures if/when linting is enabled.

Copilot uses AI. Check for mistakes.
Comment on lines 515 to 516
const addCommentHandler = args => {
// Validate comment constraints before appending to safe outputs
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

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

addCommentHandler assumes args is always defined (it later reads args.body). A malformed tool call with missing arguments can throw a TypeError and surface a confusing error message. Consider guarding access (e.g., treat missing args as {} and explicitly validate body) so failures consistently return a clear JSON-RPC -32602 "Invalid params" error.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Copilot AI commented Mar 6, 2026

@copilot

All items from issue #19734 are addressed across commits 8e8b62b, da66567, and 578bca7:

  • item_number accepts temporary IDs (string type added)
  • add_comment auto-generates and returns a temporary_id
  • Uses getOrGenerateTemporaryId helper from temporary_id.cjs
  • Pattern aligned to {3,12} to match isTemporaryId() validation

@pelikhan
Copy link
Contributor

pelikhan commented Mar 6, 2026

@copilot apply changes based on the comments in this thread

…mmentHandler

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Mar 6, 2026

@copilot apply changes based on the comments in this thread

Applied all changes from the review thread in commit bd5d4c9:

  • Removed unused generateTemporaryId import (only getOrGenerateTemporaryId is used)
  • Guarded args access in addCommentHandler — changed args.body to (args && args.body) || "" so missing/null args return a clear validation error instead of a TypeError

The pattern mismatch items (3–8 vs 3–12) were already fixed in 578bca7.

@pelikhan
Copy link
Contributor

pelikhan commented Mar 6, 2026

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Mar 6, 2026

@copilot fix tests https://github.com/github/gh-aw/actions/runs/22747874691/job/65975640309?pr=19737

Fixed in commit 5f08b4f. temporary_id.cjs was missing from SAFE_OUTPUTS_FILES in setup.sh — added it so the setup script copies the file alongside the other safe-outputs dependencies.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 6, 2026

📰 BREAKING: Smoke Copilot is now investigating this pull request. Sources say the story is developing...

@github-actions
Copy link
Contributor

github-actions bot commented Mar 6, 2026

🌑 The shadows whisper... Smoke Codex failed. The oracle requires further meditation...

@github-actions
Copy link
Contributor

github-actions bot commented Mar 6, 2026

🎬 THE ENDSmoke Claude MISSION: ACCOMPLISHED! The hero saves the day! ✨

@github-actions
Copy link
Contributor

github-actions bot commented Mar 6, 2026

✅ All tools validated successfully! Agent Container Smoke Test confirms agent container is ready.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 6, 2026

Agent Container Tool Check

Tool Status Version
bash 5.2.21
sh available
git 2.53.0
jq 1.7
yq v4.52.4
curl 8.5.0
gh 2.87.3
node v20.20.0
python3 3.12.3
go 1.24.13
java 21.0.10 (Temurin)
dotnet 10.0.102

Result: 12/12 tools available ✅

Overall Status: PASS

🔧 Tool validation by Agent Container Smoke Test ·

@github-actions
Copy link
Contributor

github-actions bot commented Mar 6, 2026

Smoke Test: Copilot - 22748465795

Test Status
GitHub MCP
Safe Inputs GH CLI
Serena MCP ❌ Not available
Playwright
Web Fetch
File Writing
Bash Tool
Discussion Interaction
Build gh-aw
Discussion Creation
Workflow Dispatch
PR Review

Overall: PASS (Serena MCP not configured in this environment)

@pelikhan — run §22748465795

📰 BREAKING: Report filed by Smoke Copilot ·

@github-actions github-actions bot removed the smoke label Mar 6, 2026
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This PR extends temporary_id support to add_comment and broadens the pattern from {3,8} to {3,12} characters. The changes enable more flexible cross-referencing between safe-output operations. The refactoring to use getOrGenerateTemporaryId centralizes validation logic cleanly. Looks good!

📰 BREAKING: Report filed by Smoke Copilot

};
};

return {
Copy link
Contributor

Choose a reason for hiding this comment

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

The addCommentHandler now returns temporary_id to the agent - great improvement for enabling cross-reference workflows where agents need to reply to or update a previously posted comment.

entry.temporary_id = tempIdResult.temporaryId;
server.debug(`temporary_id for create_project: ${entry.temporary_id}`);

// Append to safe outputs
Copy link
Contributor

Choose a reason for hiding this comment

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

Using getOrGenerateTemporaryId helper is a good refactor — it centralizes validation logic and ensures consistent error codes (-32602) across all handlers that use temporary IDs.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 6, 2026

📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤

@github-actions
Copy link
Contributor

github-actions bot commented Mar 6, 2026

Smoke Test Results — Run §22748465797

Core tests #1-10:
✅ GitHub MCP | ✅ GH CLI | ✅ Serena MCP | ✅ Make Build | ✅ Playwright
✅ Tavily Search | ✅ File Write | ✅ Bash Tool | ✅ Discussion | ✅ Agentic Workflows

PR review tests #11-17:
✅ Update PR | ✅ Review Comments (×2) | ✅ Submit Review | ✅ Resolve Thread | ✅ Add Reviewer | ✅ Push Branch | ⚠️ Close PR (skipped)

Overall: PARTIAL (1 skipped, 16/17 passed)

💥 [THE END] — Illustrated by Smoke Claude ·

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

💥 Automated smoke test review - all systems nominal! This review was generated by smoke test run 22748465797 to validate the PR review submission workflow.

💥 [THE END] — Illustrated by Smoke Claude

@pelikhan pelikhan merged commit 5ffb364 into main Mar 6, 2026
163 of 165 checks passed
@pelikhan pelikhan deleted the copilot/review-add-comment-implementation branch March 6, 2026 08:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants