fix(exec): allow skills-store and user allow_paths as working_dir#1146
Open
codebit0 wants to merge 1 commit into
Open
fix(exec): allow skills-store and user allow_paths as working_dir#1146codebit0 wants to merge 1 commit into
codebit0 wants to merge 1 commit into
Conversation
ExecTool's working_dir validation passed nil as the base allow-prefix list to resolvePathWithAllowed, while file tools (read_file, list_files, write_file, edit, send_file) all use t.allowedPrefixes seeded in wireExtraTools with the skills directories and user-configured allow_paths. The asymmetry meant agents could read files inside a skill's directory but could not cd there to run its scripts — they had to fall back to absolute paths with cat, losing the natural "cd skill && ./script" idiom. Concretely, cron-triggered seed runs hit a deny for working_dir=/.../skills-store/<skill>/<ver> while harvesting keyword data, and the agent learned to route around it with absolute paths. - Add allowedPrefixes to ExecTool and implement the existing PathAllowable interface so wireExtraTools wires it identically to the file tools. - shell.go working_dir validation now uses t.allowedPrefixes as the base (still write-class, so team-root remains excluded for cross-chat safety). - Generalize PathAllowable's doc comment — the interface is already used by write/edit tools and now exec, so the original "for read access" phrasing was misleading. - Neutralize the "read_file: access denied" log in resolvePathWithAllowed. That function is shared by read_file, list_files, read_image, send_file, and exec working_dir validation; the message now reads "path access denied" and the actual caller is identifiable via the surrounding tool_call_update event. Subagent ExecTool, constructed separately in buildSubagentToolsRegistry, deliberately does NOT inherit this wiring — that's a parallel inconsistency that does not affect top-level (cron-triggered) sessions and is left as a separate concern. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mrgoonie
requested changes
Jun 20, 2026
mrgoonie
left a comment
Contributor
There was a problem hiding this comment.
Summary: This PR is directionally useful, but it is not merge-ready yet because the same working_dir allow-list fix is missing from the credentialed-exec path, and the branch is currently conflicted.
Risk level: Medium
Mandatory gates:
- Duplicate/prior implementation: clear; no matching merged PR/issue found for exec working_dir allow_paths/skills-store parity.
- Project standards: issue found; shell working_dir resolution is duplicated for credentialed and normal exec paths, but the PR updates only the normal shell branch.
- Strategic necessity: clear value; allowing skills to run scripts from their own directory reduces brittle absolute-path workarounds in cron/skill runs.
- CI/checks: previous checks were green, but current merge state is DIRTY.
Findings: - Critical: none.
- Important: The credentialed-exec branch in
internal/tools/shell.gostill resolvesworking_dirwithallowedWriteWithTeamWorkspace(ctx, nil). Because credentialed CLIs are routed before the normal shell path, commands such as registeredgws/ghstill cannot use the newly wiredExecTool.AllowPathsprefixes asworking_dir. This leaves the exact skills-store/userallow_pathsasymmetry unresolved for secure CLI commands. Please apply the samet.allowedPrefixesbase there too, or refactor working_dir resolution into one helper used by both branches. - Important: GitHub reports
mergeStateStatus=DIRTY, so this branch needs a rebase/merge conflict resolution before it can land. - Suggestion: Add a regression test that covers both normal exec and credentialed-exec working_dir validation with an allowed skills-store or user allow_path prefix, not only the plain shell branch.
Verdict: REQUEST_CHANGES
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ExecTool'sworking_dirvalidation passednilas the base allow-prefix list toresolvePathWithAllowed, while file tools (read_file,list_files,write_file,edit,send_file) all uset.allowedPrefixesseeded inwireExtraToolswith the skills directories and user-configuredallow_paths.The asymmetry meant agents could read files inside a skill's directory but could not
cdthere to run its scripts — they had to fall back to absolute paths withcat, losing the naturalcd skill && ./scriptidiom. Concretely, cron-triggered seed runs hit a deny forworking_dir=/.../skills-store/<skill>/<ver>while harvesting keyword data, and the agent learned to route around it with absolute paths.Changes
allowedPrefixestoExecTooland implement the existingPathAllowableinterface sowireExtraToolswires it identically to the file tools.shell.goworking_dir validation now usest.allowedPrefixesas the base (still write-class, so team-root remains excluded for cross-chat safety).PathAllowable's doc comment — the interface is already used by write/edit tools and now exec, so the original "for read access" phrasing was misleading."read_file: access denied"log inresolvePathWithAllowed. That function is shared by read_file, list_files, read_image, send_file, and exec working_dir validation; the message now reads"path access denied"and the actual caller is identifiable via the surroundingtool_call_updateevent.Out of scope
Subagent ExecTool, constructed separately in
buildSubagentToolsRegistry, deliberately does NOT inherit this wiring — that's a parallel inconsistency that does not affect top-level (cron-triggered) sessions and is left as a separate concern.Test plan
tools/call name=exec working_dir=<skills-store/<skill>/<ver>>succeeds (previously denied)cd skill_dir && ./scriptpattern successfullyread_file/list_filesfor paths outside skills-store still deny as before (no widening of read scope)path access denied(notread_file: access denied)