Reject relative existing --target-dir as invalid_target before composition - #369

Fleet review (qwen38)

Lens: evidence. The fix matches issue #367's stated contract: runDirectoryInstall now refuses any target that is not absolute or not clean with error=invalid_target before any stat or composition work, and the new relative-existing test case reproduces the exact repro shape from the issue (a relative path pointing at a real directory). The claim that the case was RED before (exit 70, install_composition_failed) and GREEN after is reproducible from the PR via go test -run TestInstallDirectoryTargetFailClosedOnInvalidPaths ./internal/cli. No blocking defects found. Evidence gaps: the absolute-but-unclean branch of the new condition has no test, and the stated verification omits gofmt.

Suggestion (2)

• relative-existing fixture is created in the package working directory (internal/cli/install_directory_test.go:113): The test does os.MkdirAll("test-invalid-target-exists", 0o700) relative to the test binary's CWD (the package source dir) and defers os.RemoveAll, ignoring the error. Other tests in the same package share that CWD, so a parallel run or a leftover directory from an interrupted earlier run could interfere. Using a path inside t.TempDir() (and passing it as a relative target after t.Chdir) would keep the same repro shape without touching the checkout.
• Verification claim omits a formatting check (internal/cli/install_directory_test.go:117): The PR states the full ./internal/cli suite and go vet pass, but the reformatted failClosedCases map in the diff shows inconsistent value alignment (notably "empty"), which gofmt normalizes. If CI runs gofmt, add that to the stated verification; if it does not, align the map so the file stays gofmt-clean.

Question (1)

• Absolute-but-unclean targets are now refused with no test pinning them (internal/cli/install.go:106): The new guard !filepath.IsAbs(target) || target != filepath.Clean(target) also rejects absolute paths that merely need cleaning (e.g. /abs/dir/ with a trailing slash, /abs/./dir), which previously passed the stat check and proceeded into composition. This is consistent with the issue's stated contract ("not absolute, not clean" is malformed and must share the one invalid_target diagnostic), so I do not treat it as a defect — but no test case in the added table covers an existing, absolute, unclean target, so that behavior change is asserted only by the code, not by evidence. Please confirm the refusal of cleanable absolute paths is intended user-visible behavior, and consider a test case for it.

Verdict: ready to merge

Fleet review (glm)

Lens: requirements + adversarial. The change satisfies the explicit acceptance criterion of issue #367: an existing relative path now fails closed with error=invalid_target (exitUsage) before any stat or composition work, and the new relative-existing test case pins the exact repro shape from the issue (RED as install_composition_failed/exit 70, GREEN as invalid_target). The check runs before os.Stat, so no write, backup, or composition happens for a malformed target. The clean-vs-not check uses target != filepath.Clean(target), which matches the contract wording ('not absolute, not clean'); one consequence is a behavior change for previously-accepted inputs (see question below). No blocking findings.

Suggestion (1)

• relative-existing fixture is created in the test's working directory (internal/cli/install_directory_test.go:114): The new fixture uses a package-relative path (os.MkdirAll("test-invalid-target-exists", ...)). go test runs in the package directory so this works, but a relative fixture couples the test to its cwd; using t.TempDir() plus a relative path built from it, or t.Chdir, would make the case independent of invocation context. Not required for merge.

Question (1)

• Absolute targets with a trailing slash were accepted before and are now refused as invalid_target (internal/cli/install.go:108): The new check rejects any target where target != filepath.Clean(target). An absolute path with a trailing slash, e.g. /tmp/skills-target/, is not Clean-stable (Clean drops the slash), so it now exits with invalid_target. In v0.1.0-alpha.17 such a path passed os.Stat and the install proceeded. This conforms to the contract's literal wording ('not absolute, not clean') but is a behavior change for inputs that used to work. Is refusing trailing-slash absolute paths intended, or should the contract's 'clean' be read as covering only path traversal/artifacts like foo/../bar?

Verdict: ready to merge

Fleet review (glmflash)

Lens: adversarial

The fix is narrow and consistent with the stated contract: runDirectoryInstall now refuses any target that is not absolute or not clean before the os.Stat gate, which closes the relative-existing hole from #367 (a relative existing directory passed Stat/IsDir and surfaced as install_composition_failed/exit 70 instead of invalid_target). Boundary cases I attacked: empty target (IsAbs("")==false, refused with the same diagnostic as before), absolute-but-unclean paths like /tmp/./x (now correctly refused per the 'not clean' contract, previously they slipped into composition), trailing-slash paths (Clean normalizes them away, refused), and non-existing relative paths (unchanged refusal). No blocking defects found. One test-robustness concern below: the new relative-existing case silently degenerates into the pre-existing 'relative' case if the command under test ever runs with a different working directory, which would make the test pass even without the fix.

Suggestion (1)

• relative-existing case depends on the test binary's cwd matching the command's cwd (internal/cli/install_directory_test.go:111): The new case creates 'test-invalid-target-exists' relative to the process working directory. If the harness ever invokes the command under test with a different working directory (cmd.Dir or similar), the relative path no longer exists for the command, os.Stat fails, and the old code path already returned invalid_target — the test would then pass even if the new IsAbs/Clean check were removed.

Verdict: ready to merge

Fleet review (mimo)

Lens: requirements. The diff satisfies issue #367's repro and stated acceptance: runDirectoryInstall now refuses any non-absolute or non-clean target with error=invalid_target before os.Stat and before any composition work (install.go:108), and the new relative-existing case pins the exact repro shape the issue says previously exited 70 with install_composition_failed, asserting both the error string and that nothing was applied. Verified at the reviewed head that runDirectoryInstall is only reachable from the --target-dir branch of runInstallCommand, so the default (no flag) install path is unaffected. Two non-blocking gaps: the issue's Expected behavior names four malformed classes but the 'not clean' class has no test case, and the strict filepath.Clean comparison flips previously-working absolute inputs (trailing slash, /./, //) to invalid_target - that follows the contract as written ('not clean' is listed as malformed), but the intent for the trailing-slash form should be confirmed because the issue's repro and release-notes quote only concern relative paths.

Suggestion (1)

• 'Not clean' malformed class from the issue has no test case (internal/cli/install_directory_test.go:117): Issue install --target-dir with a relative existing path fails with opaque install_composition_failed instead of invalid_target #367's Expected behavior lists four malformed classes (not absolute, not clean, not existing, not a directory). The failClosedCases map covers relative, relative-existing, missing, file and empty, but no case exercises an absolute-but-unclean target such as /x/y/ or /x/./y that the new guard at install.go:108 rejects via filepath.Clean. The code handles the class, but nothing pins it, so a future refactor of the guard could drop the Clean check without any test failing.

Question (1)

• Confirm trailing-slash and other unclean absolute targets are meant to fail (internal/cli/install.go:108): target != filepath.Clean(target) rejects absolute targets that installed successfully before this PR: filepath.Clean strips trailing separators, so --target-dir /srv/cortex/ (and forms like /srv/./cortex or /srv//cortex) now returns error=invalid_target where v0.1.0-alpha.17 completed the install. The issue does list 'not clean' among malformed targets, so this follows the contract as written, but the repro and the release-notes quote are only about relative paths and a trailing slash is the common form in automation. Confirm the stricter reading is intended so callers of existing scripts know the flip from success to invalid_target is by design.

Verdict: ready to merge

Fleet review (deepseek)

Lens: evidence + test design. The diff closes the reported repro (issue #367): a relative path that exists on disk now stops at the new absolute+clean guard in runDirectoryInstall with error=invalid_target and exitUsage, before any stat, composition or backup work. The added relative-existing case is genuine evidence, not a tautology: before the fix that exact shape reached composition and produced error=install_composition_failed with exit 70, so the assertion on error=invalid_target discriminates the two behaviours, and the case would fail again if the guard were removed or moved after composition. Two gaps remain, neither blocking. First, the guard refuses inputs that installed successfully at alpha.17: filepath.Clean makes /tmp/skills/ (trailing slash, the usual shape of a shell completion) differ from the target, so it is now rejected as invalid_target, and on Windows it rewrites separators, so a valid C:/Users/me/skills is rejected too. The issue lists "not clean" as a malformed shape, so this may be exactly the intended contract, but the PR body presents the change as fixing only the relative-existing case and nothing states that a previously-installing absolute form is now refused. Second, the target != filepath.Clean(target) half of the new condition has no test at all: the test table covers not-absolute (relative, relative-existing, empty), not-existing (missing) and not-a-directory (file), but no case reaches the clean check. That is the branch that carries the behaviour change, and it is the one shape the issue names that stays unverified.

Suggestion (1)

• The 'not clean' clause of the new guard has no test case (internal/cli/install_directory_test.go:119): The new condition has two halves, !filepath.IsAbs(target) and target != filepath.Clean(target). Every case in failClosedCases exercises the first half or the stat check; none reaches the clean comparison, so the branch this PR adds is unverified while the relative-existing case only proves the absolute half. Missing test conditions for TestInstallDirectoryTargetFailClosedOnInvalidPaths, each an absolute existing directory that os.Stat/IsDir would accept: (a) trailing separator, e.g. filepath.Join(home, "agent-skills") + string(os.PathSeparator); (b) a dot segment, e.g. filepath.Join(home, ".", "agent-skills"); (c) a doubled separator, e.g. home + string(os.PathSeparator) + string(os.PathSeparator) + "agent-skills"; (d) a .. segment, e.g. filepath.Join(home, "other", "..", "agent-skills") after creating other. Cases (a) and (c) also pin the answer to the question above, so whichever behaviour is intended ends up recorded instead of inferred from the guard.

Question (1)

• Guard refuses absolute paths that installed before: trailing slash, and separator rewriting on Windows (internal/cli/install.go:108): filepath.Clean("/home/me/agent-skills/") is /home/me/agent-skills, so the comparison target != filepath.Clean(target) refuses a perfectly valid absolute existing directory whenever the caller keeps a trailing separator; at alpha.17 (issue install --target-dir with a relative existing path fails with opaque install_composition_failed instead of invalid_target #367's environment) that same command completed with status=completed touch=applied. On Windows filepath.Clean also rewrites / to \, so C:/Users/me/skills (IsAbs true, a valid path) is refused as well. The directory-target contract quoted in the issue treats "not clean" as malformed, so the refusal may be intended, but the PR body only claims the relative-existing fix and neither the release note nor the tests state that a non-canonical absolute form is now rejected. Please confirm the intended reading: is a trailing-slash absolute path meant to be refused as invalid_target, or normalized and installed? The answer decides whether this guard is the contract or a regression, and it is the only user-visible behaviour of the change that no acceptance criterion settles.

Verdict: ready to merge

Post-merge QA (glm)

Lens: requirements + adversarial. The change satisfies the explicit acceptance criterion of issue #367: an existing relative path now fails closed with error=invalid_target (exitUsage) before any stat or composition work, and the new relative-existing test case pins the exact repro shape from the issue (RED as install_composition_failed/exit 70, GREEN as invalid_target). The check runs before os.Stat, so no write, backup, or composition happens for a malformed target. The clean-vs-not check uses target != filepath.Clean(target), which matches the contract wording ('not absolute, not clean'); one consequence is a behavior change for previously-accepted inputs (see question below). No blocking findings.

Suggestion (1)

• relative-existing fixture is created in the test's working directory (internal/cli/install_directory_test.go:114): The new fixture uses a package-relative path (os.MkdirAll("test-invalid-target-exists", ...)). go test runs in the package directory so this works, but a relative fixture couples the test to its cwd; using t.TempDir() plus a relative path built from it, or t.Chdir, would make the case independent of invocation context. Not required for merge.

Question (1)

• Absolute targets with a trailing slash were accepted before and are now refused as invalid_target (internal/cli/install.go:108): The new check rejects any target where target != filepath.Clean(target). An absolute path with a trailing slash, e.g. /tmp/skills-target/, is not Clean-stable (Clean drops the slash), so it now exits with invalid_target. In v0.1.0-alpha.17 such a path passed os.Stat and the install proceeded. This conforms to the contract's literal wording ('not absolute, not clean') but is a behavior change for inputs that used to work. Is refusing trailing-slash absolute paths intended, or should the contract's 'clean' be read as covering only path traversal/artifacts like foo/../bar?

Verdict: clean

Post-merge QA (deepseek)

Lens: evidence + test design. The fix targets the right gate: runDirectoryInstall now refuses a non-absolute or unclean target before the os.Stat check, so a relative path that exists on disk reaches the explicit invalid_target diagnostic and exitUsage instead of failing later as install_composition_failed. The new relative-existing case is the exact shape from the issue repro (a relative path pointing at a real directory created in the test, cleaned up with defer), and it sits in the same fail-closed table as relative/missing/file/empty, so the previously untested branch is now pinned. The refusal also happens before any stat or composition work, which matches the stated contract that malformed targets fail closed before any write. One behavior change is worth confirming with the maintainer: the new absolute+clean precondition also rejects an absolute existing directory passed with a redundant separator or dot segment, which used to install successfully.

Question (1)

• Absolute targets with a trailing separator or dot segment now fail as invalid_target (internal/cli/install.go): The new precondition is !filepath.IsAbs(target) || target != filepath.Clean(target). That is a strict comparison against the cleaned form, so --target-dir /Users/x/skills/ (trailing separator) and --target-dir /Users/x/./skills are now refused with exitUsage/invalid_target, while before this merge they passed os.Stat and installed. Issue install --target-dir with a relative existing path fails with opaque install_composition_failed instead of invalid_target #367 lists 'not clean' among the malformed shapes, so this may be exactly the intended tightening and the release notes already promise that malformed targets fail closed; it is raised as a question because the change also removes a previously working invocation shape and no test pins the trailing-separator case either way. If the refusal is intended, a case for /abs/existing/ documenting it (or normalizing with filepath.Clean before the check) would make the contract testable.

Verdict: clean