From d39b7e0c349a055092571c8140ada21608e89865 Mon Sep 17 00:00:00 2001 From: Payton McIntosh Date: Wed, 20 May 2026 23:22:39 +0100 Subject: [PATCH 1/6] Add Whitaker to lint checks Run Whitaker as part of the local lint target and install the pinned Whitaker installer in CI before invoking `make lint`. Refactor the existing interpolation helpers and test modules so the new Whitaker gate passes without suppressing the lint findings. --- .github/workflows/ci.yml | 8 + AGENTS.md | 8 +- Makefile | 4 + .../src/interpolation/mod.rs | 199 +++++++++++++----- .../src/interpolation/parse.rs | 61 ++---- crates/dear-diary-config/src/settings.rs | 2 + crates/dear-diary-embeddings/src/fastembed.rs | 2 + crates/dear-diary-mcp/src/deprecation.rs | 2 + crates/dear-diary-mcp/src/server.rs | 2 + crates/dear-diary-mcp/src/server_tests.rs | 12 +- crates/dear-diary-qdrant/src/entry.rs | 2 + ...antipatterns-and-refactoring-strategies.md | 51 +++-- docs/developers-guide.md | 11 + docs/ortho-config-users-guide.md | 39 ++-- docs/release-process.md | 6 +- ...esting-in-rust-via-dependency-injection.md | 4 +- docs/rstest-bdd-users-guide.md | 31 ++- docs/rust-doctest-dry-guide.md | 16 +- docs/rust-testing-with-rstest-fixtures.md | 36 ++-- docs/users-guide.md | 10 +- 20 files changed, 307 insertions(+), 199 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1b03d37..08b456a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -13,6 +13,7 @@ jobs: env: CARGO_TERM_COLOR: always BUILD_PROFILE: debug + WHITAKER_INSTALLER_REV: f768c2e53c47df13658af1168a67851d388750bf steps: - uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 - name: Setup Rust @@ -37,6 +38,13 @@ jobs: uses: astral-sh/setup-uv@37802adc94f370d6bfd71619e3f0bf239e1f3b78 - name: Script tests run: make test-scripts + - name: Install Whitaker + run: | + cargo install --locked \ + --git https://github.com/leynos/whitaker \ + --rev "${WHITAKER_INSTALLER_REV}" \ + whitaker-installer + whitaker-installer --cranelift - name: Lint run: make lint - name: Test and Measure Coverage diff --git a/AGENTS.md b/AGENTS.md index e9949cf..6e3e4d9 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -215,15 +215,15 @@ project: ### Dependency Management - **Mandate caret requirements for all dependencies.** All crate versions - specified in `Cargo.toml` must use SemVer-compatible caret requirements - (e.g., `some-crate = "1.2.3"`). This is Cargo's default and allows for safe, + specified in `Cargo.toml` must use SemVer-compatible caret requirements (e.g., + `some-crate = "1.2.3"`). This is Cargo's default and allows for safe, non-breaking updates to minor and patch versions while preventing breaking changes from new major versions. This approach is critical for ensuring build stability and reproducibility. - **Prohibit unstable version specifiers.** The use of wildcard (`*`) or open-ended inequality (`>=`) version requirements is strictly forbidden, as - they introduce unacceptable risk and unpredictability. Tilde requirements - (`~`) should only be used where a dependency must be locked to patch-level + they introduce unacceptable risk and unpredictability. Tilde requirements ( + `~`) should only be used where a dependency must be locked to patch-level updates for a specific, documented reason. ### Error Handling diff --git a/Makefile b/Makefile index 0891132..e4e11b5 100644 --- a/Makefile +++ b/Makefile @@ -3,6 +3,8 @@ TARGET ?= dear-diary +USER_WHITAKER := $(HOME)/.local/bin/whitaker +USER_BIN_PATH := $(HOME)/.cargo/bin:$(HOME)/.local/bin:$(HOME)/.bun/bin CARGO ?= cargo BUILD_JOBS ?= RUST_FLAGS ?= -D warnings @@ -13,6 +15,7 @@ TEST_FLAGS ?= $(CARGO_FLAGS) MDLINT ?= markdownlint-cli2 NIXIE ?= nixie PYTEST ?= uv run --with pytest --with cyclopts --with syrupy python -m pytest +WHITAKER ?= $(or $(shell command -v whitaker 2>/dev/null),$(wildcard $(USER_WHITAKER)),whitaker) build: target/debug/$(TARGET) ## Build debug binary release: target/release/$(TARGET) ## Build release binary @@ -34,6 +37,7 @@ target/%/$(TARGET): ## Build binary in debug or release mode lint: ## Run Clippy with warnings denied RUSTDOCFLAGS="$(RUSTDOC_FLAGS)" $(CARGO) doc --workspace --no-deps $(CARGO) clippy $(CLIPPY_FLAGS) + PATH="$(USER_BIN_PATH):$(PATH)" RUSTFLAGS="$(RUST_FLAGS)" $(WHITAKER) --all -- $(CARGO_FLAGS) fmt: ## Format Rust and Markdown sources $(CARGO) fmt --all diff --git a/crates/dear-diary-config/src/interpolation/mod.rs b/crates/dear-diary-config/src/interpolation/mod.rs index c5f8c2c..8640a58 100644 --- a/crates/dear-diary-config/src/interpolation/mod.rs +++ b/crates/dear-diary-config/src/interpolation/mod.rs @@ -21,6 +21,70 @@ mod tests; use crate::error::ConfigError; use parse::parse_remote_url; +/// Tracks which interpolation placeholders appear in a template. +struct PlaceholderNeeds(u8); + +impl PlaceholderNeeds { + const REPO: u8 = 1 << 0; + const OWNER: u8 = 1 << 1; + const CWD: u8 = 1 << 2; + const BRANCH: u8 = 1 << 3; + + /// Builds placeholder requirements by scanning the template once per token. + fn from_template(template: &str) -> Self { + let mut flags = 0; + flags |= placeholder_flag(template, "{repo}", Self::REPO); + flags |= placeholder_flag(template, "{owner}", Self::OWNER); + flags |= placeholder_flag(template, "{cwd}", Self::CWD); + flags |= placeholder_flag(template, "{branch}", Self::BRANCH); + Self(flags) + } + + /// Returns true when the template contains no supported placeholders. + const fn is_empty(&self) -> bool { + self.0 == 0 + } + + /// Returns true when interpolation must inspect the git remote. + const fn needs_remote(&self) -> bool { + self.has(Self::REPO | Self::OWNER) + } + + /// Returns true when the repository placeholder appears in the template. + const fn needs_repo(&self) -> bool { + self.has(Self::REPO) + } + + /// Returns true when the owner placeholder appears in the template. + const fn needs_owner(&self) -> bool { + self.has(Self::OWNER) + } + + /// Returns true when the working-directory placeholder appears. + const fn needs_cwd(&self) -> bool { + self.has(Self::CWD) + } + + /// Returns true when the branch placeholder appears in the template. + const fn needs_branch(&self) -> bool { + self.has(Self::BRANCH) + } + + /// Returns true when any selected placeholder flag is set. + const fn has(&self, flag: u8) -> bool { + self.0 & flag != 0 + } +} + +/// Returns the flag when the template contains a placeholder. +fn placeholder_flag(template: &str, placeholder: &str, flag: u8) -> u8 { + if template.contains(placeholder) { + flag + } else { + 0 + } +} + /// Abstraction over git and working-directory queries for testability. /// /// Production code uses [`RealGitContext`]; tests substitute a mock. @@ -160,73 +224,110 @@ pub fn interpolate_collection_name( template: &str, git: &impl GitContext, ) -> Result { - let needs_repo = template.contains("{repo}"); - let needs_owner = template.contains("{owner}"); - let needs_cwd = template.contains("{cwd}"); - let needs_branch = template.contains("{branch}"); + let needs = PlaceholderNeeds::from_template(template); // Fast path: no placeholders at all. - if !needs_repo && !needs_owner && !needs_cwd && !needs_branch { + if needs.is_empty() { return Ok(template.to_owned()); } let mut result = template.to_owned(); - // Resolve remote-derived placeholders together (one git call). - if needs_repo || needs_owner { - let url = git.remote_url()?.ok_or_else(|| { - let affected = unresolved_remote_placeholders(needs_owner, needs_repo); - ConfigError::UnresolvablePlaceholder { - placeholder: affected, - reason: "No git remote named 'origin' is \ - configured" - .to_owned(), - } - })?; + if needs.needs_remote() { + result = replace_remote_placeholders(result, git, &needs)?; + } - let info = parse_remote_url(&url)?; - - if needs_owner { - let owner = info - .owner - .ok_or_else(|| ConfigError::UnresolvablePlaceholder { - placeholder: "owner".to_owned(), - reason: format!( - concat!( - "Remote URL '{0}' has a ", - "single-segment path with ", - "no owner component" - ), - url - ), - })?; - result = result.replace("{owner}", &owner); - } - if needs_repo { - result = result.replace("{repo}", &info.repo); + result = replace_cwd_placeholder(result, git, &needs)?; + result = replace_branch_placeholder(result, git, &needs)?; + + Ok(result) +} + +/// Replaces placeholders derived from the `origin` remote. +fn replace_remote_placeholders( + mut result: String, + git: &impl GitContext, + needs: &PlaceholderNeeds, +) -> Result { + let url = git.remote_url()?.ok_or_else(|| { + let affected = unresolved_remote_placeholders(needs.needs_owner(), needs.needs_repo()); + ConfigError::UnresolvablePlaceholder { + placeholder: affected, + reason: "No git remote named 'origin' is \ + configured" + .to_owned(), } + })?; + + let info = parse_remote_url(&url)?; + + if needs.needs_owner() { + let owner = remote_owner(&url, info.owner)?; + result = result.replace("{owner}", &owner); + } + + if needs.needs_repo() { + result = result.replace("{repo}", &info.repo); } - if needs_cwd { + Ok(result) +} + +/// Returns a remote owner or the domain error for single-segment remotes. +fn remote_owner(url: &str, owner: Option) -> Result { + owner.ok_or_else(|| ConfigError::UnresolvablePlaceholder { + placeholder: "owner".to_owned(), + reason: format!( + concat!( + "Remote URL '{0}' has a ", + "single-segment path with ", + "no owner component" + ), + url + ), + }) +} + +/// Replaces the working-directory placeholder when present. +fn replace_cwd_placeholder( + result: String, + git: &impl GitContext, + needs: &PlaceholderNeeds, +) -> Result { + if needs.needs_cwd() { let basename = git.cwd_basename()?; - result = result.replace("{cwd}", &basename); + Ok(result.replace("{cwd}", &basename)) + } else { + Ok(result) } +} - if needs_branch { +/// Replaces the branch placeholder when present. +fn replace_branch_placeholder( + result: String, + git: &impl GitContext, + needs: &PlaceholderNeeds, +) -> Result { + if needs.needs_branch() { let branch = git .branch_name()? - .ok_or_else(|| ConfigError::UnresolvablePlaceholder { - placeholder: "branch".to_owned(), - reason: concat!( - "Not on a named branch ", - "(detached HEAD or not a git repository)" - ) - .to_owned(), - })?; - result = result.replace("{branch}", &branch); + .ok_or_else(unresolvable_branch_placeholder)?; + Ok(result.replace("{branch}", &branch)) + } else { + Ok(result) } +} - Ok(result) +/// Builds the domain error for an unavailable branch name. +fn unresolvable_branch_placeholder() -> ConfigError { + ConfigError::UnresolvablePlaceholder { + placeholder: "branch".to_owned(), + reason: concat!( + "Not on a named branch ", + "(detached HEAD or not a git repository)" + ) + .to_owned(), + } } /// Builds a comma-separated list of remote-derived placeholders that diff --git a/crates/dear-diary-config/src/interpolation/parse.rs b/crates/dear-diary-config/src/interpolation/parse.rs index 1dfe011..431e782 100644 --- a/crates/dear-diary-config/src/interpolation/parse.rs +++ b/crates/dear-diary-config/src/interpolation/parse.rs @@ -53,65 +53,38 @@ pub(crate) fn parse_remote_url(raw_url: &str) -> Result let clean_path = without_prefix .strip_suffix(".git") .unwrap_or(without_prefix); + let segments = remote_path_segments(clean_path); - // Walk segments once, tracking first and last non-empty values. - let mut first_segment: Option<&str> = None; - let mut last_segment: Option<&str> = None; - let mut segment_count: usize = 0; + remote_info_from_segments(raw_url, &segments) +} - for segment in clean_path.split('/').filter(|s| !s.is_empty()) { - if first_segment.is_none() { - first_segment = Some(segment); - } - last_segment = Some(segment); - segment_count += 1; - } +/// Splits a remote path into meaningful owner/repository segments. +fn remote_path_segments(clean_path: &str) -> Vec<&str> { + clean_path.split('/').filter(|s| !s.is_empty()).collect() +} - match segment_count { - 0 => Err(ConfigError::InterpolationContextError(format!( +/// Builds remote metadata from parsed path segments. +fn remote_info_from_segments(raw_url: &str, segments: &[&str]) -> Result { + match segments { + [] => Err(ConfigError::InterpolationContextError(format!( concat!( "Cannot extract repository from remote URL ", "'{0}': path contains no segments" ), raw_url ))), - 1 => { - // Single segment — repo only, no owner. - // SAFETY (logic): segment_count == 1 guarantees - // last_segment is Some (set during the loop above). - let repo = last_segment.ok_or_else(|| { - ConfigError::InterpolationContextError(format!( - "Cannot extract repository from remote URL \ - '{raw_url}'" - )) - })?; - Ok(RemoteInfo { - owner: None, - repo: repo.to_owned(), - }) - } - _ => { + [repo] => Ok(RemoteInfo { + owner: None, + repo: (*repo).to_owned(), + }), + [owner_raw, .., repo] => { // Two or more segments — first is owner, last is repo. - // SAFETY (logic): segment_count >= 2 guarantees both Some. - let owner_raw = first_segment.ok_or_else(|| { - ConfigError::InterpolationContextError(format!( - "Cannot extract owner/repo from remote URL \ - '{raw_url}'" - )) - })?; - let repo = last_segment.ok_or_else(|| { - ConfigError::InterpolationContextError(format!( - "Cannot extract owner/repo from remote URL \ - '{raw_url}'" - )) - })?; - // Strip Source Hut tilde prefix from owner. let owner = owner_raw.strip_prefix('~').unwrap_or(owner_raw); Ok(RemoteInfo { owner: Some(owner.to_owned()), - repo: repo.to_owned(), + repo: (*repo).to_owned(), }) } } diff --git a/crates/dear-diary-config/src/settings.rs b/crates/dear-diary-config/src/settings.rs index fc2b6e0..b65e074 100644 --- a/crates/dear-diary-config/src/settings.rs +++ b/crates/dear-diary-config/src/settings.rs @@ -305,6 +305,8 @@ fn env_var_bool(name: &str) -> bool { #[cfg(test)] mod tests { + //! Unit tests for configuration defaults and validation rules. + use super::*; use rstest::rstest; diff --git a/crates/dear-diary-embeddings/src/fastembed.rs b/crates/dear-diary-embeddings/src/fastembed.rs index a32eb67..a0a4218 100644 --- a/crates/dear-diary-embeddings/src/fastembed.rs +++ b/crates/dear-diary-embeddings/src/fastembed.rs @@ -134,6 +134,8 @@ impl EmbeddingProvider for FastEmbedProvider { #[cfg(test)] mod tests { + //! Unit tests for `FastEmbed` model parsing and metadata helpers. + use super::*; #[test] diff --git a/crates/dear-diary-mcp/src/deprecation.rs b/crates/dear-diary-mcp/src/deprecation.rs index 6281a25..7d8769c 100644 --- a/crates/dear-diary-mcp/src/deprecation.rs +++ b/crates/dear-diary-mcp/src/deprecation.rs @@ -48,6 +48,8 @@ pub(crate) fn visibility( #[cfg(test)] mod tests { + //! Unit tests for deprecation visibility threshold behaviour. + use super::*; /// Test that active entries (no deprecation) are always visible. diff --git a/crates/dear-diary-mcp/src/server.rs b/crates/dear-diary-mcp/src/server.rs index 42ac4a8..0b08130 100644 --- a/crates/dear-diary-mcp/src/server.rs +++ b/crates/dear-diary-mcp/src/server.rs @@ -291,6 +291,8 @@ impl ServerHandler for DiaryServer { #[cfg(test)] mod tests { + //! Unit tests for server construction and collection-name resolution. + use super::*; use dear_diary_config::{DEFAULT_EMBEDDING_MODEL, QdrantSettings, ToolSettings}; use dear_diary_qdrant::MockQdrantConnector; diff --git a/crates/dear-diary-mcp/src/server_tests.rs b/crates/dear-diary-mcp/src/server_tests.rs index afd3a65..2a5fd35 100644 --- a/crates/dear-diary-mcp/src/server_tests.rs +++ b/crates/dear-diary-mcp/src/server_tests.rs @@ -4,7 +4,7 @@ //! allowing us to test deprecation filtering and other server behaviour //! without needing a real Qdrant instance. -use std::time::{SystemTime, UNIX_EPOCH}; +use std::time::{Duration, SystemTime, UNIX_EPOCH}; use rmcp::handler::server::wrapper::Parameters; use rmcp::model::Content; @@ -23,11 +23,15 @@ const ONE_DAY_SECS: i64 = 24 * 3600; /// Helper to get current Unix timestamp for tests. fn current_timestamp() -> i64 { - #[expect(clippy::cast_possible_wrap, reason = "Unix timestamp fits in i64")] SystemTime::now() .duration_since(UNIX_EPOCH) - .map(|d| d.as_secs() as i64) - .expect("System time should be after Unix epoch") + .map_or(0, duration_secs_i64) +} + +/// Converts a Unix timestamp duration into signed seconds for test fixtures. +#[expect(clippy::cast_possible_wrap, reason = "Unix timestamp fits in i64")] +fn duration_secs_i64(duration: Duration) -> i64 { + duration.as_secs() as i64 } /// Helper to extract text from Content, panics if not text content. diff --git a/crates/dear-diary-qdrant/src/entry.rs b/crates/dear-diary-qdrant/src/entry.rs index c043eef..1bd8f59 100644 --- a/crates/dear-diary-qdrant/src/entry.rs +++ b/crates/dear-diary-qdrant/src/entry.rs @@ -99,6 +99,8 @@ impl SearchQuery { #[cfg(test)] mod tests { + //! Unit tests for entry and search-query data helpers. + use super::*; use serde_json::json; diff --git a/docs/complexity-antipatterns-and-refactoring-strategies.md b/docs/complexity-antipatterns-and-refactoring-strategies.md index 9f9e16a..fe1e97d 100644 --- a/docs/complexity-antipatterns-and-refactoring-strategies.md +++ b/docs/complexity-antipatterns-and-refactoring-strategies.md @@ -36,18 +36,18 @@ challenges. Cyclomatic Complexity, developed by Thomas J. McCabe, Sr. in 1976, is a quantitative measure of the number of linearly independent paths through a program's source code.[^3] It essentially quantifies the structural complexity -of a program by counting decision points that can affect the execution -flow.[^4] This metric is computed using the control-flow graph of the program, -where nodes represent indivisible groups of commands, and directed edges -connect nodes if one command can immediately follow another.[^3] +of a program by counting decision points that can affect the execution flow.[ +^4] This metric is computed using the control-flow graph of the program, where +nodes represent indivisible groups of commands, and directed edges connect +nodes if one command can immediately follow another.[^3] Cyclomatic Complexity is often expressed with the formula M=E−N+2P, where E is the number of edges, N is the number of nodes, and P is the number of connected components (typically 1 for a single program or method).[^3] A simpler formulation applies to a single subroutine: -M = number of decision points + 1, where decision points include constructs -like `if` statements and conditional loops.[^3] +M = number of decision points + 1, where decision points include constructs like + `if` statements and conditional loops.[^3] Thresholds and Implications: @@ -219,8 +219,8 @@ The impact of this antipattern is significant: manipulated across these logical chunks.[^2] - **Higher Defect Rates:** The heavy tax on working memory and the risk of - feature entanglement contribute to a higher likelihood of introducing - bugs.[^9] + feature entanglement contribute to a higher likelihood of introducing bugs.[ + ^9] - **Impeded Evolvability:** Adding new features or adapting to changing requirements becomes a daunting task, as the existing complex structure @@ -429,8 +429,8 @@ state (queries).[^14] Commands are task-based and should represent specific business intentions (e.g., `BookHotelRoomCommand` rather than `SetReservationStatusCommand`).[^14] Queries -never alter data and return Data Transfer Objects optimized for display -needs.[^14] +never alter data and return Data Transfer Objects optimized for display needs.[ +^14] While Command Query Responsibility Segregation operates at a higher architectural level than a single Bumpy Road method, the principles are @@ -644,26 +644,25 @@ programming.[^25] This paradigm shift can significantly reduce cognitive complexity by abstracting away low-level control flow and state management. When developers write declarative code, they operate at a higher level of -abstraction, allowing them to reason about the program's intent more -directly.[^25] This often leads to more concise, readable, and maintainable -code because the "noise" of explicit iteration, temporary variables, and manual -state updates is minimized.[^25] Many declarative approaches also inherently -favour immutability, reduce side effects, and encourage deterministic -behaviour—common culprits for bugs and increased cognitive load in imperative -code.[^26] +abstraction, allowing them to reason about the program's intent more directly.[ +^25] This often leads to more concise, readable, and maintainable code because +the "noise" of explicit iteration, temporary variables, and manual state +updates is minimized.[^25] Many declarative approaches also inherently favour +immutability, reduce side effects, and encourage deterministic behaviour—common +culprits for bugs and increased cognitive load in imperative code.[^26] Examples include using Structured Query Language for database queries— specifying the desired dataset rather than the retrieval algorithm[^34]—or employing functional programming constructs like `map`, `filter`, and `reduce` on collections instead of writing explicit loops. Refactoring imperative code to a declarative style can start small, perhaps by converting a loop that -filters and transforms a list into a chain of `filter` and `map` -operations.[^26] The broader adoption of declarative approaches in areas like -UI development (e.g., React) and data querying signifies an industry trend -towards managing complexity by raising abstraction levels. However, the -effectiveness of declarative programming relies on well-designed underlying -abstractions; a poorly designed declarative layer might not successfully hide -complexity or could introduce its own.[^27] +filters and transforms a list into a chain of `filter` and `map` operations.[ +^26] The broader adoption of declarative approaches in areas like UI +development (e.g., React) and data querying signifies an industry trend towards +managing complexity by raising abstraction levels. However, the effectiveness +of declarative programming relies on well-designed underlying abstractions; a +poorly designed declarative layer might not successfully hide complexity or +could introduce its own.[^27] #### 3. Employing dispatcher and command patterns @@ -679,8 +678,8 @@ the object that knows how to perform it. Instead of a large conditional checking a type and then executing logic, different command objects can be instantiated based on the type, and then their `execute()` method is called. This promotes the Single Responsibility Principle, as each command class -handles a single action, making the system easier to test, extend, and -evolve.[^29] +handles a single action, making the system easier to test, extend, and evolve.[ +^29] The **Dispatcher pattern** often works in conjunction with the Command pattern. A dispatcher is a central component that receives requests (which could be diff --git a/docs/developers-guide.md b/docs/developers-guide.md index 1a1b583..80a3f5c 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -37,6 +37,17 @@ rustup toolchain install rustup component add rustc-codegen-cranelift-preview ``` +`make lint` runs Rustdoc, Clippy, and Whitaker. Install Whitaker through the +upstream installer before running the full lint target locally: + +```bash +cargo install --locked \ + --git https://github.com/leynos/whitaker \ + --rev f768c2e53c47df13658af1168a67851d388750bf \ + whitaker-installer +whitaker-installer --cranelift +``` + Linux `x86_64-unknown-linux-gnu` builds link through `clang` with `mold`: ```toml diff --git a/docs/ortho-config-users-guide.md b/docs/ortho-config-users-guide.md index 2d7eba2..55cd7ac 100644 --- a/docs/ortho-config-users-guide.md +++ b/docs/ortho-config-users-guide.md @@ -54,8 +54,8 @@ behaviour end-to-end. Run `make test` to execute the example’s coverage. The unit suite uses `rstest` fixtures to exercise parsing, validation, and command planning across parameterized edge-cases (conflicting delivery modes, blank salutations, and -custom punctuation). Behavioural coverage comes from the `cucumber-rs` runner -in `tests/cucumber.rs`, which spawns the compiled binary inside a temporary +custom punctuation). Behavioural coverage comes from the `cucumber-rs` runner in + `tests/cucumber.rs`, which spawns the compiled binary inside a temporary working directory, layers `.hello_world.toml` defaults via `cap-std`, and sets `HELLO_WORLD_*` environment variables per scenario to demonstrate precedence: configuration files < environment variables < CLI arguments. @@ -149,8 +149,8 @@ serde = { version = "1.0", features = ["derive"] } clap = { version = "4", features = ["derive"] } # required for CLI support ``` -By default, only TOML configuration files are supported. To enable JSON5 -(`.json` and `.json5`) and YAML (`.yaml` and `.yml`) support, enable the +By default, only TOML configuration files are supported. To enable JSON5 ( +`.json` and `.json5`) and YAML (`.yaml` and `.yml`) support, enable the corresponding cargo features: ```toml @@ -656,11 +656,11 @@ setting. Global options such as `--recipient` or `--salutation` are parsed via variables beneath any CLI overrides. The `greet` subcommand adds optional behaviour like a preamble (`--preamble "Good morning"`) or custom punctuation while reusing the merged global configuration. The `take-leave` subcommand -combines switches and optional arguments (`--wave`, `--gift`, -`--channel email`, `--remind-in 15`) alongside greeting adjustments -(`--preamble "Until next time"`, `--punctuation ?`) to describe how the -farewell should unfold. Each subcommand struct derives `OrthoConfig` so -defaults from `[cmds.greet]` or `[cmds.take-leave]` merge automatically when +combines switches and optional arguments (`--wave`, `--gift`, `--channel email`, + `--remind-in 15`) alongside greeting adjustments ( +`--preamble "Until next time"`, `--punctuation ?`) to describe how the farewell +should unfold. Each subcommand struct derives `OrthoConfig` so defaults from +`[cmds.greet]` or `[cmds.take-leave]` merge automatically when `load_and_merge()` is called. Behavioural tests in `examples/hello_world/tests` exercise scenarios such as @@ -691,17 +691,16 @@ for a complete example. ## Error handling -`load` and `load_and_merge_subcommand_for` return `OrthoResult`, an alias -for `Result>`. `OrthoError` wraps errors from `clap`, file -I/O and `figment`. Failures during the final merge of CLI values over -configuration sources surface as the `Merge` variant, providing clearer -diagnostics when the combined data is invalid. When multiple sources fail, the -errors are collected into the `Aggregate` variant so callers can inspect each -individual failure. Consumers should handle these errors appropriately, for -example by printing them to stderr and exiting. If required fields are missing -after merging, the crate returns `OrthoError::MissingRequiredValues` with a -user‑friendly list of missing paths and hints on how to provide them. For -example: +`load` and `load_and_merge_subcommand_for` return `OrthoResult`, an alias for + `Result>`. `OrthoError` wraps errors from `clap`, file I/O +and `figment`. Failures during the final merge of CLI values over configuration +sources surface as the `Merge` variant, providing clearer diagnostics when the +combined data is invalid. When multiple sources fail, the errors are collected +into the `Aggregate` variant so callers can inspect each individual failure. +Consumers should handle these errors appropriately, for example by printing +them to stderr and exiting. If required fields are missing after merging, the +crate returns `OrthoError::MissingRequiredValues` with a user‑friendly list of +missing paths and hints on how to provide them. For example: ```plaintext Missing required values: diff --git a/docs/release-process.md b/docs/release-process.md index b6fba50..bdec119 100644 --- a/docs/release-process.md +++ b/docs/release-process.md @@ -20,9 +20,9 @@ Each binary is named using the pattern `dear-diary--`. For Linux `x86_64-unknown-linux-gnu` and `aarch64-unknown-linux-gnu`, the workflow also produces `cargo-binstall` archives named -`dear-diary--.tar.gz`. Each archive contains the -`dear-diary` binary at the archive root, matching the -`crates/dear-diary/Cargo.toml` `[package.metadata.binstall]` configuration. +`dear-diary--.tar.gz`. Each archive contains the `dear-diary` +binary at the archive root, matching the `crates/dear-diary/Cargo.toml` +`[package.metadata.binstall]` configuration. Binaries are uploaded as soon as they are built, so they are available from the workflow run while other targets build. diff --git a/docs/reliable-testing-in-rust-via-dependency-injection.md b/docs/reliable-testing-in-rust-via-dependency-injection.md index b418875..20dd0c0 100644 --- a/docs/reliable-testing-in-rust-via-dependency-injection.md +++ b/docs/reliable-testing-in-rust-via-dependency-injection.md @@ -2,8 +2,8 @@ Writing robust, reliable, and parallelizable tests requires an intentional approach to handling external dependencies such as environment variables, the -filesystem, or the system clock. Functions that directly call `std::env::var` -or `SystemTime::now()` are difficult to test because they depend on global, +filesystem, or the system clock. Functions that directly call `std::env::var` or + `SystemTime::now()` are difficult to test because they depend on global, non-deterministic state. This leads to several problems: diff --git a/docs/rstest-bdd-users-guide.md b/docs/rstest-bdd-users-guide.md index fa1cfbf..b3a4684 100644 --- a/docs/rstest-bdd-users-guide.md +++ b/docs/rstest-bdd-users-guide.md @@ -590,11 +590,11 @@ one may filter or run them in parallel as usual. Steps or hooks may call `rstest_bdd::skip!` to stop executing the remaining steps. The macro records a `Skipped` outcome and short-circuits the scenario so -the generated test returns before evaluating the annotated function body. -Invoke `skip!()` with no arguments to record a skipped outcome without a -message. Pass an optional string to describe the reason, and use the standard -`format!` syntax to interpolate values when needed. Set the -`RSTEST_BDD_FAIL_ON_SKIPPED` environment variable to `1`, or call +the generated test returns before evaluating the annotated function body. Invoke + `skip!()` with no arguments to record a skipped outcome without a message. +Pass an optional string to describe the reason, and use the standard `format!` +syntax to interpolate values when needed. Set the `RSTEST_BDD_FAIL_ON_SKIPPED` +environment variable to `1`, or call `rstest_bdd::config::set_fail_on_skipped(true)`, to escalate skipped scenarios into test failures unless the feature or scenario carries an `@allow_skipped` tag. (Example-level tags are not yet evaluated.) @@ -650,9 +650,9 @@ that a step or scenario stopped executing. Use `rstest_bdd::assert_step_skipped!` to unwrap a `StepExecution::Skipped` outcome, optionally constraining its message, and `rstest_bdd::assert_scenario_skipped!` to inspect -[`ScenarioStatus`](crate::reporting::ScenarioStatus) records. Both macros -accept `message_absent = true` to assert that no message was provided and -substring matching to confirm that a message contains the expected reason. +[`ScenarioStatus`](crate::reporting::ScenarioStatus) records. Both macros accept + `message_absent = true` to assert that no message was provided and substring +matching to confirm that a message contains the expected reason. ```rust,no_run use rstest_bdd::{assert_scenario_skipped, assert_step_skipped, StepExecution}; @@ -778,11 +778,11 @@ Best practices for writing effective scenarios include: referring to `{u32}`), escape them as `{{` and `}}` rather than placing them inside `{name:type}`. The lexer closes the placeholder at the first `}` after the optional type hint; any characters between the `:type` and that first `}` - are ignored (for example, `{n:u32 extra}` parses as `name = n`, - `type = u32`). `name` must start with a letter or underscore and may contain - letters, digits, or underscores (`[A-Za-z_][A-Za-z0-9_]*`). Whitespace within - the type hint is ignored (for example, `{count: u32}` and `{count:u32}` are - both accepted), but whitespace is not allowed between the name and the colon. + are ignored (for example, `{n:u32 extra}` parses as `name = n`, `type = u32`). + `name` must start with a letter or underscore and may contain letters, + digits, or underscores (`[A-Za-z_][A-Za-z0-9_]*`). Whitespace within the type + hint is ignored (for example, `{count: u32}` and `{count:u32}` are both + accepted), but whitespace is not allowed between the name and the colon. Prefer the compact form `{count:u32}` in new code. When a pattern contains no placeholders, the step text must match exactly. Unknown type hints are treated as generic placeholders and capture any non-newline text using a @@ -1224,9 +1224,8 @@ rstest_bdd::reporting::json::write_snapshot(&mut buffer)?; ``` The companion `rstest_bdd::reporting::junit` module renders the same snapshot -as JUnit XML. Each skipped scenario emits a `` element with an -optional `message` attribute so continuous integration (CI) servers surface the -reason: +as JUnit XML. Each skipped scenario emits a `` element with an optional + `message` attribute so continuous integration (CI) servers surface the reason: ```rust,no_run let mut xml = String::new(); diff --git a/docs/rust-doctest-dry-guide.md b/docs/rust-doctest-dry-guide.md index 8e01f2b..eb35250 100644 --- a/docs/rust-doctest-dry-guide.md +++ b/docs/rust-doctest-dry-guide.md @@ -41,7 +41,7 @@ block found in the documentation comments[^3]: 4. **Execution and Verification**: Finally, if compilation succeeds, the resulting executable is run. The test is considered to have passed if the program runs to completion without panicking. The executable is then - deleted.[^2] + deleted. [^2] The significance of this model cannot be overstated. It effectively transforms every doctest into a true integration test.[^6] The test code does not have @@ -107,8 +107,8 @@ Doctests reside within documentation comments. Rust recognizes two types: denoted by triple back-ticks (```). While `rustdoc` defaults to Rust syntax, explicitly add the `rust` language specifier for clarity.[^3] A doctest "passes" when it compiles and runs without panicking. To assert specific -outcomes, use the standard macros `assert!`, `assert_eq!`, and -`assert_ne!`.[^3] +outcomes, use the standard macros `assert!`, `assert_eq!`, and `assert_ne!`.[ +^3] ### 2.2 The philosophy of a good example @@ -272,7 +272,7 @@ table provides a comparative reference for the most common doctest attributes. - `edition20xx`: This attribute allows an example to be tested against a specific Rust edition. This is important for crates that support multiple editions and need to demonstrate edition-specific features or migration - paths.[^4] + paths. [^4] ## The DRY principle in doctests: managing shared and complex logic @@ -401,10 +401,10 @@ builds.[^13] pub struct UnixSocket; ``` -This `any` directive ensures the struct is compiled either when the target OS -is `unix` OR when `rustdoc` is running. This correctly makes the item visible -in the generated HTML. However, it is crucial to understand that this **does -not** make the doctest for `UnixSocket` pass on non-Unix platforms. +This `any` directive ensures the struct is compiled either when the target OS is + `unix` OR when `rustdoc` is running. This correctly makes the item visible in +the generated HTML. However, it is crucial to understand that this **does not** +make the doctest for `UnixSocket` pass on non-Unix platforms. This distinction highlights the "cfg duality." The `#[cfg(doc)]` attribute controls the *table of contents* of the documentation; it determines which diff --git a/docs/rust-testing-with-rstest-fixtures.md b/docs/rust-testing-with-rstest-fixtures.md index 18f4826..544fd11 100644 --- a/docs/rust-testing-with-rstest-fixtures.md +++ b/docs/rust-testing-with-rstest-fixtures.md @@ -715,8 +715,8 @@ async fn async_data_fetcher() -> String { ``` The example above uses `async_std::task::sleep` purely as a convenient -stand-in; the fixture may call into whichever runtime the project adopts -because `rstest` simply awaits the returned future. +stand-in; the fixture may call into whichever runtime the project adopts because + `rstest` simply awaits the returned future. ### B. Writing asynchronous tests (`async fn` with `#[rstest]`) @@ -765,8 +765,8 @@ To improve the ergonomics of working with async fixtures and values in tests, signature, removing the `impl Future` boilerplate. However, the value still needs to be `.await`ed explicitly within the test body or by using `#[awt]`. - `#[awt]` (or `#[future(awt)]`): This attribute, when applied to the entire - test function (`#[awt]`) or a specific `#[future]` argument - (`#[future(awt)]`), tells `rstest` to automatically insert `.await` calls for + test function (`#[awt]`) or a specific `#[future]` argument ( + `#[future(awt)]`), tells `rstest` to automatically insert `.await` calls for those futures. ```rust,no_run @@ -1339,20 +1339,20 @@ provided by `rstest`: **Table 2:** Key `rstest` attributes quick reference -| Attribute | Core Purpose | -| ---------------------------- | -------------------------------------------------------------------------------------------- | -| #[rstest] | Marks a function as a rstest test; enables fixture injection and parameterization. | -| #[fixture] | Defines a function that provides a test fixture (setup data or services). | -| #[case(…)] | Defines a single parameterized test case with specific input values. | -| #[values(…)] | Defines a list of values for an argument, generating tests for each value or combination. | -| #[once] | Marks a fixture to be initialized only once and shared (as a static reference) across tests. | -| #[future] | Simplifies async argument types by removing impl Future boilerplate. | -| #[awt] | (Function or argument level) Automatically .awaits future arguments in async tests. | -| #[from(original_name)] | Allows renaming an injected fixture argument in the test function. | -| #[with(…)] | Overrides default arguments of a fixture for a specific test. | -| #[default(…)] | Provides default values for arguments within a fixture function. | -| #[timeout(…)] | Sets a timeout for an asynchronous test. | -| #[files("glob_pattern",…)] | Injects file paths (or contents, with mode=) matching a glob pattern as test arguments. | +| Attribute | Core Purpose | +| -------------------------- | -------------------------------------------------------------------------------------------- | +| #[rstest] | Marks a function as a rstest test; enables fixture injection and parameterization. | +| #[fixture] | Defines a function that provides a test fixture (setup data or services). | +| #[case(…)] | Defines a single parameterized test case with specific input values. | +| #[values(…)] | Defines a list of values for an argument, generating tests for each value or combination. | +| #[once] | Marks a fixture to be initialized only once and shared (as a static reference) across tests. | +| #[future] | Simplifies async argument types by removing impl Future boilerplate. | +| #[awt] | (Function or argument level) Automatically .awaits future arguments in async tests. | +| #[from(original_name)] | Allows renaming an injected fixture argument in the test function. | +| #[with(…)] | Overrides default arguments of a fixture for a specific test. | +| #[default(…)] | Provides default values for arguments within a fixture function. | +| #[timeout(…)] | Sets a timeout for an asynchronous test. | +| #[files("glob_pattern",…)] | Injects file paths (or contents, with mode=) matching a glob pattern as test arguments. | By mastering `rstest`, Rust developers can significantly elevate the quality and efficiency of their testing practices, leading to more reliable, diff --git a/docs/users-guide.md b/docs/users-guide.md index ceabd18..e6d1a09 100644 --- a/docs/users-guide.md +++ b/docs/users-guide.md @@ -364,11 +364,11 @@ The deprecation system provides graceful memory management: _Table 6: Deprecation states and visibility._ -| State | Age | Default visibility | With `include_deprecated` | -| ------------------- | ---------- | ------------------ | ------------------------- | -| Active | — | Visible | Visible | -| Recently deprecated | < 7 days | Visible (flagged) | Visible (flagged) | -| Long deprecated | ≥ 7 days | Hidden | Visible (flagged) | +| State | Age | Default visibility | With `include_deprecated` | +| ------------------- | -------- | ------------------ | ------------------------- | +| Active | — | Visible | Visible | +| Recently deprecated | < 7 days | Visible (flagged) | Visible (flagged) | +| Long deprecated | ≥ 7 days | Hidden | Visible (flagged) | Flagged entries are prefixed with `[DEPRECATED]` in search results. From 9666f8a282c0b2e9e01abf7e8968ed5e892e93e9 Mon Sep 17 00:00:00 2001 From: Payton McIntosh Date: Fri, 22 May 2026 16:40:24 +0100 Subject: [PATCH 2/6] Address Whitaker PR review follow-ups Document Whitaker's role as an architectural lint gate and add an ADR for the lint contract and interpolation refactoring strategy. Cover `Settings::from_env_with_git()` collection-name interpolation through the environment-loading path, keeping git context injectable for configuration tests. Restore contiguous Markdown footnote markers and add the reviewed clause commas so the documentation remains valid GitHub-flavoured Markdown. --- .../src/interpolation/parse.rs | 116 +++++++--- crates/dear-diary-config/src/settings.rs | 94 +------- .../dear-diary-config/src/settings_tests.rs | 212 ++++++++++++++++++ crates/dear-diary-mcp/src/server_tests.rs | 27 ++- docs/adr-001-whitaker-lint-contract.md | 58 +++++ ...antipatterns-and-refactoring-strategies.md | 26 +-- docs/developers-guide.md | 17 ++ docs/ortho-config-users-guide.md | 2 +- docs/rstest-bdd-users-guide.md | 4 +- docs/rust-doctest-dry-guide.md | 4 +- 10 files changed, 415 insertions(+), 145 deletions(-) create mode 100644 crates/dear-diary-config/src/settings_tests.rs create mode 100644 docs/adr-001-whitaker-lint-contract.md diff --git a/crates/dear-diary-config/src/interpolation/parse.rs b/crates/dear-diary-config/src/interpolation/parse.rs index 431e782..38134af 100644 --- a/crates/dear-diary-config/src/interpolation/parse.rs +++ b/crates/dear-diary-config/src/interpolation/parse.rs @@ -53,39 +53,103 @@ pub(crate) fn parse_remote_url(raw_url: &str) -> Result let clean_path = without_prefix .strip_suffix(".git") .unwrap_or(without_prefix); - let segments = remote_path_segments(clean_path); + let segments = RemotePathSegments::from_path(clean_path); remote_info_from_segments(raw_url, &segments) } -/// Splits a remote path into meaningful owner/repository segments. -fn remote_path_segments(clean_path: &str) -> Vec<&str> { - clean_path.split('/').filter(|s| !s.is_empty()).collect() +/// First, last, and count metadata for a remote path. +struct RemotePathSegments<'a> { + first: Option<&'a str>, + last: Option<&'a str>, + count: usize, +} + +impl<'a> RemotePathSegments<'a> { + /// Builds segment metadata without allocating a segment vector. + fn from_path(clean_path: &'a str) -> Self { + let mut first = None; + let mut last = None; + let mut count = 0; + + for segment in clean_path.split('/').filter(|s| !s.is_empty()) { + first.get_or_insert(segment); + last = Some(segment); + count += 1; + } + + Self { first, last, count } + } + + /// Returns true when the path contains no repository segment. + const fn is_empty(&self) -> bool { + self.count == 0 + } + + /// Returns true when the path contains a repository without an owner. + const fn is_single_segment(&self) -> bool { + self.count == 1 + } } /// Builds remote metadata from parsed path segments. -fn remote_info_from_segments(raw_url: &str, segments: &[&str]) -> Result { - match segments { - [] => Err(ConfigError::InterpolationContextError(format!( - concat!( - "Cannot extract repository from remote URL ", - "'{0}': path contains no segments" - ), - raw_url - ))), - [repo] => Ok(RemoteInfo { +fn remote_info_from_segments( + raw_url: &str, + segments: &RemotePathSegments<'_>, +) -> Result { + if segments.is_empty() { + return Err(empty_remote_path_error(raw_url)); + } + + if segments.is_single_segment() { + return Ok(RemoteInfo { owner: None, - repo: (*repo).to_owned(), - }), - [owner_raw, .., repo] => { - // Two or more segments — first is owner, last is repo. - // Strip Source Hut tilde prefix from owner. - let owner = owner_raw.strip_prefix('~').unwrap_or(owner_raw); - - Ok(RemoteInfo { - owner: Some(owner.to_owned()), - repo: (*repo).to_owned(), - }) - } + repo: last_remote_segment(raw_url, segments)?.to_owned(), + }); } + + let owner_raw = first_remote_segment(raw_url, segments)?; + let owner = owner_raw.strip_prefix('~').unwrap_or(owner_raw); + + Ok(RemoteInfo { + owner: Some(owner.to_owned()), + repo: last_remote_segment(raw_url, segments)?.to_owned(), + }) +} + +/// Builds the domain error for remote URLs without path segments. +fn empty_remote_path_error(raw_url: &str) -> ConfigError { + ConfigError::InterpolationContextError(format!( + concat!( + "Cannot extract repository from remote URL ", + "'{0}': path contains no segments" + ), + raw_url + )) +} + +/// Returns the first remote path segment. +fn first_remote_segment<'a>( + raw_url: &str, + segments: &RemotePathSegments<'a>, +) -> Result<&'a str, ConfigError> { + segments.first.ok_or_else(|| { + ConfigError::InterpolationContextError(format!( + "Cannot extract owner/repo from remote URL \ + '{raw_url}'" + )) + }) +} + +/// Returns the last remote path segment. +fn last_remote_segment<'a>( + raw_url: &str, + segments: &RemotePathSegments<'a>, +) -> Result<&'a str, ConfigError> { + segments.last.ok_or_else(|| { + ConfigError::InterpolationContextError(format!( + "Cannot extract repository from remote URL \ + '{raw_url}'" + )) + }) } diff --git a/crates/dear-diary-config/src/settings.rs b/crates/dear-diary-config/src/settings.rs index b65e074..30ba83e 100644 --- a/crates/dear-diary-config/src/settings.rs +++ b/crates/dear-diary-config/src/settings.rs @@ -304,95 +304,5 @@ fn env_var_bool(name: &str) -> bool { } #[cfg(test)] -mod tests { - //! Unit tests for configuration defaults and validation rules. - - use super::*; - use rstest::rstest; - - #[rstest] - fn test_default_tool_settings() { - let settings = ToolSettings::default(); - assert_eq!( - settings.tool_store_description, - DEFAULT_TOOL_STORE_DESCRIPTION - ); - assert_eq!( - settings.tool_find_description, - DEFAULT_TOOL_FIND_DESCRIPTION - ); - } - - #[rstest] - fn test_qdrant_settings_validate_both_set() { - let settings = QdrantSettings { - qdrant_url: Some("http://localhost:6334".to_owned()), - qdrant_local_path: Some("/tmp/qdrant".to_owned()), - ..Default::default() - }; - assert!(matches!( - settings.validate(), - Err(ConfigError::ConflictingConnectionModes) - )); - } - - #[rstest] - fn test_qdrant_settings_validate_neither_set() { - let settings = QdrantSettings::default(); - assert!(matches!( - settings.validate(), - Err(ConfigError::MissingConnectionConfig) - )); - } - - #[rstest] - fn test_qdrant_settings_validate_url_only() { - let settings = QdrantSettings { - qdrant_url: Some("http://localhost:6334".to_owned()), - ..Default::default() - }; - assert!(settings.validate().is_ok()); - } - - #[rstest] - fn test_qdrant_settings_validate_local_path_only() { - let settings = QdrantSettings { - qdrant_local_path: Some("/tmp/qdrant".to_owned()), - ..Default::default() - }; - assert!(settings.validate().is_ok()); - } - - #[rstest] - fn test_filterable_fields_map() { - let settings = QdrantSettings { - qdrant_url: Some("http://localhost:6334".to_owned()), - filterable_fields: vec![ - FilterableField { - name: "category".to_owned(), - description: "Category filter".to_owned(), - field_type: FilterableFieldType::Keyword, - condition: Some(FilterableFieldCondition::Equal), - required: false, - }, - FilterableField { - name: "priority".to_owned(), - description: "Priority filter".to_owned(), - field_type: FilterableFieldType::Integer, - condition: None, - required: false, - }, - ], - ..Default::default() - }; - - let map = settings.filterable_fields_map(); - assert_eq!(map.len(), 2); - assert!(map.contains_key("category")); - assert!(map.contains_key("priority")); - - let with_conditions = settings.filterable_fields_with_conditions(); - assert_eq!(with_conditions.len(), 1); - assert!(with_conditions.contains_key("category")); - } -} +#[path = "settings_tests.rs"] +mod tests; diff --git a/crates/dear-diary-config/src/settings_tests.rs b/crates/dear-diary-config/src/settings_tests.rs new file mode 100644 index 0000000..11b7745 --- /dev/null +++ b/crates/dear-diary-config/src/settings_tests.rs @@ -0,0 +1,212 @@ +//! Unit tests for configuration defaults and validation rules. + +use super::*; +use crate::interpolation::MockGitContext; +use rstest::rstest; +use std::collections::HashMap; +use std::sync::{Mutex, MutexGuard}; + +static ENV_MUTEX: Mutex<()> = Mutex::new(()); + +const SETTINGS_ENV_KEYS: &[&str] = &[ + "COLLECTION_NAME", + "EMBEDDING_MODEL", + "QDRANT_ALLOW_ARBITRARY_FILTER", + "QDRANT_API_KEY", + "QDRANT_LOCAL_PATH", + "QDRANT_READ_ONLY", + "QDRANT_SEARCH_LIMIT", + "QDRANT_URL", + "TOOL_FIND_DESCRIPTION", + "TOOL_STORE_DESCRIPTION", +]; + +/// Guard that restores settings-related environment variables on drop. +struct EnvGuard { + _lock: MutexGuard<'static, ()>, + saved: HashMap<&'static str, Option>, +} + +impl EnvGuard { + /// Captures and clears settings-related environment variables. + fn new() -> Self { + let lock = match ENV_MUTEX.lock() { + Ok(lock) => lock, + Err(poisoned) => poisoned.into_inner(), + }; + let saved = SETTINGS_ENV_KEYS + .iter() + .map(|key| (*key, std::env::var(key).ok())) + .collect(); + let guard = Self { _lock: lock, saved }; + for key in SETTINGS_ENV_KEYS { + Self::remove(key); + } + guard + } + + /// Sets an environment variable while the guard serializes access. + fn set(key: &'static str, value: &str) { + // SAFETY: tests mutate process-global environment only while holding + // ENV_MUTEX, and EnvGuard restores the previous values. + unsafe { + std::env::set_var(key, value); + } + } + + /// Removes an environment variable while the guard serializes access. + fn remove(key: &'static str) { + // SAFETY: tests mutate process-global environment only while holding + // ENV_MUTEX, and EnvGuard restores the previous values. + unsafe { + std::env::remove_var(key); + } + } + + /// Restores one environment variable to its captured value. + fn restore(key: &'static str, saved_value: Option<&str>) { + match saved_value { + Some(original) => Self::set(key, original), + None => Self::remove(key), + } + } +} + +impl Drop for EnvGuard { + fn drop(&mut self) { + for (key, saved_value) in &self.saved { + Self::restore(key, saved_value.as_deref()); + } + } +} + +#[rstest] +fn test_default_tool_settings() { + let settings = ToolSettings::default(); + assert_eq!( + settings.tool_store_description, + DEFAULT_TOOL_STORE_DESCRIPTION + ); + assert_eq!( + settings.tool_find_description, + DEFAULT_TOOL_FIND_DESCRIPTION + ); +} + +#[rstest] +fn test_qdrant_settings_validate_both_set() { + let settings = QdrantSettings { + qdrant_url: Some("http://localhost:6334".to_owned()), + qdrant_local_path: Some("/tmp/qdrant".to_owned()), + ..Default::default() + }; + assert!(matches!( + settings.validate(), + Err(ConfigError::ConflictingConnectionModes) + )); +} + +#[rstest] +fn test_qdrant_settings_validate_neither_set() { + let settings = QdrantSettings::default(); + assert!(matches!( + settings.validate(), + Err(ConfigError::MissingConnectionConfig) + )); +} + +#[rstest] +fn test_qdrant_settings_validate_url_only() { + let settings = QdrantSettings { + qdrant_url: Some("http://localhost:6334".to_owned()), + ..Default::default() + }; + assert!(settings.validate().is_ok()); +} + +#[rstest] +fn test_qdrant_settings_validate_local_path_only() { + let settings = QdrantSettings { + qdrant_local_path: Some("/tmp/qdrant".to_owned()), + ..Default::default() + }; + assert!(settings.validate().is_ok()); +} + +#[rstest] +fn test_filterable_fields_map() { + let settings = QdrantSettings { + qdrant_url: Some("http://localhost:6334".to_owned()), + filterable_fields: vec![ + FilterableField { + name: "category".to_owned(), + description: "Category filter".to_owned(), + field_type: FilterableFieldType::Keyword, + condition: Some(FilterableFieldCondition::Equal), + required: false, + }, + FilterableField { + name: "priority".to_owned(), + description: "Priority filter".to_owned(), + field_type: FilterableFieldType::Integer, + condition: None, + required: false, + }, + ], + ..Default::default() + }; + + let map = settings.filterable_fields_map(); + assert_eq!(map.len(), 2); + assert!(map.contains_key("category")); + assert!(map.contains_key("priority")); + + let with_conditions = settings.filterable_fields_with_conditions(); + assert_eq!(with_conditions.len(), 1); + assert!(with_conditions.contains_key("category")); +} + +#[rstest] +fn test_from_env_with_git_resolves_collection_name_placeholders() { + let _env = EnvGuard::new(); + EnvGuard::set("QDRANT_URL", "http://localhost:6334"); + EnvGuard::set("COLLECTION_NAME", "{owner}-{repo}-{cwd}-{branch}"); + + let mut git = MockGitContext::new(); + git.expect_remote_url() + .returning(|| Ok(Some("git@github.com:leynos/dear-diary.git".to_owned()))); + git.expect_cwd_basename() + .returning(|| Ok("workspace".to_owned())); + git.expect_branch_name() + .returning(|| Ok(Some("adopt-whitaker-lints".to_owned()))); + + let settings = + Settings::from_env_with_git(&git).expect("settings should load from environment"); + + assert_eq!( + settings.qdrant.collection_name.as_deref(), + Some("leynos-dear-diary-workspace-adopt-whitaker-lints") + ); + assert_eq!( + settings.qdrant.qdrant_url.as_deref(), + Some("http://localhost:6334") + ); +} + +#[rstest] +fn test_from_env_with_git_reports_unresolved_collection_name_placeholders() { + let _env = EnvGuard::new(); + EnvGuard::set("QDRANT_URL", "http://localhost:6334"); + EnvGuard::set("COLLECTION_NAME", "{owner}-{repo}"); + + let mut git = MockGitContext::new(); + git.expect_remote_url().returning(|| Ok(None)); + + let err = Settings::from_env_with_git(&git) + .expect_err("settings should reject unresolved collection placeholders"); + + assert!( + matches!(err, ConfigError::UnresolvablePlaceholder { .. }), + "expected unresolved placeholder error, got {err:?}" + ); +} diff --git a/crates/dear-diary-mcp/src/server_tests.rs b/crates/dear-diary-mcp/src/server_tests.rs index 2a5fd35..2797e70 100644 --- a/crates/dear-diary-mcp/src/server_tests.rs +++ b/crates/dear-diary-mcp/src/server_tests.rs @@ -4,7 +4,7 @@ //! allowing us to test deprecation filtering and other server behaviour //! without needing a real Qdrant instance. -use std::time::{Duration, SystemTime, UNIX_EPOCH}; +use std::time::{Duration, SystemTime, SystemTimeError, UNIX_EPOCH}; use rmcp::handler::server::wrapper::Parameters; use rmcp::model::Content; @@ -22,10 +22,10 @@ use rmcp::model::ErrorCode; const ONE_DAY_SECS: i64 = 24 * 3600; /// Helper to get current Unix timestamp for tests. -fn current_timestamp() -> i64 { +fn current_timestamp() -> Result { SystemTime::now() .duration_since(UNIX_EPOCH) - .map_or(0, duration_secs_i64) + .map(duration_secs_i64) } /// Converts a Unix timestamp duration into signed seconds for test fixtures. @@ -46,9 +46,11 @@ fn content_text(content: &Content) -> &str { /// Test that entries deprecated > 7 days ago are hidden in `qdrant_find` results. #[rstest] #[tokio::test] -async fn test_old_deprecated_entries_hidden_in_find(settings: Settings) { +async fn test_old_deprecated_entries_hidden_in_find( + settings: Settings, +) -> Result<(), SystemTimeError> { let mut connector = MockQdrantConnector::new(); - let now = current_timestamp(); + let now = current_timestamp()?; // Mock collection_exists to return true connector.expect_collection_exists().returning(|_| Ok(true)); @@ -83,14 +85,17 @@ async fn test_old_deprecated_entries_hidden_in_find(settings: Settings) { text.contains("No results found"), "Old deprecated entry should be filtered out" ); + Ok(()) } /// Test that entries deprecated > 7 days ago are shown when `include_deprecated` is true. #[rstest] #[tokio::test] -async fn test_old_deprecated_entries_visible_with_flag(settings: Settings) { +async fn test_old_deprecated_entries_visible_with_flag( + settings: Settings, +) -> Result<(), SystemTimeError> { let mut connector = MockQdrantConnector::new(); - let now = current_timestamp(); + let now = current_timestamp()?; connector.expect_collection_exists().returning(|_| Ok(true)); @@ -127,14 +132,17 @@ async fn test_old_deprecated_entries_visible_with_flag(settings: Settings) { text.contains("Old deprecated memory"), "Entry content should be present" ); + Ok(()) } /// Test that recently deprecated entries (< 7 days) are visible with [DEPRECATED] prefix. #[rstest] #[tokio::test] -async fn test_recent_deprecated_entries_visible_with_prefix(settings: Settings) { +async fn test_recent_deprecated_entries_visible_with_prefix( + settings: Settings, +) -> Result<(), SystemTimeError> { let mut connector = MockQdrantConnector::new(); - let now = current_timestamp(); + let now = current_timestamp()?; connector.expect_collection_exists().returning(|_| Ok(true)); @@ -171,6 +179,7 @@ async fn test_recent_deprecated_entries_visible_with_prefix(settings: Settings) text.contains("Recent deprecated memory"), "Entry content should be present" ); + Ok(()) } /// Test that `qdrant_store` rejects requests when server is in read-only mode. diff --git a/docs/adr-001-whitaker-lint-contract.md b/docs/adr-001-whitaker-lint-contract.md new file mode 100644 index 0000000..1b3283a --- /dev/null +++ b/docs/adr-001-whitaker-lint-contract.md @@ -0,0 +1,58 @@ +# ADR 001: Adopt Whitaker as part of the lint contract + +## Status + +Accepted. + +## Context + +Dear Diary already treats `make lint` as a required commit gate. Before this +decision, that target covered Rustdoc and Clippy, but it did not enforce +project-specific architecture and maintainability rules such as module-level +documentation, non-panicking shared helpers, capability-oriented filesystem +access, or Bumpy Road complexity limits. + +The collection-name interpolation path is a representative risk area. It runs +during configuration loading, touches git state through an injected +`GitContext`, parses remote URL shapes, and maps failures into domain errors. +That code should remain explicit and testable, but clustered branching can make +startup configuration behaviour difficult to review. + +## Decision + +The project adopts Whitaker as part of the local and CI lint contract. The +`make lint` target runs Rustdoc, Clippy, and Whitaker. Continuous Integration +(CI) installs the pinned Whitaker installer revision and runs +`whitaker-installer --cranelift` immediately before `make lint`, matching the +same contract contributors run locally. + +Whitaker findings are treated as design feedback. When the suite reports +complexity in production code, the preferred response is to extract named +helpers that keep responsibilities separate. In the interpolation path, this +means preserving `GitContext` as the git boundary, keeping remote URL parsing +in the parser module, and returning typed `ConfigError` values instead of +panicking or hiding fallibility. + +## Consequences + +- Pull requests must keep Rustdoc, Clippy, and Whitaker green before review. +- CI takes on an additional setup step for the pinned Whitaker installer. +- Refactors driven by Bumpy Road findings should favour small private helpers + over lint suppressions. +- Test helpers must avoid `.expect()` outside recognised test bodies; when a + helper can fail, it should return `Result` or keep the failure inside the + test body. +- Configuration-loading tests should cover both isolated interpolation helpers + and the `Settings::from_env_with_git()` workflow that wires environment + variables into interpolation. + +## Alternatives considered + +The project could have left Whitaker as an optional local tool. That would have +kept CI faster, but it would also allow architecture and complexity regressions +to reach review without a deterministic gate. + +The project could have suppressed the first findings and limited this change to +tool installation. That would have enabled the command path, but it would have +weakened the purpose of adding Whitaker. Fixing the interpolation and test +hygiene findings makes the new lint contract active immediately. diff --git a/docs/complexity-antipatterns-and-refactoring-strategies.md b/docs/complexity-antipatterns-and-refactoring-strategies.md index fe1e97d..23c8607 100644 --- a/docs/complexity-antipatterns-and-refactoring-strategies.md +++ b/docs/complexity-antipatterns-and-refactoring-strategies.md @@ -36,8 +36,8 @@ challenges. Cyclomatic Complexity, developed by Thomas J. McCabe, Sr. in 1976, is a quantitative measure of the number of linearly independent paths through a program's source code.[^3] It essentially quantifies the structural complexity -of a program by counting decision points that can affect the execution flow.[ -^4] This metric is computed using the control-flow graph of the program, where +of a program by counting decision points that can affect the execution +flow.[^4] This metric is computed using the control-flow graph of the program, where nodes represent indivisible groups of commands, and directed edges connect nodes if one command can immediately follow another.[^3] @@ -219,8 +219,8 @@ The impact of this antipattern is significant: manipulated across these logical chunks.[^2] - **Higher Defect Rates:** The heavy tax on working memory and the risk of - feature entanglement contribute to a higher likelihood of introducing bugs.[ - ^9] + feature entanglement contribute to a higher likelihood of introducing + bugs.[^9] - **Impeded Evolvability:** Adding new features or adapting to changing requirements becomes a daunting task, as the existing complex structure @@ -429,8 +429,8 @@ state (queries).[^14] Commands are task-based and should represent specific business intentions (e.g., `BookHotelRoomCommand` rather than `SetReservationStatusCommand`).[^14] Queries -never alter data and return Data Transfer Objects optimized for display needs.[ -^14] +never alter data and return Data Transfer Objects optimized for display +needs.[^14] While Command Query Responsibility Segregation operates at a higher architectural level than a single Bumpy Road method, the principles are @@ -644,9 +644,9 @@ programming.[^25] This paradigm shift can significantly reduce cognitive complexity by abstracting away low-level control flow and state management. When developers write declarative code, they operate at a higher level of -abstraction, allowing them to reason about the program's intent more directly.[ -^25] This often leads to more concise, readable, and maintainable code because -the "noise" of explicit iteration, temporary variables, and manual state +abstraction, allowing them to reason about the program's intent more directly.[^25] +This often leads to more concise, readable, and maintainable code because the +"noise" of explicit iteration, temporary variables, and manual state updates is minimized.[^25] Many declarative approaches also inherently favour immutability, reduce side effects, and encourage deterministic behaviour—common culprits for bugs and increased cognitive load in imperative code.[^26] @@ -656,8 +656,8 @@ specifying the desired dataset rather than the retrieval algorithm[^34]—or employing functional programming constructs like `map`, `filter`, and `reduce` on collections instead of writing explicit loops. Refactoring imperative code to a declarative style can start small, perhaps by converting a loop that -filters and transforms a list into a chain of `filter` and `map` operations.[ -^26] The broader adoption of declarative approaches in areas like UI +filters and transforms a list into a chain of `filter` and `map` +operations.[^26] The broader adoption of declarative approaches in areas like UI development (e.g., React) and data querying signifies an industry trend towards managing complexity by raising abstraction levels. However, the effectiveness of declarative programming relies on well-designed underlying abstractions; a @@ -678,8 +678,8 @@ the object that knows how to perform it. Instead of a large conditional checking a type and then executing logic, different command objects can be instantiated based on the type, and then their `execute()` method is called. This promotes the Single Responsibility Principle, as each command class -handles a single action, making the system easier to test, extend, and evolve.[ -^29] +handles a single action, making the system easier to test, extend, and +evolve.[^29] The **Dispatcher pattern** often works in conjunction with the Command pattern. A dispatcher is a central component that receives requests (which could be diff --git a/docs/developers-guide.md b/docs/developers-guide.md index 80a3f5c..57e48b4 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -48,6 +48,23 @@ cargo install --locked \ whitaker-installer --cranelift ``` +Whitaker is a Dylint-based lint suite used to catch architectural and code +health regressions that Clippy does not cover. In this workspace it enforces +rules such as module-level documentation, no panicking `expect` calls outside +recognised test bodies, and Bumpy Road complexity checks. Those checks make the +lint target a maintainability gate, not only a syntax or style gate. + +The complexity checks are intentionally active for configuration code. When +Whitaker identifies clustered branching, prefer extracting named helpers that +preserve explicit fallibility and dependency injection boundaries. For example, +collection-name interpolation keeps git access behind `GitContext`, while the +remote URL parser owns URL-shape decisions. This keeps configuration loading +testable without allowing startup parsing logic to grow into a single +multi-purpose function. + +The architectural decision is recorded in +[`docs/adr-001-whitaker-lint-contract.md`](adr-001-whitaker-lint-contract.md). + Linux `x86_64-unknown-linux-gnu` builds link through `clang` with `mold`: ```toml diff --git a/docs/ortho-config-users-guide.md b/docs/ortho-config-users-guide.md index 55cd7ac..6db729c 100644 --- a/docs/ortho-config-users-guide.md +++ b/docs/ortho-config-users-guide.md @@ -696,7 +696,7 @@ for a complete example. and `figment`. Failures during the final merge of CLI values over configuration sources surface as the `Merge` variant, providing clearer diagnostics when the combined data is invalid. When multiple sources fail, the errors are collected -into the `Aggregate` variant so callers can inspect each individual failure. +into the `Aggregate` variant, so callers can inspect each individual failure. Consumers should handle these errors appropriately, for example by printing them to stderr and exiting. If required fields are missing after merging, the crate returns `OrthoError::MissingRequiredValues` with a user‑friendly list of diff --git a/docs/rstest-bdd-users-guide.md b/docs/rstest-bdd-users-guide.md index b3a4684..ebffe06 100644 --- a/docs/rstest-bdd-users-guide.md +++ b/docs/rstest-bdd-users-guide.md @@ -651,7 +651,7 @@ that a step or scenario stopped executing. Use outcome, optionally constraining its message, and `rstest_bdd::assert_scenario_skipped!` to inspect [`ScenarioStatus`](crate::reporting::ScenarioStatus) records. Both macros accept - `message_absent = true` to assert that no message was provided and substring + `message_absent = true` to assert that no message was provided, and substring matching to confirm that a message contains the expected reason. ```rust,no_run @@ -1225,7 +1225,7 @@ rstest_bdd::reporting::json::write_snapshot(&mut buffer)?; The companion `rstest_bdd::reporting::junit` module renders the same snapshot as JUnit XML. Each skipped scenario emits a `` element with an optional - `message` attribute so continuous integration (CI) servers surface the reason: + `message` attribute, so continuous integration (CI) servers surface the reason: ```rust,no_run let mut xml = String::new(); diff --git a/docs/rust-doctest-dry-guide.md b/docs/rust-doctest-dry-guide.md index eb35250..406ae09 100644 --- a/docs/rust-doctest-dry-guide.md +++ b/docs/rust-doctest-dry-guide.md @@ -107,8 +107,8 @@ Doctests reside within documentation comments. Rust recognizes two types: denoted by triple back-ticks (```). While `rustdoc` defaults to Rust syntax, explicitly add the `rust` language specifier for clarity.[^3] A doctest "passes" when it compiles and runs without panicking. To assert specific -outcomes, use the standard macros `assert!`, `assert_eq!`, and `assert_ne!`.[ -^3] +outcomes, use the standard macros `assert!`, `assert_eq!`, and +`assert_ne!`.[^3] ### 2.2 The philosophy of a good example From 6dd636df2ee74b63c3b34add6397a2a62cf620d6 Mon Sep 17 00:00:00 2001 From: Payton McIntosh Date: Fri, 22 May 2026 16:45:30 +0100 Subject: [PATCH 3/6] Address final Whitaker review comments Cache Whitaker binaries and generated lint artefacts in CI so repeated runs do not rebuild the installer and lint suite when the pinned revision is unchanged. Replace the placeholder bitflag helper with explicit grouped booleans. Keep remote and local placeholder needs separate so the code remains readable while satisfying the denied excessive-bool lint without a suppression. --- .github/workflows/ci.yml | 11 ++++ .../src/interpolation/mod.rs | 64 +++++++++---------- 2 files changed, 43 insertions(+), 32 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 08b456a..d57905d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -36,9 +36,20 @@ jobs: !**/dist/** - name: Setup uv uses: astral-sh/setup-uv@37802adc94f370d6bfd71619e3f0bf239e1f3b78 + - name: Cache Whitaker installation + id: cache-whitaker + uses: actions/cache@v4 + with: + path: | + ~/.cargo/bin/whitaker + ~/.cargo/bin/whitaker-installer + ~/.dylint_drivers + ~/.local/share/whitaker + key: whitaker-${{ runner.os }}-${{ env.WHITAKER_INSTALLER_REV }} - name: Script tests run: make test-scripts - name: Install Whitaker + if: steps.cache-whitaker.outputs.cache-hit != 'true' run: | cargo install --locked \ --git https://github.com/leynos/whitaker \ diff --git a/crates/dear-diary-config/src/interpolation/mod.rs b/crates/dear-diary-config/src/interpolation/mod.rs index 8640a58..f57f24a 100644 --- a/crates/dear-diary-config/src/interpolation/mod.rs +++ b/crates/dear-diary-config/src/interpolation/mod.rs @@ -22,66 +22,66 @@ use crate::error::ConfigError; use parse::parse_remote_url; /// Tracks which interpolation placeholders appear in a template. -struct PlaceholderNeeds(u8); +struct PlaceholderNeeds { + remote: RemotePlaceholderNeeds, + local: LocalPlaceholderNeeds, +} -impl PlaceholderNeeds { - const REPO: u8 = 1 << 0; - const OWNER: u8 = 1 << 1; - const CWD: u8 = 1 << 2; - const BRANCH: u8 = 1 << 3; +/// Tracks placeholders resolved from git remote metadata. +struct RemotePlaceholderNeeds { + repo: bool, + owner: bool, +} +/// Tracks placeholders resolved from local repository state. +struct LocalPlaceholderNeeds { + cwd: bool, + branch: bool, +} + +impl PlaceholderNeeds { /// Builds placeholder requirements by scanning the template once per token. fn from_template(template: &str) -> Self { - let mut flags = 0; - flags |= placeholder_flag(template, "{repo}", Self::REPO); - flags |= placeholder_flag(template, "{owner}", Self::OWNER); - flags |= placeholder_flag(template, "{cwd}", Self::CWD); - flags |= placeholder_flag(template, "{branch}", Self::BRANCH); - Self(flags) + Self { + remote: RemotePlaceholderNeeds { + repo: template.contains("{repo}"), + owner: template.contains("{owner}"), + }, + local: LocalPlaceholderNeeds { + cwd: template.contains("{cwd}"), + branch: template.contains("{branch}"), + }, + } } /// Returns true when the template contains no supported placeholders. const fn is_empty(&self) -> bool { - self.0 == 0 + !self.needs_remote() && !self.needs_cwd() && !self.needs_branch() } /// Returns true when interpolation must inspect the git remote. const fn needs_remote(&self) -> bool { - self.has(Self::REPO | Self::OWNER) + self.remote.repo || self.remote.owner } /// Returns true when the repository placeholder appears in the template. const fn needs_repo(&self) -> bool { - self.has(Self::REPO) + self.remote.repo } /// Returns true when the owner placeholder appears in the template. const fn needs_owner(&self) -> bool { - self.has(Self::OWNER) + self.remote.owner } /// Returns true when the working-directory placeholder appears. const fn needs_cwd(&self) -> bool { - self.has(Self::CWD) + self.local.cwd } /// Returns true when the branch placeholder appears in the template. const fn needs_branch(&self) -> bool { - self.has(Self::BRANCH) - } - - /// Returns true when any selected placeholder flag is set. - const fn has(&self, flag: u8) -> bool { - self.0 & flag != 0 - } -} - -/// Returns the flag when the template contains a placeholder. -fn placeholder_flag(template: &str, placeholder: &str, flag: u8) -> u8 { - if template.contains(placeholder) { - flag - } else { - 0 + self.local.branch } } From 65416f3cad703e4c963c899e13759634eac40fec Mon Sep 17 00:00:00 2001 From: Payton McIntosh Date: Fri, 22 May 2026 20:28:10 +0100 Subject: [PATCH 4/6] Address settings review follow-ups Pin the Whitaker cache action to an immutable commit so CI does not depend on a floating `v4` tag. Bind settings-test environment mutation to a live `EnvGuard` instance and collapse the duplicated single-connection-mode validation tests into one parameterized `rstest` table. Use Oxford-style `recognized` spelling in the Whitaker documentation notes. --- .github/workflows/ci.yml | 2 +- .../dear-diary-config/src/settings_tests.rs | 51 +++++++++---------- docs/adr-001-whitaker-lint-contract.md | 2 +- docs/developers-guide.md | 2 +- 4 files changed, 28 insertions(+), 29 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d57905d..cce54c4 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -38,7 +38,7 @@ jobs: uses: astral-sh/setup-uv@37802adc94f370d6bfd71619e3f0bf239e1f3b78 - name: Cache Whitaker installation id: cache-whitaker - uses: actions/cache@v4 + uses: actions/cache@0057852bfaa89a56745cba8c7296529d2fc39830 with: path: | ~/.cargo/bin/whitaker diff --git a/crates/dear-diary-config/src/settings_tests.rs b/crates/dear-diary-config/src/settings_tests.rs index 11b7745..0f58a62 100644 --- a/crates/dear-diary-config/src/settings_tests.rs +++ b/crates/dear-diary-config/src/settings_tests.rs @@ -23,7 +23,7 @@ const SETTINGS_ENV_KEYS: &[&str] = &[ /// Guard that restores settings-related environment variables on drop. struct EnvGuard { - _lock: MutexGuard<'static, ()>, + lock: MutexGuard<'static, ()>, saved: HashMap<&'static str, Option>, } @@ -38,15 +38,16 @@ impl EnvGuard { .iter() .map(|key| (*key, std::env::var(key).ok())) .collect(); - let guard = Self { _lock: lock, saved }; + let guard = Self { lock, saved }; for key in SETTINGS_ENV_KEYS { - Self::remove(key); + guard.remove(key); } guard } /// Sets an environment variable while the guard serializes access. - fn set(key: &'static str, value: &str) { + fn set(&self, key: &'static str, value: &str) { + std::hint::black_box(&self.lock); // SAFETY: tests mutate process-global environment only while holding // ENV_MUTEX, and EnvGuard restores the previous values. unsafe { @@ -55,7 +56,8 @@ impl EnvGuard { } /// Removes an environment variable while the guard serializes access. - fn remove(key: &'static str) { + fn remove(&self, key: &'static str) { + std::hint::black_box(&self.lock); // SAFETY: tests mutate process-global environment only while holding // ENV_MUTEX, and EnvGuard restores the previous values. unsafe { @@ -64,10 +66,10 @@ impl EnvGuard { } /// Restores one environment variable to its captured value. - fn restore(key: &'static str, saved_value: Option<&str>) { + fn restore(&self, key: &'static str, saved_value: Option<&str>) { match saved_value { - Some(original) => Self::set(key, original), - None => Self::remove(key), + Some(original) => self.set(key, original), + None => self.remove(key), } } } @@ -75,7 +77,7 @@ impl EnvGuard { impl Drop for EnvGuard { fn drop(&mut self) { for (key, saved_value) in &self.saved { - Self::restore(key, saved_value.as_deref()); + self.restore(key, saved_value.as_deref()); } } } @@ -116,18 +118,15 @@ fn test_qdrant_settings_validate_neither_set() { } #[rstest] -fn test_qdrant_settings_validate_url_only() { +#[case(Some("http://localhost:6334".to_owned()), None)] +#[case(None, Some("/tmp/qdrant".to_owned()))] +fn test_qdrant_settings_validate_single_connection_mode( + #[case] qdrant_url: Option, + #[case] qdrant_local_path: Option, +) { let settings = QdrantSettings { - qdrant_url: Some("http://localhost:6334".to_owned()), - ..Default::default() - }; - assert!(settings.validate().is_ok()); -} - -#[rstest] -fn test_qdrant_settings_validate_local_path_only() { - let settings = QdrantSettings { - qdrant_local_path: Some("/tmp/qdrant".to_owned()), + qdrant_url, + qdrant_local_path, ..Default::default() }; assert!(settings.validate().is_ok()); @@ -168,9 +167,9 @@ fn test_filterable_fields_map() { #[rstest] fn test_from_env_with_git_resolves_collection_name_placeholders() { - let _env = EnvGuard::new(); - EnvGuard::set("QDRANT_URL", "http://localhost:6334"); - EnvGuard::set("COLLECTION_NAME", "{owner}-{repo}-{cwd}-{branch}"); + let env = EnvGuard::new(); + env.set("QDRANT_URL", "http://localhost:6334"); + env.set("COLLECTION_NAME", "{owner}-{repo}-{cwd}-{branch}"); let mut git = MockGitContext::new(); git.expect_remote_url() @@ -195,9 +194,9 @@ fn test_from_env_with_git_resolves_collection_name_placeholders() { #[rstest] fn test_from_env_with_git_reports_unresolved_collection_name_placeholders() { - let _env = EnvGuard::new(); - EnvGuard::set("QDRANT_URL", "http://localhost:6334"); - EnvGuard::set("COLLECTION_NAME", "{owner}-{repo}"); + let env = EnvGuard::new(); + env.set("QDRANT_URL", "http://localhost:6334"); + env.set("COLLECTION_NAME", "{owner}-{repo}"); let mut git = MockGitContext::new(); git.expect_remote_url().returning(|| Ok(None)); diff --git a/docs/adr-001-whitaker-lint-contract.md b/docs/adr-001-whitaker-lint-contract.md index 1b3283a..d6a3905 100644 --- a/docs/adr-001-whitaker-lint-contract.md +++ b/docs/adr-001-whitaker-lint-contract.md @@ -39,7 +39,7 @@ panicking or hiding fallibility. - CI takes on an additional setup step for the pinned Whitaker installer. - Refactors driven by Bumpy Road findings should favour small private helpers over lint suppressions. -- Test helpers must avoid `.expect()` outside recognised test bodies; when a +- Test helpers must avoid `.expect()` outside recognized test bodies; when a helper can fail, it should return `Result` or keep the failure inside the test body. - Configuration-loading tests should cover both isolated interpolation helpers diff --git a/docs/developers-guide.md b/docs/developers-guide.md index 57e48b4..cce716e 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -51,7 +51,7 @@ whitaker-installer --cranelift Whitaker is a Dylint-based lint suite used to catch architectural and code health regressions that Clippy does not cover. In this workspace it enforces rules such as module-level documentation, no panicking `expect` calls outside -recognised test bodies, and Bumpy Road complexity checks. Those checks make the +recognized test bodies, and Bumpy Road complexity checks. Those checks make the lint target a maintainability gate, not only a syntax or style gate. The complexity checks are intentionally active for configuration code. When From c0a1cab9ad27ebb4958994004b44f5b1d2b25c2b Mon Sep 17 00:00:00 2001 From: Payton McIntosh Date: Fri, 22 May 2026 20:59:32 +0100 Subject: [PATCH 5/6] Fix Whitaker cache bootstrap in CI Cache the actual `~/.local/bin/whitaker` binary produced by the installer and bump the cache key so old incomplete cache entries are not reused. Always run `whitaker-installer --cranelift` before `make lint`, matching Weaver's CI invariant that the lint executable is bootstrapped in every job even when the installer is restored from cache. --- .github/workflows/ci.yml | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index cce54c4..21e92f2 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -41,20 +41,21 @@ jobs: uses: actions/cache@0057852bfaa89a56745cba8c7296529d2fc39830 with: path: | - ~/.cargo/bin/whitaker ~/.cargo/bin/whitaker-installer + ~/.local/bin/whitaker ~/.dylint_drivers ~/.local/share/whitaker - key: whitaker-${{ runner.os }}-${{ env.WHITAKER_INSTALLER_REV }} + key: whitaker-v2-${{ runner.os }}-${{ env.WHITAKER_INSTALLER_REV }} - name: Script tests run: make test-scripts - name: Install Whitaker - if: steps.cache-whitaker.outputs.cache-hit != 'true' run: | - cargo install --locked \ - --git https://github.com/leynos/whitaker \ - --rev "${WHITAKER_INSTALLER_REV}" \ - whitaker-installer + if ! command -v whitaker-installer >/dev/null 2>&1; then + cargo install --locked \ + --git https://github.com/leynos/whitaker \ + --rev "${WHITAKER_INSTALLER_REV}" \ + whitaker-installer + fi whitaker-installer --cranelift - name: Lint run: make lint From 53578397ebcd32008ba22b6616a8fa431c3e7d6c Mon Sep 17 00:00:00 2001 From: Payton McIntosh Date: Sat, 23 May 2026 01:43:49 +0100 Subject: [PATCH 6/6] Use cache hit to skip Whitaker installer build Check the Whitaker cache step output before rebuilding `whitaker-installer`, while still running `whitaker-installer --cranelift` on every CI run so the lint executable is bootstrapped before `make lint`. --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 21e92f2..fea4cb8 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -50,7 +50,7 @@ jobs: run: make test-scripts - name: Install Whitaker run: | - if ! command -v whitaker-installer >/dev/null 2>&1; then + if [ "${{ steps.cache-whitaker.outputs.cache-hit }}" != "true" ]; then cargo install --locked \ --git https://github.com/leynos/whitaker \ --rev "${WHITAKER_INSTALLER_REV}" \