engine: bound module pathway depth by graph node count

[bpowers]

Summary

• enumerate_module_pathways previously called find_all_simple_paths(input_port, output_name, 20), silently dropping internal pathways longer than 20 nodes in deeper user modules and producing incomplete composite link scores.
• Compute the node count of the module graph (union of adjacency-list keys and edge targets) once per call and pass it as the DFS depth cap.
• Added a unit test that builds a 22-node input -> n0 -> ... -> n19 -> output chain and verifies the pathway is enumerated end to end. The test fails on the previous code and passes with the fix.

Why N is the correct bound

A simple path can visit each distinct node at most once, so the number of nodes in the graph is the principled upper bound on simple-path length. The visited set inside dfs_simple_paths already enforces simple-path semantics; the cap's only remaining job is recursion-stack safety, which N satisfies exactly.

This branch also includes a cherry-pick of serve: clear inherited GIT_* env in git probe and tests (already on ltm/480-array-reducer-polarity) so the project pre-commit hook can run cargo test without recursive invocation triggered by the simlin-serve git probe tests. Without this prep commit, the pre-commit hook fails on a test isolation issue unrelated to the LTM fix.

Fixes #312

[claude]

Review: no findings

I reviewed the diff for correctness on three axes:

1. node_count computation in ltm.rs — Counting (sources-only) + (has_incoming) over disjoint sets correctly yields the size of the union, matching the detect_output_ports convention. Manual trace of the new test confirms a 22-node simple path reaches the deepest recursion at path.len()=21 with max_depth=22, so the > max_depth guard never trips and the full pathway is enumerated. With visited already enforcing simple-path semantics, N is the principled upper bound.

2. Test fidelity — INTERMEDIATE_NODES=20 plus the bracketing input and output gives 22 distinct nodes; the loop 0..INTERMEDIATE_NODES-1 emits 19 n_i -> n_{i+1} edges, plus the two bracketing edges, for 21 links total. path_to_links yields N-1 links, so INTERMEDIATE_NODES + 1 = 21 is the correct expected length. The test would fail under the prior max_depth=20 cap (DFS returns at path.len()=21 before exploring n19's neighbor), confirming it pinpoints the bug.

3. GIT_* env stripping in simlin-serve — Correctly closes the test-isolation hole where GIT_DIR/GIT_WORK_TREE/GIT_INDEX_FILE propagated from an outer git commit (pre-commit-hook'd cargo test --workspace) into the inner git invocations. Command::env_remove is the right primitive; iterating std::env::vars() is fine here because all relevant GIT_* vars are UTF-8 in practice. find_all_simple_paths is only called from enumerate_module_pathways, so the cap change is fully self-contained — no other callers to audit.

No bugs identified.

Overall correctness: correct

[bpowers]

Adversarial review of the issue #312 fix. Verified locally on ltm/312-module-pathway-depth (HEAD 566c0f2e).

Verification evidence

• cargo build --workspace -> clean.
• cargo clippy -p simlin-engine --no-deps -> clean.
• cargo test -p simlin-engine -> 3156 unit tests + integration tests pass; new ltm::tests::test_enumerate_module_pathways_deeper_than_legacy_cap passes in <0.01s.
• Test reverse-validation: locally restored the legacy find_all_simple_paths(input_port, output_name, 20) literal and re-ran the new test. It panics on expect() with the documented diagnostic, confirming the assertion is meaningful and not a tautology.
• Cherry-pick 323744d7 byte-identical to original e656d0a9 on ltm/480-array-reducer-polarity (verified by diffing git show output for the two commits; only the SHA line differs).

Correctness of the bound (Critical: none)

The new bound is correct and sufficient. Walking through dfs_simple_paths (line 1503): path is initialized to [from], the visited set excludes target, and the recursion check path.len() > max_depth runs at function entry. The deepest reachable path.len() before adding the target is N - 1 (every non-target node), so any max_depth >= N - 1 avoids truncation. The implementer chose N, which is one frame looser than the tightest possible bound but trivially safe and easier to defend in the comment ("a simple path can visit each distinct node at most once"). Not a concern.

The node-count formula |edges.keys() \ has_incoming| + |has_incoming| is mathematically equal to |sources union targets|, the size of the actual node set. Reusing the already-built has_incoming set avoids a second HashSet allocation, so this is slightly leaner than the chain(...).collect::<HashSet<_>>().len() style used in detect_output_ports. The comment claims it "matches the convention in detect_output_ports" -- the result matches but the implementation differs; the wording is slightly imprecise but not misleading.

I confirmed the bound works on the test fixture: 21 sources, 21 targets, sources \ targets = {input}, so node_count = 1 + 21 = 22. With max_depth=22 the deepest dfs entry has path.len()=21 (21 > 22 false), so the chain is enumerated end to end. With the legacy max_depth=20, the dfs entry at path.len()=21 trips 21 > 20 and returns before reaching n19's outgoing edge to output. Confirmed by the locally-reverted-then-rerun test (panics at the expect()).

Test quality (Important: 0, Minor: 1)

• The test exists in mod tests clustered with related pathway tests. Reasonable placement.
• Asserts both pathway count (1) and link count (INTERMEDIATE_NODES + 1 = 21), so a partial-truncation regression that emitted a shortened path would still fail. Strong assertion.
• Runs in well under one second. No flakiness risk (deterministic graph construction; HashMap iteration order doesn't affect the result because the chain is single-source single-sink).
• Minor: the inline comment "A 21-element chain is enough to prove the old cap is gone without blowing past the per-test time budget" is slightly imprecise -- the chain has 21 edges between 22 nodes; "21-link chain" or "22-node chain" would read more cleanly. The later "22 nodes -> 21 directed links along the chain" comment is correct.

Simplicity / maintainability (Important: 0)

The change is ~15 production lines plus comments and a ~55-line test. The node-count computation is local to enumerate_module_pathways (the only caller of find_all_simple_paths that needs the bound), so duplicating with the slightly-different formulation in detect_output_ports is the right call -- extracting a helper for two callers with different surrounding code (one needs has_incoming for input-port detection, the other needs all_nodes and has_outgoing) would not improve clarity.

Scope discipline (Critical: 0)

PR diff touches three files: src/simlin-engine/src/ltm.rs (the fix), src/simlin-serve/src/git.rs and src/simlin-serve/tests/git_integration.rs (the cherry-pick). No tangential changes inside ltm.rs. The cherry-pick is in a different crate and is explicitly disclosed in the PR body. Coordination flag for the user: this branch and ltm/480-array-reducer-polarity both contain the same serve: clear inherited GIT_* commit. Whichever branch merges to main first will leave the other carrying a duplicate commit that must be rebased away. Not the implementer's job to resolve, but worth flagging.

Out-of-scope follow-up (Minor: 1)

The legacy 20 cap may have been doing double duty as both a recursion-depth limit and an implicit path-count limit -- the simple-path count between two nodes can be exponential in a dense graph. With the cap raised to N, pathologically dense user modules could in principle produce many paths. The Johnson's-circuit code in this same file uses an explicit TruncatedByBudget mechanism for exactly this reason; find_all_simple_paths does not. This is not a regression of #312 (the old behavior was wrong about correctness, not safety), but if real-world models surface combinatorial path-count blowup, an explicit budget on find_all_simple_paths would be the principled follow-up. Worth tracking, but does not block this PR.

Hygiene (Critical: 0)

• No emoji, no Co-Authored-By, no Generated with lines in either commit message or the PR body. Both commits use the project's component: lowercase description format.
• PR body explains the bug, the bound, the rationale (Why N is the correct bound), and discloses the cherry-pick clearly.

Praise

• The implementer reverse-validated the test by reasoning from the algorithm rather than hand-waving, and the failure mode is documented inside the expect() message so a future regression points the reader straight at the cap.
• Good explanatory comment on the bound (lines 1550-1557) -- explains the invariant, not the mechanics.
• Comment chose to reuse has_incoming rather than rebuild a fresh HashSet. Tiny, but it's the kind of detail that adds up.
• Disclosing the cherry-pick in the PR body and explaining why (pre-commit hook recursion via simlin-serve git probe tests) instead of bundling it silently. Exactly the right call.

Verdict

Fix is correct, well-tested, minimal, and within scope. No Critical or Important issues. Two Minor observations (test comment wording, follow-up on path-count budget) noted above; neither blocks merge.

[codecov]

Codecov Report

❌ Patch coverage is 90.90909% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.38%. Comparing base (dde79c3) to head (be5d957).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/simlin-serve/src/git.rs	83.33%	1 Missing ⚠️
src/simlin-serve/tests/git_integration.rs	85.71%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #489   +/-   ##
=======================================
  Coverage   82.38%   82.38%           
=======================================
  Files         238      238           
  Lines       61512    61533   +21     
=======================================
+ Hits        50679    50697   +18     
- Misses      10833    10836    +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[bpowers]

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 566c0f2e1b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Use vars_os when clearing GIT_ environment*

run_git now iterates std::env::vars(), which panics if any inherited environment key or value is non-Unicode. On Unix this is possible, so a single non-UTF-8 env var (even unrelated to git) can crash git status probing instead of returning Unavailable. Because this function is on the production path, the sanitization loop should use std::env::vars_os() (and compare keys as OsStr) to avoid panic-driven failures.

Useful? React with 👍 / 👎.

[bpowers]

Codex P2 addressed

Switched the GIT_* env-stripping loops to std::env::vars_os() with a
byte-prefix comparison via OsStr::as_encoded_bytes (Rust >= 1.74).
Fixed in be5d957. Behavior is identical in UTF-8 environments;
the change is panic-robustness only -- a non-UTF-8 env var anywhere
in the parent process can no longer crash production git status
probing.

Note: PRs #491 and #494 carry copies of the previous vars()-based
version of this commit. They will pick up the same fix when one of
the LTM PRs lands and the others rebase. PR #496 already uses an
explicit allowlist via env_remove(name) and is not affected.

[claude]

Review: I reviewed the three commits in this PR (the LTM depth-cap fix in src/simlin-engine/src/ltm.rs, the GIT_* env-stripping in simlin-serve/src/git.rs and tests/git_integration.rs, and the vars_os panic-robustness follow-up) and did not find any bugs to flag. The replacement of the hardcoded depth=20 cap with the module graph node count is sound: the formula self.edges.keys().filter(not in has_incoming).count() + has_incoming.len() correctly computes the size of the union of sources and targets, with no double-counting. For the new test (22 nodes, 21 edges), this yields 22, and since dfs_simple_paths checks path.len() > max_depth before expanding neighbors and the visited set already enforces simple-path semantics, max_depth = N is a safe upper bound. The GIT_* env-stripping in simlin-serve is the right call: it catches every variable that would redirect git away from current_dir(cwd) (GIT_DIR, GIT_WORK_TREE, GIT_INDEX_FILE, GIT_OBJECT_DIRECTORY, GIT_NAMESPACE, etc.), author/committer identity is not broken in tests because init_repo sets local user.name and user.email, and production callers only run read-only git ls-files / git status. The vars_os + as_encoded_bytes() approach correctly avoids panicking on non-UTF-8 env vars. Overall correctness verdict: Correct. The fix is well-targeted, the new test directly demonstrates the regression (it fails on the previous code), and the env-stripping change is a sensible defensive measure that should resolve the pre-commit-hook recursion described in the PR body.