Skip to content

docs(registry): state nonDerivedCheckoutSql's full marker set and population effect - #62

Merged
andrei-hasna merged 1 commit into
mainfrom
03502ac2
Aug 4, 2026
Merged

docs(registry): state nonDerivedCheckoutSql's full marker set and population effect#62
andrei-hasna merged 1 commit into
mainfrom
03502ac2

Conversation

@andrei-hasna

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

Copy link
Copy Markdown
Contributor

Closes the P2 disclosure residual from the adversarial review of #60 (todos 03502ac2). Documentation and one test. No behaviour change.

The gap

nonDerivedCheckoutSql() excludes four markers, built from derivedCheckoutTests:

class markers
path segments worktrees, .worktrees, _factory_src
path prefix /dev/shm/

Its docstring named only _factory_src, and #59 and #60 both described the change as purely the factory-mirror case. An operator debugging "why does this name resolve to nothing" had no pointer to the other three.

The population effect, and why the bare number is the wrong thing to write down

Measured read-only against the station01 registry, importing the real exported predicate rather than transcribing it:

total repo rows            : 1968
distinct names             : 1882
rows with NULL path        : 0
names w/ 0 non-derived rows: 1520
names w/ 1 non-derived row : 311
names w/ >1 non-derived    : 51
sum check (0+1+>1)         : 1882

1,520 of 1,882 reads as 81% of the registry just became unresolvable. Classifying those 1,520 by the marker responsible shows otherwise:

 1475  worktrees segment     e.g. 004ceae9-...-64451e13
                             -> /home/hasna/.hasna/loops/worktrees/open-knowledge/004ceae9-...
   45  _factory_src segment  e.g. accounts
                             -> /home/hasna/workspace/hasna/opensource/_factory_src/accounts

97% are loop/task worktree leaf directory names — task ids and uuids that were never a lookup target. The operator-visible remainder is 45 names, which are exactly the case the exclusion exists to refuse (todos c357a1f3). None is lost:

names shadowed purely by _factory_src                        : 45
of those, reachable via a NON-DERIVED sibling on same remote : 45
with NO non-derived checkout of that remote anywhere         : 0

The exclusion hides no repository — it only refuses to answer with a scratch copy. The docstring states that split rather than the bare 1,520, which overstates the operator-facing effect by ~34x.

Semantics documented alongside the markers

Two properties that are easy to misread from the SQL, both verified against the real predicate in a throwaway in-memory DB (12 cases, 0 failures):

  • Segment-anchored, not substring. …/my-worktrees and …/worktrees-scratch survive; …/worktrees/x does not.
  • The _ in _factory_src is LIKE-escaped. Unescaped it is a single-character wildcard, so …/Xfactory1src/… would be misclassified as derived. Both escape cases pass.
  • A NULL path counts as non-derived (the predicate's own IS NULL arm).

Drift guard

The new test reads both marker constants from their declarations in repos.ts and asserts each appears in the docstring, so adding a marker without documenting it fails. It is guarded against passing vacuously (asserts it parsed ≥4 markers and that the list contains a known one).

Verified red in both directions rather than merely observed green:

# before the docstring
error: expect(received).toContain(expected)
Expected to contain: "worktrees"
 0 pass, 1 fail

# with a fifth marker added to DERIVED_CHECKOUT_SEGMENTS and left undocumented
Expected to contain: "node_modules"
 0 pass, 1 fail

After the docstring: 35 pass, 0 fail in pull-request-surface.test.ts.

Deliberately NOT in this PR

The same review raised two decisions. Both are argued with evidence on todos 03502ac2 rather than fixed here:

  • buildGraph source asymmetry. The review recorded these edges as unreachable through the name-based API. That holds for getDeps (outgoing-only) and is false for queryRelated, whose bidirectional UNION joins repos ON r.id = CAST(e.source_id AS INTEGER) with no derived filter — so a derived checkout is surfaced by name to the caller. Reproduced end-to-end through the real queryRelated() against a temp DB: it returned name=alpha…/_factory_src/alpha, while getDeps returned 0 and a nonexistent name returned 0. That is a genuine defect and the remedy is one predicate on the incoming arm, but it is a runtime behaviour change and does not belong stapled to a docstring. buildGraph itself should stay as-is — its edges are legitimately reachable by raw id and via findPath, and narrowing the source set changes what is stored.
  • graph.ts ambiguity. 51 names have >1 non-derived row (workspace/Workspace case variants, and ~/.hasna/repos/clones siblings). Recommendation is ORDER BY id for determinism, not AmbiguousRepoNameError — those two lookups deliberately fall through to repoId = repoIdOrName, so throwing would change the contract for raw-id callers and start refusing 51 names that answer today.

Test status, stated honestly

Full suite on this branch: 782 pass, 1 fail (rc=1). The single failure is src/cli/docs-parity.test.ts, and it is pre-existing red on main, measured rather than assumed — checked out detached at the base commit a2b28d77 with my docstring confirmed absent, same test: rc=1, same timed out after 30000ms, 4 pass / 1 fail. This PR touches no CLI surface.

One correction for whoever picks that up: neither this worktree nor the canonical checkout had node_modules at all (error: ENOENT while resolving package 'chalk'), which alone accounted for 83 of the 84 failures seen before bun install. But the docs-parity timeout persists after bun install, so missing dependencies are a real second cause and not the explanation for that test.

Refs: todos 03502ac2, c357a1f3, f3c7ecb6.


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

…ulation effect

The predicate excludes four markers -- the `worktrees`, `.worktrees` and
`_factory_src` path segments plus the `/dev/shm/` prefix -- but its docstring
named only `_factory_src`, and PR #59 and PR #60 both described the change as
purely the factory-mirror case. An operator asking "why does this name resolve
to nothing" had no pointer to the other three.

Measured read-only against the station01 registry (1,968 rows / 1,882 distinct
names): 1,520 names have no surviving non-derived row. That headline is
dominated by ephemera -- 1,475 are loop/task worktree leaf directory names that
were never a lookup target. The operator-visible remainder is 45 names whose
only checkout is a factory mirror, which is exactly what the exclusion exists
to refuse, and all 45 remain reachable through a non-derived checkout of the
same remote. Zero names lose their last non-derived row, so the exclusion hides
no repository. The docstring carries that split rather than the bare 1,520,
which overstates the operator-facing effect by roughly 34x.

Adds a drift guard that reads both marker constants from their declarations and
fails if a marker is added without being documented. Verified in both
directions: red before the docstring ("Expected to contain: worktrees") and red
again when a fifth marker is introduced without documenting it.

No behaviour change -- documentation and one test only.

Refs: todos 03502ac2, c357a1f3, f3c7ecb6

Agent: t03502ac2-driver
@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW] GO — #62 @ 58fdc7e — lens: correctness+security+gates, reviewer unresolved-account001 (1 of 1)

Acceptance scope: pr62-derived-checkout-docstring-v1 — the exact two-file delta from freshly fetched origin/main at a2b28d7781a7a363fa52e25e5b79a8cfded41c38 to head 58fdc7ec7452127c1a4f1fa225290f754ff6bcf1: document all four derived-checkout markers and their measured population effect, and add a drift test that reads the marker declarations and the immediately preceding nonDerivedCheckoutSql docstring. Reviewed at 2026-08-04T17:19:35Z.

What I read:

  • git log --oneline origin/main..HEAD — exit 0; one commit, 58fdc7e.
  • git diff origin/main...HEAD --stat — exit 0; 2 files, 62 insertions.
  • Full diff of src/db/repos.ts and src/db/pull-request-surface.test.ts, plus surrounding marker declarations, SQL/TypeScript derived-checkout predicates, getRepo, fuzzyFindRepo, graph queries, release/dependency producer resolution, and the adjacent derived-checkout tests.
  • Current PR metadata at the exact head: base main, open, not draft, mergeable MERGEABLE, merge state CLEAN; GitHub Typecheck, Test & Build concluded SUCCESS.

Commands and gates:

  • bun install — exit 0; setup only, 496 packages installed; not treated as a repository test gate.
  • bun run typecheck — exit 0; pass; no pass/fail count is emitted by tsc --noEmit (0 diagnostics).
  • bun run test — exit 0; 783 pass, 0 fail, 3,486 expectations across 51 files. The new docstring drift test passed.
  • git diff --check origin/main...HEAD — exit 0; advisory whitespace check.

Correctness/security assessment:

  • No production code, configuration, data mutation, authorization, credential, session, or trust-boundary behavior changes. The new test reads the repository-local repos.ts source, parses the two compile-time marker declarations, positively controls that markers were found, and requires each marker in the target docstring.
  • Blocking P0/P1 findings: none.
  • Non-blocking follow-ups: none.
  • Residual coverage note: I did not recreate the dated station01 registry population measurement on station02; this does not affect the executable behavior or the drift guard validated here.

Verdict: GO for this exact head and acceptance scope.

@andrei-hasna
andrei-hasna merged commit 5ad56ee into main Aug 4, 2026
2 checks passed
@andrei-hasna
andrei-hasna deleted the 03502ac2 branch August 4, 2026 17:20
@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW] GO — #62 @ 58fdc7e — lens: correctness+docs-accuracy, reviewer repos-pr62-reviewer (1 of 1)

POST-MERGE. This PR merged at 17:20:38Z (merge commit 5ad56ee417b9) while I was measuring. The commit I reviewed and the commit that landed are the same sha: 58fdc7ec7452. Verified at blob level rather than by trusting the sha — src/db/repos.ts is e7cac0030aad at both merged origin/main and at the reviewed head, against 7f7f5fefbf18 at the base, and the positive control (same command against the base) returns rc=1, so the check can fail. Nothing I measured is invalidated by the merge.

Gate comment (posted before any code or test work, with five pre-registered suspicions): todos 03502ac2 comment 65bed425-04ae-4af5-95cb-f069ddc5be23.

Verdict rationale: nothing here is P0/P1. Had I reviewed pre-merge I would still have said GO. Two P2 follow-ups are filed, neither blocking.


1. Is it actually docs-only? — YES, provably

Not argued from the diff shape. I transpiled both versions and compared emitted code:

base transpiled bytes: 24534
head transpiled bytes: 24534
IDENTICAL EMITTED CODE: true
CONTROL (whitespace-only edit inside the SQL string) identical: false

Source grows 42,266 → 43,988 bytes; emitted code is byte-identical. The near-miss control is one extra space inside the SQL string literal — the smallest edit that could matter — and it is correctly detected, so the instrument can fail. Independently: all 27 added repos.ts lines are JSDoc continuations and none contains */. The docs-only framing is accurate.

2. Is the population claim true? — EVERY NUMBER REPRODUCES EXACTLY

Re-derived independently against a consistent read-only snapshot (sqlite3 -readonly .backup; the live store's mtime was unchanged afterwards), importing the real exported nonDerivedCheckoutSql, not a transcription:

total repo rows            : 1968   [docstring says 1,968]
distinct names             : 1882   [docstring says 1,882]
names w/ 0 non-derived rows: 1520   [docstring says 1,520]
names w/ 1 non-derived row : 311    [PR body says 311]
names w/ >1 non-derived    : 51     [PR body says 51]
sum check (0+1+>1)         : 1882 == distinct names? true
  worktrees      1475
  _factory_src   45
  names counted under BOTH worktrees and _factory_src: 0

The 1475/45 attribution needed a per-marker instrument, which the exported API does not provide, so I wrote my own and validated it against the real predicate on the whole population before using it:

rows compared                              : 1968
rows where MY attribution disagrees with the
  real exported isDerivedCheckoutPath()    : 0 (instrument validated)

And the 45: have a non-derived sibling on the same remote : 45, NO non-derived checkout of that remote anywhere: 0. The author's arithmetic is correct throughout and the 1475/45 boundary holds. My S4 suspicion — that the 1,475 would contain plausible lookup targets — failed, and I am recording that plainly: the residual names are worktree slugs (OPE-00065, 9fix-ingest-honesty), not repo names.

Two parentheticals are narrower than the data (P3, filed, conclusion unaffected): of the 1,475, only 813 (55.1%) are actually "task ids and uuids" — the other 662 are ticket ids and slugs; and only 49.7% sit under …/worktrees/<repo>/, with 46.5% deeper (…/worktrees/station01/<repo>/wt_*, …/loops/worktrees/<repo>/<uuid>).

3. Does the test constrain anything? — YES for the primary case, but it has TWO measured holes

Red-before reproduced exactly (reverted repos.ts to base, kept the new test):

error: expect(received).toContain(expected)
Expected to contain: "worktrees"
 0 pass ... 1 fail        rc=1

Baseline at head: 35 pass, 0 fail. So the guard is real, not decorative.

But the PR body claims "adding a marker without documenting it fails here", and that is false in two ways. Filed as todos c3394366.

Hole 1 — the assertion is a SUBSTRING test. Added "worktree" (singular) to DERIVED_CHECKOUT_SEGMENTS, docstring untouched:

rc=0    1 pass ... 0 fail    8 expect() calls

The 8 against a baseline of 7 proves the guard did evaluate the new marker and was satisfied by worktrees already present in the prose. Undocumented markers that pass today: worktree, src, factory, shm, dev. The author's mutation used node_modules, which is not a substring of anything in the docstring — so the mutation as run could not detect this class, and its pass was read as broader coverage than it has.

Hole 2 — the parse is double-quote-only. Added 'zzscratch' in single quotes:

rc=0    1 pass ... 0 fail    7 expect() calls     <- 7, so the marker was invisible to the parse

Near-miss control, identical marker, only the quote style differs:

rc=1    Expected to contain: "zzscratch"    0 pass ... 1 fail

The guard's own markers.length >= 4 control cannot catch this, because the four real markers still parse. (Reasoned, not measured: the guard hardcodes the two constant names, so a third marker constant would also be invisible.)

4. Was queryRelated correctly scoped out? — YES, and I am not disputing it

It is a runtime behaviour change; stapling it to a docs-only PR would have falsified the PR's own framing, which is the thing this review was asked to protect. It is filed with evidence as todos 805f3127. I checked for a duplicate before filing anything and found it already there.

The residue is a docs consequence, not a reason to have fixed it here: the closing sentence "the exclusion hides no repository, it only refuses to answer with a scratch copy" reads as a registry-wide property, while the guarantee is per-call-site (six sites use the predicate) and queryRelated's incoming arm contradicts it. One clause of scoping fixes that.

5. What the docstring does not say that an operator needs — the most useful finding

Filed as todos 91f1f4a5. The docstring's whole purpose is that "why does this name resolve to nothing" gets a pointer, and it stops one sentence short. Measured over all 45:

sibling answers to the SAME name    : 0
sibling answers to a DIFFERENT name : 45
sibling named exactly open-<name>   : 44
the one exception: iapp-infra -> hasna-xyz-infra

So the answer is nearly deterministic — the live checkout is open-<name> — and the docstring never says it, nor gives a command (repos repo --remote <org>/<name> being the form that refuses rather than guesses). Worse, src/lib/utils.ts already carries "45 of 45 factory scratch clones have a canonical sibling under a different name", and this docstring restates the 45-of-45 fact while dropping the qualifier my measurement shows is universally true.

Also: the hardcoded counts are one box's local index shipped in library source, and the new guard covers only the markers, so nothing fails when they rot. My suspicion that they were already stale was wrong — they reproduce exactly ~40 minutes on. But the rot is bursty rather than absent: rows created per day in the same table run 08-04 +6, 08-03 +1, 08-02 +3, 08-01 +10, 07-31 +71, 07-30 +378, 07-27 +242. One scan burst has moved this table by ~19% of its current total in a day.

Minor (P3): the docstring presents the IS NULL arm as load-bearing — "what lets rows with no path survive the filter" — but repos.path is TEXT NOT NULL UNIQUE, all six call sites are plain FROM repos with no LEFT JOIN, and measured NULL rows = 0.

Base staleness

Resolved from the branch, not the PR object:

origin/main            : a2b28d7781a7a363fa52e25e5b79a8cfded41c38
PR object baseRefOid   : a2b28d7781a7a363fa52e25e5b79a8cfded41c38
refs/pull/62/merge^1   : a2b28d7781a7a363fa52e25e5b79a8cfded41c38

All three agree — the PR's checks described the tree that actually landed. Not stale.

What I did NOT check

  • The full suite. I ran src/db/pull-request-surface.test.ts only (35 pass / 0 fail at head). I did not reproduce the author's 782/1 or independently confirm that src/cli/docs-parity.test.ts is pre-existing red on main — I took that disclosure at face value, and it remains unverified by me.
  • Any machine other than station01. Every population figure here, mine and the author's, is one box.
  • Whether the 45 open-<name> siblings are actually healthy checkouts — I matched on remote_url and name, not on git state.
  • queryRelated's defect itself. I read the author's account and the call sites; I did not reproduce it.
  • The third-constant blind spot in the drift guard is reasoned, not measured.

Worktree ~/.hasna/repos/worktrees/open-repos/pr62-review, restored clean after every mutation (git diff --stat empty). No merge, no push — and the merge that happened was not mine.

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