Refine Copilot SDK-mode tool permission scoping from engine config#36538
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot store the permissions configuration, toolsets, and related settings in the JSON config passed via stdin so you don’t need a parser on the JavaScript side to interpret these options. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Implemented in ec5cde4. The engine now emits |
|
@copilot remove dead code introduced in this change. Review the behavior when no toolsets are specified, as it should match the existing command-line version behavior. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed in 34254d4 and follow-up 9b5ccbc. SDK mode no longer injects an approve-all permission handler when no toolsets are configured, so it now defers to SDK defaults to match CLI no-toolset behavior, and I removed the redundant fallback path by only wiring |
|
@copilot add core info logging in the logic to allow tools to record permission-denied events, and log them as core warnings. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Implemented in 2be2a58.
|
|
@copilot usd shim.cos to provide core logging |
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (135 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
REQUEST_CHANGES — 3 medium issues and 1 test gap across the new permission-logging layer.
Blocking themes
Double logging in logPermissionDenied
Every permission denial emits both core.info and core.warning to GitHub Actions, producing two annotations with contradictory severity for the same event. The info call is redundant and should be removed.
All-or-nothing buildCoreLogger validation
The REQUIRED_CORE_LOGGER_METHODS check discards a partial core adapter (e.g. one that only implements warning) entirely, even though both call sites guard with optional chaining and would work fine with a partial object. The validation is stricter than necessary.
summarizePermissionRequest default case
Returns raw request.kind, which is undefined for any SDK kind not yet in the switch. The resulting log message reads permission denied ... undefined and is not actionable.
Missing standalone --allow-all-tools test
The critical allowAllTools=true && no allowedTools path is only covered in combination with --allow-tool write. The standalone case is not tested.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 184K
| coreLogger.info(`Copilot SDK permission denied: ${requestSummary}`); | ||
| } | ||
| if (coreLogger?.warning) { | ||
| coreLogger.warning(`Copilot SDK permission denied by workflow tool permissions: ${requestSummary}`); |
There was a problem hiding this comment.
Redundant dual-level logging for every permission denial: logPermissionDenied emits both core.info and core.warning for the same event, producing two GitHub Actions annotations with contradictory severity for each denied request.
💡 Suggested fix
Keep only the warning call — it is the correct severity for a denial event:
function logPermissionDenied(coreLogger, logger, request) {
const requestSummary = summarizePermissionRequest(request);
logger(`permission denied by workflow tool permissions: ${requestSummary}`);
if (coreLogger?.warning) {
coreLogger.warning(`Copilot SDK permission denied by workflow tool permissions: ${requestSummary}`);
}
}The info entry adds no value once the warning is present. Operators will see the same event twice at different severity levels, creating noise in the GHA annotations panel and potentially confusing automated alerting.
| case "custom-tool": | ||
| return `custom-tool(${request.toolName || "unknown"})`; | ||
| default: | ||
| return request.kind; |
There was a problem hiding this comment.
summarizePermissionRequest default returns request.kind directly, which can be undefined: If the SDK introduces a new kind value not in the switch, the log message reads permission denied ... undefined, masking the actual operation.
💡 Suggested fix
default:
return String(request.kind ?? "unknown");This ensures the log message is always a printable string even for future SDK additions, and makes the "unknown" case explicit rather than relying on implicit JS coercion.
| } | ||
| const missingMethods = REQUIRED_CORE_LOGGER_METHODS.filter(method => typeof core[method] !== "function"); | ||
| if (missingMethods.length > 0) { | ||
| logCoreLoggerFallback(harnessLogger, `missing method(s): ${missingMethods.join(", ")}`); |
There was a problem hiding this comment.
All-or-nothing validation silently discards partial core logger implementations: If global.core implements only warning (e.g. a minimal test stub or a future SDK revision), buildCoreLogger returns undefined and drops all structured logging instead of degrading gracefully.
💡 Suggested fix
return {
info: typeof core.info === "function" ? message => core.info(message) : undefined,
warning: typeof core.warning === "function" ? message => core.warning(message) : undefined,
};This removes REQUIRED_CORE_LOGGER_METHODS and the all-or-nothing guard. logPermissionDenied already guards each method with optional chaining (coreLogger?.info, coreLogger?.warning), so a partial adapter works correctly with the existing call sites.
| } | ||
| }) | ||
|
|
||
| t.Run("preserves allow-all when present", func(t *testing.T) { |
There was a problem hiding this comment.
Missing test for standalone --allow-all-tools with no --allow-tool flags: The critical branch allowAllTools=true && len(allowedToolsSet)==0 is only tested in combination with --allow-tool write. A regression that moves the early-nil-return check above the allowAllTools check would cause standalone --allow-all-tools to silently return nil (no-handler, SDK defaults) instead of approveAll, blocking all tools in production with no test failure.
💡 Suggested addition
t.Run("allow-all-tools without explicit tool entries", func(t *testing.T) {
config := buildCopilotSDKPermissionConfig([]string{"--allow-all-tools"})
if config == nil {
t.Fatal("Expected non-nil permission config for --allow-all-tools")
}
if !config.AllowAllTools {
t.Fatal("Expected AllowAllTools=true")
}
if len(config.AllowedTools) != 0 {
t.Fatalf("Expected empty AllowedTools, got %v", config.AllowedTools)
}
})
🧪 Test Quality Sentinel Report
📊 Metrics & Test Classification (16 tests analyzed)
Test Classification Details
Language SupportTests analyzed:
|
There was a problem hiding this comment.
Test Quality Sentinel: 66/100. Test quality is acceptable — 6.3% of new tests are implementation tests (threshold: 30%). Minor suggestion: consolidate the parseStdinPayload it() tests into an it.each table and add an error-path case for the reflect-enrichment test.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd, /zoom-out, and /grill-with-docs — overall solid security improvement with a few actionable gaps.
📋 Key Themes & Highlights
Key Themes
- Test coverage gaps (3 items):
allowAllTools=trueignoringallowedTools,readalways allowed, and shell prefix matching with an emptycommandsarray all lack regression tests. - Redundant logging:
logPermissionDeniedemits bothcore.infoandcore.warningfor every denial —infois noise on top of thewarning. - Secondary derivation path:
buildCopilotSDKPermissionConfigre-parses CLI flags that were just synthesised bycomputeCopilotToolArguments. If the flag names change the SDK layer silently gets an empty config. - Missing typedef documentation:
CopilotSDKPermissionConfig.allowedToolsentry format conventions are undocumented, making the type hard to use correctly without reading the implementation.
Positive Highlights
- ✅ Replacing unconditional
approveAllwith a rule-aware handler is the right security primitive — well-structured and easy to reason about. - ✅ Deterministic normalization (trim, dedup, sort) in Go before JSON serialization prevents drift between runs.
- ✅
undefinedhandler when no rules are configured correctly preserves SDK-default behavior rather than injecting a restrictive policy where none was requested. - ✅ Clear separation between the Go config-building layer and the JS enforcement layer makes the pipeline auditable.
- ✅ Comprehensive happy-path test coverage for the new JS permission handler.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 3M
| coreLogger.info(`Copilot SDK permission denied: ${requestSummary}`); | ||
| } | ||
| if (coreLogger?.warning) { | ||
| coreLogger.warning(`Copilot SDK permission denied by workflow tool permissions: ${requestSummary}`); |
There was a problem hiding this comment.
[/tdd] logPermissionDenied emits both core.info and core.warning for every denial — the info call is redundant noise and dilutes the signal in the Actions log.
💡 Suggestion
Drop the core.info call (lines 80–82) entirely. core.warning already surfaces the denial prominently in the GitHub Actions UI, and the internal logger line above it captures the same message for the harness log. Two separate core.* calls with slightly different wording ("Copilot SDK permission denied" vs "...by workflow tool permissions") will confuse readers trying to triage denied-tool warnings.
function logPermissionDenied(coreLogger, logger, request) {
const requestSummary = summarizePermissionRequest(request);
logger(`permission denied by workflow tool permissions: ${requestSummary}`);
coreLogger?.warning?.(`Copilot SDK permission denied by workflow tool permissions: ${requestSummary}`);
}| return shellRules.some(rule => { | ||
| if (rule.endsWith(":*")) { | ||
| const prefix = rule.slice(0, -2).trim(); | ||
| return prefix.length > 0 && commandIdentifiers.includes(prefix); |
There was a problem hiding this comment.
[/tdd] Prefix rules like shell(git:*) rely on request.commands being a populated array. When commands is absent or empty (e.g., for script invocations), commandIdentifiers will be [] and the prefix rule silently misses — the request is denied even though the intent is to allow it.
💡 Suggestion and missing test
Add a fallback: if commandIdentifiers is empty and fullCommandText is present, extract the first word as a fallback identifier:
const commandIdentifiers = Array.isArray(request.commands)
? request.commands.map(cmd => cmd?.identifier).filter(Boolean)
: [];
// Fallback: derive identifier from first token of fullCommandText when commands is empty.
const effectiveIdentifiers = commandIdentifiers.length > 0
? commandIdentifiers
: [fullCommand.split(/\s+/)[0]].filter(Boolean);Then use effectiveIdentifiers in the shellRules.some(...) check. Add a test:
it("matches shell prefix rule when commands array is absent", () => {
const handler = buildCopilotSDKPermissionHandler(
{ allowedTools: ["shell(git:*)"] }, approveAll
);
// No 'commands' field — SDK sends only fullCommandText
expect(
handler({ kind: "shell", commands: [], fullCommandText: "git status" })
).toEqual({ kind: "approve-once" });
});| chmod 700 %s`, customEngineCommandScriptPath, heredocDelimiter, scriptContent, heredocDelimiter, customEngineCommandScriptPath) | ||
| } | ||
|
|
||
| func buildCopilotSDKPermissionConfig(toolArgs []string) *copilotSDKPermissionConfig { |
There was a problem hiding this comment.
[/zoom-out] buildCopilotSDKPermissionConfig re-parses the serialized CLI flags (--allow-tool, --allow-all-tools) that were just built by computeCopilotToolArguments. This creates a secondary derivation path that will silently break if the CLI flag names ever change.
💡 Architectural concern
The permission config and the CLI flags are both derived from workflowData.Tools (via computeCopilotToolArguments). Instead of re-parsing the CLI args, consider passing the structured tool config directly:
PermissionConfig: buildCopilotSDKPermissionConfigFromTools(workflowData),where buildCopilotSDKPermissionConfigFromTools reads workflowData.ParsedTools or the same inputs that computeCopilotToolArguments uses. This removes the --allow-tool string as an intermediate representation and keeps one source of truth.
If keeping the current approach, at minimum add a comment documenting the coupling: toolArgs must contain the flags produced by computeCopilotToolArguments for this function to produce a correct result.
| const allowedToolEntries = new Set(normalizedAllowedTools); | ||
|
|
||
| // Keep explicit allow-all behavior when requested by the engine config. | ||
| if (allowAll) { |
There was a problem hiding this comment.
[/tdd] When allowAllTools: true, approveAll is returned immediately and allowedTools is silently ignored. This is the correct policy, but there's no test that verifies buildCopilotSDKPermissionHandler({ allowAllTools: true, allowedTools: ["shell(rm:*)"] }, approveAll) returns approveAll exactly (not a scoped handler that would block unlisted tools).
💡 Suggested test
it("returns approveAll unchanged when allowAllTools is true, ignoring allowedTools", () => {
const fakeApproveAll = () => ({ kind: "approve-once" });
const handler = buildCopilotSDKPermissionHandler(
{ allowAllTools: true, allowedTools: ["shell(rm:*)"] },
fakeApproveAll
);
expect(handler).toBe(fakeApproveAll); // must be the same reference
});Without this, a future refactor that accidentally builds a scoped handler for the allowAllTools case would block legitimately-allowed tools with no obvious failure signal.
| return allowedToolEntries.has("write"); | ||
| case "read": | ||
| // Read permissions are low-risk and are broadly expected by the agent flow. | ||
| return true; |
There was a problem hiding this comment.
[/tdd] case "read": return true is an implicit security policy (reads are always permitted regardless of the allowlist). This invariant has zero test coverage — if it's accidentally removed or the fallthrough to default: return false is refactored to cover read, the agent silently loses read access.
💡 Suggested test
it("always allows read requests regardless of allowedTools", () => {
const handler = buildCopilotSDKPermissionHandler(
{ allowedTools: [] }, // empty — nothing in allowlist
approveAll
);
// handler is undefined when allowedTools is empty, so test with a non-empty config
const handler2 = buildCopilotSDKPermissionHandler(
{ allowedTools: ["shell(git:*)"] },
approveAll
);
expect(handler2({ kind: "read", fileName: "README.md" }))
.toEqual({ kind: "approve-once" });
});| * @typedef {{ | ||
| * allowAllTools?: boolean, | ||
| * allowedTools?: string[], | ||
| * }} CopilotSDKPermissionConfig |
There was a problem hiding this comment.
[/grill-with-docs] The CopilotSDKPermissionConfig typedef documents allowAllTools and allowedTools but says nothing about the entry format convention that isAllowed expects. Without reading the implementation, a caller can't know whether to write "github", "github(get_file_contents)", "shell(git:*)", or "web_fetch" for a given permission.
💡 Suggested JSDoc addition
/**
* `@typedef` {{
* allowAllTools?: boolean,
* allowedTools?: string[],
* }} CopilotSDKPermissionConfig
*
* allowedTools entry format (mirrors --allow-tool CLI flag conventions):
* "shell" — allow all shell commands
* "shell(cmd:*)" — allow any command whose identifier matches `cmd`
* "shell(git status)" — allow the exact full command text
* "write" — allow all file writes
* "web_fetch" — allow all URL fetch requests
* "server" — allow all tools from MCP server `server`
* "server(tool)" — allow only `tool` from MCP server `server`
* "toolName" — allow a custom tool by name
*/
|
|
|
@copilot review all comments and reviews |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed in 7b71e6c. I reviewed the remaining comments/reviews and fixed the outstanding parser edge case in |
|
``
|
|
``
|
| * @param {(message: string) => void} harnessLogger | ||
| * @returns {{info?: (message: string) => void, warning?: (message: string) => void} | undefined} | ||
| */ | ||
| function buildCoreLogger(harnessLogger) { |
There was a problem hiding this comment.
Addressed in 381aa5c. actions/setup/js/copilot_harness.cjs now requires ./shim.cjs at module scope and buildCoreLogger() reads global.core from that initialization path.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot remove build core logging, just call core. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot merge main and recompile |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot SDK mode was effectively running with broad permission behavior despite workflow-level tool constraints. This update aligns SDK session permissions with engine-generated tool configuration, removes the implicit “allow everything” posture when explicit rules exist, and improves visibility into denied permission requests.
Permission model alignment (SDK ↔ engine config)
permissionConfigin the engine-generated stdin JSON payload for SDK mode.serverArgs(--allow-tool,--allow-all-tools) for permission derivation.Scoped permission handler in SDK driver
onPermissionRequest: approveAllwith a rule-aware handler.onPermissionRequestwhen explicit permission config exists.allowAllToolsbehavior viaapproveAllwhen the engine config requests full tool access.shell,shell(cmd),shell(prefix:*))server,server(tool))web_fetchURL permissionsPermission-denied observability
core.info+core.warning) via shim-backedglobal.corein the harness, with harness-log fallback when core logging is unavailable.Behavioral consistency improvements
permissionConfig.