Skip to content

fix(registry): getRepo() no longer resolves a bare name to a _factory_src scratch clone - #59

Merged
andrei-hasna merged 2 commits into
mainfrom
c357a1f3
Aug 4, 2026
Merged

fix(registry): getRepo() no longer resolves a bare name to a _factory_src scratch clone#59
andrei-hasna merged 2 commits into
mainfrom
c357a1f3

Conversation

@andrei-hasna

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

Copy link
Copy Markdown
Contributor

What

Fixes todos c357a1f3: repos repo <name> --json — the exact lookup
non-overridable rule 5 mandates for locating a repository — resolved a bare
name to a stale _factory_src factory scratch clone instead of the canonical
checkout. Reproduced live on 5 of 5 packages tested, including on this very
package (repos repo repos resolves to its own _factory_src mirror on
main right now).

Root cause

getRepo()'s exact by-name branch:

SELECT * FROM repos WHERE name = ? ORDER BY id LIMIT 2

matches the name column with no regard for whether the row is a derived
checkout. A canonical checkout of github.com/hasna/loops is indexed as
open-loops; a shallow, single-commit _factory_src/loops scratch clone of
the same remote is indexed under the bare name loops. Those are
different name values, so getRepo("loops") has exactly one exact
match — the scratch clone — and returns it: deterministic, unambiguous, and
wrong. This is not the tie AmbiguousRepoNameError exists to catch.

The derived-checkout filter (isDerivedCheckoutPath) already existed and was
already wired into getRepoByRemote() for the --remote lookup path, but
never into getRepo()'s by-name branch — so the --remote form was already
safe and the bare-name form (the one non-overridable rule 5 mandates) was not.

Fix

  • getRepo()'s by-name branch now filters isDerivedCheckoutPath rows out of
    its candidate set before deciding: one non-derived match resolves
    normally; more than one is a real AmbiguousRepoNameError; none refuses
    (returns null)
    rather than silently substituting a canonical row under a
    different name.
    • Design call: refuse, don't silently resolve-to-canonical. Substituting
      "open-loops" for a query of "loops" would be fuzzy matching wearing an
      exact-match's clothes — this package's own stated contract
      (getRepoByRemote's docstring) is "match exactly or fail". The existing
      requireRepo() not-found + fuzzy-suggestion path at the CLI layer is
      where a hint toward the canonical name belongs.
    • A useful side effect: a derived row that happens to share an exact name
      with a real checkout no longer throws AmbiguousRepoNameError on a
      conflict that was never real (new test covers this).
  • _factory_src joins DERIVED_CHECKOUT_SEGMENTS. Adding it previously
    crashed the CLI at import — assertLikeSafeMarker rejected any marker
    containing _, because unescaped SQL LIKE treats it as a single-character
    wildcard. Every LIKE pattern built from a marker is now run through a new
    escapeLikeMarker() with a matching ESCAPE '\' clause — the identical
    convention this file's own PR_RANK_ORDER already uses for owner_remote
    so assertLikeSafeMarker now accepts underscore-bearing markers
    (_factory_src, node_modules) instead of rejecting them.
  • fuzzyFindRepo (lib/utils.ts) now excludes derived checkouts from its own
    "did you mean" queries too, via a new exported nonDerivedCheckoutSql().
    Without this, getRepo() correctly refusing to resolve a bare name to the
    scratch clone was immediately undone one layer up: the CLI's own
    not-found fallback would suggest that same clone right back.

Where the fix lives, and why not elsewhere

Applied at isDerivedCheckoutPath/DERIVED_CHECKOUT_SEGMENTS — the one
predicate getRepoByRemote() already used — rather than duplicating logic in
getRepo(). fuzzyFindRepo reuses the same SQL-level definition via
nonDerivedCheckoutSql() so the three call sites (getRepoByRemote,
getRepo, fuzzyFindRepo) cannot drift into different definitions of
"derived".

Tests

10 new/updated tests across src/db/repos.test.ts,
src/db/pull-request-surface.test.ts, src/lib/utils.test.ts:

  • Confirmed failing on unfixed source first (git stash of the two
    source files, tests still applied) — each one reproduces the exact reported
    defect, e.g. getRepo("loops") returning the _factory_src row instead of
    null.
  • All pass after the fix. Also added a drift-guard test proving
    assertLikeSafeMarker and the SQL rank term agree on _factory_src, and
    that an unescaped-LIKE false positive (_ matching any character) does not
    reappear.
  • bun run typecheck clean. Full bun test green except one pre-existing,
    unrelated
    failure: src/cli/docs-parity.test.ts's CLI-crawl test times
    out at its own 30s budget under load on this box (13-30 load average during
    the run). Verified with a negative control — reproduces identically on
    unfixed main
    , same line, same ~30s mark. Not touched by this PR.

What I did NOT check

  • The auto-bootstrap re-creating a deleted _factory_src row (raised
    elsewhere on the tracking task) — out of scope for a lookup-correctness fix;
    this change makes a re-created mirror row harmless for name resolution
    regardless of when it reappears.
  • conversations, instructions, todos — the other 3 of the "5 of 5
    packages tested" in the original diagnosis. This fix is at the shared
    getRepo()/isDerivedCheckoutPath layer, so it should cover them
    identically, but I did not re-verify against their live indexed rows.
  • The adjacent, distinct default_branch field bug (open-todos,
    open-identities resolving to stale feature branches) — different failure
    shape, called out on the tracking task as a separate concern.

Fixes todos c357a1f3.


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

…_src scratch clone

`repos repo <name>` (and every other caller of getRepo()'s by-name branch,
including `repos worktree add`) matched the `name` column with no regard for
whether the match was a derived checkout. A canonical checkout of
`github.com/hasna/loops` is indexed as `open-loops`; a shallow, single-commit
`_factory_src/loops` factory scratch clone of the SAME remote is indexed under
the bare name `loops`. Those are different `name` values, so `getRepo("loops")`
had exactly one exact match — the scratch clone — and returned it:
deterministic, unambiguous, and wrong. Reproduced live on 5 of 5 packages
tested (todos c357a1f3), including on this very package (`repos repo repos`
resolved to its own _factory_src mirror).

getRepo()'s by-name branch now filters isDerivedCheckoutPath rows out of its
candidate set before deciding: one non-derived match resolves normally; more
than one is a real AmbiguousRepoNameError; none refuses (returns null) rather
than silently substituting a canonical row under a DIFFERENT name, which would
be fuzzy matching wearing an exact-match's clothes — this package's own stated
contract (getRepoByRemote's docstring) is "match exactly or fail". A derived
row that happens to share an exact name with a real checkout no longer throws
AmbiguousRepoNameError on a conflict that was never real.

_factory_src joins DERIVED_CHECKOUT_SEGMENTS. Adding it previously crashed the
CLI at import (assertLikeSafeMarker rejects `_` because unescaped LIKE treats
it as a wildcard), so every LIKE pattern built from a marker is now escaped via
escapeLikeMarker() with a matching `ESCAPE '\'` clause — the same convention
PR_RANK_ORDER already uses for owner_remote — and assertLikeSafeMarker accepts
underscores.

fuzzyFindRepo's "did you mean" suggestions exclude derived checkouts too, via
the new nonDerivedCheckoutSql() export: without it, getRepo() refusing to
resolve a bare name to the scratch clone was immediately undone by the CLI's
own not-found fallback suggesting that same clone back.

10 new/updated tests across repos.test.ts, pull-request-surface.test.ts and
utils.test.ts; confirmed failing on unfixed source (git stash) before the fix
and passing after. Full suite green except a pre-existing, load-sensitive
docs-parity CLI-crawl timeout that reproduces identically on unfixed source.

Agent: tc357a1f3-fixer
@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW] NO_GO — #59 @ 54b0c03 — lens: correctness+security+gates, reviewer unresolved-account001 (1 of 1)

Reviewed the complete origin/main...HEAD diff for all six changed files (CHANGELOG.md, src/db/pull-request-surface.test.ts, src/db/repos.test.ts, src/db/repos.ts, src/lib/utils.test.ts, and src/lib/utils.ts) and surrounding resolution/schema/caller paths, including requireRepo, resolveTargetRepo, getRepoByRemote, worktree resolution, scanner derived-checkout handling, the repository schema, and pull-request ranking.

Commands and measured results (unmodified candidate):

  • git log --oneline origin/main..HEAD — exit 0; one commit, 54b0c03.
  • git diff origin/main...HEAD --stat — exit 0; 6 files, 310 insertions, 26 deletions.
  • bun install — exit 0; setup only, 496 packages installed.
  • bun run typecheck — exit 0; pass (the typecheck surface reports no pass/fail item count).
  • bun run test — exit 0; 771 pass, 0 fail, 3,464 expectations across 50 files.
  • Focused runtime reproduction against the changed getRepo() — exit 0; one derived row followed by two real same-named rows returned /w/real-a instead of throwing AmbiguousRepoNameError; two derived rows followed by one real exact match returned null.

Blocking P0/P1 findings:

  1. P1 correctness / unsafe repo targeting — src/db/repos.ts:155-162. The query still applies ORDER BY id LIMIT 2 before isDerivedCheckoutPath() removes derived candidates. Repository names are not unique (idx_repos_name is a non-unique index), and duplicate real names are an explicitly supported fail-closed case. Therefore an older _factory_src/worktree row can consume the two-row cap: with one derived plus two real exact-name rows, the function sees only one real row and silently routes to it instead of raising ambiguity; with two derived plus one real row, it reports not found. This is reachable through repos repo <name>, requireRepo, worktree creation, GitHub operations, MCP/server lookups, and other callers that act on the returned path. Minimal remedy: exclude derived rows in the SQL candidate query before LIMIT 2 (using the shared nonDerivedCheckoutSql("path") predicate), and add regressions for both three-row orderings.

Non-blocking follow-ups: none.

Agent: unresolved-account001
@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW] GO — #59 @ 45167a0 — lens: correctness+security+gates, reviewer unresolved-account001 (1 of 1)

Focused re-review of the one blocking defect named at 54b0c030ca293c5bfd8c21408aed028b47c707cd, its fix, and direct regressions only.

Fixed in 45167a0fd389b2372cc19e58a7f7185d14144776:

  • getRepo() now applies the shared non-derived-checkout SQL predicate before ORDER BY id LIMIT 2, so derived rows cannot hide real exact-name candidates.
  • Added regressions for one derived row preceding two real matches (must throw AmbiguousRepoNameError) and two derived rows preceding one real match (must return the real checkout).

Measured verification on the repaired head:

  • Focused runtime reproduction — exit 0; one-derived/two-real now raises AmbiguousRepoNameError; two-derived/one-real now resolves /w/real.
  • bun run typecheck — exit 0; pass (no item count reported by the typecheck surface).
  • bun run test — exit 0; 773 pass, 0 fail, 3,466 expectations across 50 files.
  • shield review on the staged two-file fix — exit 0; no secret findings. It emitted nine generic SQL-template heuristic alerts: the only new location (src/db/repos.ts:155) interpolates only the literal column name path and compile-time marker arrays, so no attacker-controlled source reaches the query; the other eight locations are unchanged. Pre-push staged review also exited 0 with no staged changes.
  • git push origin HEAD:c357a1f3 — exit 0; PR head advanced from 54b0c03 to 45167a0.

Blocking P0/P1 findings: none remain.
Non-blocking follow-ups: none.

@andrei-hasna
andrei-hasna merged commit 4b7eb36 into main Aug 4, 2026
2 checks passed
@andrei-hasna
andrei-hasna deleted the c357a1f3 branch August 4, 2026 13:48
@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW] GO — #59 @ 45167a0 — lens: name-resolution correctness and the ambiguity-semantics change, reviewer repos-pr59-reviewer (1 of 1)

POST-MERGE VERIFICATION. This PR merged (4b7eb36, 2026-08-04T13:48:04Z) before this independent review completed — the only verdict at merge time was self-authored (same GitHub account wrote the code, found its own follow-up defect, fixed it, and approved the fix). This comment is the independent read that never happened pre-merge. Findings below are about the code that landed on main, not a merge recommendation.

Verified independently (own worktree, own test runs, not taken on the author's word)

  • All 10 original regression tests fail on unfixed source (origin/main), pass on 54b0c03. Reverted src/db/repos.ts + src/lib/utils.ts to origin/main, kept the new tests, ran them: 10/10 fail with the exact predicted symptom (mirror row returned instead of null/canonical). Restored the fix: 10/10 pass.
  • docs-parity.test.ts's 30s timeout is pre-existing and load-independent of this PR. Reproduced identically on unfixed origin/main (same line, ~30017ms) under today's station load (~15-17). Not a regression.
  • typecheck clean (tsc --noEmit, rc=0) at both 54b0c03 and 45167a0.
  • The ambiguity-reordering delta (54b0c03..45167a0) is correct, and I reproduced the defect it fixes myself before trusting the claim. Took the two new tests, ran them against 54b0c03's repos.ts: both fail —
    • "derived row precedes two real exact-name matches" → silently resolved to one of the two real rows instead of throwing AmbiguousRepoNameError (a derived row consumed a slot in the ORDER BY id LIMIT 2 fetch window, hiding the second real candidate from ever reaching the JS-side filter).
    • "two derived rows precede a real one" → returned null (wrongly "not found") because both LIMIT-2 slots were consumed by derived rows, hiding the sole real checkout entirely.
      Restored 45167a0's repos.ts: both pass, 75/75 across the three touched test files, 0 regressions.
      The fix is structurally right: it moves nonDerivedCheckoutSql() into the SQL WHERE clause itself, so ORDER BY id LIMIT 2 now windows over an already-non-derived set — restoring the property the original binary >1 check always relied on (LIMIT 2 is only safe for a "more than one?" question when it doesn't need to also classify which rows survive a later filter).

The three design questions from the original brief (54b0c03, still valid for the parts this delta didn't touch)

  1. Refuse over resolve-to-canonical: correct, and verified end-to-end, not just at the unit level. Traced repos repo <name> in src/cli/index.tsx: resolveTargetRepo()requireRepo()getRepo(); a null return correctly reaches fuzzyFindRepo() for a "did you mean" hint (itself now also derived-aware) before exiting 1. No silent substitution reaches the CLI's stdout.
  2. Not all doors are closed — a real fourth (and fifth/sixth) caller exists, filed as a non-blocking follow-up (todos f3c7ecb6). src/lib/ops-producers.ts:1401 resolveRepoPath() runs its own unfiltered WHERE name = ? OR path = ? OR (org||'/'||name) = ? and backs three live CLI commands (repos ops release-candidates|docs-rules-drift|dependency-refresh --repo <path-or-name>, all documented to accept a registry name). src/lib/graph.ts has three more (MCP-only). None of these were touched by this PR and none share the fixed predicate. Pre-existing, out of scope for this diff, but worth naming precisely since the PR's own "Where the fix lives" section could otherwise read as more complete than it is.
  3. The ambiguity-check reordering is the one real defect found in this review, and it was fixed in the delta before I finished judging it. Filtering derived rows out before the AmbiguousRepoNameError check is fine on its own; the LIMIT-2-before-JS-filter interaction was the actual hazard, and it's closed now (see verification above). Separately, both getRepo() and getRepoByRemote() share isDerivedCheckoutPath's marker-matching, which classifies by path segment ((^|/)worktrees/) rather than by registry provenance — a real (non-derived) checkout whose path happens to contain a worktrees/_factory_src segment outside the sanctioned worktree tree would still be misclassified and could silently resolve a genuine two-real-checkout ambiguity. This existed before this PR (already used for ranking in getRepoByRemote); this PR raises its consequence from "ranked lower" to "excluded from candidacy" in one more place. Untested, narrow, and requires a fleet-convention violation to trigger — not blocking, worth a follow-up test.

What I did NOT check

Did not re-verify the full untouched test suite (49 files) end-to-end — station load (15-17 loadavg from other agents' concurrent runs) made a full bun test run infeasible within a reasonable bound; scoped verification to the touched files plus typecheck instead. Did not check whether the auto-bootstrap re-creation of deleted _factory_src rows (raised elsewhere on todos c357a1f3) interacts with anything here — out of scope, same as the original author's own disclosure.

andrei-hasna added a commit that referenced this pull request Aug 4, 2026
chore(release): @hasna/repos v0.1.40 (#61)

Version bump and changelog only; no source change.

0.1.39 predates both bare-name resolver fixes (#59, #60) -- tag v0.1.39
carries no derived-checkout filter at all, and the installed 0.1.39 bundle
contains _factory_src in 0 of 5 dist files. 0.1.40 is therefore the first
release in which `repos repo <bare-name>` stops resolving to a stale
_factory_src scratch clone.

All four #60 sites verified behaviourally against a WAL-consistent snapshot
of the live registry: 6 failures on v0.1.39, 0 on main, with over-breadth
controls still resolving in both arms.

Agent: Silvanus
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