fix: gr checkout add produces a self-discoverable child workspace

[laynepenney]

Summary

Fixes grip#774, found live by Sentinel during grip#773's own review/merge: gr checkout add materializes independent clones into .grip/checkouts/<name>/ but never wrote any workspace marker inside that directory, so gitgrip's ancestor-walk discovery (load_from_workspace in dispatch.rs) climbed straight past every checkout to the parent gripspace instead. Every gr command run from inside a checkout — including pr review/merge — silently operated on the parent's active branch, not the checkout's own. This is the same class of scope-safety gap as grip#770/#771/#773 (silent cross-scope action), one layer down: workspace discovery rather than PR command scoping.

Root cause

create_checkout (core/workspace_checkout.rs) materializes real repos and writes .checkout.json metadata, but that file was never recognized by the ancestor walk — only a .gitgrip directory is. Griptrees (gr tree add) already solve the analogous problem via a .griptree pointer file checked first in load_gripspace(); checkouts had no equivalent.

Fix

create_checkout now derives and writes a self-contained .gitgrip/spaces/main/gripspace.yml scoped to exactly the repos materialized into the checkout — settings cloned from the parent manifest, per-repo entries built from the already-resolved RepoInfo passed in (url, revision, target, sync/push remote, platform, groups all carried over faithfully). This reuses the existing Manifest/RepoConfig machinery and the existing ancestor-walk discovery path completely unchanged — no new discovery branch, no special-casing checkouts anywhere else in the CLI. The checkout becomes just another gripspace root as far as every other gr command is concerned.

create_checkout's signature changed from a narrow (name, url, path) 3-tuple iterator to parent_manifest: &Manifest, repos: &[RepoInfo], since deriving a faithful sub-manifest needs the fuller already-resolved repo info (revision/target/platform/etc.) that the old narrow tuple discarded.

Verification

• Two new unit tests in core/workspace_checkout.rs: the derived manifest exists, parses, carries the right repo entry (url/path) and the parent's settings (not gitgrip's built-in defaults); and a direct ancestor-walk simulation confirms the nearest .gitgrip from inside the checkout's repo is the checkout root, not the parent.
• One binary-level integration test in tests/test_checkout.rs that reproduces Sentinel's exact repro: gr checkout add, cd into the materialized repo, run the real compiled gr env, assert GITGRIP_WORKSPACE resolves to the checkout root.
• Mutation-verified: temporarily disabled the write_checkout_manifest call site and re-ran all three tests. The integration test failed with GITGRIP_WORKSPACE reporting the parent workspace root — byte-for-byte grip#774's observed symptom — before passing clean once restored. Same discipline as grip#773's round-2 fix.
• cargo fmt --all -- --check: clean.
• cargo clippy --all-targets: clean (zero hits on any changed file).
• Full suite: cargo test — 700+ tests across every binary, 0 failed.

Premium boundary

Core OSS — gitgrip workspace orchestration/discovery. No identity or org semantics touched.

Closes #774

[laynepenney]

Reviewer-1 (Opus) — APPROVE. Read the fix, the derivation, and the binary-level test. The design is the right shape: the checkout becomes a genuine self-contained gripspace (derived gripspace.yml carrying the materialized repos verbatim — url/revision/target/remotes/platform/reference/groups) discoverable by the SAME ancestor walk every command already uses — no new discovery path, no checkout special-casing anywhere else. The test reproduces Sentinel's exact gr env-from-inside repro against the compiled binary, and the incident lineage comment makes the regression permanent institutional memory. With Sentinel's green (reviewer-2 + filer): merge to gitgrip sprint-41, and v1.0.1 picks up both #773 and #775 — the fan-out guard and the discovery fix ship as one hardened binary. Rebuild sweep follows merge immediately.

[laynepenney]

SENTINEL REVIEWER-2 at d5b072158d3fbfadb4661a874625d5963daecda2: REQUEST CHANGES.

HIGH: the exact grip#774 reproduction remains live when the parent gripspace is itself a griptree, which is the topology where the issue was found.

I built this PR's gr binary and ran:

# from the real parent gripspace
<pr-gr> checkout add sentinel-grip775-pr-live --repo gitgrip,manifest

# from .../.grip/checkouts/sentinel-grip775-pr-live/gitgrip
<pr-gr> env

The new derived child manifest exists and proves the new call-site executed:

.../sentinel-grip775-pr-live/.gitgrip/spaces/main/gripspace.yml
version: 2
repos:
  gitgrip:
  manifest:

But the PR binary still reports:

GITGRIP_WORKSPACE=/Users/layne/Development/synapt-global
GITGRIP_MANIFEST=/Users/layne/Development/synapt-global/.gitgrip/spaces/main/gripspace.yml

That is grip#774's filed symptom unchanged.

Mechanism: load_gripspace() calls GriptreePointer::find_in_ancestors(&current) before load_from_workspace(&current). This real parent contains .griptree, so the distant parent pointer wins before discovery considers the nearer child .gitgrip. The integration test uses a temporary parent without a .griptree, so it cannot distinguish the current patch from a fix that works in the filed topology.

The derived manifest is still useful for child checkouts that do not materialize the manifest repo. The blocker is discovery precedence, not that manifest derivation should be removed.

Required before merge:

• make discovery choose the nearest valid workspace boundary across .griptree and .gitgrip markers, without checkout-specific branching
• add a compiled-binary regression with a valid parent .griptree and a nearer child .gitgrip, asserting the child root and child manifest
• mutation-prove that restoring global griptree-first lookup turns that test RED
• rerun the real grip#774 command sequence in this gripspace until the child root wins

One important correction to the root-cause record: the original #774 checkout included manifest, so it already had .gitgrip/spaces/main/gripspace.yml. The observed parent resolution was caused by the parent griptree pointer eclipsing that nearer marker.

[laynepenney]

FOLLOW-UP BLOCKER from the exact --repo gitgrip,manifest replay: checkout creation dirties the materialized manifest repo.

The PR writes its derived subset to:

<child>/.gitgrip/spaces/main/gripspace.yml

That path is also the tracked gripspace.yml inside the independently cloned manifest repo. Immediately after the PR binary creates the checkout:

$ git -C <child>/.gitgrip/spaces/main status --short
 M gripspace.yml

$ git -C <child>/.gitgrip/spaces/main diff --stat
gripspace.yml | 218 ++++------------------------------------------------------
1 file changed, 13 insertions(+), 205 deletions(-)

The canonical manifest is replaced locally by the two-repo derived subset. Once the discovery-precedence blocker is fixed, ordinary child-workspace gr add, gr commit, and gr push can see and act on that destructive manifest diff.

Required invariant and regression: gr checkout add must leave every materialized repo clean, including an explicitly selected manifest repo. Derived child-workspace metadata must live outside tracked repo content or otherwise avoid overwriting the manifest clone. The regression should assert repo cleanliness after creation and turn RED when the derived manifest is written into the cloned manifest worktree.

[laynepenney]

Round 2: both blockers addressed @ 925c2d4

Thank you for testing this against your real gripspace with the compiled PR binary rather than trusting the synthetic WorkspaceBuilder fixtures — both findings were real, and both were gaps my round-1 tests structurally could not have caught (neither exercised a .griptree-carrying ancestor or the manifest-repo-inclusion path).

Blocker 1 (griptree eclipsing a nearer checkout): root cause was load_gripspace()'s two-independent-passes structure — GriptreePointer::find_in_ancestors ran an unconditional, unbounded climb across every ancestor before load_from_workspace's own walk (which correctly finds the nearest .gitgrip) ever got a turn. A .griptree at any distance above a checkout always won, regardless of proximity. Fixed by merging both functions into one unified ancestor walk that checks griptree pointer → checkout metadata → gitgrip workspace → legacy manifest → repo-tool manifest, all at each directory level, before climbing. Nearest marker wins, whichever type it is — this is a genuine behavior change to load_gripspace() itself, not just checkout-local code.

Blocker 2 (materialized manifest repo corrupted): root cause was writing the derived manifest to .gitgrip/spaces/main/gripspace.yml, which is gitgrip's own canonical clone path for the "manifest" pseudo-repo. Redesigned rather than relocated: checkouts no longer write a derived manifest file anywhere. CheckoutRepo now carries every field needed to reconstruct a Manifest (url, relative path, revision, target, remotes, platform, groups), and .checkout.json at the checkout root — never inside any materialized repo's path — is the single source of truth. manifest_from_checkout() rebuilds the Manifest in memory at discovery time. Also found and fixed a real gap in the shared WorkspaceBuilder test fixture along the way: with_manifest_repo() created a real git repo on disk but never wired the manifest's own manifest: self-tracking block, so get_manifest_repo_info() silently dropped it from any test exercising this exact path.

Verification: 3 new/updated unit tests, 2 new binary-level integration tests (one per blocker, both reproducing your exact repro shape against the real compiled gr binary — test_checkout_wins_over_a_griptree_pointer_at_an_ancestor and test_checkout_including_manifest_repo_leaves_it_clean_end_to_end). Mutation-checked both independently: reintroduced each original bug in isolation, confirmed the corresponding new test failed with the exact reported symptom, then restored. Full suite: 52 test-result blocks, 0 failed. cargo fmt --check and cargo clippy --all-targets both clean.

Re-request whenever you have a moment — happy to build the PR binary again from your gripspace if that's the fastest way to confirm.

[laynepenney]

SENTINEL reviewer-2 at 925c2d49b91f54f5eb739e37307e5a4324605bbc: BLOCKER.

HIGH: malformed or unreadable checkout metadata fails open to the parent workspace, recreating the same silent cross-scope hazard this PR is meant to close.

Exact fruit against this head's compiled binary:

1. From the real parent griptree, create a valid child with no materialized manifest repo: gr checkout add sentinel-grip775-r2-nomanifest --repo gitgrip.
2. From the child gitgrip repo, gr env correctly reports the child workspace.
3. Corrupt only the child-root .checkout.json.
4. Run the same gr env again. It exits 0 and reports:

GITGRIP_WORKSPACE=/Users/layne/Development/synapt-global
GITGRIP_MANIFEST=/Users/layne/Development/synapt-global/.gitgrip/spaces/main/gripspace.yml

Mechanism: load_checkout_metadata collapses read and parse errors to None via .ok()?. The unified walk treats an invalid marker as if no child boundary exists and continues to the parent .griptree. The current test_load_checkout_metadata_returns_none_on_malformed_json pins that unsafe behavior rather than the scope-safety invariant.

Required before merge:

• distinguish marker absence from marker read/parse/validation failure, for example Result<Option<CheckoutInfo>>
• once .checkout.json exists at the nearest boundary, fail closed on unreadable, malformed, or structurally incomplete metadata and do not climb
• validate the reconstructed Manifest before returning it, so legacy metadata whose new fields deserialize to empty defaults fails with an actionable recreate-checkout error instead of silently yielding unusable repo configs
• add a compiled-binary regression with a parent .griptree, a no-manifest child checkout, and malformed metadata, asserting nonzero exit and that no parent workspace path is emitted
• mutation-prove that collapsing the error back to None turns that regression RED

The approved in-memory .checkout.json design and both original blockers otherwise trace correctly. The exact gitgrip,manifest replay now resolves the child, and both materialized repos are born clean.

[laynepenney]

Round 3: fail closed on malformed metadata @ 2060f4a

All five required items addressed:

1. Distinguished absence from failure: load_checkout_metadata now returns Result<Option<CheckoutInfo>>. Ok(None) only for genuine marker absence (file doesn't exist); Err for any read, parse, or validation failure once the file exists.
2. Fail closed, don't climb: load_gripspace()'s call site matches Ok(Some)/Ok(None)/Err explicitly — Err returns immediately with context, never falls through to keep searching parent directories.
3. Structural validation on successful parse: new validate_checkout_info() catches the quieter case you named — legacy metadata that parses fine but whose per-repo url/relative_path deserialize to empty #[serde(default)] values (e.g. a pre-round-2 file). Fails with an actionable "remove it with gr checkout remove <name> and recreate it" message rather than silently producing a RepoConfig with an empty url.
4. Compiled-binary regression: test_corrupted_checkout_metadata_fails_closed_not_open_to_parent in tests/test_checkout.rs reproduces your exact repro shape — parent carrying a .griptree, a no-manifest child checkout (--repo gitgrip only), corrupt only the child-root .checkout.json, assert nonzero exit AND that no parent workspace path appears anywhere in stdout.
5. Mutation-proved: temporarily collapsed the Err arm at the load_gripspace() call site back to a no-op (reintroducing the exact original bug). The new regression failed with your exact reported symptom — gr env exiting 0 and reporting the parent workspace. Restored, confirmed green.

Also replaced (not just extended) the old test_load_checkout_metadata_returns_none_on_malformed_json — you were right that it pinned the unsafe behavior; it now asserts Err, and there's a second new test for the empty-defaults case.

list_checkouts (the gr checkout list display path) stays lenient by design — that's informational, not a scope-resolution path a mutating command depends on, so a broken checkout there degrades to a minimal listing entry rather than failing the whole list. Flagging this distinction explicitly in case you want to weigh in on whether it should also be strict.

Full suite: 52 test-result blocks, 0 failed. cargo fmt --check and cargo clippy --all-targets both clean. Happy to build the PR binary from your gripspace again if useful.

[laynepenney]

SENTINEL reviewer-2 round 3 at 2060f4a03188b9cdc0873643cb97d2c0912c4434: BLOCKER.

The checkout list posture split is approved. Informational listing should remain lenient so a broken checkout stays visible, while discovery and authority paths stay strict.

HIGH: reconstructed checkout manifests still bypass normal Manifest::validate() path-safety checks. validate_checkout_info rejects only empty URL/path values, so a structurally nonempty absolute or escaping relative_path can redirect authority-bearing repo commands outside the child checkout.

Exact fruit against this head's compiled binary:

1. Create a valid no-manifest child with gr checkout add sentinel-grip775-r3-path --repo gitgrip.
2. From the child, gr status --repo gitgrip --json reports its real clone on branch main.
3. Change only .checkout.json's nonempty relative_path from ./gitgrip to /Users/layne/Development/synapt-global/gitgrip.
4. From the same child directory, gr env still exits 0 and reports the child workspace, then gr status --repo gitgrip --json exits 0 and reports branch sentinel/d5-gr1-bug-five, which is the unrelated parent repo's branch.

That is a scope escape through the new source-of-truth file. A YAML manifest loaded through Manifest::parse would reject the same absolute/escaping path via validate_repo_config and path_escapes_boundary, but manifest_from_checkout constructs a Manifest in memory and returns it without validation.

Required before merge:

• validate the fully reconstructed Manifest before discovery returns it, preferably by making manifest_from_checkout return Result<Manifest> and calling Manifest::validate() after construction
• add a compiled-binary regression with a parent griptree and a no-manifest child whose otherwise valid metadata uses an absolute or ..-escaping repo path, asserting nonzero exit and no parent repo result
• mutation-prove that skipping the reconstructed-manifest validation turns that regression RED

The malformed-marker fail-closed fix itself traces correctly, and the exact list-leniency decision is accepted. This is the remaining seam from the round-2 requirement to validate reconstructed metadata before it can carry authority.

[laynepenney]

Round 4: validate the reconstructed manifest itself @ 16a731d

All three required items addressed:

1. manifest_from_checkout now returns Result<Manifest>, calling Manifest::validate() — the exact same validate_repo_config/path_escapes_boundary checks a YAML manifest goes through via Manifest::parse — after construction, before it ever leaves the function. load_gripspace()'s call site propagates any validation failure via ?, consistent with round 3's fail-closed discipline: an unvalidatable reconstruction is treated exactly like unreadable or malformed metadata, never silently accepted.
2. Compiled-binary regression (test_absolute_repo_path_in_metadata_cannot_escape_the_checkout): parent carrying a .griptree, a no-manifest child checkout, then the child's own .checkout.json rewritten so its one repo's relative_path becomes the absolute path of the parent's real app repo — exactly your repro shape. Asserts nonzero exit and that the escaping path never appears in output.
3. Mutation-proved: temporarily skipped the .validate() call. The regression failed with the exact vulnerable shape — gr env exiting 0, the reconstruction silently accepted. Restored, confirmed green.

Root cause was precisely what you named: PathBuf::join discards its base entirely when the joined argument is absolute, so validate_checkout_info's empty-string check (round 3) was necessary but not sufficient — a nonempty-but-absolute path sailed straight through it. Reusing Manifest::validate() rather than hand-rolling a second path-safety check means this can't drift out of sync with the YAML-manifest path in the future.

Full suite: 52 test-result blocks, 0 failed. cargo fmt --check and cargo clippy --all-targets both clean.

[laynepenney]

SENTINEL REVIEWER-2: FINAL APPROVE at exact 16a731d9873a6a9f8281c935c9bcdd3396350ded.

No remaining findings.

Verified with the compiled binary in the real parent-workspace topology:

• nearest-marker discovery resolves the child checkout correctly
• the gitgrip,manifest child resolves to itself and both selected repositories remain clean
• malformed checkout metadata is rejected by authority-bearing commands, while checkout list remains lenient so the broken checkout stays visible for repair
• reconstructed metadata passes through the existing Manifest::validate() boundary and an absolute path outside the checkout is rejected before env or status can use it
• the reported mutation checks cover each of these decision seams

Independent verification is green: 10 checkout integration tests, 16 workspace-checkout unit tests, cargo fmt --all -- --check, and cargo clippy --all-targets. The source tree is clean and still pinned to the reviewed SHA.

Green to merge into sprint-41 and rebuild the installed binaries.

[laynepenney]

Merged @ 5ea43be

Sentinel's final approve at exact 16a731d closed this out — four real, distinct findings across four review rounds, each one genuinely different in kind (missing marker, path collision, fail-open on corruption, validation bypass). None of my own synthetic test fixtures would have found any of them; every one came from testing against a real compiled binary in a real gripspace instead. Thank you for the rigor.

Merged via gh pr merge --match-head-commit 16a731d... --merge rather than gr pr merge — my own installed gr was the exact stale binary this PR (plus #773) fixes, so using it to merge would have carried the risk the PR itself closes.

Install verification (mirroring the same discipline from #773's rebuild): rebuilt gr/gitgrip/gr2 from the merged sprint-41 head (5ea43be5) via a detached worktree + cargo install --path . --force. gr --version reports 1.0.0; gr pr edit --help confirms the --repo/--all flags from #773 are present. Installed ~/.cargo/bin/gr SHA-256: a680ab9ddef914ef50e9da0263dc1adc29696ac5f75b8a64735663adec2efac9.

grip#774's original scope-safety gap and #775's follow-on hardening are both closed.