Add Brew Trust to CLI Distribution

Here's the picture. Both PRs fix the same problem — Homebrew 6.0's new "Tap Trust" model, which now prompts/errors when you brew tap + brew install a third-party tap — but they take different approaches, and your comment on #1427 is exactly what separates them.

The two PRs

#1427 (Beehtrix, external contributor) — scoped trust via fully-qualified cask

Drops brew tap entirely. Per Homebrew's Tap-Trust docs, installing a fully-qualified user/repo/name cask trusts only that one cask. Also updates the Release Channels section. Cleaner — no extra command, no repeated tap line.

#1534 (paul, internal) — trust the whole tap

Keeps the tap and adds an explicit brew trust entireio/tap, which trusts the entire tap (both casks).

How your comment connects them

On #1427 you flagged the blocker: because the casks declare conflicts_with each other, scoped-trusting just one cask still trips an error about the other, untrusted cask. You wanted time for a better solution. Beehtrix agreed it's a tap-side design issue, opened homebrew-tap PR #4 (tap README + a Brewfile … trusted: true tip), and offered to test follow-ups.

The key insight: paul's #1534 effectively sidesteps the exact blocker you raised. brew trust entireio/tap trusts the whole tap, so both the stable and nightly casks are trusted — the conflicts_with evaluation no longer chokes on an untrusted cask. So #1534 is, in practice, the "better solution" workaround you were looking for on #1427, at the cost of broader trust (whole tap vs single cask).

So the relationship is

These are competing/overlapping fixes, not complementary ones — you don't want both. The trade-off:

	#1427 (scoped)	#1534 (whole tap)
Trust granularity	single cask (tighter)	whole tap (broader)
Extra command	none	brew trust
conflicts_with issue	still hits it	avoided
Scope of doc update	Quick Start + Channels	Quick Start only

Two things worth verifying before merging either

1. Is brew trust the correct command? I'd double-check — Homebrew's Tap-Trust docs mostly describe trust via fully-qualified names and Brewfile trusted: true. If brew trust <tap> isn't a real subcommand, #1534's middle line is wrong.
2. #1534 only updates Quick Start — the "Release Channels" section lower in the README still shows the bare brew install --cask entire, which #1427 does fix. Whichever wins should update both.

My read: the ideal merge is #1534's whole-tap trust approach (because it solves the conflicts_with problem) plus #1427's completeness (Release Channels section), rather than picking one PR wholesale — pending confirmation that brew trust is a valid command.

Want me to verify the brew trust command against Homebrew's docs, or draft a consolidated change?

I've reviewed the diff and traced every helper it leans on. Here's my assessment of #1529 — Add entire clone / entire repo clone.

Summary

A well-constructed command with genuinely thoughtful security/edge-case handling. But there's one significant gap: the headline entire clone top-level command doesn't actually exist in this PR.

🔴 Blocking — the advertised entire clone is missing

The title, body, and every example in the PR description use entire clone:

…and the body explicitly states "entire clone is a top-level alias for entire repo clone."

But the diff only registers clone under the entire repo group (repo.go:31). There is no hideAsAlias(newCloneCmd(), "entire repo clone") in root.go — the established pattern for top-level aliases (root.go:117-120 does exactly this for resume/attach/explain/trace).

Compounding it: entire repo is Hidden: true and only surfaced via entire labs (repo.go:23). So as written, the only way to invoke this feature is the hidden entire repo clone — the documented entire clone returns "unknown command."

Fix: either add the top-level alias in root.go, or correct the title/body/examples to entire repo clone. Given the intent, I'd add the alias.

🟡 Worth addressing

1. entire:// passthrough skips validateClusterHost (repo_clone.go, isEntireCloneURL branch) The shorthand path validates the host on both the --cluster flag and the server-supplied chosen.ClusterHost (nice defense-in-depth, with a clear comment about the host@evil.com STS-audience token-leak vector). But a user-supplied full entire://<host>/gh/... URL is passed verbatim to git clone with no host validation. The same token-leak rationale the PR cites applies — entire://realcluster@evil.com/gh/... would sail through. It's arguably no worse than a raw git clone entire://…, so not critical, but it's inconsistent with the PR's own stated guard. Consider parsing the host out and running it through validateClusterHost.

2. Clone-URL synthesis is duplicated fmt.Sprintf("entire://%s/gh/%s/%s", ...) now appears in both repo_mirror.go:29 (mirrorRow) and the new repo_clone.go. Given this repo's explicit anti-duplication stance (CLAUDE.md, mise run dup), extract something like mirrorCloneURL(host, owner, repo) and use it in both spots.

🟢 Nits

• entireCloneURLScheme = "entire://" const is defined and used by isEntireCloneURL, but the URL builder uses a bare literal "entire://%s/..." rather than the const — minor inconsistency.
• cmd.SilenceUsage = true is set at the very top of RunE, so an invalid <repo> arg won't print usage. Matches house style elsewhere, just flagging.

What's done well (not just box-ticking)

• Empty-host cancellation guard — returning NewSilentError on form cancel instead of falling through to clone a zero-value target (the entire:///gh/... bug). Both the cancel path and the byHost[selected] miss are handled.
• Case-insensitive host normalization in both clusterdiscovery/resolve.go (cache key + /.well-known + context match) and selectCloneTarget's dedupe/flag-match — with tests proving the cache is shared across mixed-case forms.
• --cluster routing through runCoreForCluster to resolve mirrors in a different auth federation — correctly reuses the mirror create path and addresses the real cross-federation bug.
• Test coverage is solid: ref parsing (incl. metachar/dot-only vectors), target selection, client-side repo filter, invalid --cluster rejected before any network dial.

My recommendation: request changes on the missing top-level alias (#1 — it breaks the documented UX), and treat the entire:// validation gap and the URL-format dedup as review comments.

Want me to post this as a PR review, or draft inline comments for the specific items?

Yes — there's another spot in the same README that #1534 misses. Here's the picture:

Relevant to #1534 (user-facing install instructions)

PR #1534 only edits the Quick Start block (README.md:46-52). But the same README has a second Homebrew section further down — Release Channels (README.md:88-89):

These still show the bare brew install --cask entire with no tap/trust step, so #1534 would leave the README internally inconsistent. (This is the same gap I flagged earlier: #1427 does update this section; #1534 does not.)

Everything else that matched — not install instructions, leave alone

• .goreleaser.yaml:112-138 — homebrew_casks build/publish config, not docs
• .github/workflows/{release,nightly}.yml — tap token generation in CI
• CHANGELOG.md — historical entries
• versioncheck.go + tests — detects /opt/homebrew/... install paths to suggest brew upgrade (not tap/install), so Tap Trust doesn't apply
• cmd/.../copilotcli/AGENT.md, transcript/.../gemini_full.jsonl — brew commands for other tools (copilot, gemini), unrelated
• docs/install-provenance-plan.md — design doc
• scripts/migrate-sessions.sh:144 — brew install jq helper hint

Bottom line

The only other place needing the same fix is README.md:88-89 (Release Channels). #1534 should update those two lines too (with whatever trust approach lands), or it'll ship docs that contradict its own Quick Start.

Want me to add the Release Channels fix to the #1534 branch so it's complete?

All helpers exist and the wiring checks out. Here's my re-review.

Re-review: #1529 — Add entire repo clone

Verdict: all findings from my first pass are resolved. This is now in good shape — I'd approve.

Prior findings, all addressed

#	First-review finding	Resolution
🔴 1	entire clone top-level alias advertised but not wired	Resolved — commit ed8dad74 drops the alias and the title is now "Add entire repo clone". Description and implementation now agree (it lives under the hidden entire repo/labs group, consistently).
🟡 2	entire:// passthrough skips validateClusterHost	Resolved as deliberate — commit 1fc19a03 adds a clear comment: it's a raw URL the user typed, forwarded to git clone verbatim, so it's equivalent to git clone entire://… directly; the guard applies only where the URL is synthesized from a flag/API host. Defensible — I'm satisfied.
🟡 3	entire://%s/gh/%s/%s duplicated	Resolved — commit 5270dbf4 extracts mirrorCloneURL(host, owner, repo), now used by both repo_clone.go and repo_mirror.go:29 (mirrorRow), with TestMirrorCloneURL.
🟢 4	builder used a literal instead of entireCloneURLScheme	Resolved — mirrorCloneURL now builds from the entireCloneURLScheme const.

Fresh pass — no new blockers

I re-read the whole diff and verified the surrounding code:

• Test helpers all resolve — writeJSON (attribution.go:1268, matching (io.Writer, any)), coreapi.NewWithBearer, coresHandler, hostPinningClient all exist. Tests will compile.
• Flag inheritance is correct — addControlPlaneFlags/insecureHTTPRequested are registered as persistent flags on the repo group, so repo clone inherits them for the runCore/runCoreForCluster path; the entire:// passthrough dials no core and needs none.
• Picker key consistency — options carry the lowercased host as their value and byHost is keyed lowercased, so byHost[selected] always hits. The cancellation/zero-value guards (SilentError) remain intact.
• mirrorCloneURL correctness — "%s%s/gh/%s/%s" with the entire:// scheme produces the right URL; test confirms.

Still-good design (unchanged)

Empty-host cancellation guard, case-insensitive host normalization in both clusterdiscovery and selectCloneTarget, validateClusterHost on the server-supplied host, cross-federation --cluster routing, and the non-interactive --cluster-pointer error path — all solid and well-tested.

Remaining nit (non-blocking, optional)

cmd.SilenceUsage = true is set at the very top of RunE, so an invalid <repo> arg won't print usage. This matches the house pattern elsewhere in the codebase, so I wouldn't hold the PR for it.

My recommendation: approve. Want me to post this as an approving review on the PR, or leave it for you to action?

Done. I checked out the toothbrush-patch-1 branch (PR #1534) and updated the Release Channels section to match the Quick Start's tap+trust flow:

Now both Homebrew references in the README are consistent — the Quick Start block (lines 46-54) and the Release Channels list (line 90) both reflect the one-time tap/trust step.

A couple of notes:

• I made the edit locally on the PR branch but haven't committed or pushed — pushing to paul's PR branch is your call. Want me to commit and push it to toothbrush-patch-1, or leave it for you/paul?
• Reminder that the underlying brew trust approach is still unverified against Homebrew's docs, and your own comment on #1427 flagged the conflicts_with issue (trusting one cask isn't enough when both stable+nightly exist). This doc change is consistent with #1534 regardless, but you may want to resolve that before merging either PR.

I've reviewed all the hand-written code, verified the helpers, and confirmed the generated-spec change. Here's my assessment.

Review: #1531 — entire repo: visibility verb + fix get/mirror-get resolution

Verdict: solid, well-tested, I'd approve. Three logically-distinct fixes bundled, each correct. No blockers — only minor/optional notes.

What it does (and why it's right)

1. resolveRepoRef get-by-name fix (resolveref.go) — the real bug fix The name-filtered query returns the match in the singular repo field, but the code read the (always-empty) repos array, so repo get/delete <name> --project failed for every name. I confirmed ListProjectReposOutputBody already has both Repo OptRepo and Repos []Repo on main — so this is a pure 3-line hand-code fix (out.Repo.Get()), exactly correct. Crucially, they also fixed the test mock, which previously populated repos and so masked the bug. That's the right instinct — the misleading test was part of the defect.

2. resolveMirrorRef / parseMirrorCloneURL (repo_mirror.go) mirror get now accepts the entire://<cluster>/gh/<owner>/<repo> clone URL that mirror list prints (previously only a ULID worked; a URL 404'd as undecodable text/plain). Clean implementation:

• ULID short-circuits (verified by test to make zero HTTP calls).
• Lists by (cluster, provider, owner) server-side — the Cluster param pre-exists on main — and matches repo client-side with EqualFold (no get-by-coords endpoint exists).
• Reuses validateClusterHost(u.Host) so a metacharacter-laden host is rejected at the boundary. Good defense-in-depth, consistent with #1529.
• Trims a trailing .git but preserves interior dots — tested.

3. repo visibility get|set (repo.go) — new verb + codegen

• parseVisibility fails fast client-side (clear message vs opaque 422).
• set validates before resolving/dialing. Echoes back the server's authoritative value, not the user's input. Good.
• Codegen is additive — I checked the spec diff: the - lines are description reorders, and there are no removed operationIds, paths, or required fields. New surface is the visibility endpoint, Repo.visibility, and Cluster.apiUrl.
• Lives under the hidden entire repo/labs group, consistent with its siblings.

Minor / optional notes (none blocking)

1. parseVisibility is case-sensitive — "Public" is rejected, and a test locks that in. Failing fast is defensible, but a user typing a capitalized value gets an error for a near-miss. Consider strings.ToLower(strings.TrimSpace(s)) before the switch for friendliness. Subjective — your call.

2. parseMirrorCloneURL edge cases are harmless — entire://host/gh//repo (empty owner) parses with owner="" and a dot-only repo isn't rejected, but both only feed a list filter + client-side match (never a synthesized URL or filesystem path), so worst case is a "no mirror matching" error. Not worth tightening.

3. Bundling — three independent fixes + a 2,000-line codegen regen in one PR. Reasonable (visibility needs the regen, and the two resolver fixes are all entire repo UX), but the history would be cleaner as separate commits/PRs. Not a blocker.

One thing worth confirming before merge

The bulk of the diff is regenerated internal/coreapi — not eyeball-reviewable. The correctness that matters is that the spec was regenerated from the canonical entire-backend OpenAPI source (not hand-edited to match an assumed contract). The PR says "Refreshed the spec (additive only)," and my structural check agrees it's additive — but I'd want confirmation the visibility endpoint shape matches what the server actually ships, since a CLI/server contract mismatch wouldn't surface until runtime. Do you know if these specs are pulled from a pinned backend artifact?

Want me to post this as a review, or check how the coreapi spec regeneration is wired (e.g. a mise run target) to settle the last point?

Good instinct to poke at this — let me separate two things that are tangled in "make it a param," because they point in different directions.

What "public" actually means here matters more than where it sits

From the code's own comment, public = the SpiceDB public_viewer wildcard = read access for any authenticated Entire user, explicitly not anonymous. That's much closer to GitHub's internal (visible to org/enterprise members) than to GitHub's public (visible to the anonymous internet). So the label is already doing something a reader will likely misjudge — and the PR has to spend a paragraph disclaiming it.

That's the real smell, I think. Whether it's a positional or a flag, calling it public invites "oh, anyone on the internet can see this" when that's not true.

The two design axes

Axis 1 — surface shape: positional value vs. named param

• Current: repo visibility set <repo> public (positional enum)
• Param: repo visibility set <repo> --visibility public

Honestly, for a visibility set verb whose entire job is to set visibility, a positional value is fine and idiomatic — it matches the rest of entire repo (e.g. mirror create <url> [cluster] is all positional). A --visibility flag on a command called visibility set is a bit redundant. gh uses --visibility only because it's one option among many on gh repo edit; here it's the sole subject. So I'd keep it positional on ergonomics alone.

Axis 2 — value model: binary vs. extensible enum (this is the one I'd act on) The bigger question is whether public|private is the right vocabulary, and that's the part that's hard to change later because it's baked into both the CLI verb and the server enum. GitHub needed three states (public|private|internal); Entire's "public" is effectively "internal." It's plausible you'll later want a genuinely-anonymous tier, or an org-scoped tier — and then a binary public/private fights you.

The constraint that bounds all of this

The CLI can't invent a richer model than the server exposes — the endpoint is PUT /repos/{id}/visibility with body enum public|private. So if the concern is the model (more than two levels, clearer semantics), that's a server/API decision first, and the CLI just mirrors it. If the concern is purely CLI ergonomics, it's bounded by that two-value enum today.

My recommendation

1. Keep visibility as a positional value, not a flag — it's idiomatic for a visibility set verb and consistent with the existing repo commands.
2. Push the naming concern, ideally at the server enum: I'd raise whether public should be internal / org (or whatever matches the actual SpiceDB semantics), since "public = authenticated-only" is genuinely misleading and will calcify once shipped.
3. Design parseVisibility for growth regardless — it's already a switch that fails fast, so adding a third value later is cheap; just don't let callers assume binary.

Want me to (a) draft a PR comment articulating the naming/extensibility concern for paul + whoever owns the backend enum, or (b) sketch what a 3-tier private | internal | public shape would look like across the CLI and the server contract? My honest take: the positional-vs-flag question is a wash, but the public-means-authenticated naming is worth flagging before it ships.

I've reviewed all hand-written code, verified the API surface, and traced the overlaps. Here's my assessment.

Review: #1549 — fix(grant): resolve repos by name, github:handle grantees, repo clone URL (COR-699)

Verdict: the code is good and well-tested — but the branch is stale and needs a rebase before merge. Two chunks of this diff are already-done or duplicated elsewhere.

🔴 Needs a rebase — two stale/overlapping changes

1. The ireturn lint removal is already on main. The .golangci.yaml change (−37 lines removing the ireturn linter, its allow-list, and per-path exclusions) and the agentimport.go //nolint:ireturn removal are already merged via #1548 (ef481758b chore: Ditch 'ireturn' lint., which is in your current main). I confirmed ireturn appears 0 times in origin/main:.golangci.yaml. After rebasing, these two files drop out entirely — that's a big chunk of the 291 deletions that's pure noise here. Drop them.

2. The resolveRepoRef fix is duplicated with PR #1531. Both #1549 and #1531 make the identical out.Repos[0] → out.Repo.Get() fix in resolveref.go (and both rewrite the same doc comment). They will conflict on the second merge. Since both are yours, decide which PR owns it — I'd pull it out of whichever lands second, or land a tiny standalone fix PR first and rebase both on it. (Worth noting the bug is real and the fix is correct in both; ListProjectReposOutputBody.Repo pre-exists on main, so it's a pure hand-code fix.)

Net: after rebasing, this PR shrinks to the actual feature — grant UX + repo REMOTE — which is what it should be reviewed as.

✅ The actual feature is solid

• resolveGranteeProvider (resolveref.go) is the centerpiece and it's well done: rejects a ULID up front with a message pointing at the github:alice form (the exact COR-699 footgun), resolves via the pre-existing ResolveHandle endpoint, maps not-found to a friendly error, and prefers the server-normalized provider. Verified ResolveHandle/ResolveHandleParams, isCoreNotFound, parseQualifiedHandle, repoRemoteURL all exist on main.
• The flag-soup → positional grantee migration is a genuine UX win, and the revokeGrantee helper nicely unifies the project/repo routing (ULID → typed-id route, handle → resolve → by-provider route) that was previously duplicated.
• granteeName fallback (friendly name → ULID) degrades gracefully, and the grant-row/column changes are additive codegen (GranteeName OptString, Source string).
• repo get REMOTE column correctly keeps the list view lean and only adds the URL to the single-repo detail view, with - when unprovisioned.

🟡 Smaller notes (non-blocking)

1. Typed-id revoke now hardcodes GranteeType: "account". The old --grantee-type flag could carry other kinds; now ULID revoke is account-only. This is consistent with grant-add being account-only since COR-561 dropped org/team granting, so it's not a practical regression — just worth confirming no pre-existing non-account grants need CLI revocation.

2. grant org remove doesn't accept a ULID (it calls resolveGranteeProvider directly, which rejects ULIDs), while project/repo remove do. The PR body's "remove also still accepts an account ULID" is only true for project/repo. Mild asymmetry; the error message guides the user, so fine — just tighten the wording.

3. Deployment ordering: the SOURCE column and friendly GRANTEE name depend on entiredb#2232. If the CLI ships before the server, granteeName falls back to ULID (graceful) but SOURCE renders blank (empty string). Land the server side first, or it's a slightly broken-looking table.

Tests

Strong. TestGrantRemove_RouteWiring/_Idempotent now drive the full provider:handle → resolve → by-provider route and the ULID → typed-id route through cobra; TestResolveGranteeProvider asserts the ULID rejection makes zero HTTP calls; TestGrantRows locks column/row width in lockstep. Good coverage of the new paths.

Recommendation

Request changes — but only for the rebase. Ask paul to rebase on current main (drops the ireturn hunks) and reconcile the duplicate resolveRepoRef fix with #1531. The feature code itself I'd approve as-is.

Want me to post this, or to check whether #1531 or #1549 is intended to merge first so the resolveRepoRef ownership is clear?

Rebase verified. Here's the re-review.

Re-review: #1549 (rebased)

One of the two rebase findings is fully resolved; the other is a cross-PR coordination item that a rebase-on-main can't fix yet.

✅ Resolved — stale ireturn removal is gone

.golangci.yaml and agentimport/agentimport.go have dropped out of the diff entirely (file count 13 → 11, deletions 291 → 252, matching the ~39 removed lines). Confirmed ireturn is 0 occurrences on main. The diff is now just the actual feature. 👍

🟡 Still outstanding — duplicate resolveRepoRef fix vs #1531

This isn't something your rebase could resolve: main still has the buggy out.Repos[0].ID (neither PR has merged), and #1531 is still open and still carries the identical out.Repo.Get() fix. So both PRs continue to change the same lines.

This isn't a defect in #1549 — the fix is correct and needed. It's purely merge ordering: whichever of #1531 / #1549 merges first, the other must rebase and drop the now-redundant resolveRepoRef hunk (a 3-line + comment conflict). Since both are yours, just decide the order. If you tell me which lands first, I can confirm the other comes out clean.

Everything else stands (unchanged by the rebase)

The feature code is identical to what I reviewed and I'm happy with it:

• resolveGranteeProvider — ULID rejected up front with a pointer to the github:alice form, resolves via the pre-existing ResolveHandle, friendly not-found mapping, prefers server-normalized provider.
• Flag-soup → positional grantee migration + the shared revokeGrantee routing helper.
• granteeName fallback and the additive GRANTEE/ID/SOURCE columns.
• repo get REMOTE column on the detail view only.
• Tests remain strong (wiring through cobra for both routes, ULID-makes-zero-calls, row/column lockstep).

The three minor notes from before still apply and remain non-blocking: typed-id revoke hardcodes GranteeType: "account" (consistent with account-only granting), grant org remove won't take a ULID (project/repo do), and SOURCE renders blank if the CLI ships ahead of entiredb#2232.

Recommendation

Approve on the code. The only thing gating a clean merge is sequencing the resolveRepoRef fix against #1531 — not a change to this PR's logic. Want me to check which PR is further along so you can pick the merge order?

I've reviewed the full implementation, the supporting strategy/session-state changes, and verified the test coverage by name. This touches a sensitive subsystem, so I went deep.

Review: #1472 — feat: adopt active sessions across repos and worktrees

Verdict: careful, unusually well-tested work on a genuinely tricky operation. The code quality is high. My main reservations are design-level, not defects — chiefly an asymmetry in how the external-store path treats the source session.

Strengths (called out because they're real, not boilerplate)

• Concurrency is handled seriously. WithSessionStateLocks sorts + dedupes lock paths (classic multi-lock deadlock avoidance), and both adopt paths re-load and re-validate the source under the lock after selecting it unlocked — the right TOCTOU pattern. Tests explicitly cover the lock-wait races (ExternalStoreChecksTargetStateAfterLockWait, SameStoreReloadsSourceStateUnderLock, ExternalStoreRejectsSourceEndedAfterInitialSelection).
• cloneAdoptSourceState deep-clones every map/slice/pointer (token usage recursion, skill-event anchors, prompt attributions), with a dedicated aliasing test. That's exactly the bug class this kind of code usually ships.
• Transcript ownership validation (validateAdoptSourceTranscript) guards against pointing the continuing session at a transcript not owned by its registered agent.

🟡 Main design question — external path copies, same-store path moves

This is the asymmetry I'd want resolved before merge:

• Same-store (adoptFromSameSessionStore): overwrites the single state file → the session moves to the target worktree. No divergence.
• External-store (adoptFromExternalSessionStore): writes a copy into the target store and leaves the source session untouched and still PhaseActive.

So after a cross-repo adopt, the same session ID is live in two stores with divergent BaseCommit and (reset vs. original) checkpoint bookkeeping. If any hook fires in the source worktree afterward (agent still has it open, a Stop hook, etc.), you can get two divergent checkpoint histories under one session ID, both potentially condensing onto entire/checkpoints/v1. The reset of target-local checkpoint state mitigates inheritance, but not concurrent divergence.

Question for the author: why doesn't external adoption retire the source (mark ended / FullyCondensed) the way the same-store path effectively does by moving it? If "the agent moved, the source is dead" is the assumption, encoding that — end the source under the same lock — would make the two paths consistent and close the divergence window. If concurrent activity in both is actually intended, that deserves an explicit comment explaining why it's safe.

🟡 Fragility of the manual field-reset list

buildAdoptedSessionState hand-resets ~20 fields of session.State (StepCount, checkpoint offsets, turn IDs, prompt windows, owner, …) and intentionally preserves others. It's correct today and the comments are excellent, but it's an open-ended denylist: the day someone adds a new repo-specific field to session.State, adopt will silently carry it across repos with no compile-time or test signal. Consider either a guard test that fails when session.State gains a field not explicitly classified, or a short doc note at the State definition pointing here. Not a blocker, but this is the line most likely to rot.

🟡 FilesTouched attributes all current target changes to the agent

currentFilesTouched seeds from DetectFileChanges (modified/new/deleted — confirmed repo-root-relative, so the overlap check will match). But this attributes any uncommitted change in the target repo — including unrelated edits the human made — to the adopted agent session. The output does warn ("adoption attributes current changes in this repo to the adopted session"), which is the right mitigation, but it's worth being deliberate that this is acceptable for attribution-correctness, not just commit-linking.

🟢 Minor

1. Docs not updated. adopt isn't in CLAUDE.md's session subcommand list (list/info/tokens/stop/attach/resume/current), and there's nothing in docs/architecture/sessions-and-checkpoints.md. CLAUDE.md asks to keep these current when session/strategy behavior changes — this adds a new way to mutate session state, so it qualifies.
2. sessionBelongsToSourceWorktree returns true when state has neither WorktreeID nor WorktreePath. In the no-ID auto-select path that could pick a session that doesn't actually belong to --from. Edge case (malformed/old state), low risk, but a stricter default would be safer.
3. --force and --yes both bind the same opts.Force. Works, but it's slightly unusual and the per-flag help only mentions "replace an existing local state file" — the --yes semantics (confirm the same-store move) live only in the Long text.

Tests

Strong — 17 functions covering external copy, same-store move, no-op rejection, ended/condensed rejection, transcript-ownership rejection, owner clearing, clone aliasing, legacy-offset clearing, subdirectory resolution, shared-store filtering, and two prepare-commit-msg integration tests proving the adopted session actually links the next commit. This is well above the bar.

Recommendation

Approve-with-comments, with one item I'd genuinely want answered first: the copy-vs-move asymmetry and whether external adoption should retire the source session. The fragility and docs items are follow-ups, not gates.

Want me to (a) post this as a review, or (b) pull the source-retirement question directly to peyton as a focused PR comment with the divergence scenario spelled out?

I've reviewed the full implementation, verified the matching logic, and compared against the existing adapters. Here's my assessment.

Review: #1313 — Add Pi review-runner adapter

Verdict: clean, additive, and follows the established ReviewerTemplate pattern well. I'd approve with two questions worth answering first. #1312 is merged, so the diff is clean Pi-only code with no changes to shared logic.

Scope clarification (minor PR-description nuance)

The body says "Manifest matching uses tighter model ID normalization (strip provider prefix / tier suffix)." That normalization (normalizeReviewModelID / reviewRunModelMatches / modelComponentsMatch) already exists on main from #1312 — this PR doesn't touch manifest.go, it only adds a test (REDACTED) exercising it with Pi. So this PR relies on that logic rather than introducing it. Good — it means zero risk to existing agents' session matching.

🟡 Main concern — prompt passed as an argv positional (ARG_MAX risk)

buildPiReviewCmd appends the full composed prompt as a positional arg:

Review prompts embed the diff and can be large. Passing that on argv risks E2BIG at exec time on big changesets — and the two most-recently-added adapters deliberately avoid this by piping via stdin:

• geminicli: cmd.Stdin = strings.NewReader(prompt)
• codex: cmd.Stdin = strings.NewReader(prompt)
• claudecode: -p <prompt> (argv — same risk, so there's precedent, but it's the minority)

generate.go does the same (pi … <prompt>). If pi --print / --mode json can read the prompt from stdin, I'd switch both to stdin for parity with codex/gemini and to remove the large-diff failure mode. If Pi only accepts argv, that's worth a comment noting the size ceiling. Failure mode is a silently-failed Pi worker on large reviews, not a crash — but it'd be Pi-specific and confusing.

🟡 Token accounting — confirm message_end vs turn_end usage semantics

emitPiReviewTokens adds usage from both message_end and turn_end, deduped by MessageID() — but only when the key is non-empty:

The dedup test (...DedupesTurnEndForSameMessage) covers a turn_end carrying the same id as the message_end. But if Pi ever emits turn_end usage with a different or empty message id (e.g. turn-cumulative usage under a turn id), the key != ""/seen guard won't catch it and tokens double-count. The question for the author: does Pi report usage at both the message and turn level, and if so, is the turn-level figure per-turn-incremental or cumulative? If it can be cumulative, summing both is wrong regardless of id dedup. A test with an empty-id turn_end carrying usage would pin this down.

🟢 Minor

• parsePiModelList splits pi --list-models by whitespace and detects the header by the literal provider/model tokens. Fragile to format/casing changes (a model whose first column isn't recognized as a header would be parsed as a model row), but Pi owns its own output format, so acceptable.
• success defaults true, only flipped on error/aborted stopReason, and authoritative completion comes from agent_end → finished. If the stream truncates before agent_end, you correctly get Finished{false}. Consistent and fine.
• The parser continues after a JSON unmarshal error (emits RunError, keeps scanning) — good resilience; a fully garbled stream would emit many RunErrors, but that's an acceptable edge.

Positives worth noting

• Parser is well-structured: cumulative token events, message_end fallback text when no deltas streamed, unknown-event tolerance ("additive stream"), 64MB scanner ceiling.
• Test coverage is solid (argv/env shape, JSON event stream, cumulative tokens, fallback text, same-id dedup, same-agent/different-model manifest disambiguation).
• docs/architecture/review-command.md is meaningfully updated (+76) — including the Pi-is-prompt/model-driven note. Good doc hygiene, unlike some recent PRs.
• Wiring is consistently applied across every list (summary providers, default profile, master preference order, configure/help text, error messages).

Recommendation

Approve-with-comments. Neither concern is a correctness bug I can prove — both are "confirm Pi's actual behavior": (1) can the prompt go over stdin to avoid ARG_MAX, and (2) does turn_end usage ever double-count against message_end. Want me to post this as a review, or draft those two as focused inline questions for Daniel?

Both findings are live on trail 467 (PR #1313), status open, severity medium:

ID	Finding	Location
019f1412-80a…	Token double-count across message_end / turn_end — dedup only catches matching non-empty ids	reviewer.go:175 (emitPiReviewTokens)
019f1412-4b4…	Prompt passed as argv positional (ARG_MAX risk); codex/gemini use stdin	reviewer.go:38 (buildPiReviewCmd)

Two notes on how they landed:

• File-level location, not line-level. entire trail finding add --line is rejected with location.granularity=line requires non-empty selected_text, and add has no flag to supply selected text. So I attached them at file granularity and put the precise line numbers (reviewer.go:38, :175, plus the turn_end case at :97) in the body text instead.
• Idempotency keys set (--client-id pi-reviewer-prompt-argv-argmax and pi-reviewer-token-double-count), so re-running won't create duplicates.

Both are framed as "confirm Pi's actual behavior" questions rather than asserted bugs, matching my review. If you'd rather they be line-pinned, I'd need the selected source text to attach — say the word and I can dismiss-and-repost with --start-line/--end-line if that path accepts a range without selected text, or just leave them as-is.

All six findings from my #1472 review are now live on trail 613, alongside the one pre-existing finding (the EvalSymlinks one, which wasn't mine):

Medium (3):

• 019f1435-6c3 — Copy-vs-move asymmetry: external adoption leaves the source session live in two stores → divergence risk (adoptFromExternalSessionStore)
• 019f1435-77f — Fragile field-reset denylist in buildAdoptedSessionState; new State fields silently carried across repos
• 019f1435-83b — FilesTouched attribution of unrelated target changes to the agent

Low (3):

• 019f1435-c5a — Docs not updated for the new adopt verb (→ sessions-and-checkpoints.md)
• 019f1435-d0b — sessionBelongsToSourceWorktree returns true on empty worktree info
• 019f1435-db8 — --force/--yes dual-bind with identical help text

Notes:

• Same file-level location constraint as before (line-granularity needs selected text, which add has no flag for), so precise line numbers are embedded in each body.
• Each has an idempotency --client-id, so re-running is safe.
• I left the pre-existing EvalSymlinks finding untouched — it's distinct from mine and not something I raised.

I've reviewed the full command, the auth change, the privacy model, and the test coverage. This one's in good shape.

Review: #1573 — Add hidden experts CLI command

Verdict: high-quality, unusually well-tested feature. Both sensitive areas — the plain-HTTP auth gating and the privacy filtering — are sound and directly tested. I'd approve with a couple of questions, none blocking.

✅ Auth change is correctly gated (the highest-risk part)

ResolveDataAPIToken is a shared data-API path (not experts-only), so this matters beyond this command — but the gating is right:

• shouldUsePlainHTTPDiscovery returns true only when the origin scheme is already http:// AND (insecureHTTPEnabled() OR isLoopbackHTTP). A production https:// origin can never trigger the downgrade.
• The scheme-rewriting dataAPIHTTPDiscoveryTransport is only constructed when the gate passes, and it's confined to the discovery client (resolveContextForAPI). Discovery fetches the unauthenticated /.well-known, and the token exchange runs separately against the discovered (TLS) issuer — so no bearer token rides over plain HTTP. The E2E confirms this (issuer=https://…, exchange succeeded).
• Tested: TestResolveDataAPIToken_UsesPlainHTTPDiscoveryForLoopbackDataOrigin.

This mirrors the existing EnableInsecureHTTP pattern for the control plane — consistent.

✅ Privacy filtering is robust, not fragile

The mechanism is allowlist-by-struct: the response decodes into expertsResponse/expertsProfile/expertsEvidenceItem, which carry no human-identity fields, and every output path (--json, text renderExperts, TUI) re-emits the decoded struct, never the raw body. api.DecodeJSON uses plain json.Unmarshal (no DisallowUnknownFields) with a maxResponseBytes cap, so extra server-sent human fields are silently dropped rather than causing an error. TestExpertsCommandSendsQueryAndPrintsJSON seeds first_commit_author_username: peyton server-side and asserts it's absent from output — a real proof, not a token gesture. This is the safe direction (adding a field requires opting it into the struct).

✅ Other correctness that's handled well

• localExpertScope/localPathScope reject paths outside the worktree (..), handle absolute/cwd-relative/subdirectory/deleted/symlinked scopes; staged scopes are repo-root-relative (git diff --cached --name-only, correct per the repo's cwd-vs-root guidance).
• resolveExpertsRepoID is authz-safe — resolves owner/repo→ULID via the caller's accessible-repo list, so it can't reveal a repo they can't see.
• The 503 → "code search not configured" rewrite is honest: query-only and message-gated, so path-scope failures stay truthful (TestExpertsCommandDoesNotRewritePathScope503AsCodeSearch).
• Test coverage is broad (18 functions): staged additions/deletions, subdirectory resolution, ULID passthrough, --repo+--staged rejection, CRLF handling, agent-centered (not human-centered) framing.

🟡 Questions (non-blocking)

1. resolveExpertsRepoID doesn't paginate /api/v1/repos. It decodes a single body.Repos page and matches by full name. If the data API pages that endpoint, a user with many repos could hit a spurious "repo is not among your accessible repos" for a repo beyond page 1. Elsewhere the CLI uses fetchAllPages for exactly this. Does the data API (companion entire.io#2684) page this endpoint? If so, loop the pages.
2. --limit is silently clamped to [8, 20] (floor on <=0, ceil at 20). Reasonable, but a user passing --limit 50 gets 20 with no notice. Consider noting the cap in the flag help.

🟢 Minor

• localExpertScope returns (string, bool, bool, error) — two positional bools (isLocalScope, validateLocalRepo) are a bit opaque at the call site; a small result struct would read better. Low.
• Scope note: I focused on the command/auth/privacy logic. experts_tui.go (521 lines) I did not deep-review — it's read-only rendering of the already-filtered decoded data (no auth, no mutation, no raw-body access), so the privacy/security surface is covered upstream by the decode step.

Recommendation

Approve-with-comments. The only thing I'd genuinely want answered is the /api/v1/repos pagination question (#1), since it's a real "works on my machine, breaks for power users" risk. Want me to post the findings to trail 709 like the last two, or draft the pagination question as an inline comment?

All three findings are live on trail 709 (PR #1573), alongside the one pre-existing finding (the case-sensitivity one at line 244, which wasn't mine):

ID	Sev	Finding
019f1dce-b22	Medium	resolveExpertsRepoID doesn't paginate GET /api/v1/repos → spurious "not accessible" for power users
019f1dce-c12	Low	--limit silently clamped to 8–20
019f1dce-cca	Low	localExpertScope returns two opaque positional bools

I deliberately posted only the concerns — the auth gating and privacy filtering were positives in my review, not findings, so they're not logged. Same conventions as before: file-level location with line numbers in the body (line-granularity needs selected text, which add doesn't support), and idempotency --client-ids so re-runs won't duplicate.

The pagination one (medium) is the only substantive risk and hinges on the companion API's behavior — worth confirming with Suhaan whether /api/v1/repos on the data API pages before merge.

Yes — it changed, and the changes directly address my review. There's a commit 1e398d1c fix(experts): address Soph trail review on scope, limit, repo pagination. All three of my findings are resolved:

Re-review: #1573 (updated)

#	My finding	Status
🟡 Medium	resolveExpertsRepoID didn't paginate /api/v1/repos	Fixed
🟢 Low	--limit silently clamped	Fixed
🟢 Low	localExpertScope opaque positional bools	Fixed

1. Pagination (the substantive one) — properly fixed. resolveExpertsRepoID now delegates to a new listExpertsAccessibleRepos, which wraps fetchAllPages with a page_token cursor and reads next_page_token from the response — exactly the fix I suggested. And it's backed by a real test, TestResolveExpertsRepoIDPaginatesAccessibleRepoList, that puts the target repo on page 2 and asserts both requests fire (…/repos then …/repos?page_token=page2). So the power-user "not accessible" false negative is closed and regression-guarded.

2. --limit — flag help now reads "Maximum profiles to return (1–20; values above 20 are clamped)", with extracted expertsDefaultLimit/expertsMaxLimit constants. The clamp is now documented, not silent.

3. localExpertScope — now returns a named expertLocalScopeResult struct instead of (string, bool, bool, error). Readability concern gone.

Also addressed (not mine): the other reviewer's case-sensitivity finding at line 244 (4f2a630a fix(experts): compare repo names case-insensitively for local scopes), plus subdirectory path relativization and CRLF test coverage.

Unchanged (still good): the two sensitive areas I'd already cleared — the plain-HTTP auth gating (data_api.go/exchange.go) and the privacy-by-struct filtering — are untouched (+38/-1, same as before). No new concerns introduced; the growth is the pagination helper + tests.

Net: this now clears my review — I'd approve.

Want me to mark my three findings on trail 709 as resolved (they're all verifiably addressed)? I can entire trail finding resolve each by ID.

I've reviewed the full implementation, the refactor, and the test coverage, and compared against the sibling trail delete. Here's my assessment.

Review: #1462 — Add entire trail merge subcommand

Verdict: clean, well-structured, and the API-interaction correctness is solid. My main finding is a safety/UX inconsistency with its sibling command, plus a test gap on the orchestration. Approve-with-comments.

✅ What's done well

• resolveTrailBySelector extraction is a genuine dedup — runTrailShow now shares it, and the "never nil trail with nil error" invariant holds across all return paths (callers can rely on a non-nil result).
• Doesn't trust a 2xx — mergeTrailByNumber verifies the server's ok:true before reporting success, mirroring deleteTrailByNumber. Correct and consistent.
• Checks mergeability before merging, and the TOCTOU window (state changing between check and merge) degrades safely — the server re-gates on the merge endpoint, and checkTrailResponse + !res.OK surface that. Good that it doesn't rely solely on the client-side check.
• describeMergeBlockers lists blockers in server-gate order with a defensive fallback for the "not mergeable but no recognized blocker" case.
• Dry-run semantics are correct: the !mergeable gate sits before the dry-run branch, so --dry-run returns a non-zero error on an un-mergeable trail (CI-gatable) and exits 0 with a "mergeable" message otherwise.
• Helper-level test coverage is good (fetch + server-error, merge + ok:false, blocker text across states, summary rendering).

🟡 Main finding — no confirmation prompt, unlike trail delete

entire trail delete prompts for confirmation unless --force is passed ("Deletion is permanent; you are prompted to confirm unless --force is passed"). entire trail merge has no such prompt — a real run merges the branch into base immediately once gates pass.

Merging into the base branch is a comparably consequential, remote-state-changing, not-trivially-reversible operation. The mergeability gate (approvals + CI) limits what can be merged, but it doesn't protect against merging the wrong trail — e.g. running entire trail merge from the wrong branch, or a fat-fingered invocation, merges the current branch's trail with no "Merge trail #N into main? [y/N]". gh pr merge prompts by default for the same reason.

Recommendation: match the delete pattern — interactive confirm on a TTY, --force/--yes to skip for automation (dry-run unaffected). Or, if merge is intentionally prompt-free for the automation-first use case, document that choice. This is the one thing I'd want decided before merge.

🟡 Test gap — runTrailMerge orchestration is untested

The tests cover the four helpers individually (fetchTrailMergeability, mergeTrailByNumber, describeMergeBlockers, printTrailMergeability), but there's no test for runTrailMerge itself — the part with the actual behavior contract:

• --dry-run stops after the check and returns non-zero when not mergeable (the CI-gating promise in the PR body),
• an un-mergeable trail returns an error before attempting the merge,
• the check-then-merge sequencing.

That orchestration is exactly what's most likely to regress (e.g. someone reorders the dry-run branch above the gate). Worth a table test driving runTrailMerge with a fake client across mergeable/not-mergeable × dry-run/real.

🟢 Minor

• On a non-mergeable trail, the summary (with ✗ marks) prints to stdout and an error listing the same blockers goes to stderr — slight redundancy, but the separation is actually helpful for CI (parse stdout, gate on exit code). Fine as-is.

Recommendation

Approve-with-comments, with the confirmation-prompt parity (🟡 #1) as the one real decision, and the runTrailMerge test (🟡 #2) as a should-add.

Want me to post these to trail 604 like the others?