mirror of
https://gh-proxy.org/https://github.com/klinsmaya/multica.git
synced 2026-11-10 18:36:45 +08:00
VvVandweiweiwei
2b62b2b52e
* fix(cli): compare workdir paths in one namespace (MUL-4252)
The containment guard behind --content-file, --description-file and --attachment
canonicalizes the workdir but could not canonicalize a candidate path that does
not exist yet, and compared the two sides in different namespaces. That is wrong
in three ways, and only the first is merely a wrong answer.
## A path inside the workdir read as outside
filepath.EvalSymlinks fails outright when any component is missing. The fallback
resolved one level up (filepath.Dir) and otherwise called filepath.Clean, which
does not follow symlinks at all.
os.Getwd() returns the LOGICAL working directory whenever $PWD names the current
directory, and everything that writes $PWD carries the path as typed: a shell's
`cd`, and the PWD=<workdir> this repo exports to agent processes
(pkg/agent/opencode.go, deveco.go, codearts.go). So on a host whose workdir path
crosses a symlink, the base was /private/var/... while the candidate stayed
/var/..., and `--content-file reports/summary.md` was rejected as a guardrail
violation when the real problem was that `reports/` did not exist. The only
remedy the error offers is --allow-external-file, so the caller's next move is to
disable the guard and collect an unrelated not-found error on the retry.
## A path outside the workdir read as inside
With an already-canonical workdir the same fallback inverts. `escape/sub/x.md`,
where `escape` is a symlink pointing out of the workdir, read as INSIDE it: the
unresolvable tail fell back to a lexical clean that never sees the symlink.
## ".." was applied to the string, not to the followed path
filepath.Join and filepath.Clean collapse "escape/.." before any symlink is
followed, while the kernel follows `escape` out of the workdir first and then
goes up. So the guard judged a different path than os.ReadFile then opened:
guard says within=true ; os.ReadFile got "another run's file"
That is the MUL-4252 failure this guard exists to prevent — reading a file left
behind by another run — reachable with a file that exists and no race.
## A path on another volume read as an internal failure
Windows only: filepath.Rel cannot relate two absolute paths on different volumes
and returns an error, which surfaced as `Rel: can't make Z:\report.md relative to
C:\Users\...\workdir`. Two absolute paths Rel cannot relate are on different
volumes, which is as far outside the workdir as a path gets, so the guard now
answers that instead.
## The fix
util.ResolveSymlinksBestEffort canonicalizes as far as the filesystem allows: it
resolves the path as given first — the only step that can see ".." the way the
kernel does — then walks up to the deepest existing ancestor, resolves that, and
re-attaches the missing tail lexically. A mismatch inside the missing tail is
inert: a path whose intermediate directories do not exist cannot be opened at
all. Both sides of the comparison go through it.
internal/daemon/execenv already had this algorithm as resolvePathBestEffort,
unexported and documented as Python's Path.resolve(strict=False). It now
delegates, so there is one owner, and the ".." handling makes it more faithful to
that documented contract rather than less.
## Coverage
TestFileWithinWorkingDir runs its table under two working-directory shapes, and
both are load-bearing: under a symlinked workdir an unresolved candidate always
looks outside, so the symlink-escape rows can only fail under a canonical
workdir. Reverting one part of the mechanism at a time:
candidate unresolved (Clean) 5 tests / 15 subtests fail
one-level fallback 3 tests / 8 subtests fail
base side left unresolved 5 tests / 9 subtests fail
clean before resolve ("..") 2 tests / 5 subtests fail
helper walks only one level 4 tests / 9 subtests fail
The lint side is covered too: classifyLocalPathTarget uses the same predicate,
and there the same false negative flips direction — a workdir-local path with a
missing intermediate directory matched neither of its signals, so the link was
published instead of blocked.
cmd/multica has no Windows job, so the guard's Windows-only behaviour ran
nowhere: a candidate on another volume, and a directory junction, which
filepath.EvalSymlinks resolves to its own name rather than descending through it
(internal/daemon/config_windows_test.go asserts that refusal directly). A
junction inside the workdir pointing out of it therefore still reads as inside,
before this change and after it. Failing closed on a component that exists but
cannot be canonicalized would reject every path under a pnpm-style node_modules
junction, and on a host whose workdir is itself reached through a junction it
would reject every --content-file and --attachment on that host — a larger break
than the one it prevents. The windows-tagged test pins the gap with the
observations it rests on and fails with instructions if the platform starts
resolving junctions. Its CI step is scoped and runs with -v so a silent skip is
visible rather than passing as "ok".
* fix(cli): lead with the missing file when a rejected path does not exist (MUL-4252)
The workdir containment guard answers one question and reports it as if it
answered two. A path outside the workdir that does not exist got the same error as
a stale file sitting in /tmp:
--content-file path "/tmp/desc.md" resolves outside the current working
directory; write agent temp files inside the task workdir ... Pass
--allow-external-file to override.
The only remedy that error offers is the flag, so the caller's reasonable next
move is to disable the guard and retry — earning a second, unrelated "no such
file or directory". The diagnosis points away from the actual problem, and the
lesson it teaches an agent is to turn the guard off.
MUL-4252 was about reading a file that exists and belongs to another run. When
the path does not exist there is no such file to protect anyone from, so the
missing file leads, with the containment problem kept as the second clause since
the caller still needs to know the flag would not help.
pathExists treats any stat error other than "not exists" as "exists", so a
permission wall or a broken mount keeps the guard's own wording and stays
fail-closed rather than being reported as a missing file.
Both halves are pinned by mutation: dropping the branch fails
TestExternalFileErrorLeadsWithMissingFile on the missing-file assertion, and
making pathExists report everything as missing fails the same test on the
assertion that an existing external file must still be told about
--allow-external-file.
* fix(cli): preserve Windows path kinds and resolve junctions in the containment guard (MUL-4252)
Addresses the two blockers in the review on 3ec1781e:
1. absNoClean prefixed the working directory onto every non-IsAbs path,
but on Windows a root-relative path (\tmp\desc.md) resolves against
the volume ROOT and a drive-relative path (C:tmp\desc.md) against the
current directory on that drive. The guard inspected
C:\task\workdir\tmp\desc.md while os.ReadFile opened C:\tmp\desc.md,
so an in-workdir shadow could pass an outside read. absNoClean now
splits per platform (path_other.go / path_windows.go) and preserves
the kinds; a drive-relative path on an unobservable drive is handed
back uncanonicalized and fails closed through the Rel branch.
2. The junction case in workdir_guard_windows_test.go pinned a known
contract violation (EvalSymlinks refuses to descend through junctions
under the winsymlink semantics this module ships with). Resolution is
now component-wise on Windows (evalPath) and follows junctions via
os.Readlink — which answers under both winsymlink settings — with a
bounded follow budget; reparse points Readlink cannot speak for
(OneDrive placeholders) stay opaque. A junction out of the workdir is
rejected, one into it (the pnpm shape) is still admitted, and the
known-gap test is replaced by cases asserting both directions plus a
chain and a dead target.
The shared walk in ResolveSymlinksBestEffort is now component-wise on
both platforms: it applies ".." to the resolved prefix even when the
tail is missing (previously the second phase started from filepath.Abs,
which cleans first), and it never depends on what EvalSymlinks does at
a filesystem root. Unix behaviour is unchanged for every shape the
existing tests cover; the new dot-dot-in-the-walk case pins the
difference.
* fix(cli): classify link targets with the full Windows path grammar and resolve unknown reparse points by handle (MUL-4252)
Addresses the two blockers in the review on d3d370377:
1. Link targets were spliced onto the link's parent unless
filepath.IsAbs said otherwise, but the kernel applies the full path
grammar to targets: a root-relative target (\outside) resolves from
the root of the volume the link sits on, and a volume-GUID target
(\\?\Volume{GUID}\...) is absolute in the device namespace. The
resolver now classifies targets through classifyTarget, which reuses
the same grammar as absNoClean; device-namespace targets without a
Win32 spelling are kept verbatim so the containment comparison fails
closed across the namespace instead of splicing a mangled relative
path (the prefix rules mirror internal/daemon's #6883 handling).
2. An os.Readlink failure on a ModeIrregular reparse point no longer
means "does not redirect". Readlink decodes only the symlink and
mount-point tags; other name-surrogates (e.g. container-isolation
links) redirect through a filter while the lexical location stays
behind. Such components are now resolved through
GetFinalPathNameByHandle (zero desired access, so no hydration, and
FILE_FLAG_BACKUP_SEMANTICS for directories); if the handle names the
same path the component is opaque, otherwise the walk continues from
the final path. When even the handle cannot name the location the
resolver returns the new util.ErrUnresolvablePath instead of
guessing: fileWithinWorkingDir treats it as outside (fail closed)
and execenv's delegate falls back to the input, which its
containment comparison rejects.
ResolveSymlinksBestEffort now returns an error so the fail-closed case
is explicit rather than a look-alike path. Negative tests cover a
root-relative symlink target with an in-workdir shadow (the kernel's
target is verified by content before asserting rejection) and a
volume-GUID junction target with the prefix-stripped shadow.
* fix(cli): classify drive-absolute link targets before the volume check (MUL-4252)
classifyTarget asked VolumeName before IsAbs, but VolumeName is
non-empty for a drive-absolute path (C:\dir) too. A junction's absolute
target was therefore misread as drive-relative and resolved against the
working directory — a nonexistent spelling — which errored the
resolution and left the escape to be judged lexically inside the
workdir. The IsAbs branch is asked first, and the load-bearing ordering
is pinned by TestClassifyTarget, a fixture-free table over the target
grammar that would have caught this without a filesystem.
* ci: run the util link-target classification table on Windows
TestClassifyTarget pins the branch ordering classifyTarget's link-target
grammar depends on, but windows-tagged tests in internal/util had no
execution point: the windows-execenv job only ran the guard tests in
cmd/multica. Scoped to the table by name, matching the job's pattern.
* test(cli): correct classifyTarget expectations for root and base shapes
The walk base is the volume root and the working directory's components
live in the segments for a drive-relative target, and the plain-relative
branch returns the link's directory verbatim without a trailing
separator. The expectations said otherwise; the code was right.
---------
Co-authored-by: weiweiwei <[email protected]>