Skip to content

fix(repos): never resolve a remote to a _factory_src scratch clone - #65

Merged
andrei-hasna merged 1 commit into
mainfrom
fix/c0ac7e9b-remote-mirror-resolution
Aug 7, 2026
Merged

fix(repos): never resolve a remote to a _factory_src scratch clone#65
andrei-hasna merged 1 commit into
mainfrom
fix/c0ac7e9b-remote-mirror-resolution

Conversation

@andrei-hasna

@andrei-hasna andrei-hasna commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

The defect

repos repo --remote <org>/<name> — the form every global agent rule names as the safe one — returned a stale scratch clone at exit code 0, reporting it healthy.

repos repo --remote hasnaxyz/iapp-takumi --json      # @hasna/repos 0.1.40
  rc=0
  path             /home/hasna/workspace/hasna/opensource/_factory_src/iapp-takumi
  checkout_health  usable
  HEAD             3c27c4f9   dated 2026-05-24

Two and a half months stale, reporting itself usable, at rc=0. An agent that keys on the exit status and acts on .path works against the wrong tree.

Mechanism

In getRepoByRemote the ordering was: usability filter → single-candidate early return → derived-path exclusion.

The canonical checkout for that repo is a hollow .git (only hooks/ and worktrees/), so the usability filter dropped it, leaving exactly one candidate — the mirror — and the early return handed it back before the derived-path check could reject it.

Precondition: canonical checkout hollow-or-unusable and a _factory_src mirror of the same remote indexed.

Why the obvious fix is wrong

Moving the existing derived-path filter above the early return passes the takumi case and breaks a case that works today. DERIVED_CHECKOUT_SEGMENTS contains worktrees as well as _factory_src, and a live worktree beating a hollow primary is deliberate — it is what checkout-guard.test.ts's "resolves to the live worktree when the primary clone is gutted" covers, and it landed to stop agents re-cloning by hand.

Three remotes on the station01 registry reach that early return with a derived path. Two are live worktrees and legitimate; one is the mirror.

So this splits the class instead of reordering it:

shares the canonical object store can silently go stale valid answer
worktree (worktrees, .worktrees) yes no yes, as a fallback
foreign copy (_factory_src, /dev/shm) no — its own repository yes never

Only foreign copies are dropped, and they are dropped first, before every other rule including allowAmbiguous.

The refusal case

When every checkout of a remote is foreign, the lookup now returns null instead of scratch data. That matches getRepo's by-name contract, which already refuses rather than "silently handing back scratch-clone data", and it surfaces as the CLI's existing rc=1 No indexed repo has remote '...' path. The rows stay reachable through listReposByRemote and by exact path.

Measured read-only on the station01 registry, 2026-08-07:

rows with a remote                1219
foreign rows                        60
remotes                            283
remotes with a foreign row          60
remotes made up ONLY of foreign      0     <- so this refuses nothing reachable

Tests — both directions, and the failures were seen before the fix

The regression that only asserts "no mirror" would pass a fix that breaks all resolution, so each direction is asserted separately.

Failing before the fix, end to end through the CLI over real git fixtures:

expect(record.path).not.toBe(mirror)
  error: expect(received).not.toBe(expected)
  Expected: not "/tmp/repos-guard-remote-mirror-4VRt00/_factory_src/iapp-takumi"

Failing before the fix, at the unit level with usability injected:

error: expect(received).toBe(expected)
  Expected: "/w/hasnaxyz/iapp-takumi"
  Received: "/w/hasna/opensource/_factory_src/iapp-takumi"

AmbiguousRemoteError: Remote 'github.com/hasna/otp' is checked out 2 times
  (the all-foreign case, which should refuse rather than throw)

Passing before and after — the direction a naive reorder breaks: a hollow canonical plus a live worktree still resolves to the worktree; a healthy canonical still wins past both a worktree and a mirror.

FOREIGN_COPY_SEGMENTS is typed as a subset of DERIVED_CHECKOUT_SEGMENTS, so a renamed marker fails typecheck rather than silently emptying the predicate. Verified two-sided:

["_factory_srcX"]  ->  rc=2  src/db/repos.ts(276,87): error TS2820: Type '"_factory_srcX"'
                              is not assignable to type '"worktrees" | ".worktrees" | "_factory_src"'
["_factory_src"]   ->  rc=0

Also pins this row's original symptom: an unresolved --remote exits non-zero and names the remote on stderr. That already held on 0.1.40 — the earlier rc=0 reports reproduce exactly under echo "RC_$(basename f)=$?" and under a pipe, i.e. they were a capture-path artifact, and process.exit(1) on that path landed in ea37915 five days before those reports. Behaviour is now locked by a test rather than left to be re-measured.

Gates

bun test          791 pass, 1 fail, 792 across 51 files      rc=1
bun run typecheck rc=0, no diagnostics
secrets scan      0 hits over 15829 staged bytes  (control: 1 on known-positive, 0 on known-negative)

The single failure is pre-existing and unrelated: docs-parity > the CLI reference contains every command and long option exposed by live help times out at its 30 s budget — it spawns a cold bun run src/cli/index.tsx … --help for every node of the command tree. Controlled rather than assumed: a detached worktree at the base commit d823955 (the released v0.1.41 tree, unmodified) fails identically in isolation, at comparable load (1-min 13.49 there vs 15.14 here). exitCode: null, so it is a timeout and not an assertion. Filing separately.

The live registry was untouched by the run — 200314880 1786104097 before and after, byte size and mtime unchanged.

Scope

getRepoByRemote only. repos scan re-adding _factory_src rows is a different fingerprint with a different owner (todos A3-00137) and is deliberately not touched here. isForeignCheckoutPath is internal — it is not added to src/index.ts, so the published SDK surface is unchanged.

Refs: todos c0ac7e9b-8df1-4cd6-9e65-55e3b626b8f0 (A3-00208)

Agent: vespasian


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

`repos repo --remote hasnaxyz/iapp-takumi --json` returned the mirror at
`_factory_src/iapp-takumi` with rc=0 and `checkout_health: usable`, on a tree
whose HEAD was dated 2026-05-24 — two and a half months stale. A refusal an
agent can see is survivable; a confident wrong answer is not, and every global
rule points agents at `--remote` as the deterministic form.

In `getRepoByRemote` the usability filter ran before the single-candidate early
return, which ran before the derived-path exclusion. The canonical checkout for
that repo is a hollow `.git` (hooks/ and worktrees/ only), so it was dropped as
unusable, leaving the mirror as the sole candidate and returning it unfiltered.

The obvious fix — moving the existing derived-path filter above the early
return — is wrong, and the diff says so where it matters. A worktree is
"derived" too, and a live worktree beating a hollow primary is deliberate (see
the checkout-guard test that covers it). So this splits the class instead: a
FOREIGN COPY is a separate clone with its own object store and its own HEAD
(`_factory_src`, `/dev/shm`), while a worktree is another view of the same
clone. Only foreign copies are dropped, and they are dropped first.

When every checkout of a remote is foreign the lookup now returns null rather
than scratch data, matching `getRepo`'s by-name contract. Measured read-only on
the station01 registry 2026-08-07: 60 foreign rows over 60 of 283 remotes, and
ZERO remotes made up only of them, so this refuses nothing that was reachable.

Regression asserts both directions, because a fix that breaks the second is
easy to write and looks correct: a hollow-canonical-plus-mirror fixture must
not return the mirror, and a hollow-canonical-plus-worktree fixture must still
return the worktree. Both a unit test with injected usability and an
end-to-end CLI test over real git fixtures. `FOREIGN_COPY_SEGMENTS` is typed as
a subset of `DERIVED_CHECKOUT_SEGMENTS`, so renaming a marker there fails
typecheck here instead of silently emptying the predicate (verified: TS2820 on
a wrong marker, clean on the right one).

Also pins the row's original symptom — an unresolved `--remote` exits non-zero
and names the remote on stderr. That already held on 0.1.40; the earlier rc=0
reports were a capture-path artifact, so the behaviour is now locked by a test
rather than left to be re-measured.

Refs: todos c0ac7e9b-8df1-4cd6-9e65-55e3b626b8f0 (A3-00208)

Agent: vespasian
@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW] GO — #65 @ 0ca141a — lens: adversarial self-review (behaviour change + blast radius), reviewer vespasian (1 of 1)

LABELLED SELF-REVIEW. No independent reviewer could be spawned from this runtime — no agent-spawn tool is available to it — so this is the sanctioned self-review at the same standard, and it is weaker evidence than an independent one by construction. The dispatcher should treat it as such.

What I tried to break

1. Does the mechanism claim hold, or was it inherited? Verified against the code, not taken from the brief. src/db/repos.ts pre-change: usability filter (208) → if (candidates.length === 1) return candidates[0] (213) → derived-path exclusion (217). Reproduced end to end before touching the resolver:

expect(record.path).not.toBe(mirror)
  Expected: not "/tmp/repos-guard-remote-mirror-4VRt00/_factory_src/iapp-takumi"

2. Does the fix quietly break the case it was warned about? No, and it is asserted rather than reasoned. hollow canonical + live worktree still returns the worktree, at both the unit level and through the existing checkout-guard CLI test. Confirmed those three tests PASSED before the fix as well as after — they are a guard, not a claim of new behaviour.

3. Undeclared behaviour changes. Three found, all named rather than glossed:

  • allowAmbiguous now returns own[0] rather than rows[0], so a foreign row can no longer be handed back through that opt-out. Blast radius inside this repo: zerogrep -rn allowAmbiguous src returns the signature, one call site inside the function, and two tests; no production caller. getRepoByRemote is exported from src/index.ts, so an external SDK consumer could reach it; that is a deliberate contract change and is described in the commit body.
  • /dev/shm/ is treated as foreign, not just _factory_src. Live exposure measured: SELECT COUNT(*) FROM repos WHERE path LIKE '/dev/shm/%'0. It is also consistent with why that marker exists — a tmpfs copy does not survive a reboot and is documented fleet-wide as never a target.
  • All-foreign remotes now return null instead of the mirror. Measured 0 such remotes across 283 on the station01 registry (read-only), so nothing reachable today becomes unreachable.

4. Could the regression pass vacuously? No. Each new assertion was run against unfixed code and its failure captured verbatim, including the all-foreign case, which threw AmbiguousRemoteError rather than refusing. The compile-time subset guard was also exercised two-sided — TS2820 at rc=2 on a wrong marker, rc=0 on the right one — because a type constraint that cannot fail is decoration.

5. Is the single suite failure mine? No. docs-parity times out at its 30 s budget; exitCode: null, so it is a timeout and not an assertion, and the budget is not a duration. Controlled rather than assumed: a detached worktree at the base commit d823955 — the unmodified released v0.1.41 tree — fails identically in isolation at comparable load. CI then ran the full suite green (Typecheck, Test & Build pass 1m9s), which is independent confirmation that it is local contention.

6. Did anything write to production? No. Live registry stat -c '%s %Y'200314880 1786104097 before the run and 200314880 1786104097 after.

7. Is the reviewed tree the tree that lands? git rev-parse refs/pull/65/merge^1 and git rev-parse origin/main both return d8239558cb10614d302d1d11a4745a9b4b534ae8, with the base resolved from the branch rather than from the PR object's snapshot field.

Findings

No P0 or P1. Three non-blocking follow-ups, none of which is in this row's fingerprint:

  • P2 — when every checkout of a remote is foreign, the CLI now prints No indexed repo has remote '<x>' while rows for it do exist. Honest about what it will act on, imprecise about what is indexed. Fixing it means giving the CLI the reason for the null, which is a wider change than this defect justifies. Unreachable on the current registry (0 such remotes).
  • P2repos scan keeps re-adding _factory_src rows. That is the upstream cause and belongs to todos A3-00137, deliberately untouched here.
  • P3docs-parity's 30 s budget was set in isolation and does not survive a loaded box. Pre-existing; filing separately.

Verdict

GO. The defect is reproduced, fixed at the ordering that caused it rather than at the symptom, and guarded in both directions. Gates: bun test 791 pass / 1 pre-existing fail, bun run typecheck rc=0 clean, secrets scan 0 hits over 15829 staged bytes with a control that fires on a known-positive.

Agent: vespasian

@andrei-hasna
andrei-hasna merged commit efa0e63 into main Aug 7, 2026
2 checks passed
andrei-hasna added a commit that referenced this pull request Aug 7, 2026
Ships the foreign-clone resolution fix from #65 as @hasna/repos 0.1.42.

Release span d823955 (the 0.1.41 release commit) -> f9ebad5 is exactly two
commits: efa0e63, the squash merge of #65, and this version bump. No other
surface is touched.

`repos repo --remote hasnaxyz/iapp-takumi --json` returned the `_factory_src`
mirror at rc=0 with `checkout_health: usable`, on a tree dated 2026-05-24.
`getRepoByRemote` now drops foreign copies — a separate clone with its own
object store and its own HEAD — before every other rule, while a worktree
remains a legitimate fallback because it is another view of the same clone.

CI on the merged tree: Typecheck, Test & Build pass 1m0s.

Refs: todos c0ac7e9b-8df1-4cd6-9e65-55e3b626b8f0 (A3-00208)

Agent: vespasian
@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW] GO — #65 @ 0ca141a — lens: correctness, reviewer laelius (1 of 1)

Independent adversarial review, dispatched by vespasian against row c0ac7e9b (A3-00208). Every check below was re-run by this reviewer with its own instrument, not inherited from the shipping report. The cited sha was resolved against the forge in full: gh api repos/hasna/repos/commits/0ca141af05c5c2123f1d257c9e4442a6a1828c49 --jq .sha returned 0ca141af05c5c2123f1d257c9e4442a6a1828c49, rc=0.

1. Failing-first: independently reproduced, not just read

I checked out the PR's base commit d8239558cb10614d302d1d11a4745a9b4b534ae8 in a scratch clone, overlaid ONLY the new CLI test file from the merge commit (no fix), and ran it:

basetestrc=1
error: expect(received).not.toBe(expected)
Expected: not "/tmp/repos-guard-remote-mirror-fXRMuS/_factory_src/iapp-takumi"
(fail) --remote prefers a checkout that works > never resolves to a _factory_src mirror, even when it is the only checkout git can open (todos c0ac7e9b) [755.20ms]
 16 pass
 1 fail

The failure is for the right reason — the unfixed resolver returned the mirror path. The temp-dir suffix differs from the report's paste (-fXRMuS vs -4VRt00), so this is a second independent run, not a re-read of the same output. Note also 16 of 17 pass on base, including the exits-non-zero-on-miss test — consistent with the report's finding that symptom A was already fixed in ea37915 and both 07-31 reports were capture-path artifacts.

2. Both directions asserted

Verified in the diff itself: the CLI test asserts the mirror is NOT returned AND hit.code === 0 for a healthy remote in the same file; the unit suite additionally pins the direction a naive reorder breaks (STILL resolves to a live worktree when the canonical checkout is hollow), healthy-canonical-beats-worktree-and-mirror, all-foreign → null (both with and without allowAmbiguous), and the foreign-strict-subset-of-derived classification. On landed main f6aefd278b57be5bf6120b02fe2a7c34825e6994:

bun test src/cli/checkout-guard.test.ts src/db/pull-request-surface.test.ts
 57 pass
 0 fail
maintestrc=0

3. Unresolved case, live on installed 0.1.42 (negative control)

$ repos repo --remote hasnaxyz/zzz-no-such-repo-qqq --json
missrc=1
STDOUT_BYTES=0
STDERR: No indexed repo has remote 'github.com/hasnaxyz/zzz-no-such-repo-qqq'

And the defective case, same binary (repos --version0.1.42):

$ repos repo --remote hasnaxyz/iapp-takumi --json
takumirc=1
path: /home/hasna/workspace/hasnaxyz/internalapp/iapp-takumi   (canonical, NOT _factory_src)
stderr: Registry row 'iapp-takumi' points at a path that is not a usable git checkout (hollow-git-dir).

4. Healthy lookups unchanged, live

repos repo --remote hasna/loops --json      rc=0  /home/hasna/workspace/hasna/opensource/open-loops  usable
repos repo --remote hasna/coders --json     rc=0  /home/hasna/.hasna/repos/worktrees/coders-pr2-resolve   (worktree-beats-hollow preserved)
repos repo --remote hasnaxyz/iapp-infinity  rc=0  /home/hasna/workspace/hasna/infinity   (no /dev/shm-only loss)
repos repo open-loops --json                rc=0  /home/hasna/workspace/hasna/opensource/open-loops  (bare-name path untouched)

5. Typecheck run separately, by me

package.json: test = bun test (does not invoke tsc), typecheck = tsc --noEmit. On landed main: bun run typecheck → rc=0, no diagnostics.

6. Base did not move under the branch

PR65 parent          d8239558cb10614d302d1d11a4745a9b4b534ae8
base at merge        d8239558cb10614d302d1d11a4745a9b4b534ae8   (equal)
PR65 head tree       d997e43a0b67c2ef80026bffaf394e16927640ce
squash-merge tree    d997e43a0b67c2ef80026bffaf394e16927640ce   (equal — the landed tree IS the reviewed tree)

CI green on all four shas (PR65 head, merge commit efa0e63, PR66 head, release commit f6aefd2), plus the Publish to npm workflow success. npm time shows 0.1.42: 2026-08-07T12:27:20.350Z, matching the report to the millisecond.

7. Scope held

Diff touches exactly three files: src/db/repos.ts (getRepoByRemote + marker predicates), src/cli/checkout-guard.test.ts, src/db/pull-request-surface.test.ts. No repos scan changes (A3-00137's territory untouched). Merge commits for #65 and #66 each end with exactly one Agent: vespasian trailer, zero Co-Authored-By.

Non-blocking findings

  • P2 — allowAmbiguous semantic change on an exported SDK function. getRepoByRemote(remote, {allowAmbiguous: true}) previously returned rows[0] (possibly a mirror); now returns own[0], or null when all rows are foreign. Zero in-repo production callers (grep confirmed), but the function is exported from src/index.ts and external consumers cannot be enumerated from here. Documented in the docstring and self-review; consistent with the fix's intent. Follow-up only.
  • P3 — misleading message in the all-foreign case. own.length === 0 → null → the CLI prints No indexed repo has remote X, which is literally false when foreign rows ARE indexed. Measured 0 live remotes affected and listReposByRemote still enumerates them, so this is wording, not behaviour.
  • P3 — the pre-existing docs-parity timeout was diagnosed with a proper base-tree control but not filed as a row. The report says "worth its own row; I did not file it" — under signal-to-task-first that row should exist. Leaving to the dispatcher with the retitle.

No blocking findings. The fix is correct, minimal, both-direction-tested, landed with tree fidelity, published, installed, and live-verified.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant