fix(core): honor ACCEPT_EDITS working-directory auto-allow for edit tools and unify preset-aware permission input

[kevinyang03]

AgentScope-Java Version

2.0.3-SNAPSHOT

Description

Fixes #2870.

Background: ACCEPT_EDITS was documented (javadoc on PermissionMode / AdditionalWorkingDirectory, plus permission-system.md in en/zh) to auto-allow file edits inside working directories, but the engine's ACCEPT_EDITS branch only handled read-only tools and nothing ever consumed PermissionContextState#getWorkingDirectories(), so edit tools always fell through to the default ASK — silently DENY under DONT_ASK. During review a second, independent issue surfaced: the permission gate evaluated the raw model input while ToolExecutor merges preset parameters, so a preset-supplied file path was invisible at permission time.

Changes:

• Add a declarative tool-side path contract: @Tool(filePathParams = {...}) / ToolBase.builder().filePathParams(...), bridged by ReflectiveFunctionTool. The default ToolBase#checkPermissions then applies strict extraction (missing/blank declared param → PASSTHROUGH, never a subset auto-allow; non-string value → bypass-immune Safety-ASK), a dangerous-path check, and the ACCEPT_EDITS working-directory check (ALLOW only when every declared path provably resolves inside the scope).
• Add ToolFilePathResolver + the ToolBase.resolveExecutionPath hook so a tool proves its own execution landing point instead of the framework guessing. Relative paths are never auto-allowed without a resolver. WriteFileTool implements the resolver via FileToolUtils.resolveLexical, which validatePath now shares so permission and execution cannot drift apart. The harness FilesystemTool cannot prove its landing point (AbstractFilesystem root/mode/namespace depends on the runtime context), so it fails closed: write_file/edit_file fall back to ASK under ACCEPT_EDITS — documented in en/zh as a known caveat rather than papered over.
• Harden symlink handling with component-wise resolution (resolveEffective) that mirrors kernel semantics: dangling links are expanded to their targets (outside scope → refused; inside scope → allowed), and cycles, unreadable links and chains over 256 steps return null and fail closed — the scope check refuses them and the dangerous check treats them as dangerous. The dangerous check evaluates both the JVM-CWD-anchored form and the tool-resolved landing point, and the whole self-check converts unexpected runtime failures into a bypass-immune Safety-ASK.
• Unify preset-aware permission input: ToolRegistry.effectiveInput is now the single merge source (preset wins), used by both ToolExecutor and the ReActAgent permission gate, so permission evaluates exactly the input that will be executed. This is a security fix in its own right.
• Update docs in en + zh: the filePathParams section in permission-system.md and the new @Tool attribute row in tool.md.

How to test:

• mvn -pl agentscope-core test -Dtest='ToolBaseFilePathParamsTest,PermissionEngineTest,ReflectiveFunctionToolTest' — 36-case ToolBaseFilePathParamsTest (dangling/cyclic/chained symlinks, dual-anchor dangerous landing, resolver-gated relative paths, strict extraction, exception fail-closed), the engine AcceptEditsWorkingDir group, and annotation-bridge guards through a real Toolkit registration.
• mvn -pl agentscope-core test -Dtest='PresetPermissionGateTest' — preset dst outside the scope pauses the agent with PERMISSION_ASKING instead of auto-allowing; preset dst inside executes normally; merge semantics (preset wins, fresh map).
• mvn -pl agentscope-harness -am test -Dtest='FilesystemToolPermissionTest' -DfailIfNoTests=false — harness end-to-end: in-scope write falls back to ASK because the resolver is opaque; outside write asks; .bashrc edit under BYPASS stays bypass-immune Safety-ASK.
• Full suites: mvn -pl agentscope-harness -am test passes locally (core + harness, 0 failures; harness: 847 tests, 3 pre-existing skips).

Checklist

Please check the following items before code is ready to be reviewed.

• Code has been formatted with mvn spotless:apply — spotless:check passes on both modules
• All tests are passing (mvn test) — core + harness full suites green (harness: 847 tests, 0 failures); the full-reactor build runs in CI
• Javadoc comments are complete and follow project conventions — mvn -pl agentscope-core javadoc:javadoc passes with 0 errors
• Related documentation has been updated (e.g. links, examples, etc.) — permission-system.md (en/zh) documents filePathParams, the resolver contract and the harness caveat; tool.md (en/zh) registers the new @Tool attribute
• Code is ready for review

[codecov]

Codecov Report

❌ Patch coverage is 77.65957% with 42 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...rc/main/java/io/agentscope/core/tool/ToolBase.java	80.00%	15 Missing and 16 partials ⚠️
...va/io/agentscope/core/tool/file/WriteFileTool.java	20.00%	3 Missing and 1 partial ⚠️
...o/agentscope/core/tool/ReflectiveFunctionTool.java	60.00%	1 Missing and 1 partial ⚠️
...ain/java/io/agentscope/core/tool/ToolRegistry.java	71.42%	0 Missing and 2 partials ⚠️
...va/io/agentscope/core/tool/file/FileToolUtils.java	71.42%	2 Missing ⚠️
...e/src/main/java/io/agentscope/core/ReActAgent.java	66.66%	0 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

[guslegend0510]

[P1] 悬空符号链接可以把写入带到工作目录外

Files.exists() 会跟随链接；当工作目录内的链接指向尚未创建的外部文件时，它返回 false，随后代码把链接当成普通路径重新拼回，错误判定为工作目录内。
我用真实 WriteFileTool 复现：权限结果为 ALLOW，随后成功在工作目录外创建了目标 .bashrc。
建议使用 NOFOLLOW_LINKS 识别每一层符号链接，解析不了、悬空或循环时 fail closed，不能自动 ALLOW。

[guslegend0510]

[P1] 权限检查与工具执行使用了不同的相对路径基准

权限端把相对路径固定解析到“第一个 working directory”；但：

• WriteFileTool 实际按自己的 baseDir 或 JVM CWD 解析；
• FilesystemTool 还会经过 WorkspacePathNormalizer 和具体 AbstractFilesystem 的路径策略。
  实测 working directory 为 /tmp/allowed、工具 baseDir=/tmp/actual 时，created.txt 被判定 ALLOW，实际写进 /tmp/actual/created.txt。
  通用 ToolBase 不应猜测执行基准。应由工具提供与真实执行完全一致的无副作用 path resolver；无法确认时，相对路径只能 PASSTHROUGH/ASK。

[guslegend0510]

[P1] 权限检查没有包含 preset 参数
权限门检查的是模型原始输入，而 ToolExecutor 在之后才把 preset 参数合并进去。比如：

• filePathParams={"src","dst"}
• 模型传入工作目录内的 src
• dst 是注册时 preset 的工作目录外路径
  当前代码忽略缺失的 dst，按 src 返回 ALLOW，实际方法却会收到外部 dst。
  权限检查必须使用与执行阶段完全一致的 effective input，包括 preset 覆盖规则；声明路径缺失或类型不符时也不能按剩余子集自动放行。

[Aias00]

Blocking — dangling symlink escapes the working scope.

Files.exists(Path) follows links and returns false for a symlink whose target does not exist, so this loop walks past the link to its parent, resolves the parent, and re-appends the link name. A dangling symlink therefore resolves inside the working directory while the actual write follows it and lands outside.

Verified on macOS/JDK:

scope/escape -> /tmp/outside_target      (target does not exist)
Files.exists(p)                 = false   <- loop walks up
Files.exists(p, NOFOLLOW_LINKS) = true
Files.isSymbolicLink(p)         = true
resolveReal  => /private/tmp/scope/escape   startsWith(scope) == true -> ALLOW
Files.writeString("scope/escape", ...)   => creates /tmp/outside_target

The existing SymlinkResolution tests only cover links whose target already exists, so they miss this branch.

Suggested fix: walk with Files.exists(existing, LinkOption.NOFOLLOW_LINKS) and, whenever a component is a symbolic link, resolve it explicitly (Files.readSymbolicLink + recursive resolution). If the link cannot be resolved (dangling, cyclic, unreadable), fail closed — return the unresolvable marker so isPathInWorkingScope yields false and the engine falls back to ASK. A regression test for the dangling case would also close the gap.

[Aias00]

The two checks use different anchors for relative paths, which makes them inconsistent:

• isDangerousPath(filePath) -> Path.of(expandTilde(p)).toAbsolutePath() resolves against the JVM CWD.
• isPathInWorkingScope(p, context) -> resolveAgainstWorkingDirs resolves against workingDirs entry 0.

When CWD and workingDirs[0] differ, a relative argument can pass the dangerous check under one anchor and be auto-allowed under the other. Minimal example: workingDirs[0] = ~/.ssh, argument "id_rsa" -> dangerous check evaluates ~/project/id_rsa (not dangerous) while the scope check evaluates ~/.ssh/id_rsa (in scope) -> ALLOW on a dangerous file.

The same mismatch exists against the tool layer: WriteFileTool resolves relative paths against its own baseDir and the harness write_file/edit_file resolve against the workspace, neither of which is workingDirs[0]. The permission grant is then computed for a path the tool never writes to.

Suggested fix: resolve each declared path once into a single normalised+real Path (relative -> workingDirs[0], matching the documented rule), then run both the dangerous check and the containment check against that one resolved value. That also lets isDangerousAbsolute be reused directly on the resolved path instead of re-deriving it from the raw string.

[Aias00]

Fail-open on resolution error: both catch (IOException ignored) blocks return the lexical path when toRealPath() fails (permission denied on an ancestor, transient FS error, symlink loop). The scope check then compares unresolved paths and can grant ALLOW for a path that in reality points outside.

For a permission decision the safe default is the other way round. Please have resolveReal signal "unresolvable" (e.g. Optional.empty() or a sentinel) and make isPathInWorkingScope return false in that case — the engine then falls back to ASK, which is the same outcome as today for any path outside the scope.

[Aias00]

The design is the right one: keeping the engine tool-agnostic and putting a declarative filePathParams contract on the tool side is much better than the engine guessing parameter names, and the "declare nothing -> plain PASSTHROUGH" fallback makes this a no-op for every existing tool. The dangerous-path check running before the auto-allow, the all-must-be-in-scope rule for multi-path tools, and the never-implicitly-authorise-CWD decision are all correct. Docs in en+zh and the 32 tests are in good shape.

I am requesting changes on one thing, because this code turns a silent ASK into an ALLOW and therefore has to be tight: the symlink hardening has a hole for dangling symlinks. Files.exists follows links and returns false when the target does not exist, so resolveReal walks past the link, resolves the parent, re-appends the name, and reports the path as inside the scope — while Files.writeString follows the link and creates the file wherever it points. I reproduced it locally (details inline). The two symlink tests only cover links whose target already exists, so the branch is untested.

Two further points inline, non-blocking on their own but worth fixing in the same pass: the dangerous-path check and the scope check resolve relative paths against different anchors (JVM CWD vs workingDirs[0]), which also diverges from how WriteFileTool (baseDir) and the harness tools (workspace) resolve them; and resolveReal fails open when toRealPath() throws, where fail-closed is the right default for a permission decision.

Once the dangling-symlink case is closed with a regression test, I am happy to approve.

[kevinyang03]

@guslegend0510 感谢您细致的评审，对应修复已全部实现并推送(ec90e8d)，逐条回应如下：

[P1] 悬空符号链接逃逸（ToolBase L381） — 已修复。resolveReal 重构为组件级 resolveEffective：逐段识别并展开符号链接，镜像内核解析语义——悬空链接的目标不再被当作普通文件名拼回，而是被展开后参与包含性判定（指向外部 → 不自动放行；指向工作目录内部的目标 → 仍按内核语义放行，因为真实写入会落在该目标上）；链接环、不可读链接与超深链（256 步上限）一律 fail-closed。新增 6 个回归用例：悬空指向外部（您复现的场景）、悬空指向内部、悬空指向危险文件（.bashrc → Safety-ASK）、链式链接逃逸、相对目标按链接父目录解析、链接环 → Safety-ASK。

[P1] 权限与执行的相对路径基准不一致（L357） — 已按您的建议实现"工具提供无副作用 resolver"契约：新增 ToolFilePathResolver 接口 + ToolBase.resolveExecutionPath 钩子，ReflectiveFunctionTool 自动委托。通用层默认对相对路径 fail-closed（不自动放行）；WriteFileTool 实现 resolver，且权限侧与执行侧共用同一个 FileToolUtils.resolveLexical（validatePath 内部也改用它，两者不会再漂移）；harness FilesystemTool 因落点取决于 AbstractFilesystem 的 root/mode/namespace 与 RuntimeContext（权限时刻不可见），显式返回不可证明 → 工作目录内写也回落 ASK——即您说的"无法确认时只能 PASSTHROUGH/ASK"。测试：builtinWriteFileToolWithBaseDirCatchesDangerousResolverLanding（baseDir 锚定 .ssh/config 被双锚点危险检查拦截）、builtinWriteFileToolRelativePathWithBaseDirAllowsInScope、harness 端到端改写为 ...IsNotAutoAllowedBecauseResolverIsOpaque。

[P1] 权限检查未包含 preset 参数（L308） — 已修复。合并逻辑收敛为 ToolRegistry.effectiveInput 单一权威（preset 优先），ToolExecutor 与 ReActAgent 权限门都路由到它——权限评估的输入与执行输入完全同源；同时声明参数缺失/空白一律不再按剩余子集放行。新增 PresetPermissionGateTest 端到端 3 例：preset dst 在工作目录外 → agent 必须暂停（PERMISSION_ASKING）而非静默放行；preset dst 在范围内 → 正常执行；合并语义（preset 覆盖、返回新 map）。

验证：agentscope-core 与 agentscope-harness 全量测试通过（harness 845 tests，0 失败），ToolBaseFilePathParamsTest 扩至 36 个用例；spotless 干净，javadoc 0 错误。

再次感谢您花时间逐条验证并给出如此具体的修复方向，期待您的再次 review。

[kevinyang03]

@Aias00 Thanks for the thorough review — and for the kind words on the overall design. All three points are addressed and pushed (ec90e8d).

Blocking — dangling symlink escape. Fixed with a component-wise resolver (resolveEffective) that mirrors kernel path resolution: every symlink component — including dangling ones — is expanded via readSymbolicLink with recursive, cycle-guarded resolution (256-step cap), and the containment check runs against the expanded landing point. A dangling link inside the scope that points outside is now expanded to the outside target and refused; a dangling link whose target is inside the scope still auto-allows, since that is where the kernel would actually create the file. Cycles, unreadable links and overly deep chains return null and fail closed. Regression tests added for exactly the branch your repro exposed, plus the surrounding matrix: danglingSymlinkToOutsideIsNotAutoAllowed, danglingSymlinkToInsideStillAllowed, danglingSymlinkToDangerousFileSafetyAsks, cyclicSymlinkIsSafetyAsk, chainedSymlinkToOutsidePassthrough, relativeTargetSymlinkResolvesAgainstLinkParent.

Anchor mismatch between the dangerous check and the scope check. The dangerous check now evaluates both anchors for the same raw argument — the JVM-CWD-anchored lexical form and the tool-resolved execution landing point — with each candidate symlink-resolved before matching, so a relative argument whose CWD form looks harmless cannot be auto-allowed against a dangerous resolver landing. Covered by resolverAnchoredDangerousLandingSafetyAsks and builtinWriteFileToolWithBaseDirCatchesDangerousResolverLanding (real WriteFileTool(baseDir) through a real Toolkit registration).

Fail-open on resolution errors. resolveEffective now signals unresolvable paths with null: the scope check returns false for them and the dangerous check treats them as dangerous — both fail closed, falling back to the engine default ASK. The whole self-check is additionally wrapped so any unexpected runtime failure surfaces as a bypass-immune Safety-ASK instead of leaking into a mode-based ALLOW (unexpectedExceptionSafetyAsks, nulBytePathDoesNotThrow).

While in there, the permission gate and ToolExecutor now both evaluate the same effective input (preset parameters merged via a single ToolRegistry.effectiveInput source), and tools that cannot prove their execution landing point fail closed instead of guessing — the harness FilesystemTool therefore falls back to ASK under ACCEPT_EDITS rather than auto-allowing against an unverifiable base.

Verification: full agentscope-core and agentscope-harness suites pass (harness: 845 tests, 0 failures); ToolBaseFilePathParamsTest grew to 36 cases; spotless clean; javadoc clean.

You mentioned you'd be happy to approve once the dangling-symlink case is closed with a regression test — that's now in place. Would appreciate another look when you have a moment.

[Aias00]

The POSIX semantics here are right, and I re-ran my original repro against this exact resolver: dangling->outside = refused, dangling->inside = allowed, cycle = null, symlinked-dir/new-file = refused, plain-new-file = allowed, ../ = refused (6/6).

One platform gap worth a follow-up (not blocking): on Windows, Files.isSymbolicLink only reports true symlinks. Directory junctions and other reparse points are commonly not reported as symbolic links, and 8.3 short names (C:\\PROGRA~1\\...) resolve to a different lexical string than the configured working directory — both would slip past this walk and past the startsWith comparison. A Files.isSymbolicLink(current) || Files.readAttributes(current, BasicFileAttributes.class, NOFOLLOW_LINKS).isOther() check would cover junctions; short names would need getRealPath/toRealPath on the existing prefix. Given ACCEPT_EDITS is most used on developer workstations (including Windows, per the issue reporter), a short "Windows reparse points are not expanded" note in the javadoc or docs would at least keep the limitation visible.

[Aias00]

Heads-up for the release notes: this is a behaviour change on a protected extension point, not just an internal helper. isDangerousPath now returns true for any path that cannot be symlink-resolved with certainty (cycle, unreadable link) and for InvalidPathException, where it previously returned false.

That is the right direction for a security check, and I confirmed no production caller in this repo depends on the old result (only tests do). But downstream subclasses that call isDangerousPath to decide whether to block an operation will now see true more often — for example a path containing an unreadable symlink component. Worth an @since/javadoc line stating the fail-closed contract so the change is discoverable at the API level, not only in the docs.

[Aias00]

The blocker is closed. I extracted resolveEffective and re-ran it against the exact repro from my previous review:

case	before	now
dangling symlink -> outside target	ALLOW (escape)	refused
dangling symlink -> inside target	ALLOW	ALLOW (matches kernel semantics)
cyclic A->B->A	n/a	null -> fail closed
new file through symlinked dir	refused	refused
plain new file inside scope	ALLOW	ALLOW
../ escape	refused	refused

Component-wise expansion with readSymbolicLink, a branch-local linkChain for cycle detection and a 256-step cap is the right shape, and the regression matrix covers precisely the branch that was missing.

The anchor mismatch is also genuinely resolved — and better than the fix I suggested. Rather than picking one anchor, resolveExecutionPath makes the tool prove its landing point and returns Optional.empty() when it cannot, so relative paths simply never auto-allow. Combined with isDangerousPath evaluating both the CWD-anchored and the resolver-anchored form, the two checks can no longer disagree. Fail-closed on null, plus the RuntimeException -> Safety-ASK wrapper, closes the fail-open path as well.

Two things I particularly appreciate:

• The preset-parameter unification is a real find. The gate previously evaluated the raw input while ToolExecutor merged presets, so a preset file-path parameter was invisible at permission time. Routing both through ToolRegistry.effectiveInput is the correct fix, and ReActAgent/ToolExecutor staying consistent is essential for this whole check to mean anything.
• Documenting the harness caveat rather than papering over it. FilesystemTool returning empty for every path means write_file/edit_file still ASK under ACCEPT_EDITS, so #2870 is only fixed for tools that can prove their landing point (the core WriteFileTool among them). Fail-closed is the right call given AbstractFilesystem roots depend on the runtime context, and the en/zh docs now say so explicitly — that honesty is worth more than a guessy auto-allow.

CI is green on both platforms.

Two non-blocking items inline: Windows junctions / 8.3 short names are not covered by isSymbolicLink+startsWith (worth a doc note), and isDangerousPath now fails closed on unresolvable paths, which is a behaviour change on a protected extension point that deserves an @since line.

One process note: the PR has grown from ~1.2k to ~2.1k lines and now carries two distinct changes — the filePathParams gate and the preset-input unification. Both are warranted and I am approving as-is, but please make sure the squash-merge message calls out the preset fix, since it is a security fix in its own right and would otherwise be invisible in the changelog.

Thanks for the thorough follow-through.

[guslegend0510]

LGTM

[kevinyang03]

@Aias00 Thanks for the approval, and for re-running your repro against the new resolver — really appreciated.

Two updates on your notes:

• The PR title and description now cover both changes explicitly, so the squash-merge message will call out the preset-input unification alongside the ACCEPT_EDITS fix.
• For the two non-blocking items, I'll land the @since/fail-closed line on isDangerousPath and the Windows reparse-point note in a small follow-up PR right after this merges — keeps this branch from churning post-approval. If you'd rather have them in this PR, just say the word and I'll push them now.

Thanks again for the thorough follow-through. Also thanks @guslegend0510 for the LGTM!