test(p2): pin loose error/output assertions to exact strings (#62 P2)

CLAUDE.md

@@ -89,10 +89,15 @@ After destructive operations (delete, nuke), `GitOpsManager::reopen()` must be c
 
 ### Testing Approach
 
-Integration tests in `tests/` use a `TestHarness` (in `tests/common/mod.rs`) that creates temporary git repos and invokes the compiled binary. Test files:
+Integration tests in `tests/` use a `TestHarness` (in `tests/common/mod.rs`) that creates temporary git repos and invokes the compiled binary. Beyond running the CLI, the harness inspects real git state — `index_gitlink_mode`, `gitmodules_entries`, `submodule_config_entries`, `git_modules_dir_exists` — and tests should assert on that rather than on printed output where both are available. Test files:
 - `integration_tests.rs` — end-to-end CLI behavior
 - `command_contract_tests.rs` — CLI command argument contracts
 - `config_tests.rs` — configuration parsing/serialization
 - `sparse_checkout_tests.rs` — sparse checkout behavior
 - `error_handling_tests.rs` — error conditions and messages
-- `performance_tests.rs` — benchmarks (uses criterion)
+- `fallback_tests.rs` — the gix→git2→CLI fallback chain, via the `GitOpsManager::without_gix` and `::forcing_cli_add` injection seams
+- `git_ops_tests.rs` — the `GitOperations` backends directly
+- `security_tests.rs` — path traversal, symlink escape, and command/flag injection containment
+- `performance_tests.rs` — timing and peak-allocation ceilings (plain `#[test]`s, not criterion)
+
+Criterion benchmarks live separately in `benches/benchmark.rs` (`cargo bench`).

src/lib.rs

@@ -2,6 +2,10 @@
 //
 // SPDX-License-Identifier: LicenseRef-PlainMIT OR MIT
 #![cfg_attr(coverage_nightly, feature(coverage_attribute))]
+// Cargo.toml sets `unsafe_code = "deny"` workspace-wide, which a single
+// `#![allow(unsafe_code)]` can opt back out of. The test crate needs that escape
+// hatch for its tracking allocator; the library does not, so forbid it here.
+#![forbid(unsafe_code)]
 //! A Rust CLI tool for managing Git submodules with enhanced features and user-friendly configuration.
 //! This module is exposed for integration testing; it is not intended for public use and may contain unstable APIs.
 

src/main.rs

@@ -2,6 +2,8 @@
 //
 // SPDX-License-Identifier: LicenseRef-PlainMIT OR MIT
 #![cfg_attr(coverage_nightly, feature(coverage_attribute))]
+// See the note in lib.rs: `deny` is opt-out-able per file, `forbid` is not.
+#![forbid(unsafe_code)]
 #![doc = r"
 Main entry point for the submod CLI tool.
 

tests/command_contract_tests.rs

@@ -44,10 +44,16 @@ mod tests {
         );
 
         let stdout = String::from_utf8_lossy(&output.stdout);
-        // Bash completion scripts always contain the function/complete keyword
+        // A usable bash completion script needs both halves: the function that
+        // produces candidates, and the registration that binds it to the command.
+        // ("submod" alone would be satisfied by any output mentioning the binary.)
         assert!(
-            stdout.contains("complete") || stdout.contains("_submod") || stdout.contains("submod"),
-            "bash completion script should reference submod; got: {stdout}"
+            stdout.contains("_submod()"),
+            "bash completion script should define the completion function; got: {stdout}"
+        );
+        assert!(
+            stdout.contains("complete -F _submod"),
+            "bash completion script should register the function for submod; got: {stdout}"
         );
     }
 
@@ -230,9 +236,11 @@ mod tests {
         let init_verbose = harness
             .run_submod_success(&["init", "--verbose"])
             .expect("Failed to run init --verbose");
+        // The first init above already initialized it, so a second run must say so
+        // by name rather than claim to initialize it again.
         assert!(
-            init_verbose.contains("lazy-lib") || init_verbose.contains("already initialized"),
-            "Verbose init should mention the submodule; got: {init_verbose}"
+            init_verbose.contains("lazy-lib already initialized"),
+            "Verbose init should report the submodule as already initialized; got: {init_verbose}"
         );
     }
 
@@ -269,9 +277,14 @@ mod tests {
             .run_submod_success(&["nuke-it-from-orbit", "--all", "--kill"])
             .expect("Failed to nuke all");
 
+        // --all must nuke every submodule, not just the first one it finds.
+        assert!(
+            stdout.contains("Nuking submodule 'nuke-a'..."),
+            "Expected nuke progress for nuke-a; got: {stdout}"
+        );
         assert!(
-            stdout.contains("Nuking") || stdout.contains("💥"),
-            "Expected nuke progress output; got: {stdout}"
+            stdout.contains("Nuking submodule 'nuke-b'..."),
+            "Expected nuke progress for nuke-b; got: {stdout}"
         );
 
         let config_after = harness.read_config().expect("Failed to read config");
@@ -313,9 +326,15 @@ mod tests {
             .run_submod_success(&["nuke-it-from-orbit", "reinit-lib"])
             .expect("Failed to nuke-and-reinit");
 
+        // Without --kill both halves must happen; either one alone is the bug this
+        // test exists to catch, so assert them separately rather than as a disjunction.
+        assert!(
+            stdout.contains("Nuking submodule 'reinit-lib'..."),
+            "Expected nuke progress; got: {stdout}"
+        );
         assert!(
-            stdout.contains("Nuking") || stdout.contains("Reinitializing"),
-            "Expected nuke/reinit progress; got: {stdout}"
+            stdout.contains("Reinitializing submodule 'reinit-lib'..."),
+            "Expected reinit progress; got: {stdout}"
         );
 
         // After reinit, submodule should exist again
@@ -565,9 +584,9 @@ mod tests {
             .run_submod_success(&["change", "movable-lib", "--path", "lib/moved"])
             .expect("Failed to change submodule path");
 
-        // Should confirm the update
+        // Should confirm the update, naming the submodule it re-added at the new path.
         assert!(
-            stdout.contains("Added submodule") || stdout.contains("movable-lib"),
+            stdout.contains("Added submodule movable-lib"),
             "Expected confirmation of path change; got: {stdout}"
         );
 
@@ -1063,9 +1082,11 @@ mod tests {
         let stdout_verbose = harness
             .run_submod_success(&["init", "--verbose"])
             .expect("Failed to run init --verbose");
+        // "Initializing" || "initialized" could not distinguish the two states; the
+        // default init above ran first, so this run must report the already-done case.
         assert!(
-            stdout_verbose.contains("Initializing") || stdout_verbose.contains("initialized"),
-            "verbose init output should mention initialization; got: {stdout_verbose}"
+            stdout_verbose.contains("init-contract already initialized"),
+            "verbose init output should name the already-initialized submodule; got: {stdout_verbose}"
         );
     }
 

tests/integration_tests.rs

@@ -192,7 +192,13 @@ sparse_paths = ["src"]
             .run_submod_success(&["update"])
             .expect("Failed to run update");
 
-        assert!(stdout.contains("Updated") || stdout.contains("Already up to date"));
+        // The one submodule in the config must be counted. `update` does not name
+        // the submodules it touched — it forwards the raw fetch output and then
+        // prints this summary — so the count is the assertable contract here.
+        assert!(
+            stdout.contains("Updated 1 submodule(s)"),
+            "update should report how many submodules it updated; got: {stdout}"
+        );
     }
 
     /// `update` must check the submodule worktree out to the commit recorded as
@@ -586,11 +592,17 @@ active = true
         let output = harness
             .run_submod(&["check", "--verbose"])
             .expect("Failed to run submod");
-        assert!(!output.status.success());
+        assert_eq!(output.status.code(), Some(1));
 
         let stderr = String::from_utf8_lossy(&output.stderr);
         assert!(
-            stderr.contains("Failed to create manager") || stderr.contains("Repository not found")
+            stderr.contains("Failed to create manager"),
+            "error should name the operation that failed; got: {stderr}"
+        );
+        // The underlying cause must survive the wrapping, not be flattened away.
+        assert!(
+            stderr.contains("Repository not found"),
+            "error should preserve the underlying cause; got: {stderr}"
         );
     }
 
@@ -611,9 +623,12 @@ active = true
             ])
             .expect("Failed to run submod");
 
-        assert!(!output.status.success());
+        assert_eq!(output.status.code(), Some(1));
         let stderr = String::from_utf8_lossy(&output.stderr);
-        assert!(stderr.contains("Failed to add submodule") || stderr.contains("clone failed"));
+        assert!(
+            stderr.contains("Failed to add submodule"),
+            "error should name the operation that failed; got: {stderr}"
+        );
     }
 
     #[test]
@@ -1042,9 +1057,13 @@ active = true
             ])
             .expect("Failed to run generate-config");
 
-        assert!(!output.status.success());
+        assert_eq!(output.status.code(), Some(1));
         let stderr = String::from_utf8_lossy(&output.stderr);
-        assert!(stderr.contains("already exists") || stderr.contains("Use --force"));
+        // Both the refusal and the way out of it must be in the message.
+        assert!(
+            stderr.contains("already exists. Use --force to overwrite."),
+            "error should refuse to clobber and name the override flag; got: {stderr}"
+        );
     }
 
     #[test]
@@ -1073,7 +1092,10 @@ active = true
             .run_submod_success(&["nuke-it-from-orbit", "nuke-lib", "--kill"])
             .expect("Failed to nuke submodule");
 
-        assert!(stdout.contains("Nuking") || stdout.contains("💥"));
+        assert!(
+            stdout.contains("💥 Nuking submodule 'nuke-lib'..."),
+            "nuke should report the submodule it is nuking; got: {stdout}"
+        );
 
         // Config should not contain the submodule anymore
         let config = harness.read_config().expect("Failed to read config");