---
phase: 35.3-psyche-sync-setup-ux-pass-error-display-doctor-partial-docs
reviewed: 2026-05-29T00:00:00Z
depth: standard
files_reviewed: 6
files_reviewed_list:
  - src/owl/psyche_sync_setup.rs
  - src/common/sync.rs
  - src/common/git.rs
  - src/common/owlery.rs
  - src/owl/doctor.rs
  - plugin/spt/skills/psyche-sync-setup/SKILL.md
findings:
  critical: 0
  warning: 5
  info: 4
  total: 9
status: issues_found
---

# Phase 35.3: Code Review Report

**Reviewed:** 2026-05-29
**Depth:** standard
**Files Reviewed:** 6
**Status:** issues_found

## Summary

Reviewed the four Phase 35.3 changes against the diff `4da4cb8..908c5b2`:

1. **Error display flip** (`psyche_sync_setup.rs:75`, `{:?}`→`{}`) — correct and regression-locked by tests in both `sync.rs` and `git.rs`. No defects.
2. **Guarded cwd-basename fallback** (`owlery.rs::project_name_from_cwd_path`) — the git-ref / path-traversal vector raised in the brief is **mostly** closed: `validate_id_chars` rejects `..`, `/`, spaces and ref-metachars, and the resulting branch is always `p-`/`a-`-prefixed (never bare-positional), so leading-dash argv injection does not reach git. Two residual gaps remain (WR-01, WR-02).
3. **Partial-sync `ls-remote` probe** (`doctor.rs::probe_partial_sync`) — arg-vector safe and timeout-bounded as claimed. But the 500ms bound is too tight for a real network `ls-remote`, producing systematically misleading output (WR-03), and the local-config substring check has a false-positive class (WR-04). Collapse rule and Fail-count semantics are preserved (verified).
4. **SKILL.md doc edits** — accurate, but one claim drifts from the implementation (IN-04).

No BLOCKER-class defects (no injection that reaches a shell or unguarded git positional, no data loss, no crash). The findings below are correctness/robustness degradations and clarity issues.

## Warnings

### WR-01: `validate_id_chars` accepts all-dash / leading-dash basenames; relies on the `p-`/`a-` prefix as the sole flag-injection guard

**File:** `src/common/owlery.rs:1070` (guard), `src/common/tracked.rs:149-153` (`validate_id_chars`), `src/common/tracked.rs:459-468` (worktree add)
**Issue:** The brief specifically asks to vet the basename fallback as a git-ref-name / argv injection vector. `validate_id_chars` is `!s.is_empty() && all(alphanumeric|_|-)`. A real on-disk directory named `-x`, `--`, or `--upload-pack` passes validation (every char is `-`/alnum). The basename then becomes branch `p-{name}` (e.g. `p---upload-pack`) and `rel_path = ../projects/{name}`. The only thing preventing a leading-`-` from being parsed as a git flag is the hard-coded `p-`/`a-`/`../` prefix at the *current* call sites — `validate_id_chars` itself provides **no** leading-dash or pure-punctuation rejection. This is defense-in-depth that works today but is fragile: any future caller that passes a validated name into a bare git positional (no `p-`/`../` prefix) inherits a flag-injection hole, and `git check-ref-format` would also reject names like `p-` + trailing-dash / consecutive-`..`-free-but-`.lock`-suffixed values that `validate_id_chars` lets through.
**Fix:** Tighten `validate_id_chars` (or add a dedicated `validate_project_basename`) to additionally reject a leading `-` and an all-punctuation name:
```rust
pub fn validate_id_chars(s: &str) -> bool {
    !s.is_empty()
        && !s.starts_with('-')
        && s.chars().any(|c| c.is_ascii_alphanumeric())
        && s.chars().all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-')
}
```
At minimum, add a regression test asserting `project_name_from_cwd_path` rejects a `-leading` basename so the prefix-as-guard assumption is pinned.

### WR-02: `project_name_from_cwd_path` rejects bytes git would accept but accepts a name git's ref grammar still forbids in edge cases

**File:** `src/common/owlery.rs:1069-1076`
**Issue:** The guarded fallback equates "safe project name" with `validate_id_chars` (`[A-Za-z0-9_-]+`). Git ref grammar (`git check-ref-format`) additionally forbids a trailing `.lock`, a leading/trailing `-` after the `p-` join in some git versions, and names that are exactly `@`. While `[A-Za-z0-9_-]+` is a strict subset that excludes most of these, a basename like `x.lock` is *rejected* by `validate_id_chars` (dot not allowed) — good — but a basename ending in a way that, once prefixed `p-`, still trips `check-ref-format` is theoretically possible and is silently funneled into `git worktree add -b p-{name}`. The failure mode is soft (worktree add returns `Nonzero`, caller soft-fails), so this is not a BLOCKER, but the resulting `p-{name}` branch is never validated against git's actual ref grammar before use.
**Fix:** This is acceptable given the soft-fail posture downstream, but document the residual gap explicitly in the rustdoc (the current doc claims `validate_id_chars` makes the name ref-safe, which overstates the guarantee). Alternatively run a `git check-ref-format --branch "p-{name}"` probe before committing to the name.

### WR-03: partial-sync `ls-remote` probe's 500ms bound is too tight for a real remote, so a healthy remote routinely reports the D-04 "remote unverified" degrade wording

**File:** `src/owl/doctor.rs:1156-1162`, `src/common/git.rs:575`
**Issue:** `probe_partial_sync` calls `run_git_with_timeout(&["ls-remote", "origin"], seed_dir)`, and `run_git_with_timeout` hard-codes `Duration::from_millis(500)`. A real `git ls-remote` against `https://github.com/...` performs DNS + TLS + HTTPS auth handshake, which commonly exceeds 500ms on a cold connection or slow link. The probe will therefore frequently time out → `remote_ok = false` → emit the D-04 "origin configured locally, remote unverified" row even when the remote is fully reachable. This degrades the diagnostic to near-noise on the exact path it was added to clarify (state=Unset + origin present). The doc comment claims the 500ms bound is the "T-35.3-06 DoS bound" — but it doubles as a correctness bound that is mis-sized for a network round-trip.
**Fix:** `run_git_with_timeout` is shared with the 500ms stamp path and cannot simply be widened. Either (a) add a timeout-parameterized variant (or use `run_git_checked` with a network-appropriate budget such as `SYNC_PROBE_TIMEOUT` / a few seconds) for the `ls-remote` call, or (b) soften the D-04 wording so a timeout is not presented as "unverified remote" (which an operator reads as "remote broken"). Reusing the 30s `SYNC_TIMEOUT` family (already the project's git-network budget) is the consistent choice:
```rust
let remote_ok = crate::common::git::run_git_checked(
    &["-C", &seed_dir.to_string_lossy(), "ls-remote", "origin"],
    None,
    crate::common::sync::SYNC_TIMEOUT, // 30s — the established network budget
).is_ok();
```

### WR-04: origin-presence check is a raw substring match on git config; matches a commented-out or non-`origin` remote whose URL contains the literal token

**File:** `src/owl/doctor.rs:1146-1149`
**Issue:** `has_origin` is `cfg.contains("[remote \"origin\"]")` over the concatenated `seed/config` and `seed/.git/config` contents. This is a substring scan, not a parse: a config that contains the literal text `[remote "origin"]` inside a comment (`# removed [remote "origin"]`), or a multi-valued/duplicated section left by a partial teardown, or even a URL value that embeds that exact bracketed string, trips the probe. The consequence is a spurious `sync:partial` Warn row (and a needless `ls-remote` network call) when no usable origin actually exists. It is Warn-only and idempotent, so not a BLOCKER, but it undermines the probe's precision.
**Fix:** Use git itself for the presence check (it is already spawning git for the reachability probe). `git -C seed config --get remote.origin.url` returning a non-empty value is the authoritative test and collapses the two checks into one:
```rust
let origin_url = crate::common::git::run_git_with_timeout(
    &["config", "--get", "remote.origin.url"], seed_dir);
let Some(url) = origin_url.filter(|u| !u.is_empty()) else { return None; };
```
(then run the reachability probe with the network budget from WR-03).

### WR-05: `is_within_tracked_psyche_dir` lexical-fallback can false-negative a psyche-internal path when canonicalize succeeds on one side only

**File:** `src/common/owlery.rs:1098-1103`
**Issue:** When `path.canonicalize()` fails (nonexistent / permission) it falls back to the raw `path`, while `psyche_dir().canonicalize()` may succeed (it is created eagerly by `psyche_dir()`). Comparing a non-canonical `path` against a canonical `psyche_canon` via `starts_with` can miss a containment that only holds after normalization (e.g. a `..`-bearing or symlinked path, or Windows `\\?\` vs plain prefix). The guarded fallback in `project_name_from_cwd_path` already short-circuits nonexistent paths (`!path.exists() → None`) before reaching the `tracked` rejection, and `validate_id_chars` rejects `..`, so the practical exposure is small — but the helper is also reachable via the git-ancestor branch (step c) where the path *does* exist on disk, and there the asymmetric canonicalization is a real (if narrow) bypass of the psyche-internal `tracked` rejection.
**Fix:** Canonicalize both sides consistently or compare after normalizing both to the same prefix discipline (e.g. route both through `owlery::to_forward_slash` after a best-effort canonicalize, or bail to a conservative `true` when only one side canonicalizes). Add a test exercising a psyche-internal path that exists on disk to pin the git-ancestor `tracked`-rejection branch.

## Info

### IN-01: `format!` with no interpolation in doctor duplicate detail

**File:** `src/owl/doctor.rs:775-777`
**Issue:** `let mut detail = format!("stale-leftover: flat duplicate of nested perch (hint: rm -rf)");` — `format!` over a literal with no args; clippy `useless_format`. Pre-existing (not a 35.3 change) but sits in the reviewed file.
**Fix:** Use `.to_string()`: `let mut detail = "stale-leftover: flat duplicate of nested perch (hint: rm -rf)".to_string();`

### IN-02: dead constant `SYNC_PROBE_TIMEOUT` (2s) carries `#[allow(dead_code)]` and would be the natural fix for WR-03

**File:** `src/common/sync.rs:58-59`
**Issue:** `SYNC_PROBE_TIMEOUT = Duration::from_secs(2)` is defined, `#[allow(dead_code)]`, and unused. It is exactly the kind of network-appropriate budget the partial-sync `ls-remote` probe (WR-03) should be using. Its continued dead-code status signals an intended-but-unwired timeout path.
**Fix:** Wire it into the partial-sync reachability probe (resolving WR-03) and drop the `#[allow(dead_code)]`, or remove the constant if genuinely unused.

### IN-03: partial-sync probe reads both `seed/config` and `seed/.git/config` but the seed is always bare (`git init --bare`), so the `.git/config` branch is effectively dead

**File:** `src/owl/doctor.rs:1146`
**Issue:** Per `tracked.rs` the seed is created via `git init --bare seed/`, so config always lives at `seed/config`; the `seed/.git/config` fallback can never match for the real seed. Harmless (it just reads a non-existent file and `filter_map`s the error away) but the "non-bare layout" comment describes a layout this codebase never produces.
**Fix:** Either drop the `.git/config` element (the seed is bare by construction) or keep it but downgrade the comment to "defensive — the seed is always bare here."

### IN-04: SKILL.md Recovery step 2 claims doctor surfaces "any per-branch sync failure reason" in the Unset/partial path, but per-branch rows only render when state==Enabled

**File:** `plugin/spt/skills/psyche-sync-setup/SKILL.md:138-140`
**Issue:** Step 2 of the new Recovery section says running `$OWL doctor` "surfaces a partial-setup Warn row plus any per-branch sync failure reason." In `check_sync_status`, the per-branch rows are produced only after the `state != Enabled → return` short-circuit is *not* taken (doctor.rs:1252-1253) — i.e. only when `state == Enabled`. In the exact failed-setup scenario this Recovery section addresses (state stuck at `Unset`), there is by construction no per-branch row, only the `sync:partial` row. The doc overstates what the operator will see.
**Fix:** Reword to: "surfaces a partial-setup Warn row (and, once sync is enabled, any per-branch failure reason)." Keeps the claim accurate for both states.

---

_Reviewed: 2026-05-29_
_Reviewer: Claude (gsd-code-reviewer)_
_Depth: standard_