engine: handle array reducer polarity in ltm analyzer

[bpowers]

Summary

• Add explicit polarity cases for SUM, MEAN, array MAX, array MIN (pass-through of arg polarity).
• STDDEV and RANK explicitly return Unknown (non-monotone).
• Previously all array reducers fell through Expr2::App(_, _, _) and contributed Unknown.

Why

Variables computed via SUM(x[]) or MEAN(x[]) now contribute meaningful polarity, so loops crossing them are no longer misclassified Undetermined when the polarity is in fact recoverable.

Tests

• New unit tests in src/simlin-engine/src/ltm.rs exercising each reducer case.
• Existing simulate_ltm tests pass unchanged.

Note on the second commit

The first commit on this branch (serve: clear inherited GIT_* env in git probe and tests) is a prerequisite that fixes pre-existing test isolation bugs in simlin-serve's git tests. Those tests git init a tempdir and run git commit, but inherit the outer GIT_DIR / GIT_INDEX_FILE env vars set by the project's pre-commit hook, causing them to operate on the outer repo and recursively trigger the outer hook. Without this fix the pre-commit hook fails on every commit attempt that touches anything else. The fix is a port of the same change living unmerged on ltm/308-canonical-cycle-dedup, extended here to also cover tests/watcher_git::must_git (which was missed in the original).

Fixes #480

[claude]

[P2] MEAN polarity is order-dependent for variadic args

src/simlin-engine/src/ltm.rs lines 2089–2099: the comment claims the variadic MEAN form combines arg polarities "the same way Add does", but the first match arm (LinkPolarity::Unknown, pol) => combined = pol does not check whether the prior Unknown polarity came from an arg that references from_var. As a result, when an Unknown-polarity, from_var-dependent arg appears before a known-polarity arg, the dependency is silently overwritten. For example, MEAN(ABS(x), x) returns Positive, while MEAN(x, ABS(x)) correctly returns Unknown — same expression, different result depending on argument order. Add at lines 2123–2130 handles this correctly by guarding the first arm with expr_references_var(left, from_var); the MEAN loop needs the equivalent guard (something like: if arg_pol == Unknown and expr_references_var(arg, from_var), return Unknown immediately). The bug is narrow in practice because variadic scalar MEAN(a, b, c) is uncommon in SD models, and the new tests only exercise the single-arg array form, so the case is not covered.

Overall correctness

Incorrect. The variadic MEAN polarity combining logic is order-dependent and can produce a confidently wrong polarity (Positive/Negative) where it should be Unknown. The Sum / Max / Min / Stddev / Rank arms and the unrelated simlin-serve GIT_* env-stripping change look fine.

[claude]

Review

[P2] MEAN variadic form: dependent-Unknown on an early arg is silently dropped

In src/simlin-engine/src/ltm.rs:2084-2102, the per-arg state machine doesn't actually mirror Add's "any disagreement collapses to Unknown" semantics that the comment claims. When the first iteration's arg_pol == Unknown, it always takes the (LinkPolarity::Unknown, pol) arm and leaves combined = Unknown without ever checking whether the offending arg references from_var. A subsequent arg with a known polarity then overwrites combined, so a non-monotone dependency on from_var is silently lost.

Concrete trigger: MEAN(ABS(x), x) (variadic scalar form) with current_polarity = Positive.

1. iter 1, arg = ABS(x) → arg_pol = Unknown; (Unknown, Unknown) matches the first arm, combined = Unknown. The fact that ABS(x) references x is never checked.
2. iter 2, arg = x → arg_pol = Positive; (Unknown, Positive) matches the first arm, combined = Positive.
3. Returns Positive, but the true result is Unknown since ABS(x) is non-monotone in x.

The single-argument array form (MEAN(x[*])) is fine because only one iteration runs; the bug only manifests in the variadic form, which the comment specifically calls out as covered. A simple fix is to short-circuit on any Unknown that references from_var before updating combined (matching what BinaryOp::Add does on each operand independently). The new test only exercises array-form MEAN, so the variadic case is also untested.

Overall correctness

Incorrect — the variadic-MEAN polarity case is buggy as written; the rest of the patch (SUM, MAX/MIN array form, STDDEV/RANK, and the simlin-serve GIT_* env-stripping) looks correct.

[bpowers]

Adversarial review

Polarity fix is on the right track for the headline cases in #480 (SUM, single-arg array MAX/MIN, STDDEV, RANK), but the variadic MEAN combiner has a real correctness asymmetry that I was able to reproduce. Other than that, this is clean: scope discipline good, prep commit is strictly better than #489's version, follow-up #492 is well-scoped, all verifications pass locally.

Verification (run locally)

• cargo test -p simlin-engine --lib -> 3156 passed, 0 failed
• cargo test -p simlin-engine --features file_io --test simulate_ltm -> 48 passed, 0 failed, 1 ignored
• cargo test -p simlin-serve --tests -> all green
• cargo clippy -p simlin-engine --all-targets --no-deps -> clean
• cargo fmt --all -- --check -> clean
• New test test_analyze_expr_polarity_array_reducers verified failing on unmodified production code (checked by reverting only the new match arms): assertion left == right failed: SUM(x) should pass through positive polarity / left: Unknown / right: Positive.

Important

1. MEAN multi-arg combiner is asymmetric with Add (ltm.rs:2084-2103).

The comment promises "we combine arg polarities the same way Add does (any disagreement collapses to Unknown)", but the implementation does not. When the FIRST iterated arg returns Unknown AND references from_var (e.g. ABS(x), SIN(x)), the (Unknown(combined), Unknown(arg_pol)) match takes the first arm and leaves combined = Unknown without ever checking expr_references_var on that arg. A later arg with known polarity then sets combined via the same first arm. Net effect: the prior arg's "Unknown but depends on from_var" is silently dropped.

Reproducer (I added this test temporarily and it fails):

let abs_x = Expr2::App(BuiltinFn::Abs(Box::new(x_var_expr)), None, Loc::default());
let mean = Expr2::App(BuiltinFn::Mean(vec![abs_x, x_var_expr.clone()]), None, Loc::default());
analyze_expr_polarity_with_context(&mean, &x_var, LinkPolarity::Positive, None);
// returns Positive; the equivalent `Add(ABS(x), x)` returns Unknown.

Suggested fix (lifts the references_var check before the match so it covers the first arg too):

for arg in args {
    let arg_pol = analyze_expr_polarity_with_context(arg, from_var, current_polarity, variables);
    if arg_pol == LinkPolarity::Unknown && expr_references_var(arg, from_var) {
        return LinkPolarity::Unknown;
    }
    match (combined, arg_pol) {
        (LinkPolarity::Unknown, pol) => combined = pol,
        (_, LinkPolarity::Unknown) => {} // prev branch already proved no from_var ref
        (a, b) if a == b => {}
        _ => return LinkPolarity::Unknown,
    }
}

For the single-arg MEAN(x[*]) shape that #480 actually points at, this code path is correct (one iteration, returns the arg polarity). But the variadic scalar MEAN(a, b, c) form is also Vec-shaped at the AST level and routes through this same loop, so the bug is reachable in practice.

2. The new test does not cover the multi-arg MEAN path.

Both mean_pos and mean_neg construct BuiltinFn::Mean(vec![single_inner]) -- the loop runs exactly once, so the asymmetry above is not exercised. Adding at least one MEAN(positive_arg, negative_arg) -> Unknown case (and ideally the MEAN(ABS(x), x) case) would have caught this. The TDD claim "each new case was confirmed failing on unmodified code before the fix landed" is true but only for cases the test set actually covers.

Minor

3. Commit message uses "Fixes #480".

Project guidance in CLAUDE.md says explicitly: DO NOT use "fixes"/"resolves" or emoji in commit messages. PR body has the same trailer. Easy fix on the next force-push: drop the trailer (PR body already has the gh-supported "Fixes #480" trailer for auto-close, so the commit message version is redundant anyway).

Praise

• Builtin variant arities match builtins.rs exactly: Sum(Box<Expr>), Mean(Vec<Expr>), Max/Min(Box<Expr>, Option<Box<Expr>>), Stddev(Box<Expr>), Rank(Box<Expr>, Box<Expr>). No tuple-arity bugs.
• Single-arg Max(_, None) / Min(_, None) correctly distinguished from the existing scalar two-arg Max(_, Some(_)) / Min(_, Some(_)) arm at line 2043. The two arms cannot collide.
• Each non-MEAN reducer test covers BOTH positive and negative argument polarity, so a buggy "always Positive" fallthrough would have failed.
• STDDEV and RANK explicitly return Unknown; both arms exercised by tests.
• Test is colocated with other analyze_expr_polarity_* tests, runs in <1ms.
• Issue #492 is real, well-scoped to the GF strict-monotonicity epsilon (NOT the reducer cases, which are correctly fixed here), references #480 and #491, carries the ltm label.
• Scope discipline is excellent: only the polarity fix and the prep commit; no drive-by churn.
• No emoji, no Co-Authored-By, no "Generated with" trailers.

Cross-branch coordination on the GIT_* prep commit

I diffed against the version of this commit on ltm/312-module-pathway-depth (PR #489, sha 323744d7):

• The shared files (src/simlin-serve/src/git.rs, src/simlin-serve/tests/git_integration.rs) are byte-identical: git diff 323744d7 16c59b07 -- src/simlin-serve/src/git.rs src/simlin-serve/tests/git_integration.rs returns empty.
• This branch's version (16c59b0) additionally extends the env-stripping to tests/watcher_git::must_git. That extension is genuinely needed -- tests/watcher_git.rs lines 129-137 do git init, git config user.email/user.name, git add, git commit on a TempDir, which is exactly the path that breaks under the project pre-commit hook.
• The commit message on this branch correctly mentions tests/watcher_git::must_git and pluralizes "workspace test failures". So the message and the diff agree, no yellow flag there.

Concrete recommendation: Land this PR's prep commit first (or extract it to a standalone PR), then rebase PR #489 to drop the duplicate. PR #489's version is a strict subset; landing it first would either force this branch to rebase (clean -- the shared diff is already identical) or to carry the watcher_git extension as a follow-up commit. The former is fine; the latter loses the bisect-friendliness of having one prep commit own the whole fix.

For reference: PRs #490 and #493 do NOT carry this prep commit (contrary to the review brief's expectation). Their diffs only touch engine sources, so they don't cross the simlin-serve/git.rs -> pre-commit-hook landmine. Once #491's prep commit lands on main, #489 rebases cleanly and #490/#493 are unaffected.

Verdict

Comment-only, changes-required (Important #1 should be fixed before merge -- the bug is small but real, the analyzer is supposed to be conservative and this branch slightly weakens that, and the fix is one extra line). Once the MEAN combiner is corrected and the test set is extended to cover the multi-arg path, this is ready.

[bpowers]

Review feedback addressed

Important (variadic MEAN): fixed in 46b3f73. Added the suggested test cases (MEAN(ABS(x), x) -> Unknown, MEAN(x, x) -> Positive, MEAN(x, -x) -> Unknown). Verified the reproducer test fails on the prior commit and passes here.

Minor ("Fixes #480" in commit message): noted. Per CLAUDE.md "create a new commit rather than amending" rule, the prior commit stands; the PR body's "Fixes #480" is the canonical auto-close.

[claude]

[P1] Inner Expr2::Subscript still returns Unknown, so SUM(x[*]) / MEAN(x[*]) polarity isn't actually recovered

File: src/simlin-engine/src/ltm.rs:2081-2118

The new arms for Sum, Mean, and array Max/Min recurse into their argument via analyze_expr_polarity_with_context, but that function has no match arm for Expr2::Subscript and falls through to _ => LinkPolarity::Unknown (line 2280). The production case this PR targets — SUM(x[*]) / MEAN(x[*]) — parses to Sum(Box::new(Subscript(x, [Wildcard], ...))) (see src/simlin-engine/src/parser/tests.rs:118-126 and the Expr1::Subscript lowering at src/simlin-engine/src/ast/expr2.rs:832-894, which preserves the Subscript variant), so the inner recursion immediately returns Unknown and the x → total_pop link is still misclassified Undetermined. The new unit tests pass only because they wrap a plain Expr2::Var(x_var) rather than a Subscript; the existing tests/simulate_ltm.rs::test_cross_dim_sum_algebraic covers SUM(population[*]) but doesn't assert on the resulting polarity, so this gap isn't caught. To deliver the benefit promised in the PR description, analyze_expr_polarity_with_context also needs an Expr2::Subscript(ident, _, _, _) arm that returns current_polarity when ident (or its module-normalized form) matches from_var.

---

Overall correctness verdict: not correct as written — the patch is internally consistent and its tests pass, but the stated user-visible improvement (fixing #480 for SUM(x[*]) / MEAN(x[*])) doesn't materialize because the inner Subscript reference still resolves to Unknown. The simlin-serve git env-stripping commit looks fine.

[codecov]

Codecov Report

❌ Patch coverage is 90.21739% with 36 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.43%. Comparing base (957b20f) to head (78a1233).
⚠️ Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
src/simlin-engine/tests/simulate_ltm.rs	84.09%	14 Missing ⚠️
src/simlin-engine/src/db_analysis.rs	94.01%	10 Missing ⚠️
src/simlin-engine/src/db_ltm.rs	79.59%	10 Missing ⚠️
src/simlin-engine/src/analysis.rs	50.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #491      +/-   ##
==========================================
+ Coverage   82.39%   82.43%   +0.03%     
==========================================
  Files         243      243              
  Lines       61533    61861     +328     
==========================================
+ Hits        50701    50993     +292     
- Misses      10832    10868      +36     

☔ 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

[bpowers]

Latest review feedback addressed

P1 (Subscript not handled): confirmed the bug -- new test exercising
Sum(Subscript(x, [Wildcard])) returned Unknown on the prior commit.
Fixed in 70fa369 by adding an Expr2::Subscript arm mirroring
the Var handler. Added unit tests covering the real parsed shape
(SUM(x[]) Positive/Negative pass-through, no-match Unknown,
MEAN/MAX/MIN(x[]) pass-through, STDDEV/RANK(x[*]) Unknown). Also
extended test_cross_dim_sum_algebraic with a link-polarity assertion
(population -> total_pop must be Positive) and a detected-loop check
(no loop should remain Undetermined); both fail on the prior commit
and pass here.

The two earlier P2 comments about variadic MEAN order-dependence
were already addressed in 46b3f73.

PR is now marked ready for review.

[claude]

Reviewed the changes. Findings:

No bugs found.

Overall correctness: correct.

Notes from the review:

• The new Expr2::Subscript arm correctly mirrors the Var handler — only the array identifier is compared to from_var, and the comment accurately notes that reducer monotonicity makes the indices irrelevant for the reducer paths. Cases where from_var appears only in the indices (e.g. y[x]) still collapse to Unknown via the downstream expr_references_var checks in Add/Sub/Div/Mean, which do walk index expressions.
• The variadic Mean combiner correctly hoists the "Unknown + references from_var" short-circuit ahead of the per-arg combiner. Without that hoist, a first-arg ABS(x) would seed combined = Unknown and then be silently overwritten by a later known-polarity arg, producing a wrong Positive — test_variadic_mean_polarity_with_non_monotone_arg exercises both orderings.
• Removing the wildcard _ => LinkPolarity::Unknown arm makes the match exhaustive on Expr2. This is an improvement: future variants will now produce a compile error instead of silently being treated as Unknown.
• STDDEV/RANK explicit-Unknown arms are necessary (they sit before the Expr2::App(_, _, _) => Unknown fallthrough but are kept for clarity and to document the non-monotonicity reason).
• The GIT_* env-stripping in simlin-serve is a clean fix; tests configure user.name/user.email locally via git config, so stripping inherited GIT_AUTHOR_*/GIT_COMMITTER_* is safe.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 70fa3696a6

ℹ️ 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 to avoid panic when clearing GIT_ env*

The new sanitization loop iterates std::env::vars(), which panics if any inherited environment key or value is non-Unicode; on Unix this is possible and would make git probing fail before git is even spawned. This is a regression from the previous implementation (which did not enumerate env vars) and can break simlin-serve in real shells/launchers that inject non-UTF8 env entries. Use std::env::vars_os() (or Command::env_clear + selective re-add) so the probe remains robust.

Useful? React with 👍 / 👎.

[bpowers]

Rebased onto PR #490 (stacked PR)

This PR is now stacked on top of #490 (ltm/485-polarity-confidence). When #490 merges, GitHub auto-updates this PR's base.

• The polarity analyzer arms for SUM, MEAN, array MAX, array MIN, STDDEV, RANK now live in src/simlin-engine/src/ltm/polarity.rs (post-#498 module split).
• The variadic MEAN fix (hoisted expr_references_var check) is preserved.
• The Expr2::Subscript handler that finally delivered the headline SUM(x[*]) case is preserved with its dedicated tests.
• The catch-all _ => Unknown arm removal is preserved (all Expr2 variants now matched).
• Unit tests split between ltm/polarity.rs's tests sub-mod and ltm/tests.rs per the new layout.
• tests/simulate_ltm.rs::test_cross_dim_sum_algebraic polarity assertions preserved.
• Older GIT_* prep commits dropped; supersession chain is #496 narrowed allowlist > #489 vars_os > older vars().

mergeable: MERGEABLE on top of base ltm/485-polarity-confidence.

[claude]

Review

I reviewed the polarity analyzer changes (src/simlin-engine/src/ltm/polarity.rs) and tests (src/simlin-engine/src/ltm/tests.rs, src/simlin-engine/tests/simulate_ltm.rs).

Findings

No blocking issues found.

A few observations worth noting (not blocking):

• The new Expr2::Subscript arm doesn't recurse into the index expressions. For the dominant production case Subscript(from_var, [Wildcard]), this is correct, and for Subscript(other_var, [expr_referencing_from_var]) the surrounding combiners (Add/Sub/Div/Mul/Max/Min/Mean) correctly use expr_references_var to detect the dependency. There is one narrow edge case the arm could mis-classify: Subscript(from_var, [expr_referencing_from_var]) (e.g. arr[INT(arr[i])]), where the arm returns current_polarity while the index expression makes the relationship genuinely non-monotone. This is an uncommon pattern in SD models and not a regression; flagging only because the arm's docstring claims this case is fully handled.

• The variadic Mean hoist correctly mirrors Add's self-reference handling. The match arm ordering ((_, Unknown) first) means an Unknown that doesn't reference from_var is silently skipped after the hoist guard, which matches the documented behavior.

• The Stddev/Rank arms returning Unknown are independently correct (variance has no fixed sign, rank depends on the rest of the array). Removing the trailing _ => Unknown catch-all is safe: Expr2's 7 variants are all matched explicitly, so future variant additions become a compile error rather than silent miscategorization, as the commit message intends.

• Test coverage is thorough: synthetic shape (Sum(Var(x))), production shape (Sum(Subscript(x, [Wildcard]))), variadic Mean order-independence with ABS(x), and an integration assertion in test_cross_dim_sum_algebraic that exercises compute_link_polarities end-to-end.

Overall correctness verdict

Correct. The patch is a focused, well-tested fix for an understated polarity case. The reducer arms are mathematically sound, the Expr2::Subscript arm is necessary for the patch to actually fire on real models, and the existing simulate_ltm tests continue to pass.