feat: require approval for gh issue create#152
Conversation
Co-authored-by: Son Dao <son.dao@katalon.com>
There was a problem hiding this comment.
Pull request overview
Routes gh issue create through the existing Slack-button human approval path instead of executing immediately. Effective args (with Thor footer and trigger-user assignee already applied) are stored in a new "gh" approval-store namespace, the /exec/mcp resolve resolver gains a local executor for ghIssueCreate, and the created-issue correlation alias is only registered after the approved gh command succeeds. Fail-closed when there's no Thor session, anchor, or Slack thread.
Changes:
- Add
ghIssueCreateapproval tool type, schema, and Slack presentation in@thor/common, plus a"gh"approval store namespace andexecuteGhIssueCreateApprovalexecutor in remote-cli's mcp-handler. - Branch
/exec/gh issue createintorequestGhIssueCreateApprovaland extract issue-URL alias helpers into a newgh-issue-alias.ts. - Update E2E, unit tests, skill docs, and add a plan doc to reflect the new approval-required behavior.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/common/src/approval-events.ts | Adds ghIssueCreate tool name, schema, discriminated union member, and no-op disclaimer injection. |
| packages/common/src/approval-presentation.ts | Adds Slack presentation builder and shellQuote helper for ghIssueCreate. |
| packages/common/src/index.ts | Re-exports GhIssueCreateApprovalArgsSchema. |
| packages/common/src/proxies.ts | Excludes ghIssueCreate from the MCP-approved-tool inventory consistency check. |
| packages/common/src/proxies.test.ts | Updates registry test to skip ghIssueCreate. |
| packages/remote-cli/src/mcp-handler.ts | Adds "gh" approval store, display-arg parser, executeGhIssueCreateApproval, and requestGhIssueCreateApproval. |
| packages/remote-cli/src/index.ts | Routes gh issue create to approval; moves alias helpers to a new module. |
| packages/remote-cli/src/gh-issue-alias.ts | New module exporting registerGitCorrelationAlias, parseCreatedIssueCorrelationKey, and registerCreatedIssueCorrelationAlias. |
| packages/remote-cli/src/gh-disclaimer.test.ts | Updates issue-create tests to expect approval flow and alias registration only after approval resolve. |
| scripts/test-e2e.sh | Updates attribution E2E to expect fail-closed and exec_gh_pending_approval log markers. |
| docker/opencode/config/skills/using-gh/SKILL.md | Documents that issue creation requires human approval. |
| docs/plan/2026052501_gh-issue-create-approval.md | New design/plan document. |
Comments suppressed due to low confidence (1)
packages/remote-cli/src/index.ts:663
- After this diff, validated
gh issue createrequests are always routed into the approval branch above (the only wayeffectiveArgs[0] === "issue" && effectiveArgs[1] === "create"can fall through is ifisGhHelpRequest(effectiveArgs)is true, i.e.--help/-h, which never produces an issue URL). The post-exec alias registration on lines 660–662 is now dead code for issue creation —registerCreatedIssueCorrelationAliasis only invoked fromexecuteGhIssueCreateApprovalin mcp-handler.ts. Consider removing this branch to avoid future confusion.
if ((result.exitCode ?? 0) === 0) {
if (effectiveArgs[0] === "issue" && effectiveArgs[1] === "create") {
registerCreatedIssueCorrelationAlias(ids.sessionId, cwd, result.stdout);
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
withGhAttribution previously appended --assignee even when the args were a help invocation, which made the post-attribution isGhHelpRequest check miss and routed `gh issue create --help` into the human approval flow. Bail out of attribution as soon as we detect a help request so the help path stays read-only regardless of attribution order.
When an approved gh issue create exited nonzero, remote-cli returned the raw gh stderr to the gateway. The gateway's isResolvedApprovalExecutionFailure gate only forwards bodies whose stderr matches the MCP-shaped "Error calling \"...\"" pattern, so raw gh failures were swallowed: the Slack card flipped to "resolution failed" but no approval-outcome event was enqueued, leaving the agent unaware that its approved write had failed. Prefix the failure stderr with `Error calling "gh issue create": ...` so the gh approval path matches the existing MCP attempted-side-effect contract; the gateway now forwards the outcome and re-enters the runner with the same "do not replay, choose a safe recovery action" guidance used for MCP failures.
Approval resolution previously had two parallel execution paths — the MCP one routed through executeUpstreamCall, the gh one through executeGhIssueCreateApproval — sharing only the approval store. The split leaked: the gh path silently dropped audit-log writes on failure, and required an MCP-shaped "Error calling X" stderr prefix so the gateway could recognize it as an attempted side-effect failure. Introduce an ApprovalExecutor interface with two implementations (createMcpApprovalExecutor, createGhIssueCreateApprovalExecutor) that each return a structured ApprovalExecutionOutcome carrying a sideEffectAttempted flag. resolveApprovalActionOnce now owns the audit-log write, store transitions, and wire shape on every branch; executors only know how to do the thing and post-success bookkeeping (alias registration). Wire ExecResult.sideEffectAttempted into the gateway's resolveApproval gate, replacing the stderr regex that previously did double duty as the forwarding gate and a sanitized summary extractor. Delete extractApprovalFailureCategory and isResolvedApprovalExecutionFailure. The Slack-card privacy boundary now surfaces only JSON-structured fields from stdout — raw stderr never reaches the card, and the regex no longer pretends to sanitize. Direct (non-approval) MCP calls keep their own wrapper (executeDirectMcpCall) since they still want the input-schema hint appended to stderr on agent-visible errors.
The approval-executor refactor stopped passing the raw MCP CallToolResult to writeToolCallLog on the success branch, so worklog entries lost the result.isError / result.content fields that downstream auditors (including the jira attribution e2e) rely on to detect upstream-reported failures. Add a rawResult field to ApprovalExecutionOutcome, have the MCP executor populate it from the SDK callTool response, and forward it through the worklog write in resolveApprovalActionOnce. The gh executor leaves it undefined — there is no upstream-shaped result to preserve.
The pre-refactor approval flow let CallToolResult.isError silently propagate as a wrapper-level "success" (exitCode 0) — the e2e then had to forensically reconstruct the failure by inspecting worklog.result.isError through an OR chain of four possible failure signals. The previous fix (8c33fb8) preserved the raw MCP CallToolResult in the worklog so that backchannel kept working, but the asymmetry remained: SDK throws became non-zero exits, MCP-spec isError envelopes did not. Detect isError at both call sites (createMcpApprovalExecutor and executeDirectMcpCall) and surface it the same way as an SDK throw — the unwrapped error text becomes stderr, exitCode is 1, sideEffectAttempted stays true so the gateway re-enters the agent with the same "do not replay" guidance. Approval store transitions to error-pending, matching the SDK-throw retry semantics. Drop the rawResult field on ApprovalExecutionOutcome — failure paths already carry the error text in stderr, success paths now write the unwrapped stdout string to the worklog instead of the raw CallToolResult blob. The worklog loses content[] granularity but gains symmetry: every tool call records args + (result | error) as text. Collapse the e2e assertion that prompted the previous fix to a single exitCode != 0 check, since wrapper-level failure signal is now canonical. Add a unit test asserting the isError branch surfaces as a failure with sideEffectAttempted: true and the action stays pending for retry.
Two CI regressions from the isError normalization: 1. The worklog write for approved MCP calls used pendingAction.args (pre-attribution) instead of what the executor actually sent to the upstream. The jira attribution e2e expects to see assignee_account_id in the worklog, which is only added by withJiraAttribution inside the executor — pre-refactor executeUpstreamCall received the attributed args and logged them naturally. Surface the args the executor sent via a new effectiveArgs field on ApprovalExecutionOutcome, populated by createMcpApprovalExecutor from the upstreamArgs it built. The resolver writes outcome.effectiveArgs ?? pendingAction.args on both success and failure worklog branches. 2. The e2e expected_final_status='approved' assertion no longer holds when jira_assignee_live=true. With isError now normalized to a side-effect-attempted failure, the THORE2E fake-project-key create correctly leaves the action pending for retry instead of falsely flipping it to approved. Update the e2e expectation to pending and document why. Add a unit-level guard so this class of bug surfaces in the unit suite instead of only in e2e: the isError test now also asserts the worklog records the attributed args (assignee_account_id). To make worklog inspection possible from tests, replace the no-op writeToolCallLogFn stub with one that captures entries into a per-test array.
Collapse the ApprovalExecutor interface and factory functions into two plain async functions (runMcpApproval, runGhIssueCreateApproval). Extract a single createPendingApproval helper used by both the MCP approve branch and the gh issue create flow. Restore the shared callUpstreamWithLogging helper used by direct MCP calls and the Jira account lookup so they stop hand-rolling the same try/log/worklog block. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Will revert before merge. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This reverts commit 36c349c. Approval-card e2e passed on this branch; restoring the original gate. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
@copilot resolve the merge conflicts in this pull request |
|
@copilot resolve the merge conflicts in this pull request |
…ntract Drop Slack button/originating-thread and server-side footer/assignee mechanics from the gh issue create skill entry. Describe only the agent-observable approval contract (approval_required payload + action ID, poll with approval status <id>), matching the MCP approval wording in build.md and AGENTS.md rule #10. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Trim the gh issue create entry to the agent-observable approval contract and match the MCP approve-classified tool wording in build.md. Remove server-side mechanics the agent cannot observe or act on (Slack button delivery, traceability footer / trigger-user assignee injection, github:issue session-alias routing), per AGENTS.md rule #10. Use the same framing, verb, and <action-id> placeholder as build.md's approval CLI description: requires approval, returns an action ID instead of the result, check status with `approval status <action-id>`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Remove the standalone APPROVAL_TOOL_NAMES array and its proxy inventory drift assertion/test. ApprovalToolName now derives from the ApprovalRequiredEventPayloadSchema discriminated union, which is the real source of truth (it maps each tool to its args schema). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Summary
gh issue createbehind Slack human approval instead of immediate executionTesting
AI-generated — verify before acting. View Thor context