Repository navigation
fix(analyst): keep write tools denied under config grants and allow reading global skill files #1597
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,14 +1,23 @@ | ||
| export * as Wildcard from "./wildcard" | ||
|
|
||
| // altimate_change start — preserve POSIX backslashes when matching filesystem paths | ||
| export function match(input: string, pattern: string) { | ||
| return matchNormalized(input.replaceAll("\\", "/"), pattern.replaceAll("\\", "/")) | ||
| } | ||
|
|
||
| export function matchPath(input: string, pattern: string) { | ||
| if (process.platform === "win32") return match(input, pattern) | ||
| return matchNormalized(input, pattern) | ||
| } | ||
|
|
||
| function matchNormalized(input: string, pattern: string) { | ||
| let escaped = pattern | ||
| .replace(/[.+^${}()|[\]\\]/g, "\\$&") | ||
| .replace(/\*/g, ".*") | ||
| .replace(/\?/g, ".") | ||
|
|
||
| if (escaped.endsWith(" .*")) escaped = escaped.slice(0, -3) + "( .*)?" | ||
|
|
||
| return new RegExp("^" + escaped + "$", process.platform === "win32" ? "si" : "s").test(input) | ||
| } | ||
| // altimate_change end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -334,6 +334,9 @@ export const layer = Layer.effect( | |
| question: "allow", | ||
| // Skills return instruction text only (no side effects), so they are safe for a read-only agent. | ||
| skill: "allow", | ||
| // "*": "deny" above also overrides the default allow for skill directories; add it back | ||
| // so files bundled with a global skill (stored outside the project) stay readable. | ||
| external_directory: Object.fromEntries(skillDirs.map((dir) => [path.join(dir, "*"), "allow"])), | ||
| // Workspace memory: read, reload and view the audit log. Reloading syncs like the | ||
| // load every session runs (it can drop a local copy of a block the workspace has | ||
| // archived), but cannot create or edit memory. Saving and deleting stay denied, so a | ||
|
|
@@ -633,12 +636,48 @@ export const layer = Layer.effect( | |
| // Ensure Truncate.GLOB is allowed unless explicitly configured | ||
| for (const name in agents) { | ||
| const agent = agents[name] | ||
| // altimate_change start — keep analyst's shell and write tools disabled after config and safety rules | ||
| // Permission.disabled requires the final bash rule to deny "*". Without | ||
| // it, narrower safety rules leave the tool exposed to commands that emit | ||
| // no permission patterns (e.g. `>out.txt`). data-explorer is capped above. | ||
| // The built-in tools that change files, memory, config, connections or external | ||
| // systems are denied again too: last-match-wins would otherwise let a config grant | ||
| // such as "*": "allow" turn them back on for the read-only agent. Most of them never | ||
|
Collaborator
There was a problem hiding this comment. Minor (earlier, deferred, not blocking): a global The post-config re-deny below covers built-in tools only. This is deliberate. It is documented on |
||
| // call ctx.ask, so removing them from the tool list is their only gate. batch is | ||
| // denied because it runs other tools without that tool-list filter. | ||
| if (name === "analyst") { | ||
|
Collaborator
There was a problem hiding this comment. Minor: a global These post-config denies cover built-in tools only. A global Suggestion: for analyst, ignore wildcard-only grants for non-built-in tools, and honour only grants that name the tool (or MCP server prefix) or come from
Contributor
Author
There was a problem hiding this comment. You read it right: it is deliberate. AI-10226 rejected an allow-list cap so that MCP and custom tools a team adds to analyst keep working, and the post-config deny covers built-ins only. I agree the docs have to say plainly what a global Ignoring wildcard-only grants for non-built-in tools would change how config precedence works for analyst, so I have not done that in this PR. It is listed as a follow-up in the PR description.
Collaborator
There was a problem hiding this comment. Minor (non-blocking, reasonably deferred): a global There is no code change here, and that's fine. It's deliberate, now documented on all three docs pages with the two workarounds, and listed as a follow-up in the PR description. I'm noting it so the open item stays visible. It does not block this PR.
Contributor
Author
There was a problem hiding this comment. Thanks, agreed. It stays deferred, is documented on the three pages with both workarounds, and is in the PR follow-ups. |
||
| // Keep what config resolved for data-explorer (it may deny or ask), but nothing broader. | ||
| const dataExplorerAction = Permission.evaluate("task", "data-explorer", agent.permission).action | ||
| agent.permission = Permission.merge( | ||
| agent.permission, | ||
| Permission.fromConfig({ | ||
| ...Object.fromEntries( | ||
| [ | ||
| "bash", | ||
| "sql_execute_write", | ||
| "edit", | ||
| "write", | ||
| "apply_patch", | ||
| "altimate_memory_write", | ||
| "altimate_memory_delete", | ||
| "altimate_memory_extract", | ||
| "training_import", | ||
| "warehouse_add", | ||
| "warehouse_remove", | ||
| "warehouse_install_driver", | ||
| "mcp_discover", | ||
| "datamate_manager", | ||
| "sample_setup", | ||
| "feedback_submit", | ||
| // Runs caller-supplied SQL without the write classification sql_execute applies. | ||
| "data_diff", | ||
| "batch", | ||
| ].map((tool) => [tool, "deny" as const]), | ||
| ), | ||
| // A trailing "*" deny is what removes task from the tool list when nothing is allowed. | ||
| task: dataExplorerAction === "deny" ? "deny" : { "*": "deny", "data-explorer": dataExplorerAction }, | ||
| }), | ||
| ) | ||
| } | ||
| // altimate_change end | ||
| const explicit = agent.permission.some((r) => { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor (not blocking): a read through an allowed link into a subfolder that an earlier rule or layer denies is still allowed, and no test or doc pins this
Config: global
external_directory: {"<root>/real/secret/*": "deny"}plusagent.<name>.permission.external_directory: {"<root>/link/*": "allow"}, withlink -> real. The same happens with both rules in one map, deny first. A read oflink/secret/fsends[link/secret/*, real/secret/*]. The written glob is allowed bylink/*, and the resolved glob is allowed by thelink/*resolved spelling that follows the deny, so the result isallow.This is not a regression. Main allows the same request because it asks only for the written glob. The same rules written without a link (
real/secret/*: deny, thenreal/*: allow) also allow under last-match-wins. Making the earlier narrower deny win here is the ordering approach that broke the deny-volume/allow-project carve-out last round. But the agent-level test (analyst-data-explorer.test.ts:625-657) asserts only the direct read andreal/other, while its comment says the deny "must survive a per-agent allow written through a link". This sentence can also be read as promising that a deny is enforced through any link.Suggestion: pin the intended decision with
decide(layered, glob("link", "secret"), glob("real", "secret"))(expectallow) innext.test.tsand in the agent test. Add one sentence here, along these lines: "An allow for a symlinked folder also covers the folder it resolves to, so put a deny for one of its subfolders after that allow." This is safe to do in a follow-up.