Fleet review (qwen38)

Lens: evidence. The PR implements exactly what issue #372 asks: one shared validTargetDir check (absolute and filepath.Clean, extracted from the #369 install check) now gates doctor, runDirectoryUninstall, runDirectoryUpdate, and install before any directory read or write, with the same error=invalid_target exit 64 diagnostic. The new test pins all four commands against a relative existing dir, a relative missing path, and an absolute-but-unclean path that resolves to the existing dir, asserting exit code, exact stderr, empty stdout, an unchanged directory listing (including a foreign sentinel file), and that no path is created — solid, reproducible evidence for the acceptance criterion on Unix. Install behavior is unchanged (pure extraction) and documented absolute-path behavior stays pinned by the four named pre-existing tests. Rollback is a single-commit revert with no format changes. No blocking defect found; two evidence gaps are recorded as a question and a suggestion. Context output was complete (not truncated).

Suggestion (1)

• Windows target shapes rely on the manual harness only (internal/cli/directory_lifecycle_test.go:129): The runtime-harness section claims drive-relative (C:rel), rooted-without-drive (\Users...\rel) and mixed-separator (C:...\smoke/rel) targets are refused with exit 64 on Windows. filepath.IsAbs/filepath.Clean have exactly these platform quirks, yet the automated test uses only Unix-shaped targets (the unclean case is built with string(filepath.Separator) and a '.' element); it passes on windows/amd64 but asserts nothing Windows-specific. Those claims are currently reproducible only through the author-run container/VM harness. Consider adding GOOS=windows-gated cases for the three claimed shapes to the test so this evidence becomes machine-reproducible; behavior in this PR is fine as-is since the shared function is the exact one install (Reject relative existing --target-dir as invalid_target before composition #369) already shipped.

Question (1)

• update: which refusal wins when both target and catalog are invalid is untested (internal/cli/update.go:238): The PR body claims every --target-dir command runs the target check 'before it reads or writes the directory', and the Before/After table shows update previously returning exit 70 error=invalid_catalog for a relative target with an unusable catalog. The validTargetDir call is the first statement of runDirectoryUpdate, but the diff does not show where the caller resolves/validates the catalog relative to this call, and the new test always passes a valid catalog ('../../catalog' with --apply, explicitly so the refusal cannot come from the catalog). So for cortex update --target-dir <relative> --catalog <invalid> the shipped diagnostic (invalid_target vs invalid_catalog) is asserted only in the PR description, not by any test. Please confirm (or pin in a test) which diagnostic is emitted when both are invalid, and that it is the intended one for the shared-contract goal of doctor --target-dir accepts relative paths while install refuses them as invalid_target #372.

Verdict: ready to merge

Fleet review (glmflash)

Lens: adversarial. The fix correctly extracts the #369 install check into a shared validTargetDir (absolute && Clean-identical) and applies it in doctor, update, and uninstall before any stat, read, or write, which satisfies issue #372's stated direction (a target install refuses must be refused the same way everywhere). I attacked boundary paths: Windows drive-relative and rooted-without-drive paths fail IsAbs; trailing separators and '/./' segments fail the Clean comparison; empty --target-dir still routes to host doctor as before; the check runs before doctor's probe write and before update's stat, so the previous misleading invalid_catalog/target_absent outcomes and the doctor probe-file write can no longer occur for relative targets. The new test exercises all four commands through the binary entry point (runWithDependencies) against existing, missing, and unclean-absolute targets and asserts no writes, including the foreign sentinel and the not-created missing path. One nuance: uninstall remains stricter than the shared check (skillroot also refuses the filesystem root and mixed separators, e.g. Windows 'C:...\smoke/rel' passes validTargetDir but is refused by uninstall), so the docs phrase 'refuse ... the same way' is slightly overbroad — but this divergence is fail-closed, explicitly documented in the PR's out-of-scope notes, and does not contradict the issue's one-directional contract.

Suggestion (1)

• Docs say doctor/update/uninstall refuse 'the same way', but uninstall is stricter (docs/operating.md:80): validTargetDir accepts absolute clean paths that skillroot.NewUninstallRoot still refuses (filesystem root, mixed separators like 'C:\...\smoke/rel' on Windows, which Clean normalizes to a valid path). The new doc bullet says these commands refuse a relative or unclean --target-dir 'the same way'; for uninstall the refusal set is a strict superset. The PR body documents this divergence honestly, but the user-facing doc does not.

Verdict: ready to merge

Fleet review (glm)

Lens: requirements + adversarial. The diff satisfies issue #372's expected direction: the #369 absolute-and-clean check is extracted into one shared validTargetDir and is now the first thing runDirectoryInstall, runDoctorCommand, runDirectoryUninstall, and runDirectoryUpdate do with --target-dir, before any stat, plan, or write. A code search over the repo confirms --target-dir is consumed only by install.go, doctor.go, and update.go, so no command that takes the flag was missed. Install and uninstall behavior is unchanged (uninstall keeps the stricter skillroot validation behind the shared check, both producing the same invalid_target diagnostic). The new test covers the three adversarial shapes that matter (relative existing, relative missing, absolute-but-unclean naming a real directory), asserts exit 64 with exactly error=invalid_target, empty stdout, no writes to the existing directory, and that the missing relative path is not created. The Windows-harness matrix (drive-relative, rooted-without-drive, mixed separators) is consistent with filepath.IsAbs/filepath.Clean semantics. No blocking findings.

No findings.

Verdict: ready to merge

Fleet review (mimo)

Lens: requirements. The diff fully implements the path-form half of issue #372: I verified by source inspection that exactly four commands consume --target-dir (install, doctor, update, uninstall) and all four now run the shared validTargetDir guard before any stat or write, with the uniform error=invalid_target / exit 64 diagnostic, plus a test pinning 4 commands x 3 relative/unclean targets. However, against the issue's Expected direction read literally - 'a target that install would refuse as invalid_target should be refused the same way by every command that consumes --target-dir' - the criterion is satisfied only partially: install also refuses absolute missing and non-directory targets as invalid_target (install.go:119-121), while doctor (doctor.go:122 -> 134) and update (update.go:243-245) still accept those same absolute targets with different diagnostics, and the PR's own docs edit states the two rules adjacently without reconciling them (operating.md:81-82). One clarification question remains on the --target-dir= form, which install accepts but doctor/update/uninstall reject as invalid_command before any target validation.

Blocking (1)

• Issue doctor --target-dir accepts relative paths while install refuses them as invalid_target #372 criterion satisfied only for relative/unclean targets; doctor and update still accept absolute targets install refuses as invalid_target (internal/cli/doctor.go:122): Issue doctor --target-dir accepts relative paths while install refuses them as invalid_target #372's Expected direction is explicit: 'a target that install would refuse as invalid_target should be refused the same way by every command that consumes --target-dir, so the directory-target contract stays consistent across the CLI.' At the reviewed head the shared guard validTargetDir (install.go:102) checks only path form (absolute + clean). install additionally refuses any absolute target that is missing or not a directory as error=invalid_target, exit 64 (install.go:119-121). doctor still forwards such a target at doctor.go:122 and reports presence=absent with exit 0 (doctor.go:134); a regular file yields presence=unreadable with exit 3 (exitConflict). update still stats the path at update.go:243 and returns reason=target_absent with exit 2 (writeUpdateRefusal returns exitUnknown=2). docs/operating.md:81, untouched by this PR, documents install's refusal set as 'the path is missing, relative, or not a directory', while the sentence added by this PR at docs/operating.md:82 claims only 'a relative or unclean --target-dir' is refused by doctor, update, and uninstall - the PR narrows the stated scope without reconciling it with the issue criterion or listing it under its Out of Scope section. The new test TestDirectoryTargetCommandsRefuseRelativeTargets exercises only relative and unclean targets, so the divergence is covered by no assertion. Note for triage: doctor's documented contract (docs/operating.md table) is to report a directory target's presence, so the issue criterion as written and the preserved presence=absent behavior are in tension - the intended scope needs an explicit decision rather than the silent narrowing.
  • Failure scenario: Given the absolute path /abs/missing that does not exist: cortex install --target-dir /abs/missing exits 64 with stderr error=invalid_target, but cortex doctor --target-dir /abs/missing exits 0 with stdout target=/abs/missing presence=absent, and cortex update --target-dir /abs/missing --catalog <catalog> --apply exits 2 with stdout ending reason=target_absent - none of them refuses the target the way install would, contradicting the quoted criterion. Given an absolute path that is a regular file: install exits 64 error=invalid_target, doctor exits 3 presence=unreadable, update exits 2 reason=target_absent. The commands still disagree on what a valid target is, which is the exact inconsistency doctor --target-dir accepts relative paths while install refuses them as invalid_target #372 reports.

Question (1)

• Does 'refused the same way' in doctor --target-dir accepts relative paths while install refuses them as invalid_target #372 cover the --target-dir= form? (internal/cli/doctor.go:103): install parses both --target-dir <path> and --target-dir=<path> (install.go:80-86) and refuses a relative value in the equals form as error=invalid_target, exit 64. doctor (doctor.go:103), uninstall (doctor.go:577), and update (update.go:151) accept only the space-separated form: the equals-form token falls into their default/unknown-argument branch and returns error=invalid_command, exit 64, before any target validation runs. Issue doctor --target-dir accepts relative paths while install refuses them as invalid_target #372 asks that a target install would refuse as invalid_target 'be refused the same way by every command that consumes --target-dir'. The invocation is refused in every case (exit 64, no writes), but the diagnostic token differs by command for the same user input. This parse asymmetry predates this PR (flag handling from feat: report directory targets in cortex doctor #343/Reject relative existing --target-dir as invalid_target before composition #369) and the new test exercises only the space-separated form, so please confirm whether equals-form parity is in scope for doctor --target-dir accepts relative paths while install refuses them as invalid_target #372 or tracked separately.

Verdict: not ready

Fleet review (deepseek)

Lens: evidence + test design. The shared validTargetDir check is wired correctly into install, doctor, update, and uninstall (all four --target-dir consumers in internal/cli), and the new test drives every command through runWithDependencies and asserts exit 64, exact stderr, empty stdout, and an unchanged target listing, so the relative and unclean refusal classes are genuinely pinned for doctor and update. One member of the class the PR claims to close survives: an empty --target-dir value is tested before the guard, and for uninstall that branch is the destructive all-roots path. Evidence and test design: the recorded RED/GREEN evidence is reproducible from the diff for the three covered inputs, but the new test has no case for the empty value or the --target-dir= form, and its uninstall subtests cannot distinguish the added guard from the pre-existing skillroot check.

Blocking (1)

• An empty --target-dir value never reaches the shared check (internal/cli/doctor.go:115): runDoctorCommand returns early on targetDir == "" (doctor.go:115) before the new validTargetDir guard at doctor.go:118, and runUninstallCommand does the same at doctor.go:589 before the guard inside runDirectoryUninstall at doctor.go:598. So --target-dir "" is consumed by both commands without ever being validated, while cortex install --target-dir "" reaches runDirectoryInstall and is refused as invalid_target (install.go:115). The PR body states that every --target-dir command now runs the same check before it reads or writes the directory, and issue doctor --target-dir accepts relative paths while install refuses them as invalid_target #372 requires a target that install refuses as invalid_target to be refused the same way by every command that consumes --target-dir. For doctor and uninstall that statement does not hold, and the uninstall branch is not read-only. cortex update --target-dir "" at least refuses, but with invalid_command instead of invalid_target.
  • Failure scenario: cortex uninstall --target-dir "$TARGET" with TARGET unset (or cortex uninstall --target-dir "") takes the targetDir == "" branch at doctor.go:589 and runs the canonical uninstall across every present runtime root: it removes all Cortex-owned skills and exits 0 instead of exiting 64 with error=invalid_target, which is what cortex install --target-dir "" does for the same value. cortex doctor --target-dir "" similarly runs the full host runtime probe and reports exit 0 with the host matrix, silently ignoring the flag the caller passed, which is the ambiguity issue doctor --target-dir accepts relative paths while install refuses them as invalid_target #372 describes for relative paths.

Suggestion (2)

• New test does not cover the empty value, the --target-dir= form, or writes outside the target (internal/cli/directory_lifecycle_test.go:169): The test covers three targets (relative existing, relative missing, absolute unclean) against four commands. Missing test conditions, each with input and expected outcome: (1) --target-dir "" for install, doctor, update, uninstall, expecting exit 64 and stderr exactly error=invalid_target\n; this is the input that today still reaches the host probe and the canonical uninstall, so it is the one condition that would have caught the remaining hole. (2) install --target-dir=./rel, the form parseTargetDir accepts through its --target-dir= branch (install.go:80), expecting exit 64 and error=invalid_target\n. (3) an assertion that the caller's home directory is unchanged, since the PR claims a refusal writes nothing and the test only compares the target directory listing and checks that relative/ was not created; install and uninstall both write under home on the success path, so home is the other place a refusal must leave untouched.
• The added uninstall guard is not distinguishable from the existing skillroot check (internal/cli/doctor.go:598): skillroot.validPath (internal/skillroot/resolve.go:242) already requires filepath.IsAbs, filepath.Clean(path) == path, no foreign separator, and a non-root path, so runDirectoryUninstall already produced exit 64 error=invalid_target for all three inputs the new test uses. The uninstall subtests therefore pass with or without the line added at doctor.go:598, and the PR's claim that uninstall now shares the check explicitly is not evidenced by the test. That is consistent with the body's statement that uninstall behavior is unchanged, but it means the guard's only observable effect is error precedence, and no test distinguishes it.

Verdict: not ready

Thanks for the reviews. I pushed ff42157.

Empty --target-dir value (deepseek, blocking). Confirmed and fixed. On main, cortex uninstall --target-dir "" ran the canonical all-roots uninstall and exited 0, so an unset $TARGET removed every Cortex-owned skill. doctor --target-dir "" ran the host report, and update refused it as invalid_command. doctor, uninstall, and update now track whether the flag was given, so an empty value reaches validTargetDir and is refused with error=invalid_target, exit 64. The refusal test's uninstall dependencies now fail the test if the canonical roots are ever resolved, so it cannot reach a real home even before a fix (that is how the RED run caught it: canonical uninstall roots were resolved). Differential with the real binary, main vs this head, same inputs, Linux container: only the three empty-value lines differ. doctor, uninstall, and update --runtime pi without --target-dir, plus every valid absolute target, give identical exit code, stdout, and stderr.

Absolute missing or non-directory targets (mimo, blocking). This one needs your call, because the issue text and doctor's documented contract pull in different directions. My claim comment and your acceptance ("one refusal test per consuming command, error=invalid_target, exit 64, no writes") scoped #372 to the path form. Doctor's documented job (#343, the docs/operating.md table) is to report presence=absent for a missing target, and refusing it would remove that. I kept the path-form scope, and the doc now says exactly that: empty, relative, or unclean targets are refused the same way everywhere, while a missing absolute target keeps each command's own answer (doctor presence=absent, update reason=target_absent). If you want update to refuse a missing target as invalid_target too, I'm happy to do it as a follow-up.

--target-dir=<value> form (mimo; deepseek). Install accepts the equals form; doctor, update, and uninstall reject it as invalid_command before any target check. Both refuse with exit 64 and no writes, and the parse asymmetry predates this PR (#343/#369). I added a test that pins install refusing --target-dir=<relative> as invalid_target. Extending the equals form to the other commands adds accepted syntax, so I'd keep it for a separate issue if you want it.

Windows path shapes (qwen38). The test now adds Windows-only targets: drive-relative (C:rel), rooted without a drive (\Users\...\rel), and mixed separators (C:\...\dir/rel). All 30 subtests pass natively on Windows 11 (this head merged with #386 in a scratch worktree), and 18 pass on Linux.

Target vs catalog precedence (qwen38). The target check is the first statement of runDirectoryUpdate, and the catalog is read only after it. Pinned by update/target-checked-before-catalog: a relative target with a missing catalog gives error=invalid_target.

Writes outside the target (deepseek). Each subtest now also asserts the home directory is unchanged after the refusal.

Uninstall guard indistinguishable (deepseek). Right. skillroot.validPath already refuses every input the shared check refuses, the empty value included, so for uninstall the shared call only fixes precedence and no test can tell it apart. I kept it so all four commands visibly run the same rule; the observable uninstall fix here is the empty-value routing, pinned by uninstall/empty.

Docs vs uninstall strictness (glmflash). The doc now says uninstall also refuses a filesystem root. One correction: mixed separators like C:\...\smoke/rel do not pass validTargetDir on Windows, because filepath.Clean rewrites / to \ and the comparison fails; the new Windows subtest pins that for every command.

Verification: Linux container (non-root, LF): go vet ./... and go test ./... clean; policy contracts pass; binary differential as above. Windows 11: the refusal test, 30/30.

Fleet review (qwen38)

Lens: evidence. Issue #372's criterion is that any target install refuses as invalid_target must be refused the same way by every command consuming --target-dir, before any read or write. The diff shares the extracted validTargetDir (absolute and clean) across doctor, update, and uninstall, and tracks flag presence so an empty value can no longer fall back to the host report or the all-roots uninstall. Evidence audit: TestDirectoryTargetCommandsRefuseRelativeTargets goes through runWithDependencies (the binary's entry point) and covers all four commands against four representative target shapes (existing relative dir with a foreign sentinel, missing relative path, absolute-but-unclean path naming the existing directory, empty value), asserting exit 64, stderr exactly error=invalid_target, empty stdout, an unchanged target listing, an uncreated missing path, and an unchanged home; the guarded resolveRoots makes even a RED run unable to touch a real home. I verified from the head source that the helpers exist (canonicalHome install_test.go:41, directoryInstallDependencies install_directory_test.go:16), exitUsage = 64 (doctor.go:35), and the parseUpdateArgs tail only whitelists pi/opencode/claude-code, so runtimeID == "" genuinely implies directory mode — the PR's key claim holds. Every acceptance criterion is backed by a reproducible test; the Windows and container harness runs are additional asserted evidence that the test gates cleanly on non-Windows platforms. One diagnostic-ordering nuance is recorded as a question; nothing blocks.

Question (1)

• Incomplete update command line yields invalid_command, not invalid_target (internal/cli/update.go:179): parseUpdateArgs returns ok=false (invalid_command, exit 64) when --target-dir is set with the directory mode's other arguments incomplete (missing --catalog, or --adopt-guidance), before runDirectoryUpdate ever runs the target check. So cortex update --target-dir <relative> without --catalog reports invalid_command while cortex doctor --target-dir <relative> reports invalid_target. This is a safe refusal either way and the parse-before-validate order predates this PR, but docs/operating.md now says update 'refuses an empty, relative, or unclean --target-dir' with the same error, which is only true once the rest of the directory-mode arguments are valid. Does the doc want that qualifier, or is the usage-error precedence intentional and fine as is?

Verdict: ready to merge

Fleet review (glm)

Lens: requirements + adversarial. The PR satisfies issue #372's core requirement: the #369 target check is extracted into validTargetDir and every command that consumes --target-dir (doctor, update, uninstall) now refuses relative, unclean, and empty values with exit 64 error=invalid_target before reading or writing anything. The previously destructive uninstall --target-dir "" fallback to the canonical all-roots uninstall is now refused, which is the most important safety win. The test matrix is thorough (relative existing/missing, unclean absolute naming an existing dir, empty, Windows drive-relative/rooted-without-drive/mixed-separator shapes, equals-form, target-before-catalog precedence) and the guarded uninstall dependencies plus home-unchanged assertions make a RED run safe. Docs match the new behavior. No blocking defects found; two clarifications below, one of which the author already surfaced for a maintainer ruling.

Question (2)

• Missing absolute target still diverges from install, per the issue's literal direction (internal/cli/update.go:244): Issue doctor --target-dir accepts relative paths while install refuses them as invalid_target #372's expected direction says a target install would refuse as invalid_target should be refused the same way by every command consuming --target-dir. Documented install behavior refuses a missing absolute directory as invalid_target (docs: 'the path is missing, relative, or not a directory'), but this PR keeps update's reason=target_absent and doctor's presence=absent for missing absolute targets. The named problem in the issue (relative/unclean/empty divergence) is fixed and tested; this residual divergence is explicitly documented and deferred to a maintainer decision. My position as reviewer: keeping doctor's presence report for a missing absolute target is coherent, because doctor's documented job is to report presence, and forcing uniformity there would break its purpose; update refusing with target_absent is also defensible. But it needs an explicit ruling from Juan so the directory-target contract is written down, not just inferred.
• Mode selection now keys on empty runtime ID; confirm --runtime "" cannot route here (internal/cli/update.go:81): runWithUpdateDependencies now selects directory mode with if runtimeID == "", relying on the claim that the runtime mode never returns an empty runtime ID. The diff does not show whether parseUpdateArgs rejects an explicitly empty --runtime "" value. If it is accepted (ok=true with runtimeID="" and targetSet=false), that invocation now exits 64 error=invalid_target via runDirectoryUpdate("") instead of taking the runtime path as before. Either way the command fails safely with exit 64, so this is a diagnostic question, not a defect — but the invariant 'runtime mode never returns an empty runtime ID' should hold by construction (e.g. an explicit ok=false for an empty --runtime value), not by convention.

Verdict: ready to merge

Fleet review (deepseek)

Lens: evidence + test design. I re-reviewed from scratch at head ff42157 (my previous review of 1227271 blocked on the empty-value hole; the targetSet rework and the empty target in the new table resolve it, and I confirmed both commands in the head sources). The extraction of the #369 check into validTargetDir and its adoption in doctor, update and uninstall is correct. The empty-value handling is the substantive fix: the old targetDir != "" sentinel made --target-dir "" fall through to the host runtime report (doctor) and, worse, to the canonical all-roots uninstall (uninstall); both now return exit 64 error=invalid_target. I checked the head sources rather than only the diff: all four commands that consume the flag (install, doctor, update, uninstall; no other non-test internal/cli file mentions --target-dir) route through the shared check; the new runtimeID == "" dispatch in runWithUpdateDependencies is equivalent to the old targetDir != "" test because parseUpdateArgs returns ok == true with an empty runtime ID only on the directory branch, and rejects a missing or unknown --runtime as invalid_command before that point; and the guarded uninstall dependency cannot reach the canonical roots, so a missing guard would fail the test instead of touching a real home. The evidence arithmetic is honest (4 commands x 4 targets + 2 = the 18 subtests claimed on Linux; 4 x 7 + 2 = the 30 claimed on Windows), every subtest pins exit 64, the exact stderr and empty stdout, and checks that neither the target listing, the home directory, nor the missing path changed. No blocking defect. The remaining gap is the missing-absolute-target carve-out this PR discloses, which the issue's stated direction does not obviously allow and which needs a recorded decision rather than a code change.

Suggestion (1)

• New table has no accepted-target control case and no trailing-separator case (internal/cli/directory_lifecycle_test.go:186): The table covers existing (relative), missing (relative), unclean (<cwd>/./<dir>) and empty, plus the Windows shapes, and every subtest asserts a refusal. Two conditions are unpinned. First, there is no subtest proving validTargetDir does not over-refuse a valid absolute target, so a future change to the check that rejects legitimate targets would keep this whole table green; the acceptance evidence lives in TestDoctorDirectoryTargetStates, TestUpdateDirectoryTargetPlansAndApplies and TestUninstallDirectoryTargetRemovesOnlyOwnedFiles, which is fine but is exactly the coupling this test exists to guard. Second, a trailing separator is not in the table: --target-dir /home/u/agent-skills/ passes filepath.IsAbs but fails target == filepath.Clean(target), so doctor, update and uninstall now exit 64 for a target they accepted on main (only install already refused it), and shell tab completion produces that form for directory arguments. If that refusal is intended, pin it with a subtest and name the form in docs/operating.md instead of relying on the word "unclean" to carry it; if it is not, normalize with filepath.Clean before the check.

Question (1)

• Missing absolute target still answers differently in doctor and update than in install (docs/operating.md:84): The linked issue's expected direction says: "a target that install would refuse as invalid_target should be refused the same way by every command that consumes --target-dir, so the directory-target contract stays consistent across the CLI." An absolute, clean, missing path is still refused by install (cortex install --target-dir /abs/missing -> exit 64 error=invalid_target) while cortex doctor --target-dir /abs/missing exits 0 with presence=absent and cortex update --target-dir /abs/missing --catalog C --apply exits 2 with reason=target_absent. The PR body escalates exactly this to a maintainer decision and the new doc paragraph (docs/operating.md:84) records it, so I am not classifying it as blocking: the criterion is phrased as a direction, and refusing would remove doctor's documented presence report. What is missing is a durable record of the decision: either update doctor --target-dir accepts relative paths while install refuses them as invalid_target #372's expected direction to name the carve-out, or open the follow-up that decides whether update should also answer invalid_target. Otherwise the next consistency pass will reopen it as a defect. Also worth stating explicitly in that paragraph: uninstall's own answer for a missing absolute target is neither presence=absent nor target_absent, so "each command's own answer" currently covers three different behaviors under one phrase.

Verdict: ready to merge

Fleet review (glmflash)

Lens: adversarial. Attacked the target-validation boundaries across all four --target-dir consumers and the mode-dispatch change in update, reading the head sources directly. Verified: (1) validTargetDir reproduces the #369 install check exactly (absolute + Clean-identical), so no command is looser than install; Windows drive-relative and rooted-without-drive paths fail IsAbs, and trailing separators, '/./' segments, and mixed separators fail the Clean comparison. (2) The update dispatch on runtimeID == "" cannot misroute a runtime-mode invocation: parseUpdateArgs rejects an empty runtime ID with ok=false -> invalid_command, and ok=true with an empty runtime ID only comes from the targetSet branch, so bare 'cortex update' is still invalid_command, not invalid_target. (3) The targetSet tracking closes two silent fallthroughs (uninstall --target-dir "" reaching the canonical all-roots uninstall, doctor --target-dir "" reaching the host report) and makes the duplicate-flag check strictly stricter (previously --target-dir "" --target-dir x silently used the second value). (4) Code search over the repo shows doctor, install, and update are the only non-test consumers of --target-dir (uninstall lives in doctor.go), so coverage is complete. (5) The refusal tests guard the real home via a failing resolveRoots and assert no writes, including the foreign sentinel and the not-created missing path. The check runs before doctor's probe write and before update's stat, so the misleading invalid_catalog/target_absent outcomes for relative targets can no longer occur. Issue #372's contract is one-directional (a target install refuses must be refused everywhere); uninstall being stricter is fail-closed and fine. No blocking findings.

Suggestion (2)

• Docs mention uninstall's filesystem-root refusal but not its mixed-separator refusal (docs/operating.md:80): validTargetDir accepts paths that skillroot.NewUninstallRoot still refuses: besides the filesystem root (which the new doc bullet does mention), Windows mixed-separator targets like 'C:...\smoke/rel' pass validTargetDir (Clean normalizes the separators) but are refused by uninstall. The doc says every command refuses an invalid target 'the same way'; for uninstall the refusal set is a strict superset. The PR body documents this divergence honestly in its out-of-scope notes, but the user-facing doc does not.
• Duplicate-flag behavior change for doctor/uninstall is undocumented in the PR table (internal/cli/doctor.go:104): The duplicate --target-dir check moved from targetDir != "" to targetSet. A sequence like --target-dir "" --target-dir x previously passed the duplicate check (the empty value did not trip targetDir != "") and silently used the second value; it now exits invalid_command. Strictly safer, but the PR's before/after table documents only the single empty value, so this stricter duplicate handling is an unlisted behavior change.

Verdict: ready to merge

Thanks for this round. I pushed 12c5662 and ccf3ebf, and updated the PR body for the current head.

Usage errors before the target check (qwen38). Confirmed with the built binary at ff42157: update --target-dir rel without --catalog, and with --adopt-guidance, give exit 64 error=invalid_command; with a valid or missing catalog they give exit 64 error=invalid_target. I kept the usage-error precedence. A malformed command line is rejected before any argument is interpreted, both refusals use exit 64 with no writes, and changing the order would mean reworking update's argument parsing, which predates this PR. The doc now says: "A malformed command line is still refused first as invalid_command (for example update --target-dir <dir> without --catalog)."

Trailing separator (deepseek). A real behavior change from this PR, now pinned and documented. On main, /abs/dir/ gave: doctor exit 0 with a presence report, update exit 70 invalid_catalog, install and uninstall exit 64 invalid_target. Now all four refuse it as invalid_target, which is what install has done since #369. A trailing-separator row is in the table for every command, and docs/operating.md names the form ("pass /path/dir, not /path/dir/"). I kept the refusal instead of normalizing with filepath.Clean, because normalizing would also change install's #369 contract.

Valid-target control case (deepseek). Added doctor/accepts-valid-absolute, and TestValidTargetDir pins the check directly: absolute clean is accepted; relative, ./, unclean, .., trailing separator, and empty are refused.

--runtime "" (glm). It can't reach the directory mode: parseUpdateArgs only returns ok for the pi, opencode, and claude-code runtime IDs, so an empty --runtime is a usage error. Now pinned by update/empty-runtime-is-a-usage-error (exit 64, invalid_command).

Mixed separators (glmflash). These don't pass validTargetDir. On Windows filepath.Clean rewrites / to \, so C:\...\smoke/rel differs from its cleaned form and is refused. The mixed-separators Windows subtest pins that for all four commands, install included, and install runs only validTargetDir. Uninstall's stricter skillroot check adds only the filesystem root, which the doc names.

Duplicate flag with an empty first value (glmflash). Right, that's a stricter behavior: --target-dir "" --target-dir x used the second value and is now invalid_command. Added to the PR body's before/after table.

Missing absolute targets (glm, deepseek). This is still the open decision from my previous comment: doctor's presence report vs the issue's literal wording. I'll record whichever ruling you make, either in #372's expected direction or as a follow-up issue for update.

Verification at ccf3ebf: Linux container (non-root, LF) with go vet ./... and go test ./... clean, and 24 refusal subtests; Windows 11 with 36 refusal subtests (#386 merged in a scratch worktree); policy contracts pass.

Fleet review (mimo)

Lens: requirements. Reviewed head ccf3ebf (the PR moved from ff42157 to ccf3ebf during this run; the two new commits add trailing-separator, empty-runtime and valid-absolute control subtests plus TestValidTargetDir, and a docs note that malformed command lines are refused as invalid_command before the target check). Against issue #372 the PR satisfies the acceptance bar set in the issue's approval thread: the #369 path-form check is extracted into the single validTargetDir (install.go:102) and runs before any stat, read or write in all four --target-dir consumers (install, doctor, uninstall, update - confirmed by code search as the only consumers), and the refusal test pins every command against relative, missing-relative, unclean, trailing-separator and empty targets (plus Windows shapes) asserting exit 64, exact error=invalid_target stderr, empty stdout, an unchanged target listing, an unchanged home, and non-creation of the missing path; the empty --target-dir value now reaches the guard instead of falling back to the host doctor report or the canonical all-roots uninstall; docs/operating.md documents the shared contract, the invalid_command precedence, and the deliberate divergence. The single criterion not satisfied as literally worded in the issue body (missing and non-directory absolute targets keep each command's own answer) is a conflict between two texts of the issue itself - the body's Expected direction versus the approval thread's 'keep the existing absolute-path behavior pinned' and doctor's documented presence-reporting contract - and it is now explicitly documented in the PR, listed under Out of Scope as a maintainer decision raised in the review thread, and escalated by the author for a ruling (question 1); question 2 re-asks the equals-form scope call carried from my previous review. No blocking findings.

Question (2)

• Scope ruling needed: does doctor --target-dir accepts relative paths while install refuses them as invalid_target #372's criterion cover missing and non-directory absolute targets? (docs/operating.md:85): Issue doctor --target-dir accepts relative paths while install refuses them as invalid_target #372's Expected direction says 'a target that install would refuse as invalid_target should be refused the same way by every command that consumes --target-dir'. install refuses a missing absolute path and an absolute non-directory as error=invalid_target, exit 64 (install.go:119-121), while at this head doctor answers presence=absent with exit 0 and update answers reason=target_absent with exit 2 for the same targets - the behavior this PR deliberately keeps (docs/operating.md:85, and Out of Scope in the PR body). Two other explicit texts pull the other way: the issue's approval thread sets the acceptance bar as 'one refusal test per consuming command (error=invalid_target, exit 64, no writes)' and approves keeping the existing absolute-path behavior pinned, and doctor's documented job (feat: report directory targets in cortex doctor #343, operating.md table) is to report a target's presence, so refusing an absent target would make presence=absent unreachable. The author has escalated exactly this call ('This one needs your call, because the issue text and doctor's documented contract pull in different directions') and offers update's refusal as a follow-up. Please rule: amend doctor --target-dir accepts relative paths while install refuses them as invalid_target #372's wording to the path-form scope, or take the offered follow-up. Note the test matrix's 'missing' target is relative only, so neither reading is pinned by a test at this head.
• Is --target-dir= parity part of doctor --target-dir accepts relative paths while install refuses them as invalid_target #372 or a separate issue? (internal/cli/doctor.go:110): install parses both --target-dir and --target-dir= and refuses a relative equals-form value as error=invalid_target, exit 64 (pinned by the new install/equals-form subtest). doctor, update, and uninstall only parse the space-separated form, so cortex doctor --target-dir=agent-skills falls into their unknown-argument branch and is refused earlier as error=invalid_command, exit 64, with no writes (doctor.go:110). The same relative value is therefore refused either way, but with a different diagnostic token per command. The asymmetry predates this PR (feat: report directory targets in cortex doctor #343/Reject relative existing --target-dir as invalid_target before composition #369) and the PR body now lists extending the form as a separate issue 'if you want it'. Confirm whether equals-form diagnostic parity is in scope for doctor --target-dir accepts relative paths while install refuses them as invalid_target #372 or tracked separately, so the criterion can be closed.

Verdict: ready to merge

Fleet review (qwen38)

Lens: evidence. The PR implements issue #372's acceptance criterion directly: one shared validTargetDir check (absolute and filepath.Clean, extracted from the #369 install check) now gates install, doctor, update, and uninstall before any directory read or write, with the same error=invalid_target exit 64 diagnostic. Evidence is strong and reproducible on Unix: TestDirectoryTargetCommandsRefuseRelativeTargets drives every command through the binary entry point (runWithDependencies) against five target shapes (relative existing, relative missing, absolute-but-unclean naming the existing dir, trailing separator, empty), asserts the exact diagnostic, empty stdout, unchanged target listing including a foreign sentinel, unchanged home, and that no path is created; guardedUninstallDependencies fails closed so a RED run cannot touch a real home; update/target-checked-before-catalog pins that the target refusal precedes catalog resolution; TestValidTargetDir pins the helper itself, including over-refusal cases. Install behavior is a pure extraction, unchanged, and absolute-path behavior stays pinned by the four named pre-existing tests. Rollback is a single-commit revert. A stale findings file from an interrupted earlier run was re-verified against head ccf3ebf: its two concerns (catalog-precedence untested, no Windows-shaped test cases) are both addressed by tests present in this head and were dropped. No blocking defect found; two evidence notes remain below. Context output was complete (not truncated).

Suggestion (1)

• The Windows target-shape cases exist in the test but are not machine-reproducible from the ordinary CI lane (internal/cli/directory_lifecycle_test.go:143): The runtime.GOOS == "windows" block adds drive-relative, rooted-without-drive, and mixed-separator cases, which is exactly what an earlier draft of this review asked for. However, filepath.IsAbs/filepath.Clean have precisely these platform quirks, and the PR notes main does not build natively on Windows until fix(filetxn): read directory identity through per-OS variants #386 lands — the Windows GREEN runs (36 subtests, real cortex.exe) were done in a scratch worktree with fix(filetxn): read directory identity through per-OS variants #386 merged and are not reproducible from this PR's Linux CI lane. Nothing in the shared check suggests a Windows defect (it is the exact function install shipped with Reject relative existing --target-dir as invalid_target before composition #369), so this is not blocking; recording the exact Windows invocation in the issue thread would make the release evidence auditable later.

Question (1)

• Runtime-vs-directory routing now keys on runtimeID == ""; is the never-empty-runtimeID invariant pinned by a test? (internal/cli/update.go:80): The routing condition changed from targetDir != "" to runtimeID == "", and the comment asserts 'The runtime mode always carries a runtime ID'. The new test update/empty-runtime-is-a-usage-error pins the explicit --runtime "" form as invalid_command, and the full-suite GREEN claim is good supporting evidence. But the part of parseUpdateArgs that guarantees a non-empty runtime ID when the directory branch is not selected is not visible in the diff, and if that default ever regressed, a runtime-mode update would silently route into runDirectoryUpdate with an empty target and report error=invalid_target instead of the runtime report. Please confirm there is an existing end-to-end test that runs runtime-mode update (no --target-dir) through runWithUpdateDependencies so the invariant is pinned by a test, not only by the comment.

Verdict: ready to merge

Fleet review (deepseek)

Lens: evidence + test design. Reviewed at head ccf3ebf, merging my earlier review of ff42157. The two gaps I raised there are now closed: the table carries a trailing-separator target and an accepted-absolute control (doctor/accepts-valid-absolute, asserting exit 0, empty stderr and presence=present), so the table both refuses the wrong paths and proves the shared check does not over-refuse. The change delivers what #372 asks for: validTargetDir is extracted from install and called by doctor, update and uninstall before they stat, plan, apply or write, and the targetSet bookkeeping makes an explicit empty value reach that check instead of silently falling back to the host report or, for uninstall, to the canonical all-roots scan. The evidence is proportionate and reproducible from the PR: every subtest asserts exit 64 with exactly error=invalid_target and empty stdout, checks that the target listing with its foreign sentinel, the temp home, and the missing relative path are all unchanged, and guardedUninstallDependencies fails the test if the canonical roots are ever resolved, so a missing refusal cannot reach a real home. No blocking defect. The open items are one disclosed carve-out that still needs a recorded decision and three test-hygiene gaps, none of which changes the shipped behavior.

Suggestion (3)

• The fixture directory is created inside the package source tree (internal/cli/directory_lifecycle_test.go:151): existing is a bare relative name and this test's working directory is the internal/cli package directory in the checkout, so go test creates test-relative-target-372/ inside the repository and removes it only through t.Cleanup. A killed or panicking run leaves it in the tree, where it is visible to the inventory and gofmt checks and to any git status based tooling. The three forms the test needs (clean absolute, <abs>/./<dir> unclean, and <abs>/<dir>/ trailing separator) can all be derived from a t.TempDir() root, so the fixture can live outside the checkout without weakening a single assertion.
• The duplicate-flag row of the PR table is the one changed behavior with no subtest, and install's side of it is unstated (internal/cli/directory_lifecycle_test.go:216): Row 11 (doctor/uninstall --target-dir "" --target-dir /abs -> exit 64 error=invalid_command) is new behavior produced precisely by replacing targetDir != "" with targetSet, yet the added subtests stop at the refusal table plus the equals-form, precedence, empty-runtime and control cases. Every other row has an assertion. One subtest per command asserting exit 64 with exactly error=invalid_command pins it. Related: the table and the docs paragraph leave out install, whose duplicate handling was not switched to targetSet; the PR should say whether install still accepts a second --target-dir after an empty first value, so the four commands read as consistent or as deliberately different.
• No new subtest pins the runtime mode as the control for the new dispatch condition (internal/cli/update.go:82): The dispatch changed from targetDir != "" to runtimeID == "", so the routing decision now rests on the invariant in the comment: the runtime mode always returns a non-empty runtime ID. The added test pins the explicit-empty edge (update/empty-runtime-is-a-usage-error) but never the ordinary runtime invocation, so the only guard against a future change that lets a runtime-mode parse return an empty id is the pre-existing runtime update tests, and that coupling is invisible from this test. One control subtest, update --catalog <valid> with no --runtime and no --target-dir, asserting the runtime report rather than invalid_target, states the invariant where someone changing the parser will trip over it.

Question (1)

• A missing absolute target still answers differently in doctor, update and uninstall, and that carve-out has no recorded decision (docs/operating.md:84): The linked issue's expected direction says a target that install would refuse as invalid_target should be refused the same way by every command that consumes --target-dir. An absolute, clean, missing path is still refused only by install: cortex doctor --target-dir /abs/missing exits 0 with presence=absent, cortex update --target-dir /abs/missing --catalog C --apply exits 2 with reason=target_absent, and uninstall answers in its own way again. I am not calling this blocking: the criterion is phrased as a direction, doctor's documented job is to report presence, and the PR body escalates the update half to a maintainer decision. What is missing is a durable record of that decision, and the new paragraph here compresses three different behaviors into each command's own answer. Either amend doctor --target-dir accepts relative paths while install refuses them as invalid_target #372's expected direction to name the carve-out, or open the follow-up that decides whether update should refuse with invalid_target; otherwise the next consistency pass reopens this as a defect against a doc sentence instead of a decision.

Verdict: ready to merge

Fleet review (glm)

Lens: requirements + adversarial. The PR satisfies issue #372's core requirement: the #369 target check is extracted into validTargetDir and every command that consumes --target-dir (doctor, update, uninstall) now refuses relative, unclean, and empty values with exit 64 error=invalid_target before reading or writing anything. The previously destructive uninstall --target-dir "" fallback to the canonical all-roots uninstall is now refused, which is the most important safety win. The test matrix is thorough (relative existing/missing, unclean absolute naming an existing dir, empty, Windows drive-relative/rooted-without-drive/mixed-separator shapes, equals-form, target-before-catalog precedence) and the guarded uninstall dependencies plus home-unchanged assertions make a RED run safe. Docs match the new behavior. No blocking defects found; two clarifications below, one of which the author already surfaced for a maintainer ruling.

Question (2)

• Missing absolute target still diverges from install, per the issue's literal direction (internal/cli/update.go:244): Issue doctor --target-dir accepts relative paths while install refuses them as invalid_target #372's expected direction says a target install would refuse as invalid_target should be refused the same way by every command consuming --target-dir. Documented install behavior refuses a missing absolute directory as invalid_target (docs: 'the path is missing, relative, or not a directory'), but this PR keeps update's reason=target_absent and doctor's presence=absent for missing absolute targets. The named problem in the issue (relative/unclean/empty divergence) is fixed and tested; this residual divergence is explicitly documented and deferred to a maintainer decision. My position as reviewer: keeping doctor's presence report for a missing absolute target is coherent, because doctor's documented job is to report presence, and forcing uniformity there would break its purpose; update refusing with target_absent is also defensible. But it needs an explicit ruling from Juan so the directory-target contract is written down, not just inferred.
• Mode selection now keys on empty runtime ID; confirm --runtime "" cannot route here (internal/cli/update.go:81): runWithUpdateDependencies now selects directory mode with if runtimeID == "", relying on the claim that the runtime mode never returns an empty runtime ID. The diff does not show whether parseUpdateArgs rejects an explicitly empty --runtime "" value. If it is accepted (ok=true with runtimeID="" and targetSet=false), that invocation now exits 64 error=invalid_target via runDirectoryUpdate("") instead of taking the runtime path as before. Either way the command fails safely with exit 64, so this is a diagnostic question, not a defect — but the invariant 'runtime mode never returns an empty runtime ID' should hold by construction (e.g. an explicit ok=false for an empty --runtime value), not by convention.

Verdict: ready to merge

Fleet review (glmflash)

Lens: adversarial. Reviewed refactor-ia/cortex #388 (head ccf3ebf) against issue #372. The diff extracts the #369 install check into a shared validTargetDir (absolute && Clean-identical) and applies it before any read or write in doctor, update, and uninstall, closing the empty-value fallthroughs (doctor's host report on --target-dir "" and, worse, uninstall's canonical all-roots run) and making the duplicate-flag checks strictly stricter (previously --target-dir "" --target-dir x silently used the second value; the PR table documents the new invalid_command). I verified the Windows path shapes against the Go stdlib source (internal/filepathlite/path_windows.go:184): IsAbs is false for rooted-without-drive (\foo) and drive-relative (C:foo) paths and true only for volume+separator, so the test's Windows refusal cases are consistent with validTargetDir, and trailing separators, '/./' segments, parent elements, and mixed separators all fail the Clean comparison. The update dispatch on runtimeID == "" cannot misroute a runtime invocation: parseUpdateArgs rejects an empty --runtime value with --catalog and no --target-dir as invalid_command (test-pinned), and ok parse with empty runtime only comes from the targetSet branch. The new tests guard the real home via a failing resolveRoots, assert no writes (foreign sentinel, not-created missing path), and pin a valid-absolute control case against over-refusal; the check runs before doctor's probe write and before update's stat, so the misleading invalid_catalog/target_absent outcomes for relative targets can no longer occur. The only deviation from the issue's literal wording (a missing absolute target keeps doctor's presence=absent and update's reason=target_absent instead of being refused as install would) is disclosed, documented, and flagged for maintainer decision in the PR body; it creates no concrete wrong-behavior scenario, so it is recorded as a question, not blocking. No blocking findings.

Suggestion (2)

• Docs mention uninstall's filesystem-root refusal but not its mixed-separator refusal (docs/operating.md:80): validTargetDir accepts paths skillroot.NewUninstallRoot still refuses: besides the filesystem root (which the new doc bullet mentions), Windows mixed-separator targets like 'C:\dir\smoke/rel' pass validTargetDir (Clean normalizes separators) but are refused by uninstall, whose refusal set is a strict superset. The PR body documents this divergence honestly in its out-of-scope notes, but the user-facing doc says every command refuses an invalid target 'the same way'.
• Contradictory input --runtime "" --target-dir <abs> silently selects directory mode (internal/cli/update.go:147): parseUpdateArgs accepts an empty --runtime value followed by --target-dir and ignores the empty runtime, whereas the same command with a non-empty runtime is refused as invalid_command. Verified pre-existing behavior (the old parser selected directory mode the same way), not a regression, and the target check still runs; but the parser treats contradictory input differently depending on whether the runtime value is empty. The new test pins only --runtime "" without --target-dir.

Question (1)

• Missing absolute targets are carved out of the issue's literal consistency rule (docs/operating.md:84): Issue doctor --target-dir accepts relative paths while install refuses them as invalid_target #372 says a target install would refuse as invalid_target should be refused the same way by every command consuming --target-dir, and install refuses a missing absolute path as invalid_target (docs: 'the path is missing, relative, or not a directory'). The PR keeps doctor's presence=absent and update's reason=target_absent for missing absolute targets. The PR discloses this and frames it as a maintainer decision; recording an explicit ruling would settle the directory-target contract either way. As second-line evidence: refusing missing targets in doctor would defeat doctor's documented purpose (reporting presence), so keeping each command's own answer looks right, but it should be written down.

Verdict: ready to merge

The remaining blocking item from correction #390 (missing and non-directory absolute targets) needs your scope ruling. I laid out the three options, with the current behavior per command, in #390. I'll hold further pushes to this branch until then, so the ruling and the latest non-blocking suggestions land in one push.

Maintainer decision (option 2): update --target-dir refuses an absolute target that is missing or not a directory as invalid_target (exit 64), the same as install. doctor keeps reporting presence=absent without refusing. Note: #358 (PR #430) makes uninstall --target-dir on a missing root fail closed (exit 3); #388 must be rebased on it and keep both behaviors.

Updated per the #390 decision: merged current main (kept #388's target-dir tests and #430's missing-root uninstall tests) and added 79102df.

• update --target-dir with a missing or non-directory absolute target now refuses with error=invalid_target, exit 64, before reading the catalog (was reason=target_absent, exit 2). It reuses install's check through usableTargetDir.
• doctor is unchanged: presence=absent, exit 0, pinned by exact stdout.
• uninstall --target-dir on a missing root keeps exit 3 (Corrección: feat(lifecycle): update and uninstall directory targets via recorded ownership (#354) #358).
• Also from the review notes: the relative-path fixture now lives in a temp directory; duplicate --target-dir flags are covered for doctor, update and uninstall.
• Behavior change to note: update --runtime "" --target-dir <abs> now refuses as invalid_command (exit 64); before, it fell back to directory mode.
• Not done: the runtime-mode control case and the Windows mixed-separator doc note.
  Checks: go vet ./..., go test ./... -count=1 (35 packages) and the policy suite pass.

Fleet review (deepseek)

Lens: evidence + test design. Reviewed at head 95f1a8c, merging my earlier review of ccf3ebf and the open correction issue #390. The update half of the correction is delivered: runDirectoryUpdate now calls usableTargetDir before the stat, the plan and the apply, so an absolute missing or non-directory target is refused as error=invalid_target (exit 64) the way install refuses it, parseUpdateArgs tracks targetSet/runtimeSet so a given-but-empty --target-dir reaches that check instead of the runtime mode, and TestUpdateDirectoryTargetRefusesMissingOrNonDirectory plus the new docs paragraph pin the result. The previous hygiene gaps are also closed: the relative fixture now lives under t.Chdir(t.TempDir()), every table row including the duplicate-flag row has a subtest, and the guarded uninstall dependency makes a missing refusal fail the test before it can reach a real home. Doctor is the half that did not change: runDoctorCommand applies only validTargetDir, so a missing absolute path still answers presence=absent with exit 0 and a regular file still answers presence=unreadable with exit 3, while install refuses both. That divergence is now pinned by TestDoctorDirectoryTargetReportsMissingAsAbsent and documented rather than removed, which makes it deliberate but still contradicts the acceptance criterion of the still-open #390 and #372's expected direction; that is my one blocking finding. Everything else in the diff is well evidenced and reproducible.

Blocking (1)

• Doctor still accepts the absolute missing and non-directory targets that install refuses as invalid_target (internal/cli/doctor.go:136): Issue doctor --target-dir accepts relative paths while install refuses them as invalid_target #372 expected direction: "Share one target validation between install, doctor (and update/uninstall where applicable): a target that install would refuse as invalid_target should be refused the same way by every command that consumes --target-dir, so the directory-target contract stays consistent across the CLI." The open correction issue Corrección: fix(cli): refuse relative --target-dir in doctor, update, and uninstall (#388) #390 states the same criterion for doctor explicitly, quoting cortex doctor --target-dir /abs/missing exiting 0 with presence=absent and doctor exiting 3 presence=unreadable for a regular file. At this head runDoctorCommand (doctor.go:119-123) calls only validTargetDir, which checks path form (absolute and clean) and nothing else, then forwards the path to runDoctorDirectoryTarget (doctor.go:130-150): the missing path reaches the presence=absent branch at line 136 and returns exitOK, the regular file reaches presence=unreadable at line 138 and returns exitConflict. install refuses both through usableTargetDir as invalid_target exit 64, and update now does too. The new test TestDoctorDirectoryTargetReportsMissingAsAbsent pins the divergent behavior and the docs paragraph documents it, so this is not an oversight, but a documented deviation does not satisfy a criterion that says the scenario must no longer reproduce. Either doctor applies the same existence/shape check as install and update (and its documented presence=absent contract changes with it), or the maintainer amends the doctor --target-dir accepts relative paths while install refuses them as invalid_target #372/Corrección: fix(cli): refuse relative --target-dir in doctor, update, and uninstall (#388) #390 criterion to record doctor's report-only answer as the intended carve-out; the current state leaves the CLI contract undecided while the correction issue stays open.
  • Failure scenario: Given a path that does not exist, cortex doctor --target-dir /abs/missing exits 0 with stdout target=/abs/missing presence=absent, and given a regular file, cortex doctor --target-dir /abs/file exits 3 with presence=unreadable; cortex install --target-dir /abs/missing and cortex install --target-dir /abs/file both exit 64 with stderr error=invalid_target, and update now matches install. An operator who validates a directory target by asking the command whose documented job is to report on it (a script running cortex doctor --target-dir "$dir" and proceeding on exit 0) is told the target is acceptable and then has the following cortex install --target-dir "$dir" refuse it, which is the exact cross-command inconsistency doctor --target-dir accepts relative paths while install refuses them as invalid_target #372 reports.

Suggestion (2)

• The PR's Out of Scope bullet still claims update keeps reason=target_absent, contradicting this diff and its own table (internal/cli/update.go:245): The body says under Chain Dependencies / Out of Scope: "A missing absolute target keeps each command's own answer (doctor presence=absent, update reason=target_absent), because doctor's documented job is to report presence. Whether update should also refuse it as invalid_target is a maintainer decision, raised in the review thread." That is no longer what the code does: this hunk replaces the stat plus writeUpdateRefusal(..., "target_absent") with usableTargetDir and error=invalid_target, reason=target_absent no longer exists anywhere in update.go, the PR's own comparison table above it lists update --apply, missing as exit 64 error=invalid_target, and the docs paragraph now states it. A reader checking the scope statement against the diff gets two opposite answers about a user-visible exit code change (2 to 64). Rewriting that bullet to say update now matches install, and leaving only the doctor half as the open decision, keeps the PR's own record consistent with what it ships.
• No subtest pins the runtime-mode side of the new empty-runtime dispatch (internal/cli/update.go:82): The dispatch changed from targetDir != "" to runtimeID == "", so directory mode is now selected by an absent runtime ID and the invariant in the comment (the runtime mode always carries a runtime ID) is what keeps a runtime update out of runDirectoryUpdate. The added subtests cover the negative edges (update/empty-runtime-is-a-usage-error, update/empty-runtime-with-target-is-a-usage-error) but none asserts the positive control with a real runtime ID, so the invariant is only held by the pre-existing runtime update tests. One subtest, update --runtime pi --catalog <shipped>, asserting the plan path (not error=invalid_target) states the invariant where the next parser change has to see it.

Verdict: not ready

Fleet review (glm)

Lens: requirements + adversarial. The diff satisfies issue #372's expected direction at head 182af6b: one shared validTargetDir check (absolute and filepath.Clean) now runs in doctor, update, and uninstall before any read or write, matching install's #369 refusal exactly (exit 64, error=invalid_target, empty stdout). Adversarial edges are covered: empty --target-dir no longer falls back to doctor's host report, update's runtime mode, or — the dangerous one — uninstall's canonical all-roots uninstall (guarded by a resolveRoots stub that fails the test if canonical roots are ever resolved); trailing-separator, unclean, and Windows drive-relative/rooted-without-drive/mixed-separator shapes are all refused; duplicate flags after an empty value are invalid_command instead of silently overriding. The earlier open questions are resolved at this head: update now refuses a missing or non-directory absolute target as invalid_target the same way install does (pinned by TestUpdateDirectoryTargetRefusesMissingOrNonDirectory, the #390 ruling), while doctor keeps its documented presence=absent report for missing absolute targets; and an explicit empty --runtime is pinned as invalid_command by a dedicated subtest, so the directory mode cannot be reached through an empty runtime ID. Control cases pin that valid absolute targets are untouched and uninstall's stricter skillroot check is preserved. No blocking findings.

No findings.

Verdict: ready to merge

Fleet review (qwen38)

Lens: evidence. Reviewed head 182af6b (context diff matches the GitHub files API: docs/operating.md, internal/cli/directory_lifecycle_test.go, doctor.go, install.go, update.go). Against issue #372 and correction #390 the acceptance evidence is strong and reproducible: one shared validTargetDir check now gates all four --target-dir consumers before any stat, plan, read, or write; TestDirectoryTargetCommandsRefuseRelativeTargets drives every command through runWithDependencies (the binary entry point) across relative-existing, relative-missing, unclean, trailing-separator, and empty shapes, asserting exit 64, exact stderr, empty stdout, an unchanged target listing including a foreign sentinel, unchanged home, and that no path is created; guardedUninstallDependencies makes the destructive all-roots fallthrough fail the test instead of touching a real home. The remaining #390 item was resolved by the maintainer ruling (option 2, 2026-11-12): update now refuses a missing or non-directory absolute target as invalid_target via usableTargetDir, pinned by TestUpdateDirectoryTargetRefusesMissingOrNonDirectory, while doctor deliberately keeps presence=absent, pinned by TestDoctorDirectoryTargetReportsMissingAsAbsent — the criterion's doctor half was explicitly amended, so its literal text no longer blocks this head. Residual evidence caveats (stale body section, Windows subtest results taken with #386 merged in a scratch worktree) are non-blocking.

Suggestion (2)

• PR body's Out of Scope still says the update missing-absolute refusal is an open maintainer decision, contradicting the shipped Corrección: fix(cli): refuse relative --target-dir in doctor, update, and uninstall (#388) #390 option-2 behavior (internal/cli/update.go:244): The shipped code, docs/operating.md, and the test comment all present the refusal of a missing or non-directory absolute target as invalid_target (exit 64) as the settled Corrección: fix(cli): refuse relative --target-dir in doctor, update, and uninstall (#388) #390 ruling (maintainer decision of 2026-11-12, option 2), while the body's Out of Scope bullet still reads 'Whether update should also refuse it as invalid_target is a maintainer decision, raised in the review thread'. Readers of the PR description get the opposite signal from the head itself. Update the body (and ideally record the ruling link) so the description matches the delivered contract; also worth closing correction issue Corrección: fix(cli): refuse relative --target-dir in doctor, update, and uninstall (#388) #390 since its adjudicated scope is implemented and pinned.
• The Windows subtest evidence (36 subtests) is not reproducible from this head alone until fix(filetxn): read directory identity through per-OS variants #386 lands (internal/cli/directory_lifecycle_test.go): The Focused Tests section states the 36-subtest Windows pass was produced 'with fix(filetxn): read directory identity through per-OS variants #386 merged in a scratch worktree' because main does not build natively on Windows until fix(filetxn): read directory identity through per-OS variants #386. So the Windows target shapes (drive-relative, rooted-without-drive, mixed separators) rest on an assertion made outside this branch's exact state; the runtime.GOOS-guarded table in TestDirectoryTargetCommandsRefuseRelativeTargets will only prove them on a Windows-runnable base. Linux evidence is the primary contract and is fully reproducible from the PR, so this is a recorded limitation rather than a defect — worth noting so nobody treats the Windows row as independently verified at this head.

Verdict: ready to merge

Fleet review (mimo)

Lens: requirements. At the reviewed head, issue #372's path-form criterion is fully implemented: validTargetDir (internal/cli/install.go:102) is the single shared check (absolute and filepath.Clean) run by install (through usableTargetDir), doctor (internal/cli/doctor.go:120), uninstall (internal/cli/doctor.go:730) and update (internal/cli/update.go:245) before any stat, read or write; a code sweep at head confirms install/doctor/update/uninstall are the only --target-dir consumers; the empty-value fallthroughs (doctor's host report, uninstall's canonical all-roots run, update's runtime mode) are closed by flag-presence tracking; and TestDirectoryTargetCommandsRefuseRelativeTargets pins all four commands across relative existing/missing/unclean/trailing-separator/empty and Windows shapes plus a valid-absolute control, with guarded uninstall dependencies that fail the test if canonical roots are ever resolved. The literal reading of #372's Expected direction ('a target that install would refuse as invalid_target ... refused the same way by every command that consumes --target-dir') still diverges for missing or non-directory absolute targets on doctor (exit 0 presence=absent) and uninstall (exit 3 not_installed), but that divergence is an explicit maintainer ruling rather than a silent narrowing: on correction issue #390 the maintainer chose option 2 (2026-11-12) - update refuses those targets as invalid_target, doctor keeps its presence report, uninstall keeps exit 3 from #358 - the decision was implemented in 79102df, and both behaviors are pinned by TestUpdateDirectoryTargetRefusesMissingOrNonDirectory and TestDoctorDirectoryTargetReportsMissingAsAbsent. docs/operating.md documents all three behaviors at head. One non-blocking item: the PR body's Chain Dependencies / Out of Scope section still describes the pre-ruling state (update reason=target_absent, decision 'raised in the review thread'), contradicting the code, tests and docs that merged. Note: the head moved from 95f1a8c (reviewed via context) to the merged head 182af6b during this run; the delta is only the merge of main carrying #434's qa-usage changes (operating.md, doctor.go qaUsage, qa_test.go, qasetup.go) and touches none of this PR's files or line references. No blocking findings.

Suggestion (1)

• PR body out-of-scope section is stale against the merged head: update no longer answers reason=target_absent (internal/cli/update.go:245): The PR body's 'Chain Dependencies / Out of Scope' still states: 'A missing absolute target keeps each command's own answer (doctor presence=absent, update reason=target_absent) ... Whether update should also refuse it as invalid_target is a maintainer decision, raised in the review thread.' At the reviewed head that is false for update: runDirectoryUpdate (internal/cli/update.go:245) refuses a missing or non-directory absolute target as error=invalid_target exit 64 through usableTargetDir, TestUpdateDirectoryTargetRefusesMissingOrNonDirectory pins it as 'the Corrección: fix(cli): refuse relative --target-dir in doctor, update, and uninstall (#388) #390 ruling', the maintainer's option-2 decision on Corrección: fix(cli): refuse relative --target-dir in doctor, update, and uninstall (#388) #390 (2026-11-12) was implemented in 79102df, and the docs bullet added in this PR says 'Update, like install, also refuses an absolute target that is missing or not a directory as invalid_target (exit 64)'. The body's Changes and Focused Tests sections likewise describe only the pre-ruling state (validTargetDir-only check, 'GREEN at head ccf3ebf'). Code, tests and docs agree with each other; only the body contradicts them, and the body is the record read at merge review.

Verdict: ready to merge