test(common): add integration tests for layered config precedence

[geoffjay]

Adds 10 new integration tests to agentd_common::config covering empty files, unknown TOML keys, full three-layer precedence, concurrent loading, and per-service edge cases. All 33 tests pass.

Closes #1199

[geoffjay]

Review: test(common): add integration tests for layered config precedence

Stack position: This PR (issue-1199) is stacked on PR #1201 (issue-1194), which currently has needs-rework. It cannot merge until #1201 is fixed and landed. Once #1201 is reworked — particularly the schema additions for MonitorConfig, HookConfig, and IndexConfig — this branch will need a restack and the new fields should get coverage here too (see below).

The test work itself is solid. Reviewing it now so feedback is ready to act on immediately after the parent lands.

---

Bugfix in existing tests — correct

Adding ENV_LOCK to test_load_with_config_file and test_precedence_file_beats_default is correct and important. Both call load_from_path(Some(path)) which runs apply_env_overrides internally, so they were silently racing with the env-var-setting tests. Good catch.

---

New tests — assessment

Test	Verdict	Notes
test_load_empty_file_uses_defaults	✅	Correct — empty TOML is valid and all fields fall back to Default
test_load_unknown_keys_are_ignored	✅	Correctly validates serde's ignore-unknown-fields behaviour for [unknown_section]
test_full_precedence_chain	✅ with note	See comment below
test_env_absent_preserves_file_value	✅	Clean test for the no-env-var path
test_partial_toml_leaves_other_services_at_defaults	✅	Good exhaustive sweep of all 11 service ports
test_all_default_ports_match_spec	✅	Useful regression guard; port table is clear
test_concurrent_load_returns_consistent_results	✅ with note	See comment below
test_hook_notify_url_from_file	✅	Correctly exercises the Option<String> field via load_from_path
test_mcp_urls_from_file	✅	Good spot-check for partial MCP URL override
test_index_languages_from_file_overrides_default	✅	Not a duplicate — exercises load_from_path → merge() path, unlike the existing test_toml_services_index_languages_override which calls toml::from_str directly

---

Minor observations (non-blocking)

1. test_full_precedence_chain — comment is slightly misleading

// Remove env var BEFORE asserting so a panic can't leave it set
env::remove_var("AGENTD_ASK_PORT");

The comment implies a panic in load_from_path would still clean up — it wouldn't, since remove_var is called after .expect(...). Reordering remove_var before the assertions (the current position) is correct for protecting against assertion panics, but not load_from_path panics. Not a real risk here, but the comment overpromises. The existing env-override tests follow the same set→use→remove pattern, so this is style-consistent; just worth a more accurate comment like: // Remove before assertions so a test-assert panic cannot leave the var set.

2. test_concurrent_load_returns_consistent_results — no ENV_LOCK

The test spawns 8 threads all calling load_from_path(None) without holding ENV_LOCK. Each thread reads env vars via apply_env_overrides. If an env-var-setting test runs concurrently (e.g., test_env_override_log_level), some threads could see log_level = "trace" while others see "info", and the cross-thread equality check would fail intermittently. In practice the ENV_LOCK serialisation makes this unlikely, but the concurrent test is the one place where not holding the lock creates a real race window rather than just a theoretical one. Consider acquiring ENV_LOCK for the duration of the spawned-threads test.

3. Redundant use std::io::Write; in new test functions

use std::io::Write; is already imported at the top of mod tests. The five new tests that re-import it locally are correct but noisy — Clippy may flag them on some lint configurations. They can be removed safely.

---

Coverage gap to address after restack

Once #1201's rework adds the missing schema fields, this PR should add tests for:

• MonitorConfig: cpu_alert_threshold, memory_alert_threshold, disk_alert_threshold, history_size (metric snapshots)
• HookConfig: notify_on_failure, notify_on_long_running, long_running_threshold_ms
• IndexConfig: embedding_endpoint, embedding_api_key, lance_table, watch_interval_secs, ignore_patterns
• AGENTD_MONITOR_HISTORY_SIZE / AGENTD_HOOK_HISTORY_SIZE (whichever naming resolution #1201 adopts for the collision)

The existing test_partial_toml_leaves_other_services_at_defaults and test_all_default_ports_match_spec are good templates for the new fields.

---

Summary: Bugfix is correct, new tests are well-structured with good comments. Three minor items above — none blocking. Hold for parent #1201 to land, restack, then add coverage for the new schema fields.

[geoffjay]

This change is part of the following stack:

• feat(common): add config module with TOML schema and layered loading #1201
  • test(common): add integration tests for layered config precedence #1202 ◀
    • feat(cli): add config init and config show subcommands #1203
      • refactor: migrate services with existing config.rs to shared TOML config #1204
        • feat: add config structs for services with inline env var config #1205
          • feat(config): add ValidateConfig trait and per-service validation #1224
            • docs: add configuration system documentation #1207

_{Change managed by git-spice.}