fix: workspace-relative read paths for view tool + awk in go-source-analysis#39958
Conversation
…lysis The daily-compiler-quality workflow was hitting the tool denial limit (5/5) because the Copilot SDK's `view` tool delivers file paths as absolute paths (e.g. /home/runner/work/gh-aw/gh-aw/pkg/workflow/file.go) but the go-source-analysis shared import only grants shell(cat pkg/**/*.go) — a workspace-relative glob. The isReadPathAllowedByShellRules permission function did not know about the workspace root, so it could not strip the GITHUB_WORKSPACE prefix before matching against the relative pattern, causing every view() call to be denied (3 denials). The agent then fell back to Python heredocs which are also denied (2 more denials → limit hit). Fixes: 1. copilot_sdk_permissions.cjs: Add optional workspaceRoot parameter to isReadPathAllowedByShellRules. When the requested path is absolute and the pattern is relative, try matching the path after stripping the workspace prefix. 2. buildCopilotSDKPermissionHandler: Accept workspaceRoot in options and thread it through to the read permission check. 3. copilot_sdk_session.cjs: Pass process.env.GITHUB_WORKSPACE as workspaceRoot when building the permission handler. 4. go-source-analysis.md: Add awk to bash tools as a capable non-Python alternative for complex text analysis (e.g. function-length counting). 5. Recompile daily-compiler-quality, duplicate-code-detector, semantic-function-refactor, and spec-extractor (all import go-source-analysis.md). Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes Copilot SDK permission handling so read/view() requests delivered as absolute paths can still be approved by workflows that grant only workspace-relative shell globs (e.g. shell(cat pkg/**/*.go)), and updates the shared Go analysis workflow allowlist to include awk to avoid blocked Python fallbacks.
Changes:
- Extend
isReadPathAllowedByShellRulesto optionally match absolute paths against relative patterns by stripping a configured workspace root. - Plumb
GITHUB_WORKSPACEthrough the Copilot SDK session so the permission handler has workspace context, and add a regression test for the absolute-path case. - Add
awkto.github/workflows/shared/go-source-analysis.mdand recompile dependent workflows’.lock.ymloutputs.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/copilot_sdk_session.cjs | Passes GITHUB_WORKSPACE to the permission handler as workspaceRoot. |
| actions/setup/js/copilot_sdk_permissions.cjs | Adds workspace-aware fallback matching for absolute read paths vs relative shell patterns. |
| actions/setup/js/copilot_sdk_driver.test.cjs | Adds a test covering approval of absolute workspace paths under a relative cat pkg/**/*.go shell rule. |
| .github/workflows/shared/go-source-analysis.md | Adds awk to the shared bash tool allowlist for Go source analysis workflows. |
| .github/workflows/daily-compiler-quality.lock.yml | Recompiled workflow output to include the updated allowlist (notably shell(awk)). |
| .github/workflows/duplicate-code-detector.lock.yml | Recompiled workflow output to reflect shared workflow changes. |
| .github/workflows/semantic-function-refactor.lock.yml | Recompiled workflow output to include the updated allowlist (notably Bash(awk)). |
| .github/workflows/spec-extractor.lock.yml | Recompiled workflow output to include the updated allowlist (notably shell(awk)). |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 8/8 changed files
- Comments generated: 2
| let relativeRequestedPath; | ||
| if (workspaceRoot && normalizedRequestedPath.startsWith("/")) { | ||
| const normalizedWorkspace = normalizePermissionPath(workspaceRoot); | ||
| if (normalizedRequestedPath.startsWith(normalizedWorkspace + "/")) { | ||
| relativeRequestedPath = normalizedRequestedPath.slice(normalizedWorkspace.length + 1); | ||
| } | ||
| } |
| - head -n * pkg/**/*.go | ||
| - grep -r "func " pkg --include="*.go" | ||
| - cat pkg/**/*.go | ||
| - awk |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed for PR #39958: PR does not have the 'implementation' label and has 0 new lines of code in business logic directories (≤100 threshold). Neither Condition A nor Condition B is met. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnose and /tdd — commenting with improvement suggestions, no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Path traversal gap:
normalizePermissionPathdoes not resolve..components before glob-matching the workspace-relative path. The.goextension in the currentpkg/**/*.gopattern guards against exploitation today, but this is latent risk for future patterns without extension constraints. - Test coverage scope: Integration test in
copilot_sdk_driver.test.cjsis thorough for the main allow/deny matrix, but there is no dedicated unit test file forcopilot_sdk_permissions.cjs, and theGITHUB_WORKSPACEunset case is not explicitly verified. logOptionssemantic creep:workspaceRootis execution context, not a logging option — minor but worth addressing for long-term clarity.awkallowlist breadth: Unlike every other bash entry ingo-source-analysis.md,awkcarries no path restriction.
Positive Highlights
- ✅ Root cause correctly identified and surgically fixed — the abstraction mismatch between SDK absolute paths and workflow-relative patterns is cleanly resolved
- ✅ Security boundary is tight: the workspace-prefix check uses
startsWith(normalizedWorkspace + "/")so/home/runner/work/gh-aw/gh-aw-evil/pkg/foo.gois correctly rejected - ✅ New integration test explicitly validates the
/other/workspace/pkg/and/etc/passwddenial cases — good security-first test authorship - ✅
awkaddition has a clear operational motivation (breaking thepython3heredoc fallback chain) and is documented in the PR body - ✅ Four recompiled lock files are a natural consequence of the shared workflow change; no independent review needed
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer
| if (workspaceRoot && normalizedRequestedPath.startsWith("/")) { | ||
| const normalizedWorkspace = normalizePermissionPath(workspaceRoot); | ||
| if (normalizedRequestedPath.startsWith(normalizedWorkspace + "/")) { | ||
| relativeRequestedPath = normalizedRequestedPath.slice(normalizedWorkspace.length + 1); |
There was a problem hiding this comment.
[/diagnose] normalizePermissionPath only strips trailing slashes and converts backslashes — it does not resolve .. components. A crafted absolute path like /home/runner/work/gh-aw/gh-aw/pkg/../../AGENTS.md produces relativeRequestedPath = "pkg/../../AGENTS.md", which path.posix.matchesGlob could match against pkg/**/* patterns that lack an extension constraint. For the current pkg/**/*.go the .go requirement saves us, but future unrestricted patterns would be vulnerable.
💡 Suggested hardening
Normalise the relative path before glob-matching:
if (relativeRequestedPath !== undefined) {
relativeRequestedPath = path.posix.normalize(relativeRequestedPath);
if (relativeRequestedPath.startsWith("..") || relativeRequestedPath.startsWith("/")) {
relativeRequestedPath = undefined; // escaping path — discard
}
}And add a regression test:
expect(onPermissionRequest({ kind: "read", path: "/home/runner/work/gh-aw/gh-aw/pkg/../../AGENTS.md", intention: "" }))
.toEqual({ kind: "reject", feedback: "Tool invocation is not allowed by workflow tool permissions." });| * @returns {boolean} | ||
| */ | ||
| function isReadPathAllowedByShellRules(requestedPath, allowedPathPatterns) { | ||
| function isReadPathAllowedByShellRules(requestedPath, allowedPathPatterns, workspaceRoot) { |
There was a problem hiding this comment.
[/tdd] The core logic change in isReadPathAllowedByShellRules is only exercised via the full driver integration test — there is no copilot_sdk_permissions.test.cjs. Dedicated unit tests for this function would be faster to run, easier to extend, and would let you test workspaceRoot edge cases (trailing slash on root, workspaceRoot undefined, empty string) without spinning up an SDK mock.
💡 Example unit test structure
// copilot_sdk_permissions.test.cjs (new file)
const { isReadPathAllowedByShellRules } = require('./copilot_sdk_permissions.cjs');
describe('isReadPathAllowedByShellRules', () => {
it('allows absolute path within workspace via relative pattern', () => {
expect(isReadPathAllowedByShellRules(
'/home/runner/work/repo/pkg/foo.go',
['pkg/**/*.go'],
'/home/runner/work/repo'
)).toBe(true);
});
it('denies path outside workspace even if it contains matching segment', () => {
expect(isReadPathAllowedByShellRules(
'/other/workspace/pkg/foo.go',
['pkg/**/*.go'],
'/home/runner/work/repo'
)).toBe(false);
});
it('behaves correctly when workspaceRoot is undefined', () => {
expect(isReadPathAllowedByShellRules(
'/home/runner/work/repo/pkg/foo.go',
['pkg/**/*.go'],
undefined
)).toBe(false);
});
});| * @param {CopilotSDKPermissionConfig | undefined} permissionConfig | ||
| * @param {import("@github/copilot-sdk").PermissionHandler} approveAll | ||
| * @param {{coreLogger?: CopilotSDKCoreLogger, logger?: (msg: string) => void, onDenied?: (requestSummary: string) => void}=} logOptions | ||
| * @param {{coreLogger?: CopilotSDKCoreLogger, logger?: (msg: string) => void, onDenied?: (requestSummary: string) => void, workspaceRoot?: string}=} logOptions |
There was a problem hiding this comment.
[/zoom-out] workspaceRoot is a runtime execution-context value, not a logging option. Bundling it into logOptions works, but the type name creates a false impression that this object is only about diagnostics. A future reader adding a new logOptions field might not think to look here for execution-critical inputs.
💡 Alternative: use a dedicated options bag or extend permissionConfig
Option A — add a second options parameter (minimal churn):
function buildCopilotSDKPermissionHandler(permissionConfig, approveAll, logOptions, runtimeOptions) {
// runtimeOptions: { workspaceRoot?: string }
}Option B — extend permissionConfig (already the right shape for execution context):
// CopilotSDKPermissionConfig
{
allowAllTools?: boolean;
allowedTools?: string[];
workspaceRoot?: string; // absolute path used to relativise SDK-emitted absolute paths
}Either approach makes the intent of the parameter clearer at the call site.
| - head -n * pkg/**/*.go | ||
| - grep -r "func " pkg --include="*.go" | ||
| - cat pkg/**/*.go | ||
| - awk |
There was a problem hiding this comment.
[/zoom-out] Every other bash entry in this file scopes its path — cat pkg/**/*.go, wc -l pkg/**/*.go, grep -r "func " pkg .... The bare awk entry grants the command with no argument restriction, so an agent could invoke awk '{print}' /etc/shadow and the shell permission would be approved (file-read permission is a separate gate, but defence-in-depth argues for scoping here too).
💡 Suggestion: scope awk to pkg files
- awk '{...}' pkg/**/*.go # or the most common expected formIf the expected use is function-length counting across all Go files, awk -F '' '{...}' pkg/**/*.go would be more precise and consistent with the other entries.
| }); | ||
| }); | ||
|
|
||
| it("allows read requests when absolute path matches a workspace-relative shell pattern", async () => { |
There was a problem hiding this comment.
[/tdd] The new test only exercises the path where GITHUB_WORKSPACE is set. There's no counterpart case where workspaceRoot is undefined (i.e., GITHUB_WORKSPACE unset at session creation time) — this would verify that the pre-existing absolute-path denial still works unchanged, and that the new code doesn't accidentally widen permissions when the env var is absent.
💡 Suggested additional case
it("denies absolute read paths when GITHUB_WORKSPACE is not set", async () => {
const prevWorkspace = process.env.GITHUB_WORKSPACE;
delete process.env.GITHUB_WORKSPACE;
try {
// ... same mock setup ...
// Absolute path must be denied because no workspaceRoot is available.
expect(onPermissionRequest({ kind: "read", path: "/home/runner/work/gh-aw/gh-aw/pkg/workflow/compiler.go", intention: "" }))
.toEqual({ kind: "reject", feedback: "Tool invocation is not allowed by workflow tool permissions." });
} finally {
if (prevWorkspace === undefined) delete process.env.GITHUB_WORKSPACE;
else process.env.GITHUB_WORKSPACE = prevWorkspace;
}
});
🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics & Test Classification (1 test analyzed)
Go: 0 ( Test inflation note: The test file 📋 Test Detail —
|
| # | Path | Expected |
|---|---|---|
| 1–3 | Absolute workspace-rooted .go files under pkg/workflow/ |
approve-once |
| 4 | Relative pkg/workflow/compiler.go |
approve-once (backwards-compat) |
| 5 | /.../gh-aw/AGENTS.md (in workspace, outside pkg/**) |
reject |
| 6 | /etc/passwd (outside workspace entirely) |
reject |
| 7 | /other/workspace/pkg/workflow/file.go (contains /pkg/ but wrong root) |
reject ⭐ adversarial edge case |
Mocking: All vi.fn() calls stub external SDK lifecycle methods (createSession, sendAndWait, disconnect, stop, start, RuntimeConnection.forUri) — these are legitimate infrastructure stubs for an external SDK, not internal business-logic mocks.
Minor gaps (non-blocking):
disconnectandstopare stubbed but never asserted as called — a lifecycle-cleanup regression would pass silentlyRuntimeConnection.forUriis never verified to receive the correctsdkUrisendAndWaitcall arguments are not asserted — the test only reads the permission callback extracted fromcreateSession, so it would pass even if the agent loop never sent the prompt
Verdict
✅ Check passed. 0% implementation tests (threshold: 30%). The single new test enforces a behavioral contract — that the workspace-relative path expansion logic correctly approves in-scope absolute paths and rejects out-of-scope ones, including an adversarial case where a foreign-workspace path contains the permitted pattern substring.
🧪 Test quality analysis by Test Quality Sentinel · ◷
There was a problem hiding this comment.
The core fix in isReadPathAllowedByShellRules is correct and well-reasoned: pre-computing relativeRequestedPath once outside the some() loop, guarding with workspaceRoot && startsWith("/") before the prefix check, and the !normalizedPattern.startsWith("/") guard on the fallback are all the right calls. The test covers the exact failure scenario from the bug report and correctly rejects out-of-workspace and non-matching paths.
Two non-blocking concerns are flagged inline:
### Summary of findings
Medium — Lexical workspace containment enables symlink escape (new attack surface)
isReadPathAllowedByShellRules line ~184: the new startsWith(normalizedWorkspace + "/") check is purely textual. A committed symlink at pkg/leaked.go → /host/secrets/file.go passes the containment check and matchesGlob approves it, causing the SDK to read the symlink target outside the workspace. This attack path did not exist before this PR because absolute paths were previously always denied against relative patterns. Practical risk is low (requires repo-level compromise to plant the symlink), but this is a security boundary and symlink hygiene is fragile. See inline comment for a realpathSync-based fix.
Medium — awk breaks go-source-analysis.md's scoped-only convention
go-source-analysis.md line 15: every prior tool entry in this shared component is path-scoped to pkg/. Adding bare awk (no path restriction) breaks that principle and gives all 4 importing workflows the ability to run awk against any sandbox-accessible file or invoke shells via system(). The practical impact is limited because the compiled lock files already include unrestricted shell(cat) from other shared components, but the contract of this shared file has widened silently.
🔎 Code quality review by PR Code Quality Reviewer
| - head -n * pkg/**/*.go | ||
| - grep -r "func " pkg --include="*.go" | ||
| - cat pkg/**/*.go | ||
| - awk |
There was a problem hiding this comment.
awk breaks the scoped-only convention of this shared component: every other tool listed here is path-restricted to pkg/; bare awk is unrestricted and can read any sandbox-accessible file or invoke shell commands via system().
💡 Analysis and suggestion
All prior entries in this shared workflow are scoped:
- wc -l pkg/**/*.go
- head -n * pkg/**/*.go
- grep -r "func " pkg --include="*.go"
- cat pkg/**/*.go
GNU awk is fundamentally different — it can read arbitrary files (awk '{print}' /tmp/gh-aw/cache-memory/file) and execute shell commands (awk 'BEGIN{system("env")}' /dev/null). The compiled lock files happen to include unrestricted shell(cat) from other shared components, so the practical containment boundary does not change. But this shared component silently widens its contract for all 4 importing workflows without documentation.
Preferred alternative if awk is only needed for Go source analysis:
- awk pkg/**/*.goThat pattern is consistent with the rest of the list and is the form extractReadablePathPatternsFromShellRule would recognise as path-scoped (though that function currently only handles cat, xargs+cat, and ls prefixes, so an awk path restriction would not gate the SDK view permission handler either way).
| let relativeRequestedPath; | ||
| if (workspaceRoot && normalizedRequestedPath.startsWith("/")) { | ||
| const normalizedWorkspace = normalizePermissionPath(workspaceRoot); | ||
| if (normalizedRequestedPath.startsWith(normalizedWorkspace + "/")) { |
There was a problem hiding this comment.
Workspace-prefix containment is purely lexical — symlinks under pkg/ can escape the workspace boundary: the startsWith(normalizedWorkspace + "/") check does not resolve symlinks, so a symlinked path like /workspace/pkg/leaked.go → /secrets/leaked.go passes the containment check and matchesGlob("pkg/leaked.go", "pkg/**/*.go") approves it, ultimately reading the symlink target outside the workspace.
💡 Details and suggested mitigation
Attack path (newly enabled by this PR):
- Workspace contains
pkg/leaked.go→ symlink →/host/secrets/leaked.go - Agent calls
view("/workspace/pkg/leaked.go") normalizedRequestedPath.startsWith("/workspace/")→ ✅relativeRequestedPath = "pkg/leaked.go"matchesGlob("pkg/leaked.go", "pkg/**/*.go")→ ✅ (approved)- OS follows symlink → reads
/host/secrets/leaked.go
Before this PR, step 3 was the first check and absolute paths would fail to match relative patterns, so this attack was not possible via the view tool.
Practical risk level: Low-to-medium. An adversarial symlink would need to be committed to the repo (or injected into the runner checkout), which requires compromise of the repo itself. But the permission handler is a security boundary and relying on upstream symlink hygiene is fragile.
Suggested fix: Resolve symlinks before the workspace containment check:
const fs = require("fs");
// ...
let realRequestedPath = normalizedRequestedPath;
try {
realRequestedPath = normalizePermissionPath(fs.realpathSync(requestedPath));
} catch {
// file may not exist yet; fall through to lexical check
}
let realWorkspace;
try {
realWorkspace = normalizePermissionPath(fs.realpathSync(workspaceRoot));
} catch {
realWorkspace = normalizedWorkspace;
}
if (realRequestedPath.startsWith(realWorkspace + "/")) {
relativeRequestedPath = realRequestedPath.slice(realWorkspace.length + 1);
}Alternatively, document that symlinks are intentionally out-of-scope for this permission model.
The
daily-compiler-qualityworkflow hit the tool denial limit (5/5) because the Copilot SDK'sviewtool sends absolute paths to the permission handler, butgo-source-analysis.mdonly grantsshell(cat pkg/**/*.go)— a workspace-relative pattern.isReadPathAllowedByShellRuleshad no workspace context, so everyview()call was denied (3 denials), then the agent fell back topython3heredocs which are also blocked (2 more → session stopped).Permission handler fix
isReadPathAllowedByShellRules— new optionalworkspaceRootparameter; when the requested path is absolute and the pattern is relative, strips the workspace prefix and retries the glob matchbuildCopilotSDKPermissionHandler—logOptionsextended withworkspaceRoot?: stringcopilot_sdk_session.cjs— passesprocess.env.GITHUB_WORKSPACEasworkspaceRootShared workflow fix
go-source-analysis.md— addsawkto the bash tool allowlist as a capable non-Python alternative for text analysis (function-length counting, etc.), preventing fallback topython3heredocsgo-source-analysis.md(daily-compiler-quality,duplicate-code-detector,semantic-function-refactor,spec-extractor)