Test suite audit: tests verify output/config text, not git state (fallback, destructive ops, security under-covered)

[bashandbone]

Summary

An audit of the test suite found that while it is large (~6,550 lines in tests/ plus ~398 inline unit tests) and looks thorough, it has a consistent structural weakness: most integration tests assert on printed output or on the submod.toml text, not on the actual git/filesystem state the tool exists to manipulate. This lets a large fraction of tests pass even when core behavior is broken — and has already let at least one real bug ship undetected (fetchRecurse silently dropped, see P0-3).

This issue tracks the gaps and the recommended fixes. Findings have been spot-verified against the source.

---

Cross-cutting theme: tests verify narration, not effects

submod's job is to mutate git state (gitlinks, .git/modules, .gitmodules, .git/config, worktrees), but the tests largely check what was printed or what landed in submod.toml:

• add (tests/integration_tests.rs:52) checks submod.toml + that a .git exists. Never verifies the index gitlink (mode 160000), the .gitmodules entry, the submodule.<name> config section, or that the submodule HEAD is the right commit. The shallow-clone test (tests/integration_tests.rs:887) is the only add test that shells out to git rev-parse.
• delete / nuke assert only !config.contains("[name]") (tests/integration_tests.rs:648, :854; tests/command_contract_tests.rs:244, :1331). Never check the worktree is gone, .git/modules/<name|path> is removed, the index gitlink is cleared, or the .gitmodules entry is gone. The delete code itself notes path-keyed artifacts "may linger and prevent a clean re-add" (src/git_manager.rs:1605).
• update / sync are smoke tests (tests/integration_tests.rs:169, :295): they update against a remote that never advanced (guaranteed no-op), asserted via stdout.contains("Updated") || contains("Already up to date").
• check/status asserts only printed substrings; is_dirty is a stub that always reports clean when HEAD resolves (src/git_manager.rs:380), and no test can catch that.

---

P0 — Highest priority

P0-1: Fallback architecture (the core design) is largely unverified

• No failure-injection seam: gix_ops is a private field with no way to disable gix or force git2/CLI. The only reachable fallbacks are 8 gix methods hard-coded to Err (src/git_ops/gix_ops.rs:317,677,701,707,713,719,725,731) and an incidental 2-part-config-key quirk. For every op gix actually implements (read/write_gitmodules, init/update/delete/deinit, list, fetch), git2's fallback cannot be exercised.
• The CLI last-resort path is dead code from the suite's perspective: the add_submodule CLI fallback + partial-state cleanup (src/git_ops/mod.rs:316-429) only runs if git2.add_submodule fails, and nothing makes it fail. apply_sparse_checkout_reaches_cli_fallback (tests/fallback_tests.rs:258) asserts nothing on the success path.
• Of ~23 "fallback" tests, only 3 actually force a fallback and assert a correct result; several are mislabeled (e.g. manager_write_gitmodules_succeeds_despite_gix_limitations exercises a path gix handles fine).
• reopen() is never tested for its actual hazard (delete-then-re-add same name in one process); its fatal/non-fatal asymmetry (git2 fatal, gix warning) is untested. Fallback warning logs are never asserted.

Fix: add a failure-injection seam (e.g. GitOpsManager::new taking gix: Option<...>, or a feature-gated failable gix) so git2 fallback and CLI last-resort can be tested for correct results.

P0-2: Destructive ops verified only against TOML, never git cleanup

delete_submodule_by_name (src/git_manager.rs:1581) removes config sections, deletes .git/modules/<path> and .git/modules/<name>, and calls reopen() — none of which any test checks. Note git2's delete_submodule deliberately does NOT touch .gitmodules (src/git_ops/git2_ops.rs:431, "left to higher-level logic"), so a stale .gitmodules entry after delete is a plausible undetectable bug.

Fix: after delete/nuke --kill, assert: worktree gone, .git/modules/<name and path> gone, git ls-files --stage <path> empty, .gitmodules entry gone, git config --get-regexp ^submodule\. empty.

P0-3: Real bug — fetchRecurse / [defaults] branch silently dropped (masked by tests)

The serde field is fetch_recurse (snake_case) with no rename/alias; the struct has no rename_all and no deny_unknown_fields (src/config.rs:71). So fetchRecurse in submod.toml is an unknown key, silently dropped — parsed value is always None. test_config_with_all_git_options "passes" only because it asserts the file text still contains fetchRecurse = "always" (tests/config_tests.rs:164), trivially true since save_config is append-only/verbatim. [defaults] branch is likewise dropped (SubmoduleDefaults has no branch field). Related: SerializableFetchRecurse serializes as always/never via serde but true/false via gitmodules, and from_gitmodules("always") returns Err — no bridge test.

Fix: add serde aliases (fetchRecurse) + deny_unknown_fields; add tests asserting parsed values, not file text; add a cross-system (serde↔gitmodules) round-trip test for FetchRecurse.

---

P1

• update against an advanced remote is untested. Add a test that moves the remote forward, runs update, and asserts the submodule HEAD followed. The git2 update path including the rebase/merge→checkout downgrade (src/git_ops/git2_ops.rs:397) is unverified.
• add does not verify real git state (gitlink mode 160000, .gitmodules, submodule.<name> config, HEAD == remote).
• check/status stub: is_dirty (src/git_manager.rs:380) always reports clean; no test asserts a dirty submodule is reported dirty. get_submodule_status OIDs/flags are never asserted (tests/git_ops_tests.rs:315).
• reopen() / stale-state: add add → delete → add same name+path in one process test.
• CLI-over-file precedence in Config::load (src/config.rs:1048) is untested; the git2 bridge (to_git2_options / TryFrom ... Git2SubmoduleOptions) and SubmoduleUpdateOptions::from_options recursive-flag logic are untested.
• save_config is append-only (src/git_manager.rs:210): edits to existing sections are silently not written back; comment/order "preservation" is only proven against a no-op. Add real load→modify→save→reload tests.

---

P2

• Security is effectively untested. tests/security_tests.rs has 2 tests, both the same vector (path component starting with -). Null-byte handling is faked by the harness — run_submod (tests/common/mod.rs:223) intercepts \0 args and returns a fabricated failure without launching the binary. Missing: command/flag injection via submodule URL or name (CVE-2018-17456 class), malicious .gitmodules fed to generate-config, symlink escapes, asserted path-traversal containment.
• Tests that cannot fail meaningfully — delete or fix:
  • test_invalid_config_file_path (tests/error_handling_tests.rs:67) — zero assertions.
  • test_sparse_checkout_empty_patterns (tests/sparse_checkout_tests.rs:321) — else { /* failure also acceptable */ }.
  • test_invalid_sparse_checkout_patterns (tests/error_handling_tests.rs:196) — path-traversal guard whose only assertion is assert!(!stderr.is_empty()) behind failure; passes if traversal is silently accepted.
  • Recurring assert!(!stderr.is_empty()) (tests/error_handling_tests.rs:230,312,334,494) detects "failed somehow," not the cause.
  • permission_denied / config_file_locked silently no-op as root (zero coverage in CI containers).
• Error-message assertions are wide || disjunctions (contains("Failed to <verb>") || contains(name)) that are near-unfailable; no test asserts a specific exit code.
• Idempotency/partial-failure untested: add-same-submodule-twice, delete-nonexistent, and failed-add-leaves-no-partial-state (orphan lib/... dir, dangling .git/modules, half-written .gitmodules).
• merge_from drops other.use_git_default_sparse_checkout (src/config.rs:212) — latent bug, no merge test exposes it.

---

P3

• Sparse checkout: strongest area (tests/sparse_checkout_tests.rs:23 checks file contents AND that excluded dirs are absent), but several tests (:74, :231, :358) only check wanted paths exist, not that excluded paths are absent — so they don't prove sparseness. All lean on get_sparse_checkout_file_path (tests/common/mod.rs:288), which falls back to locations[0] even when nothing exists (:357), masking "file not where expected."
• Performance tests aren't benchmarks despite the CLAUDE.md criterion claim — Instant + loose ceilings (30s/120s) that catch only hangs; test_memory_usage… measures no memory; "concurrent" tests run sequentially.
• Concurrency is the stated reason tests are serialized (.config/nextest.toml, .git/index.lock contention), yet no test exercises concurrent ops or a held lock.
• Zero-coverage public surface in src/config.rs: SubmoduleUpdateOptions::* (:298), from_options_and_settings (:495), update_with_settings (:577), set_sparse_paths_for (:897), loaders load_from_file/load_with_git_sync/sync_with_git_config.

---

Suggested first PRs (concrete, high-signal)

1. P0-2 — git-state assertions for delete/nuke/add.
2. P0-3 — fix fetchRecurse/defaults.branch silent-drop + parsed-value tests.
3. P0-1 — failure-injection seam to actually test git2/CLI fallback.
4. P1 — update against an advanced remote.

_{Audit performed via multi-agent review of tests/ and src/; claims spot-verified against source.}

[bashandbone]

P0-3 (fetchRecurse silent-drop + write/read round-trip) is addressed in #63 — includes the read-side rename, a serde-correct writer value, and three TDD regression tests (deserialize, as_config_value, and a full write_full_config round-trip). The remaining audit items (P0-1, P0-2, P1+) are still open.

[bashandbone]

P0-2 (git-state assertions for add/delete/nuke) is addressed in #64. New TestHarness helpers inspect the index gitlink, .gitmodules, submodule.* config, and .git/modules/<path>; the delete/nuke tests use pre-delete guards so cleanup assertions can't pass vacuously, plus a delete→re-add test proving cleanup is thorough enough for a clean re-add.

Notably, probing the real binary first found no cleanup bug — current delete/nuke git cleanup is correct, so these are green characterization tests that lock the behavior in. (Only cosmetic leftover: an empty .git/modules/<parent> dir, which doesn't block re-add and isn't asserted on.)

Remaining open: P0-1 (fallback failure-injection seam), P1+ (update-against-advanced-remote, is_dirty stub, security tests, etc.).

[bashandbone]

P0-1 addressed in #65 (branch test/p0-1-fallback-injection-seam).

Added a failure-injection seam — GitOpsManager::without_gix + gix_enabled() — so git2's fallback path can be forced and asserted for correct results rather than left as dead code. 7 new tests in tests/fallback_tests.rs: seam proof, git2 read/write/add (real git state: gitlink 160000, .gitmodules, submodule.* config), deinit partial-cleanup characterization, gix↔git2 list parity, and the in-process reopen() hazard (add→delete→reopen→re-add same name+path), verified load-bearing (removing reopen() fails the re-add with the stale-state error).

Two draft assertions surfaced real git2 behaviors, kept as characterization: git2 write_gitmodules can't create new entries / self-reverts url via submodule.sync(); git2 delete_submodule deliberately leaves .gitmodules (the documented note at git2_ops.rs:431).

Remaining under P0-1, deferred — needs a failable-gix seam (more invasive than this git2-only seam): forcing the CLI last-resort add_submodule path (requires both gix and git2 to fail), and asserting the gix→git2 fallback warning logs.

Full suite: 529 pass. Remaining audit items: P1 (update vs advanced remote, is_dirty stub, CLI-over-file precedence, append-only save_config), P2, P3.

[bashandbone]

P0-1 fully addressed — PR #66 closes the two sub-items #65 deferred:

• CLI last-resort path of add_submodule is now exercised for correct results via a GitOpsManager::forcing_cli_add fault-injection seam (mirrors the merged without_gix pattern). Two tests: one asserts the CLI branch produces real git state (gitlink 160000, .gitmodules, name-keyed submodule.* config, list_submodules); the other seeds the partial debris a failed git2 attempt leaves behind and asserts the CLI branch cleans it up and re-adds with the real url.
• Fallback warning logs are now asserted: a --verbose run of the binary emits the gix→git2 fallback warning to stderr, and the non-verbose run stays silent (the pair proves the assertion discriminates on --verbose).

Load-bearing-ness confirmed by sabotaging the CLI subcommand and watching both state tests fail. 534 tests pass. With #63/#64/#65/#66 merged, P0 is complete.

[bashandbone]

P1 — update against an advanced remote addressed in #67.

Writing the requested test (move the remote forward, run update, assert the submodule HEAD) surfaced a masked bug: submod update fetched but never checked the worktree out to the superproject-recorded gitlink commit — the defining job of git submodule update. The gix update path treated checkout as a no-op (// Fetch complete above), and since gix succeeded, git2's submodule.update() (which does the checkout) was never reached. So this became a TDD bug-fix + characterization, per the issue's own framing (weak tests masking a real effect bug).

• Fix (11 lines): gix update now delegates the Checkout/Unspecified strategies to git2 (mirroring the existing Merge/Rebase fallback), so the worktree is checked out to the recorded commit.
• update_checks_out_recorded_gitlink_commit — drove the fix (RED→GREEN): records the gitlink ahead of the worktree, asserts update advances the worktree to the recorded commit.
• update_against_advanced_remote_fetches_without_moving_head — characterizes the advanced-remote case: the fetch genuinely pulls the new object (non-vacuous — absent before, present after), but HEAD follows the recorded gitlink, not the remote tip (plain git submodule update, not --remote). Replaces the prior guaranteed-no-op smoke test.

536 tests pass; sync/init (which share the update path) unaffected.

Remaining P1: is_dirty stub always-clean, CLI-over-file precedence in Config::load, append-only save_config (load→modify→save→reload). The reopen() add→delete→add-same-name and add real-git-state P1 bullets were already covered by #65/#64/#66.