kitconfig.repo_root()'s fallback is depth-specific — it breaks the scripts/devkit/lib/ layout its own docstring promises

[topij]

Found by CodeRabbit on the cs-toolkit adoption (in-parallel-oy/cs-toolkit#1791), and
confirmed by execution.

The claim

scripts/lib/kitconfig.py:53

def repo_root(start: Path | None = None) -> Path:
    """Nearest ancestor carrying a ``.git`` entry ... Walking up for the marker —
    rather than counting ``parents[N]`` — is what lets the kit be vendored at any
    depth (``scripts/devkit/lib/``) without rewriting a single path."""
    here = (start or Path(__file__)).resolve()
    for candidate in (here, *here.parents):
        if (candidate / ".git").exists():
            return candidate
    return here.parents[2] if len(here.parents) >= 3 else here.parent

The docstring names scripts/devkit/lib/ as the layout this supports. The
fallback then does exactly what the docstring says it avoids — counts
parents[N] — and [2] is calibrated for the kit's own scripts/lib/ layout:

layout	parents[2]	correct?
scripts/lib/kitconfig.py (kit's own)	<repo>	✓
scripts/devkit/lib/kitconfig.py (vendored, and the one the docstring names)	<repo>/scripts	✗

Reproduction

$ mkdir -p /tmp/v1/repo/scripts/devkit/lib          # deliberately NO .git
$ cp scripts/lib/kitconfig.py /tmp/v1/repo/scripts/devkit/lib/
$ cd /tmp/v1/repo/scripts/devkit/lib && python3 -c "
import kitconfig, pathlib
print('returned :', kitconfig.repo_root())
print('expected :', pathlib.Path('/tmp/v1/repo').resolve())"

returned : /tmp/v1/repo/scripts
expected : /tmp/v1/repo

load_config() then looks for <repo>/scripts/config/dev-model.yaml and raises
FileNotFoundError with a hint pointing at the wrong path.

Scope

Latent, not live, for a normal checkout — the .git walk succeeds first, so the
fallback only fires where there is no .git marker at all: exported tarballs,
GIT_DIR-only setups, some git worktree edge cases. But it is precisely the
vendored-at-depth case the kit now actively recommends: /adopt Step 1 says to
vendor under scripts/devkit/ when the adopter's scripts/ has colliding names,
and cs-toolkit is the first real repo to take that branch.

Suggested fix

Probe for a second marker before falling back to arithmetic:

     for candidate in (here, *here.parents):
         if (candidate / ".git").exists():
             return candidate
-    return here.parents[2] if len(here.parents) >= 3 else here.parent
+    for candidate in (here, *here.parents):
+        if (candidate / DEFAULT_CONFIG_PATH).is_file():
+            return candidate
+    return here.parents[2] if len(here.parents) >= 3 else here.parent

This restores the "vendored at any depth" property for the fallback path too, and
removes the last parents[N] count from the function.

Worth a test that runs repo_root() from a scripts/devkit/lib/ tree with no
.git — there is currently no coverage of the fallback branch at any depth, which
is why a docstring and its own code could disagree this plainly.

Found on: in-parallel-oy/cs-toolkit#1791

[topij]

Attempted twice, regressed twice, reverted — analysis so the next attempt doesn't repeat it

Tried in PR #63 and removed there. This issue stays open. The naive fix is worse
than the bug, and the reason is structural rather than a coding slip, so it's worth
writing down.

What was tried

Attempt 1 — probe upward for config/dev-model.yaml when the .git walk fails.
Unbounded, it walks to /. On a .git-less tree nested under a directory owning a
foreign config it returns that project's root:

BRANCH repo_root() -> .../outer                      ← foreign project
MAIN   repo_root() -> .../outer/releases/proj-1.2.3  ← correct

main's depth arithmetic gets that case right. So attempt 1 was a strict
regression, and in the worst direction: archive_plan_sessions rewrites the
paths it resolves, so a wrong-but-loud FileNotFoundError became a silent write
against someone else's repo.

Attempt 2 — bound the probe to the depth of the deepest prescribed layout. Still
escapes. The bound was calibrated for the deepest layout and applied
unconditionally, so in the shallowest layout it reaches exactly one directory
above the root:

foreign config as the IMMEDIATE parent, no padding:
  HEAD  repo_root() -> .../outer         ESCAPED
  main  repo_root() -> .../outer/inner   correct

Both escape tests written for attempt 2 nested the tree under a spare releases/
level, which pushed the foreign config exactly one index past the bound — so they
passed while the real case escaped. The padding was the bug in the test, and it
is why attempt 2 shipped believing the case was covered.

Why a depth bound cannot work

The root sits at a different index per layout:

layout	root
<root>/scripts/lib/kitconfig.py (the kit's own)	parents[2]
<root>/scripts/devkit/lib/kitconfig.py (/adopt's)	parents[3]

No single bound distinguishes "the root" from "one above the root" without already
knowing the layout it is trying to discover. Any fixed bound delivers "escapes less
far", never "cannot escape".

The shape that could work

Validate a candidate against its own paths.engines: when a candidate carrying
config/dev-model.yaml is found, read paths.engines from it and accept the
candidate only if the engine file actually lives under <candidate>/<paths.engines>/.
A foreign config fails that check and the walk continues.

• kit's own: engines scripts, file under <root>/scripts/ → accept
• vendored: engines scripts/devkit, file under <root>/scripts/devkit/ → accept
• foreign parent: its paths.engines does not contain our file → reject, keep walking

This needs no bound and is layout-independent. Two caveats for whoever builds it:
pr_watch resolves its root at import time and is stdlib-only, so it cannot lean on
kitconfig; and repo_root must not recurse through load_config.

Scope note — it's three resolvers, not one

Any fix must cover all three or the issue is only a third closed:

• scripts/lib/kitconfig.py::repo_root
• scripts/lib/devmodel_config.py::_repo_root
• scripts/pr_watch.py::_find_repo_root — and this one's docstring still makes the
  depth-independence claim this issue is about. REPO_ROOT here feeds every
  gh/git subprocess cwd= and the state root, so an escape points a merge-gate
  engine at a different repository.

Meanwhile

PR #63 documents the limitation in all three, and the suite now asserts it explicitly
(test_repo_root_without_git_is_depth_arithmetic_and_stays_inside_the_tree) plus the
property that must hold regardless: the answer stays inside the tree. Failing
loudly in the right repo beats resolving quietly into the wrong one.

Worth weighing before building this: the fallback only fires when there is no
.git anywhere up the chain. Every real checkout — including the adopter that
prompted this issue — takes the unbounded .git walk and is unaffected.

[topij]

Reopened — auto-closed in error by PR #63, which explicitly does not fix this.

The cause is worth recording because it is a trap, not a slip. The PR body was edited before merge to retract the closure claim, and the retraction read:

Closes #58. **Does NOT close #60** — see below

GitHub's linked-issue parser matches close #60 and does not care about the Does NOT in front of it. Negating a closing keyword in prose still arms it. The safe form is to avoid the keyword adjacent to the number entirely — e.g. "this PR does not fix issue 60" or "#60 stays open".

Nothing about the analysis changes: the fix was attempted twice, regressed twice, and was removed. See the previous comment for both attempts, why a depth bound cannot work, and the paths.engines-validation shape that could.

[topij]

Reopened again. Auto-closed a second time by the merge of PR #64 — a docs-only wrap-up that fixes nothing here.

The cause is a recursion worth recording, because it is now the second occurrence and the second one came from the sentence describing the first:

• Occurrence 1 — PR fix: lint the kit's own Python; #60 attempted, regressed, and reverted (#58) #63's body was edited to retract its closure claim. The retraction read Does NOT clo-se #60 (hyphen inserted here deliberately). GitHub matched the keyword and ignored the negation.
• Occurrence 2 — PR chore: update handoff — cs-toolkit Phase 1 adopted; kit lint shipped; #60 reverted #64's body listed that lesson as a learned item, quoting the phrase verbatim. Quoting the trigger is the trigger. Merged 17:57:14, auto-closed 17:57:15.

So the rule is stronger than "do not negate a closing keyword":

In a PR body or commit message, never place a closing-style keyword within a sentence of an issue number you do not intend to close — including when quoting, negating, or explaining the hazard itself. Write issue 60 without the hash, or restructure the sentence so the two are not adjacent.

File contents are unaffected — GitHub only parses PR bodies and commit messages — so the friction-log entry recording this can safely keep the literal phrasing.

Nothing about the technical analysis changes: the fix was attempted twice, regressed twice, and was removed. See the earlier comment for both attempts, why a depth bound cannot work, and the paths.engines-validation shape that could.