project_root_for's ancestor walk is still unbounded when the file's directory has no table and no project

[mixelpixx]

Follow-up to the fix in ec705c3 (from @Pytonballoon810's #188), which stops the search when the file's own directory carries a sym-lib-table or fp-lib-table. That covers the reproducible failure and the hermetic-fixture case. It does not bound the walk in general.

Residual

project_root_for (crates/konnect-core/src/tools/library.rs) still climbs ancestors without limit when the schematic's directory carries neither a library table nor a .kicad_pro:

~/Documents/Other.kicad_pro      <- unrelated project
~/Documents/work/foo.kicad_sch   <- no table, no project beside it

The walk leaves work/, finds Other.kicad_pro, and every library lookup for foo.kicad_sch then resolves ${KIPRJMOD} against ~/Documents. Silently — the caller gets the wrong library, not an error.

Narrower than the original bug, since a real project normally has its .kicad_pro beside or above its schematics, but the failure mode is the same and it is invisible when it happens.

Why it wasn't fixed in the hotfix

Bounding the walk is a design decision rather than a one-liner, and it landed as a patch on a release cut hours earlier. Options, none obviously right:

• Stop at a VCS root (.git) — pragmatic, but couples library resolution to source control.
• Depth limit — arbitrary, and wrong for a deeply nested sheet hierarchy.
• Never cross above the user's home directory — helps the common case, does nothing for a shared parent inside it.
• Require an explicit project argument when resolution is ambiguous, and report the ambiguity — the most honest, the most disruptive.

Acceptance

• A schematic with no table and no project beside it, nested under an unrelated .kicad_pro, does not silently resolve against that project.
• Whatever bound is chosen is stated in the doc comment, with the reasoning.
• A deep sheet hierarchy (<proj>/sheets/sub/x.kicad_sch) still resolves at the project root — that is a_sheet_in_a_subdirectory_resolves_against_the_project_table.
• If ambiguity is reported rather than guessed, it is a structured error naming both candidate roots.

[mixelpixx]

For context: the fix this follows up on shipped in v0.3.1 — https://github.com/mixelpixx/Konnect/releases/tag/v0.3.1. The release notes state this residual explicitly as a known limitation rather than implying library resolution is fully bounded.

[anyn99]

Acceptance 1 and acceptance 3 describe the same filesystem shape and expect
opposite answers.

1)  ~/Documents/Other.kicad_pro          3)  <proj>/board.kicad_pro
    ~/Documents/work/foo.kicad_sch           <proj>/sheets/child.kicad_sch
    must NOT resolve                         must resolve

Both are a schematic one directory below a directory holding a .kicad_pro,
with no library table beside it. No bound on the walk separates them, because
the distance is the same in both — which is why the .git root, the depth limit
and the $HOME boundary all fail here rather than only in awkward cases.

What could separate them is membership: does the project's sheet tree actually
contain the file. That bound already exists in the repo — owning_project_root
in sch_export.rs, added by #263 — and it is depth-bounded by
MAX_HIERARCHY_DEPTH, cycle-safe, and tested by
unrelated_neighbour_is_left_alone.

But it cannot be applied to library resolution as the criteria stand, and
the reason is in the fixture acceptance 3 names. In
a_sheet_in_a_subdirectory_resolves_against_the_project_table, the project has
a board.kicad_pro and no board.kicad_sch:

std::fs::write(proj.path().join("board.kicad_pro"), "{}\n").unwrap();
let child = sheets.join("child.kicad_sch");
std::fs::write(&child, "(kicad_sch)\n").unwrap();

child.kicad_sch is referenced by nothing, and there is no root schematic to
walk — project_root_schematic requires <stem>.kicad_sch beside the
.kicad_pro. Membership is undecidable there, so a membership rule rejects it
and the test fails. The contrast with unrelated_neighbour_is_left_alone is
exactly this: that fixture does have a root schematic referencing a child,
which is why the stranger can be told apart from a real sub-sheet.

So the choice looks like this:

• Make membership the rule, and give the acceptance-3 fixture a root
  schematic that references sheets/child.kicad_sch — which is what a real
  project looks like anyway. Both criteria then hold, and the implementation is
  lifting owning_project_root next to project_root_for rather than writing
  anything new.
• Or accept that the two cases are indistinguishable from the filesystem and
  take the fourth option, reporting the ambiguity — which acceptance 4 already
  anticipates, down to naming both candidate roots.

Either is defensible; they are not both reachable while the acceptance-3 fixture
has no sheet tree. Happy to build whichever you prefer.

[neusse]

I retested this against current main through the actual symbol-library resolver and can reproduce the residual deterministically: a loose work/foo.kicad_sch inherits an unrelated ancestor project's sym-lib-table and resolves ${KIPRJMOD} into that project. So this is still live, not only a theoretical edge case.

I agree with @anyn99 that sheet-tree membership is the only sound discriminator. Directory depth, $HOME, or a Git boundary cannot distinguish the two filesystem shapes in the acceptance criteria.

My recommendation is to combine membership with the structured-conflict direction already described in the roadmap:

1. A sym-lib-table/fp-lib-table beside the schematic remains authoritative.
2. An ancestor project is accepted only when its matching root schematic exists and its sheet hierarchy contains the target file.
3. Exactly one verified owner resolves normally.
4. If an ancestor project is present but membership cannot be established, or more than one owner matches, fail closed with the existing structured conflict error and name the schematic directory plus the candidate project roots. Do not guess.

Most of the hard part already exists in sch_export.rs: the membership traversal is depth-bounded by MAX_HIERARCHY_DEPTH, cycle-safe, and tested. This should be extracted into a shared resolver and used by both export and library resolution. The five schematic symbol-loading call sites should resolve ownership before mutation; batch placement should preflight it once so a conflict cannot leave a partial edit.

The current deep-sheet library test should also be corrected to model a real KiCad hierarchy: create board.kicad_sch and have it reference sheets/child.kicad_sch. The present fixture has only board.kicad_pro, so it cannot prove project membership.

This looks like one focused PR rather than a new subsystem. @mixelpixx, would you confirm that this membership-plus-structured-conflict policy is the intended resolution? If so, @anyn99 has already offered to build it.

[mixelpixx]

Confirmed: sheet-tree membership, with a structured conflict when membership cannot be established

@anyn99, @neusse — this is the intended resolution. Building it is unblocked.

I verified every load-bearing claim in this thread against main before confirming, because the policy turns on them:

• owning_project_root (sch_export.rs:1014) exists, is depth-bounded by MAX_HIERARCHY_DEPTH (20), and is cycle-safe through its visited set — a_reference_cycle_terminates covers it.
• unrelated_neighbour_is_left_alone works because its fixture has a real sheet tree: proj.kicad_sch references child.kicad_sch, so stranger.kicad_sch is distinguishable.
• a_sheet_in_a_subdirectory_resolves_against_the_project_table writes board.kicad_pro and no board.kicad_sch, and its sheets/child.kicad_sch is referenced by nothing.

@anyn99's central point is correct and it is a defect in how I wrote this issue. Acceptance 1 and acceptance 3 describe the same filesystem shape and demand opposite answers. No bound on the walk can separate them — which is why all four options I listed fail, not just awkwardly but in principle. I should have caught that when I wrote them. Membership is the only property that distinguishes the two.

The policy

1. A sym-lib-table / fp-lib-table beside the schematic stays authoritative and short-circuits everything below. Unchanged.
2. An ancestor project is accepted only when its root schematic exists and its sheet hierarchy contains the target file.
3. Exactly one verified owner resolves normally.
4. When an ancestor project exists but membership cannot be established — no root schematic, target not reachable, or more than one candidate owner — fail closed with the existing structured conflict, naming the schematic's directory and every candidate project root. Do not guess.

One clarification so this does not over-refuse: the refusal in (4) applies only when there is a candidate ancestor project to be wrong about. A loose schematic with no ancestor project at all has nothing to conflict with — it resolves against whatever tables it can legitimately reach, or none, exactly as it does today. The failure being closed here is "I found a project and cannot prove it owns you", not "I found nothing".

On the acceptance-3 fixture

Correct it, and give it a root schematic that references sheets/child.kicad_sch. This is a fixture defect, not a reason to weaken the rule. A .kicad_pro with no root sheet and an orphan child schematic beneath it is not a shape KiCad produces — and this project's standing rule is that fixtures model real KiCad output, precisely because a hand-made fixture tends to encode the same wrong assumption as the code. Amending it makes acceptance 1 and 3 simultaneously satisfiable, which they are not today.

Acceptance criteria as amended: 1, 2 and 4 stand as written; 3 now reads "a deep sheet hierarchy still resolves at the project root when the root sheet references it."

Scope

Extract the membership traversal out of sch_export.rs into a shared resolver and use it from both export and library resolution — I would rather it lived beside project_root_for in library.rs than be duplicated. KiCadSymbolSource::for_file has 8 call sites across library.rs, sch_batch.rs, sch_components.rs and sch_wiring.rs; resolve ownership before mutation at each.

@neusse's preflight point is right and worth making a requirement: batch placement must resolve ownership once, up front, so a conflict cannot leave a half-applied edit.

Release implication

A case that silently resolved against the wrong project now returns a structured error. A former silent success becoming a refusal is a minor by the rule this project has applied since v0.3.0 — worth knowing when it lands, and the notes must say plainly what now refuses.

Who builds it

@anyn99 offered first and did the analysis that reframed the issue; @neusse did the design review and the reproduction against current main. Either of you — please settle it between yourselves and assign the issue to whoever takes it. We now have a written convention for exactly that (GOVERNANCE.md): claim by assignment, because a claim living only in a comment is invisible to everyone not reading the thread. That invisibility is most of why this issue sat.

[neusse]

Coordination ask first: @anyn99, since you volunteered first, are you comfortable with @dubesinhower finishing the existing implementation in #388? If so, please say so here; then we can assign #189 to @dubesinhower and add the claimed label. If you still intend to implement it yourself, please say that instead so we can assign it to you and avoid two people rebuilding the same fix.

This is the path of least resistance because the design decision is already complete, #388 already contains most of the implementation, and the prerequisite foundation through #381 is now merged. The remaining work is not another redesign: #388 needs to be rebuilt cleanly from current main, retaining only its unique ownership-resolution work and correcting it to match the confirmed policy:

• a table beside the schematic remains authoritative;
• an ancestor project is accepted only when its root schematic proves sheet-tree membership;
• a candidate ancestor whose ownership cannot be proven produces the structured conflict, naming the schematic directory and all candidate roots;
• a loose schematic with no candidate ancestor remains allowed;
• batch mutation resolves ownership once during preflight;
• the deep-sheet fixture must model a real KiCad hierarchy.

A conventional rebase of all nine existing #388 commits would preserve obsolete copies of prerequisite work and create avoidable conflict resolution. Rebuilding the PR from current main with only the corrected unique change is both safer and cheaper to review.

This issue is also the key to the remaining schematic PR sequence. #389, #392, #393, and #394 are stacked on the old #388 history. Once #388 lands cleanly, those PRs can be reconstructed as a simple linear stack containing only their unique commits. Until ownership is settled and #388 is corrected, every later PR carries the same stale base and duplicated prerequisite history.

So the efficient route is:

1. agree on one implementer and assign project_root_for's ancestor walk is still unbounded when the file's directory has no table and no project #189;
2. rebuild and merge corrected fix(project): resolve schematic ownership structurally #388 from current main;
3. restack fix(schematic): preserve all hierarchical placement instances #389 → fix(schematic): parse remaining mutation targets structurally #392 → fix(schematic): preserve connectivity on component delete #393 → fix(schematic): derive component mutations from readback #394 once, in that order;
4. review each PR only for its unique diff rather than repeatedly reviewing copied prerequisites.

That decision here prevents duplicate effort now and substantially reduces rebasing, review work, and regression risk across the whole remaining stack.

[dubesinhower]

@anyn99, @neusse — I can finish the existing implementation in #388 if @anyn99 is comfortable with that handoff. I acknowledge that @anyn99 volunteered first and supplied the membership analysis; I will wait for that confirmation and the agreed assignment/claimed label before rewriting the PR.

I checked the current implementation against the confirmed policy. The review is correct: it currently drops missing/unreadable candidate roots, returns a projectless result for an unreachable target, and uses ambiguous_target for multiple proven owners. Those cases need the existing conflict, with the schematic directory and all candidate roots. A beside-the-schematic library table remains authoritative, and a loose schematic with no ancestor project candidate remains allowed.

After the handoff is agreed, I will rebuild only the unique ownership change on current main, preserve the shared structural traversal and batch preflight, correct the contradictory regressions, verify the real deep-sheet fixture, and run the full contributor gate. The already-merged prerequisite copies will be excluded.

I will keep the dependent branches unchanged until their immediate prerequisites merge, following #388 → #389 → #392 → #393 → #394, then the separate #390 IPC change. No assignment, restack, or policy correction is being claimed as complete by this reply.

[anyn99]

No objections here, please go ahead @dubesinhower !
For the future: I am not here for the creds, just for making this software better! I will always find new ways to improve the code, so no need to wait out of courtesy 😉

[neusse]

Completed by #388 (merge commit bce8f34dca4e4abb66f09d6bc0cd7b9be59eb27b).

Acceptance accounting:

• A schematic beneath an unrelated ancestor project no longer silently resolves against it; unproven ownership returns structured conflict.
• The resolver's documented bound is sheet-tree membership, with cycle detection and MAX_HIERARCHY_DEPTH, rather than an arbitrary filesystem-depth rule.
• Deep child sheets resolve to the project root when a real root schematic references them; the regression uses KiCad's complex-hierarchy fixture.
• Ownership conflicts name the schematic directory and every candidate root in structured paths and the message.
• Adjacent library tables remain authoritative, genuinely loose schematics remain allowed, and batch/symbol-loading paths preflight before writes.

All ten hosted checks passed on the PR head. The complete post-merge local gate passed on the exact merge commit: formatting, Clippy with warnings denied, workspace library/tests, and documentation tests.

The separate existing placement-instance completeness problem remains tracked by #383/#389 and was not part of #189.