Phase 4: Feature flags, root engine facade, and modular docs#81
Phase 4: Feature flags, root engine facade, and modular docs#81
Conversation
Add a detailed ExecPlan for Phase 4 feature flags introducing gated compilation for solvers and stores. This living document outlines the purpose, progress checklist, decisions, testing, validation criteria, and integration steps to implement feature flags that let consumers choose solver and store implementations while maintaining a stable default build. It serves as the authoritative guide for the feature flag implementation and verification process. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughSummarise: Add an optional OR‑Tools solver crate and feature flags; split the monolithic SQLite store into modular spatial_index/sqlite modules; gate SQLite store, ingestion and solver selection behind features; introduce CliError::MissingFeature; update tests, docs, CI matrix and Makefile. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as CLI
participant Builder as SolverBuilder
participant Store as SqlitePoiStore
participant Provider as HttpTravelTimeProvider
participant Scorer as UserRelevanceScorer
participant Solver as Solver
CLI->>Builder: build_solver(config)
alt store-sqlite enabled
Builder->>Store: open(database_path, index_path)
alt open succeeds
Builder->>Provider: construct travel-time provider
Builder->>Scorer: construct scorer
Builder->>Builder: select solver by features
alt solver-vrp enabled
Builder->>Solver: construct VrpSolver(store, provider, scorer)
Solver-->>Builder: Ok(Box<dyn Solver>)
else solver-ortools enabled
Builder->>Solver: construct OrtoolsSolver(store, provider, scorer)
Solver-->>Builder: Ok(Box<dyn Solver>)
else no solver enabled
Builder-->>CLI: Err(MissingFeature{"solver-vrp or solver-ortools","solve"})
end
else open failed
Builder-->>CLI: Err(SqlitePoiStoreError::OpenDatabase)
end
else store-sqlite disabled
Builder-->>CLI: Err(MissingFeature{"store-sqlite","solve/ingest"})
end
Builder-->>CLI: return solver or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.rs📄 CodeRabbit inference engine (AGENTS.md)
Files:
⚙️ CodeRabbit configuration file
Files:
🧬 Code graph analysis (1)wildside-cli/src/lib.rs (6)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (2)
Comment |
Reviewer's GuideIntroduces a root facade crate with workspace-wide Phase 4 feature flags (solver-vrp, solver-ortools, store-sqlite), gates SQLite store and solver wiring in core and CLI behind those features, adds an OR-Tools placeholder solver crate, and updates docs/tests to exercise feature-enabled and disabled paths including CLI error behavior. Sequence diagram for CLI solve feature-flagged solver and store selectionsequenceDiagram
actor User
participant CLI as wildside_cli
participant Builder as DefaultSolveSolverBuilder
participant Store as SqlitePoiStore
participant Vrp as VrpSolver
participant Ortools as OrtoolsSolver
User->>CLI: run_solve(args)
CLI->>Builder: build(&SolveConfig)
alt feature store-sqlite enabled
Builder->>Store: open(pois.db, pois.rstar)
Store-->>Builder: SqlitePoiStore or error
alt open ok
Builder->>Builder: build_solver_with_features(store, provider, scorer)
alt features solver-vrp && solver-ortools disabled or enabled
Note over Builder,Vrp: cfg(all(feature = store-sqlite, feature = solver-vrp))
Builder-->>CLI: Ok(Box<VrpSolver>)
else only solver-ortools enabled
Note over Builder,Ortools: cfg(all(feature = store-sqlite, not(feature = solver-vrp), feature = solver-ortools))
Builder-->>CLI: Ok(Box<OrtoolsSolver>)
else no solver features enabled
Builder-->>CLI: Err(CliError::MissingFeature{ feature: "solver-vrp or solver-ortools", action: "solve" })
end
else open error
Builder-->>CLI: Err(CliError::OpenPoiStore)
end
else feature store-sqlite disabled
Builder-->>CLI: Err(CliError::MissingFeature{ feature: "store-sqlite", action: "solve" })
end
CLI-->>User: prints result or error
Class diagram for the new OrtoolsSolver and feature-gated engine facadeclassDiagram
class Solver {
<<trait>>
+solve(request: &SolveRequest) Result~SolveResponse, SolveError~
}
class SolveRequest {
}
class SolveResponse {
}
class SolveError {
<<enum>>
+InvalidRequest
+other_variants
}
class PoiStore {
<<trait>>
}
class TravelTimeProvider {
<<trait>>
}
class Scorer {
<<trait>>
}
class OrtoolsSolver~S,T,C~ {
<<generic>>
-_store: S
-_travel_time_provider: T
-_scorer: C
+new(store: S, travel_time_provider: T, scorer: C) OrtoolsSolver~S,T,C~
+solve(request: &SolveRequest) Result~SolveResponse, SolveError~
}
class VrpSolver {
+new(store: SqlitePoiStore, provider: HttpTravelTimeProvider, scorer: UserRelevanceScorer) VrpSolver
+solve(request: &SolveRequest) Result~SolveResponse, SolveError~
}
class SqlitePoiStore {
}
class HttpTravelTimeProvider {
}
class UserRelevanceScorer {
}
class wildside_engine_facade {
<<crate facade>>
+Diagnostics
+InterestProfile
+PoiStore
+PointOfInterest
+Route
+SolveError
+SolveRequest
+SolveResponse
+Solver
+Theme
+TravelTimeError
+TravelTimeMatrix
+TravelTimeProvider
+SqlitePoiStore ~cfg(store-sqlite)~
+SqlitePoiStoreError ~cfg(store-sqlite)~
+VrpSolver ~cfg(solver-vrp)~
+OrtoolsSolver ~cfg(solver-ortools)~
}
%% Relationships
Solver <|.. OrtoolsSolver
Solver <|.. VrpSolver
OrtoolsSolver ..> PoiStore
OrtoolsSolver ..> TravelTimeProvider
OrtoolsSolver ..> Scorer
VrpSolver ..> SqlitePoiStore
VrpSolver ..> HttpTravelTimeProvider
VrpSolver ..> UserRelevanceScorer
wildside_engine_facade ..> VrpSolver
wildside_engine_facade ..> OrtoolsSolver
wildside_engine_facade ..> SqlitePoiStore
Flow diagram for compile-time solver selection logicflowchart TD
A["Start: compile wildside-cli"] --> B{feature store-sqlite?}
B -- no --> E["DefaultSolveSolverBuilder.build returns CliError::MissingFeature{ feature: store-sqlite, action: solve }"]
B -- yes --> C{feature solver-vrp?}
C -- yes --> F["build_solver_with_features uses VrpSolver"]
C -- no --> D{feature solver-ortools?}
D -- yes --> G["build_solver_with_features uses OrtoolsSolver (placeholder)"]
D -- no --> H["build_solver_with_features returns CliError::MissingFeature{ feature: solver-vrp or solver-ortools, action: solve }"]
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
- Introduce a new root package `wildside-engine` with optional features: - `solver-vrp` (default) - `solver-ortools` (optional stub solver crate added) - `store-sqlite` (optional SQLite POI store) - Gate `SqlitePoiStore` and solver usage behind feature flags in `wildside-core` and `wildside-cli` - Forward features from member crates via `dep:` dependencies for a single source of truth - Add CLI errors and tests for missing features to handle no-default-features builds gracefully - Remove monolithic `wildside-core::store` module; split into submodules under feature flag - Introduce tests and behavioural scenarios exercising feature selection and fallback - Update documentation and roadmap to reflect feature flag strategy and implementation This enables flexible builds and prepares for future expansion of solver and store backends. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
leynos
changed the title
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 5 issues, and left some high level feedback:
- The multiple
build_solver_with_featuresimplementations inwildside-cli/src/solve.rsrely on a growing set of#[cfg]combinations; consider centralising solver selection (e.g. with a small enum orcfg_if!block) so adding a new solver or changing priority only touches one place. - In
wildside-core, all spatial index helpers and SQLite store types are gated behindstore-sqlite, but the module-level visibility is still public; if these are not intended as part of the stable public API for non-SQLite builds, consider tightening visibility (e.g.pub(crate)where possible) to avoid accidental external coupling.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The multiple `build_solver_with_features` implementations in `wildside-cli/src/solve.rs` rely on a growing set of `#[cfg]` combinations; consider centralising solver selection (e.g. with a small enum or `cfg_if!` block) so adding a new solver or changing priority only touches one place.
- In `wildside-core`, all spatial index helpers and SQLite store types are gated behind `store-sqlite`, but the module-level visibility is still public; if these are not intended as part of the stable public API for non-SQLite builds, consider tightening visibility (e.g. `pub(crate)` where possible) to avoid accidental external coupling.
## Individual Comments
### Comment 1
<location> `wildside-cli/src/solve.rs:23` </location>
<code_context>
+ not(feature = "solver-vrp")
+))]
+use wildside_solver_ortools::OrtoolsSolver;
+#[cfg(all(feature = "store-sqlite", feature = "solver-vrp"))]
use wildside_solver_vrp::VrpSolver;
</code_context>
<issue_to_address>
**issue (complexity):** Consider centralizing the feature-flag-based solver selection and dependency setup into a few helpers/types so the cfg logic is no longer duplicated across multiple functions, impls, and tests.
You can keep the feature behavior but collapse most of the `cfg` surface by:
1. Moving solver selection to a type alias (or small helper).
2. Removing the split `impl` for `SolveSolverBuilder`.
3. Reusing the same selection logic in tests instead of mirroring cfgs.
### 1. Collapse the three `build_solver_with_features` fns
Instead of three cfg‑gated fns with identical signatures, define a type alias for the selected solver and keep a single construction site:
```rust
#[cfg(all(feature = "store-sqlite", feature = "solver-vrp"))]
type SelectedSolver = VrpSolver;
#[cfg(all(
feature = "store-sqlite",
not(feature = "solver-vrp"),
feature = "solver-ortools"
))]
type SelectedSolver = OrtoolsSolver;
```
Then one function handles all runtime construction:
```rust
#[cfg(feature = "store-sqlite")]
fn build_solver_with_features(
store: SqlitePoiStore,
provider: HttpTravelTimeProvider,
scorer: UserRelevanceScorer,
) -> Result<Box<dyn Solver>, CliError> {
#[cfg(any(feature = "solver-vrp", feature = "solver-ortools"))]
{
Ok(Box::new(SelectedSolver::new(store, provider, scorer)))
}
#[cfg(all(not(feature = "solver-vrp"), not(feature = "solver-ortools")))]
{
// prevent unused warnings
let _ = (store, provider, scorer);
Err(CliError::MissingFeature {
feature: "solver-vrp or solver-ortools",
action: "solve",
})
}
}
```
This keeps the combinatorial feature logic in one place (the type alias) and removes duplicated signatures and multiple cfg blocks on the same function name.
### 2. Avoid splitting the `SolveSolverBuilder` impl
You can hide the `store-sqlite` dependency behind a small helper and keep a single impl for `SolveSolverBuilder`:
```rust
fn make_store_and_deps(
config: &SolveConfig,
) -> Result<(SqlitePoiStore, HttpTravelTimeProvider, UserRelevanceScorer), CliError> {
#[cfg(feature = "store-sqlite")]
{
let store = SqlitePoiStore::open(
config.pois_db.as_std_path(),
config.spatial_index.as_std_path(),
)?;
let scorer =
UserRelevanceScorer::with_defaults(&config.pois_db, &config.popularity)?;
let provider = HttpTravelTimeProvider::new(config.osrm_base_url.clone())
.map_err(|source| CliError::BuildTravelTimeProvider {
base_url: config.osrm_base_url.clone(),
source,
})?;
Ok((store, provider, scorer))
}
#[cfg(not(feature = "store-sqlite"))]
{
let _ = config;
Err(CliError::MissingFeature {
feature: "store-sqlite",
action: "solve",
})
}
}
```
Then the impl becomes cfg‑free and easier to audit:
```rust
impl SolveSolverBuilder for DefaultSolveSolverBuilder {
fn build(&self, config: &SolveConfig) -> Result<Box<dyn Solver>, CliError> {
let (store, provider, scorer) = make_store_and_deps(config)?;
build_solver_with_features(store, provider, scorer)
}
}
```
Now all feature logic is localized in `SelectedSolver`/`build_solver_with_features` and `make_store_and_deps`, not spread over multiple impl blocks.
### 3. Reuse selection logic in tests
Instead of mirroring the solver selection cfgs in a `SOLVER_SELECTION` const and three separate test fns, you can test the same logic used in production (or at least the same `SelectedSolver` cfg):
```rust
#[cfg(test)]
mod feature_flag_tests {
use super::*;
#[test]
fn solver_selection_matches_features() {
#[cfg(feature = "solver-vrp")]
assert_eq!(std::any::type_name::<SelectedSolver>(), std::any::type_name::<VrpSolver>());
#[cfg(all(not(feature = "solver-vrp"), feature = "solver-ortools"))]
assert_eq!(std::any::type_name::<SelectedSolver>(), std::any::type_name::<OrtoolsSolver>());
#[cfg(all(not(feature = "solver-vrp"), not(feature = "solver-ortools")))]
{
// here you can assert that building fails with MissingFeature
let builder = DefaultSolveSolverBuilder;
let cfg = SolveConfig::default();
let err = builder.build(&cfg).unwrap_err();
// assert on err if desired
}
}
}
```
This removes a separate “cfg maze” in tests and directly verifies the same feature selection surface that production uses.
</issue_to_address>
### Comment 2
<location> `wildside-solver-ortools/src/lib.rs:14` </location>
<code_context>
+
+/// Placeholder solver for the optional OR-Tools backend.
+#[derive(Debug)]
+pub struct OrtoolsSolver<S, T, C>
+where
+ S: PoiStore,
</code_context>
<issue_to_address>
**issue (review_instructions):** Add unit and behavioural tests for the new `solver-ortools` feature and `OrtoolsSolver` implementation.
Add unit tests that exercise `OrtoolsSolver::new` and `solve`, asserting that it returns the documented placeholder `SolveError::InvalidRequest` and that the type integrates correctly with the `Solver` trait. In addition, add behavioural tests (for example using `rstest-bdd`) that run the engine/CLI with the `solver-ortools` feature enabled to verify end-to-end selection and error behaviour for this solver. The new `solver-ortools` feature must not ship without both unit and behavioural coverage.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural and unit tests are required.
</details>
</issue_to_address>
### Comment 3
<location> `wildside-cli/src/error.rs:37` </location>
<code_context>
},
+ /// The requested operation requires a missing compile-time feature.
+ #[error("{action} requires the `{feature}` feature to be enabled")]
+ MissingFeature {
+ feature: &'static str,
+ action: &'static str,
</code_context>
<issue_to_address>
**issue (review_instructions):** Add tests that cover the new `MissingFeature` CLI error for disabled solver/store feature paths.
Add both unit and behavioural tests that exercise the new `CliError::MissingFeature` paths for operations such as `ingest` and `solve` when `store-sqlite`, `solver-vrp`, or `solver-ortools` are disabled. For example, add rstest-bdd scenarios in the new `feature_flags` test files that invoke the CLI under different feature combinations and assert that the user-facing output reflects `MissingFeature` with the correct `feature` and `action` values. Ensure unit tests also validate the error formatting and mapping from configuration/feature state to this error.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural and unit tests are required.
</details>
</issue_to_address>
### Comment 4
<location> `wildside-cli/src/solve.rs:191` </location>
<code_context>
+}
+
+#[cfg(all(feature = "store-sqlite", feature = "solver-vrp"))]
+fn build_solver_with_features(
+ store: SqlitePoiStore,
+ provider: HttpTravelTimeProvider,
</code_context>
<issue_to_address>
**issue (review_instructions):** Add behavioural tests for the new feature-flag-based solver selection logic in `build_solver_with_features`.
Keep the unit tests in `feature_flag_tests`, but add behavioural tests that verify end-to-end solver selection via the CLI for the various feature combinations (`solver-vrp` only, `solver-ortools` only, both, and neither). Implement rstest-bdd scenarios that run `run_solve` (or the public CLI entry point) under each combination and assert which solver implementation is used or that an appropriate `MissingFeature` error is produced. Ensure these behavioural tests live in the new `feature_flags` BDD files instead of leaving them effectively empty.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural and unit tests are required.
</details>
</issue_to_address>
### Comment 5
<location> `docs/users-guide.md:74` </location>
<code_context>
-JSON tag payloads, and I/O or SQLite errors.[^8]
+store is `SqlitePoiStore`, which is available when the `store-sqlite` feature
+is enabled. It opens two artefacts: a read-only SQLite database and a
+serialised R\*-tree. The loader verifies both files by reading a `WSPI` magic
+header, checking the format version (`2`), and ensuring that every indexed
+point exists in the database. Failing checks raise `SqlitePoiStoreError`,
</code_context>
<issue_to_address>
**suggestion (review_instructions):** "serialised" should use -ize spelling ("serialized") to match the specified en-GB-oxendic convention.
The style guide here explicitly calls for -ize spellings. Please change "serialised" to "serialized" in this sentence.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Use en-GB-oxendic (-ize / -yse / -our) spelling and grammar.
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/wildside-engine-design.md (1)
1-1124: File exceeds maximum length guideline.The file contains 1124 lines, which exceeds the 400-line maximum specified in the coding guidelines. While the PR's changes (lines 921-939) are localized and appropriate, the file length violates the established limit and should be addressed.
The coding guidelines state: "Files must not exceed 400 lines in length. Large modules must be decomposed."
Given the scope of this document, decomposition options include:
- Split by major sections (e.g., separate files for data ingestion, solver implementation, testing strategy)
- Extract implementation details into separate design documents
- Move works cited and detailed comparisons to appendices
Do you want me to generate a proposed decomposition structure showing how this design document could be split into multiple files whilst maintaining cross-references and narrative flow?
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (27)
Cargo.tomldocs/execplans/phase-4-feature-flags.mddocs/roadmap.mddocs/users-guide.mddocs/wildside-engine-design.mdsrc/lib.rswildside-cli/Cargo.tomlwildside-cli/src/error.rswildside-cli/src/lib.rswildside-cli/src/solve.rswildside-cli/src/tests/feature_flag_steps.rswildside-cli/src/tests/feature_flags.rswildside-cli/src/tests/helpers.rswildside-cli/src/tests/mod.rswildside-cli/src/tests/pipeline.rswildside-cli/src/tests/pipeline_steps.rswildside-cli/tests/features/feature_flags.featurewildside-core/Cargo.tomlwildside-core/src/lib.rswildside-core/src/store.rswildside-core/src/store/mod.rswildside-core/src/store/spatial_index.rswildside-core/src/store/sqlite.rswildside-core/src/test_support.rswildside-core/tests/sqlite_poi_store_behaviour.rswildside-solver-ortools/Cargo.tomlwildside-solver-ortools/src/lib.rs
💤 Files with no reviewable changes (1)
- wildside-core/src/store.rs
🧰 Additional context used
📓 Path-based instructions (6)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Runmake check-fmt,make lint, andmake testbefore committing in Rust projects.
make check-fmtexecutescargo fmt --workspace -- --checkto validate formatting across the entire workspace without modifying files.
make lintexecutescargo clippy --workspace --all-targets --all-features -- -D warningsto lint every target with all features enabled and deny all Clippy warnings.
make testexecutescargo test --workspaceto run the full workspace test suite.
Clippy warnings MUST be disallowed in Rust code.
Fix any warnings emitted during tests in the Rust code itself rather than silencing them.
Where a Rust function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a Rust function has too many parameters, group related parameters in meaningfully named structs.
Where a Rust function is returning a large error, consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new functionality in Rust. Run both before and after making any change.
Every Rust module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs in Rust using Rustdoc comments (///) so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessarymutbindings in Rust.
Handle errors with theResulttype instead of panicking in Rust where feasible.
Avoidunsafecode in Rust unless absolutely necessary, and document any usage clearly.
Place function attributes after doc comments in Rust.
Do not usereturnin single-line Rust functions.
Use predicate functions for conditional criteria in Rust with more than two branches.
Rust lints must not be silenced except as a last resort.
Rust lint rule suppressions must be tightly scoped and include a clear reason.
Preferexpectoverallowin Rust.
Userstestfixtures for shared setup in Rust tests.
Replace duplicated Rust tests with `#[rstest(...)...
Files:
wildside-cli/src/tests/mod.rswildside-cli/src/tests/feature_flags.rswildside-core/tests/sqlite_poi_store_behaviour.rswildside-core/src/lib.rswildside-cli/src/tests/helpers.rswildside-solver-ortools/src/lib.rswildside-core/src/store/mod.rswildside-cli/src/error.rswildside-cli/src/tests/feature_flag_steps.rswildside-cli/src/tests/pipeline_steps.rswildside-core/src/store/sqlite.rswildside-cli/src/tests/pipeline.rswildside-core/src/store/spatial_index.rswildside-cli/src/lib.rswildside-core/src/test_support.rswildside-cli/src/solve.rssrc/lib.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.
- Adhere to single responsibility and CQRS
- Place function attributes after doc comments.
- Do not use
returnin single-line functions.- Move conditionals with >2 branches into a predicate function.
- Avoid
unsafeunless absolutely necessary.- Every module must begin with a
//!doc comment that explains the module's purpose and utility.- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.- Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
- Use
rstestfixtures for shared setup and to avoid repetition between tests.- Replace duplicated tests with
#[rstest(...)]parameterised cases.- Prefer
mockallfor mocks/stubs.- Prefer
.expect()over.unwrap()in tests..expect()and.unwrap()are forbidden outside of tests. Errors must be propagated.- Ensure that any API or behavioural changes are reflected in the documentation in
docs/- Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/- Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
- Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and...
Files:
wildside-cli/src/tests/mod.rswildside-cli/src/tests/feature_flags.rswildside-core/tests/sqlite_poi_store_behaviour.rswildside-core/src/lib.rswildside-cli/src/tests/helpers.rswildside-solver-ortools/src/lib.rswildside-core/src/store/mod.rswildside-cli/src/error.rswildside-cli/src/tests/feature_flag_steps.rswildside-cli/src/tests/pipeline_steps.rswildside-core/src/store/sqlite.rswildside-cli/src/tests/pipeline.rswildside-core/src/store/spatial_index.rswildside-cli/src/lib.rswildside-core/src/test_support.rswildside-cli/src/solve.rssrc/lib.rs
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: Use the markdown files within thedocs/directory as a knowledge base and source of truth for project requirements, dependency choices, and architectural decisions.
When new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve, proactively update the relevant file(s) in thedocs/directory to reflect the latest state. Ensure the documentation remains accurate and current.
Documentation must use en-GB-oxendict spelling and grammar ("-ize" / "-yse" / "-our") with the exception of the LICENSE filename.
docs/**/*.md: Use British English based on the Oxford English Dictionary locale en-GB, including -ize suffixes (e.g., realize, organization), -lyse suffixes (e.g., analyse, paralyse), -our suffixes (e.g., colour, behaviour), -re suffixes (e.g., centre, calibre), double 'l' (e.g., cancelled, counsellor), maintained 'e' (e.g., likeable, rateable), -ogue suffixes (e.g., analogue, catalogue)
Use the Oxford comma in documentation: 'ships, planes, and hovercraft' where it aids comprehension
Treat company names as collective nouns, for example: 'Concordat Industries are expanding'
Write headings in sentence case
Use Markdown headings (#, ##, ###, and so on) in order without skipping levels
Follow markdownlint recommendations
Provide code blocks and lists using standard Markdown syntax
Always provide a language identifier for fenced code blocks; use 'plaintext' for non-code text
Use '-' as the first level bullet and renumber lists when items change
Prefer inline links using 'text' or angle brackets around the URL
Ensure blank lines before and after bulleted lists and fenced blocks
Ensure tables have a delimiter line below the header row
Expand any uncommon acronym on first use, for example, Continuous Integration (CI)
Wrap paragraphs at 80 columns
Do not wrap tables
Use GitHub-flavoured numeric footnotes referenced as '[^1]'
Footnotes must be numbered in order of appearance in the document
Caption ev...
Files:
docs/roadmap.mddocs/users-guide.mddocs/wildside-engine-design.mddocs/execplans/phase-4-feature-flags.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Validate Markdown files usingmake markdownlint.
Runmake fmtafter any documentation changes to format all Markdown files and fix table markup.
Validate Mermaid diagrams in Markdown files by runningmake nixie.
Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks in Markdown must be wrapped at 120 columns.
Markdown tables and headings must not be wrapped.
Use dashes (-) for list bullets in Markdown.
Use GitHub-flavoured Markdown footnotes ([^1]) for references and footnotes.
Files:
docs/roadmap.mddocs/users-guide.mddocs/wildside-engine-design.mddocs/execplans/phase-4-feature-flags.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/roadmap.mddocs/users-guide.mddocs/wildside-engine-design.mddocs/execplans/phase-4-feature-flags.md
docs/**/!(README).md
📄 CodeRabbit inference engine (docs/documentation-style-guide.md)
Avoid first and second person personal pronouns outside the README.md file
Files:
docs/roadmap.mddocs/users-guide.mddocs/wildside-engine-design.mddocs/execplans/phase-4-feature-flags.md
docs/**/*.{md,rs}
📄 CodeRabbit inference engine (docs/documentation-style-guide.md)
Wrap code at 120 columns
Files:
docs/roadmap.mddocs/users-guide.mddocs/wildside-engine-design.mddocs/execplans/phase-4-feature-flags.md
Cargo.toml
📄 CodeRabbit inference engine (AGENTS.md)
Cargo.toml: Use explicit version ranges inCargo.tomland keep dependencies up-to-date.
All crate versions specified inCargo.tomlmust use SemVer-compatible caret requirements (e.g.,some-crate = "1.2.3"). Wildcard (*) or open-ended inequality (>=) version requirements are strictly forbidden.
Tilde requirements (~) in Rust dependencies should only be used where a dependency must be locked to patch-level updates for a specific, documented reason.
Files:
Cargo.toml
🧠 Learnings (1)
📚 Learning: 2026-01-02T03:31:47.671Z
Learnt from: CR
Repo: leynos/wildside-engine PR: 0
File: docs/roadmap.md:0-0
Timestamp: 2026-01-02T03:31:47.671Z
Learning: Applies to docs/wildside-solver-ortools/src/**/*.rs : Create a `wildside-solver-ortools` crate (conditionally compiled via the `ortools` feature flag) implementing the `Solver` trait using OR-Tools CP-SAT solver for the Orienteering Problem
Applied to files:
docs/roadmap.md
🧬 Code graph analysis (7)
wildside-cli/src/tests/feature_flags.rs (3)
wildside-cli/src/tests/helpers.rs (2)
write_utf8(14-18)new(44-74)wildside-cli/src/tests/feature_flag_steps.rs (2)
ingest_requires_store_sqlite(103-105)new(23-35)wildside-cli/src/lib.rs (1)
run_ingest(63-66)
wildside-cli/src/tests/feature_flag_steps.rs (2)
wildside-cli/src/lib.rs (1)
run_ingest(63-66)wildside-cli/src/tests/feature_flags.rs (1)
ingest_requires_store_sqlite(13-36)
wildside-core/src/store/sqlite.rs (3)
wildside-core/src/store/spatial_index.rs (2)
load_index_entries(132-169)poi(181-187)wildside-core/tests/sqlite_poi_store_behaviour.rs (2)
bbox(78-80)new(29-38)wildside-core/src/poi.rs (1)
envelope(46-48)
wildside-core/src/store/spatial_index.rs (1)
wildside-core/src/store/sqlite.rs (2)
open(73-95)poi(206-212)
wildside-cli/src/lib.rs (5)
wildside-core/src/store/spatial_index.rs (1)
write_spatial_index(99-104)wildside-data/src/wikidata/etl/mod.rs (1)
extract_linked_entity_claims(153-198)wildside-data/src/wikidata/store/persistence.rs (1)
persist_claims_to_path(285-295)wildside-data/src/ingest/mod.rs (1)
ingest_osm_pbf_report(149-202)wildside-data/src/ingest/sqlite.rs (1)
persist_pois_to_sqlite(97-122)
wildside-core/src/test_support.rs (2)
wildside-core/tests/poi_store_behaviour.rs (1)
store(19-22)wildside-core/src/store/spatial_index.rs (1)
write_spatial_index(99-104)
wildside-cli/src/solve.rs (2)
wildside-fs/src/lib.rs (1)
open_utf8_file(10-12)wildside-solver-ortools/src/lib.rs (1)
new(32-38)
🪛 LanguageTool
docs/roadmap.md
[style] ~200-~200: Consider using the typographical ellipsis character here instead.
Context: ...side-data/sqlite"] ``` - [x] Use #[cfg(feature = "...")] attributes to conditionally compile...
(ELLIPSIS)
docs/users-guide.md
[style] ~73-~73: Would you like to use the Oxford spelling “serialized”? The spelling ‘serialised’ is also correct.
Context: ...acts: a read-only SQLite database and a serialised R*-tree. The loader verifies both file...
(OXFORD_SPELLING_Z_NOT_S)
docs/wildside-engine-design.md
[uncategorized] ~921-~921: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...e Flags:** The engine uses feature flags so consumers can select the solver and s...
(COMMA_COMPOUND_SENTENCE_2)
[uncategorized] ~925-~925: Loose punctuation mark.
Context: ...["solver-vrp", "store-sqlite", "serde"]`: The default build includes the nati...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~929-~929: Loose punctuation mark.
Context: ...equest/response types. - solver-vrp: Enables the native Rust solver backed b...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~932-~932: Loose punctuation mark.
Context: ...tures are enabled. - solver-ortools: Enables the optional OR-Tools solver ba...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~935-~935: Loose punctuation mark.
Context: ...T integration lands. - store-sqlite: Enables the SQLite-backed POI store and...
(UNLIKELY_OPENING_PUNCTUATION)
docs/execplans/phase-4-feature-flags.md
[uncategorized] ~13-~13: The hyphen in fully-tested is redundant.
Context: ...le keeping the default build stable and fully-tested. Success is observable when the workspa...
(ADVERB_LY_HYPHEN_FIX)
[uncategorized] ~62-~62: Possible missing comma found.
Context: ...RP solver when both solver features are enabled and ship a placeholder OR-Tools solve...
(AI_HYDRA_LEO_MISSING_COMMA)
[misspelling] ~84-~84: This word is normally spelled with a hyphen.
Context: ...ted in wildside-core/src/store.rs and re- exported from wildside-core/src/lib.rs. The ro...
(EN_COMPOUNDS_RE_EXPORTED)
[style] ~87-~87: Consider using the typographical ellipsis character here instead.
Context: ...lite, with conditional compilation via #[cfg(feature = "...")]. Testing guidance is in docs/rust...
(ELLIPSIS)
[style] ~122-~122: Consider using the typographical ellipsis character here instead.
Context: ...y re-export the SQLite store and to add doc(cfg(...)) hints where appropriate. Adjust exis...
(ELLIPSIS)
[style] ~125-~125: Consider using a more formal replacement.
Context: ... compiled when the feature is enabled. After that, update wildside-cli to respect the f...
(AFTER_THAT)
[style] ~141-~141: Consider using the typographical ellipsis character here instead.
Context: ...and scenarios are guarded with the same #[cfg(feature = "...")] selectors so they only compile unde...
(ELLIPSIS)
[style] ~183-~183: Consider removing “of” to be more concise
Context: ...ceptance The change is acceptable when all of the following are true: - make check-fmt...
(ALL_OF_THE)
[uncategorized] ~185-~185: Loose punctuation mark.
Context: ... following are true: - make check-fmt, make lint, and make test succeed wi...
(UNLIKELY_OPENING_PUNCTUATION)
[style] ~198-~198: Consider using the typographical ellipsis character here instead.
Context: ... that the module is behind the intended #[cfg(feature = "...")] guard and that its dependencies are...
(ELLIPSIS)
[uncategorized] ~216-~216: Loose punctuation mark.
Context: ...nifest with these names: - solver-vrp: enables wildside-solver-vrp. - `solve...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~217-~217: Loose punctuation mark.
Context: ...wildside-solver-vrp. - solver-ortools: enables wildside-solver-ortools. - s...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~218-~218: Loose punctuation mark.
Context: ...ldside-solver-ortools. - store-sqlite`: enables the SQLite-backed POI store. T...
(UNLIKELY_OPENING_PUNCTUATION)
🔍 Remote MCP
Let me search for more specific information about rstest-bdd testing patterns and any potential issues with feature-gated tests.
Summary of Relevant Context for PR Review
Based on my search for additional context, here's what would be helpful for reviewing this Phase 4 feature flags PR:
Rust Feature Flags Best Practices
Features are defined in the [features] table in Cargo.toml, and each feature specifies an array of other features or optional dependencies that it enables. The PR implementation aligns with standard patterns:
- Code can conditionally include support at compile time using #[cfg(feature = "name")] attributes, with Cargo setting features using the rustc --cfg flag.
- Dependencies automatically enable default features unless default-features = false is specified, which can make it difficult to ensure defaults aren't enabled, especially when dependencies appear multiple times in the dependency graph, so every package must ensure that default-features = false is specified to avoid enabling them.
Review Note: The PR's approach of using default-features = false in wildside-cli for wildside-core is appropriate here.
API Design Considerations
You should avoid feature-gating methods on public traits, as external implementors won't know whether to implement feature-gated methods or not.
Review Point: The PR avoids this by keeping the PoiStore trait public and unconditional while gating only the SQLite-specific implementations (SqlitePoiStore, SqlitePoiStoreError).
Feature Combinatorics
If your crate has N independent features, then all of the 2^N possible build combinations can occur in practice, so your CI system should cover all of these combinations in all available test variants.
Critical Review Focus: With 3 independent features (solver-vrp, solver-ortools, store-sqlite), the PR creates 2³=8 possible build combinations. CI should validate these systematically.
Testing Framework (rstest-bdd)
rstest-bdd keeps unit and acceptance tests on the same runner (cargo test), offers first-class fixtures shared between test types, and provides ergonomic step definitions with #[given], #[when], #[then] with typed placeholders.
.feature files are processed at compile time as static scenarios, and step definitions must be known at compile time (no dynamic registration).
Review Note: The PR correctly uses #[cfg(feature = ...)] on test modules like feature_flags.rs and pipeline.rs since rstest-bdd scenarios are compile-time resolved. Test files that reference features unavailable in a given build must be conditionally compiled themselves.
Workspace Dependency Management
The PR follows recommended patterns by using workspace dependency tables where dependencies are inherited by specifying them in [workspace.dependencies], then adding them to [dependencies] with workspace = true, with the caveat that the [workspace.dependencies] table itself is not allowed to specify optional.
Key Review Considerations
- Feature Matrix Coverage: Verify CI tests all 8 feature combinations (default, --no-default-features, individual features, and combinations).
- Trait Stability: The unconditional
PoiStoretrait and conditional implementations provide good API stability. - Compile-Time Gating: Test module gating with
#[cfg]is necessary for rstest-bdd scenarios to compile correctly. - Optional Dependencies: Confirm wildside-solver-ortools and solver implementations are properly marked optional in Cargo.toml files.
[::web_search::]
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Sourcery review
- GitHub Check: Sourcery review
🔇 Additional comments (66)
docs/roadmap.md (4)
181-182: LGTM: Checklist item accurately reflects completion.The feature flag naming ("solver-ortools") is consistent with the implementation across workspace manifests.
183-184: LGTM: Feature forwarding implementation is complete.The checklist accurately reflects that feature flags are forwarded from member crates through the root manifest.
200-201: LGTM: Conditional compilation is correctly marked complete.The implementation uses
#[cfg(feature = "...")]attributes as required.
212-213: LGTM: Feature flag naming is consistent.The update from "ortools" to "solver-ortools" aligns with the actual feature names defined in the workspace manifests.
wildside-core/Cargo.toml (3)
9-9: LGTM: Correct feature gating for rstar.The rstar crate remains a required dependency while its serde feature is conditionally activated through line 25, which is the appropriate pattern.
10-10: LGTM: SQLite dependency correctly gated.The optional flag and bundled feature are appropriate for feature-gated storage.
14-14: Bincode is actively used inwildside-core/src/store/sqlite.rsandwildside-core/src/store/spatial_index.rsfor serialisation. The dependency configuration is correct and necessary.docs/execplans/phase-4-feature-flags.md (7)
10-17: LGTM: Purpose is clearly stated with measurable success criteria.The section clearly articulates the goals, scope, and observable success criteria for the feature flag implementation.
19-36: LGTM: Progress tracking is comprehensive and timestamped.The checklist provides a clear record of completed work with ISO 8601 timestamps.
37-43: LGTM: Valuable documentation of workspace dependency constraints.This discovery explains the path dependency strategy used in
wildside-cli/Cargo.tomlline 20.
95-148: LGTM: Implementation plan is comprehensive and actionable.The plan provides clear, step-by-step guidance for implementing feature flags across the workspace with appropriate gating strategies.
150-180: LGTM: Concrete steps provide clear execution commands.The shell commands are well-structured with appropriate log capture for validation tracking.
181-193: LGTM: Validation criteria are clear and measurable.The acceptance criteria provide specific, testable conditions for success with appropriate feature combinations.
212-229: LGTM: Interface specifications are precise and actionable.The required feature names and public API surface are clearly documented with code examples.
wildside-cli/Cargo.toml (2)
20-20: Path dependency correctly works around workspace default-features constraint.This change implements the decision documented in
docs/execplans/phase-4-feature-flags.mdlines 51-54. The path override is necessary because workspace dependency inheritance forbidsdefault-features = false.Based on learnings, the wildside-cli crate needs independent control over which wildside-core features are enabled, specifically to disable store-sqlite by default and enable it only through the CLI's own feature flags.
23-24: LGTM: Solver dependencies correctly marked optional.Both VRP and OR-Tools solvers are properly gated behind their respective features.
wildside-solver-ortools/Cargo.toml (1)
1-14: Manifest structure and placeholder implementation are correct.The package metadata and dependency declarations follow workspace conventions appropriately. The crate is correctly marked as unpublished. The
OrtoolsSolverstruct properly implements theSolvertrait and returnsSolveError::InvalidRequestas specified in the execution plan, reserving the API surface for future OR-Tools integration without pulling native dependencies.Cargo.toml (2)
1-21: Feature flag structure is well-designed.The root package structure correctly implements feature flags with appropriate dependency gating. Using
default-features = falseforwildside-coreat line 11 is correct—it prevents unintended feature activation when the crate appears multiple times in the dependency graph.The
store-sqlitefeature correctly depends onserdeat line 20, ensuring serialisation support is available when the SQLite store is enabled.Verify that CI tests all 8 feature combinations (2³ from
solver-vrp,solver-ortools,store-sqlite). Per Rust best practices, all possible build combinations should be validated.#!/bin/bash # Description: Check if CI workflows test multiple feature combinations # Search for feature flag testing patterns in CI configuration fd -e yml -e yaml . .github/workflows | xargs rg -n "all-features|no-default-features|features"
53-54: Verify rstest-bdd 0.3.0 compatibility.The upgrade from rstest-bdd 0.2.0 to 0.3.0 may introduce breaking changes affecting test scenarios. Confirm that all existing BDD tests compile and pass with this version.
Search the web for rstest-bdd 0.3.0 release notes to identify any breaking changes:
rstest-bdd 0.3.0 release notes breaking changeswildside-cli/src/tests/helpers.rs (1)
4-5: Feature gating is correctly applied to test helpers.All new store-sqlite-dependent helpers are consistently gated behind
#[cfg(feature = "store-sqlite")]. This includes the base64 import (lines 4-5),read_utf8(lines 20-24), and the fixture helpers (lines 135-165).The use of
.expect()throughout these functions is acceptable in test code as per the coding guidelines.Also applies to: 20-24, 135-165
wildside-cli/src/tests/mod.rs (1)
9-10: Module declarations are correctly ungated.The new modules
feature_flag_stepsandfeature_flagsare declared without feature gates at the module level. This is appropriate if these modules test behaviour when features are DISABLED (testingMissingFeatureerror paths). The modules themselves must handle any necessary internal feature gating.Verify that
feature_flag_steps.rsandfeature_flags.rsapply appropriate#[cfg]gating internally for any feature-dependent test code:#!/bin/bash # Description: Check feature gating in new test modules # Look for cfg attributes in the new test modules rg -n "cfg\(feature" wildside-cli/src/tests/feature_flag*.rswildside-cli/src/tests/pipeline.rs (1)
3-4: Module-level feature gating is correctly applied.Adding
#![cfg(feature = "store-sqlite")]at line 3 appropriately gates the entire test module, which depends onSqlitePoiStorethroughout. This prevents compilation errors when the feature is disabled and aligns with rstest-bdd's compile-time scenario resolution requirements.The blank line at line 4 improves readability between the attribute and imports.
wildside-cli/src/tests/pipeline_steps.rs (1)
3-4: Feature gating is consistent and correct for rstest-bdd scenarios.The module-level
#![cfg(feature = "store-sqlite")]at line 3 correctly gates rstest-bdd scenarios that depend onSqlitePoiStore. This matches the gating approach inpipeline.rsand is necessary because rstest-bdd processes scenarios at compile time—the entire module must be excluded when the feature is unavailable.wildside-core/tests/sqlite_poi_store_behaviour.rs (1)
3-4: Feature gating correctly applied for rstest-bdd scenarios.The module-level
#[cfg(feature = "store-sqlite")]attribute ensures the test file compiles only when the feature is enabled. This aligns with rstest-bdd's compile-time scenario resolution and avoids build failures when the feature is disabled.wildside-cli/tests/features/feature_flags.feature (1)
1-6: BDD scenario correctly defines feature-gating behaviour.The scenario structure follows proper Gherkin syntax and clearly documents the expected behaviour when the store-sqlite feature is disabled. The steps map to implementations in the feature_flag_steps module.
docs/wildside-engine-design.md (1)
921-939: Feature flag documentation accurately reflects implementation.The feature flag section correctly documents:
- Default features (solver-vrp, store-sqlite, serde)
- Individual feature purposes and behaviour
- Preference handling for multiple solver features
- Future feature roadmap
The content is well-structured and provides clear guidance for consumers selecting feature combinations.
wildside-core/src/lib.rs (1)
21-23: LGTM!The feature-gated re-exports are correctly structured.
PoiStoreremains unconditionally available whilstSqlitePoiStoreandSqlitePoiStoreErrorare appropriately gated behindstore-sqlite. This aligns with the API design guidance to avoid feature-gating public trait methods whilst gating concrete implementations.wildside-cli/src/error.rs (2)
35-40: LGTM!The
MissingFeaturevariant is well-designed with clear, actionable error messaging. Keeping it unconditionally compiled ensures graceful degradation when features are disabled.
92-98: Feature gating is correctly applied.The
WriteSpatialIndexandOpenPoiStorevariants are appropriately gated behindstore-sqlite, ensuring the error enum compiles cleanly regardless of feature configuration.Also applies to: 121-123
wildside-core/src/store/mod.rs (3)
1-10: LGTM!Module documentation clearly explains purpose and utility. The feature-gated submodule structure is well-organised.
21-75: Well-documented trait with comprehensive examples.The
PoiStoretrait documentation is thorough, including coordinate system conventions, antimeridian caveats, and a working example. The trait remains unconditionally available whilst concrete implementations are feature-gated—this is the correct approach.
77-126: Excellent parameterised test coverage.The boundary condition tests using
#[rstest(...)]parameterised cases are well-structured, covering edges, corners, and just-outside scenarios. This aligns with the coding guidelines for using rstest fixtures and parameterised cases.src/lib.rs (3)
1-6: LGTM!Module documentation is clear and
#![forbid(unsafe_code)]appropriately enforces safety at the crate level.
13-20: Feature-gated re-exports are correctly structured.The conditional re-exports for
SqlitePoiStore,VrpSolver, andOrtoolsSolverfollow the established pattern. Consumers can depend on the facade and selectively enable backends via feature flags.
8-11: Add missing re-exports fromwildside_core.The facade omits
ScorerandSolveRequestValidationError, both of which are exported bywildside_core. Include these in the re-export block at lines 8–11 alongside the existing types.wildside-solver-ortools/src/lib.rs (1)
1-7: LGTM!Module documentation clearly states this is a placeholder and
#![forbid(unsafe_code)]is appropriate.wildside-core/src/store/spatial_index.rs (4)
1-5: LGTM!Module documentation clearly explains the purpose and relationship to SQLite-backed POI store.
32-69: Well-structured error types.Error variants are comprehensive with clear messages and proper source attribution. The distinction between read errors (
SpatialIndexError) and write errors (SpatialIndexWriteError) is appropriate.
181-199: Test fixtures use.expect()appropriately.The use of
.expect()in test fixtures is acceptable per coding guidelines (allow-expect-in-tests = true). The messages are clear.
283-295: Remove this test or expand it with additional assertions.The test
write_index_persists_spatial_index_fileadds no value—load_index_entries_round_trips_entriesalready verifies round-trip serialisation and format compatibility. Both tests call the samewrite_indexfunction and validate the same data structure. Either consolidate these checks or remove the redundant test.wildside-cli/src/tests/feature_flag_steps.rs (3)
1-11: LGTM!Module-level feature gate
#![cfg(not(feature = "store-sqlite"))]correctly ensures these tests only compile whenstore-sqliteis disabled, which is necessary because rstest-bdd scenarios are compile-time resolved.
13-52: Test world implementation is sound.
RefCellusage for interior mutability in the test world is appropriate for BDD-style tests where steps mutate shared state. The.expect()calls are acceptable within test code.
82-97: Assertion logic is correct.The
thenstep properly validates thatMissingFeatureis returned with the expectedfeatureandactionvalues. Using pattern matching with assertions inside is clear and idiomatic.wildside-core/src/test_support.rs (4)
7-24: LGTM!Feature-gated imports are correctly scoped. The combined
#[cfg(all(any(test, feature = "test-support"), feature = "store-sqlite"))]gates ensure sqlite-related utilities compile only when both conditions are met.
26-65: LGTM!
MemoryStoreremains unconditionally available for tests, providing a simple in-memoryPoiStoreimplementation. The linear-scan approach is appropriate for small test datasets.
67-101: LGTM!The sqlite test utilities are now correctly gated behind both
test-supportandstore-sqlitefeatures, ensuring they compile only when the underlying sqlite store functionality is available.
103-145: LGTM!
UnitTravelTimeProviderandTagScorerremain gated only bytest-support, which is correct as they have no sqlite dependencies.wildside-cli/src/lib.rs (5)
4-22: LGTM!Feature-gated imports are correctly aligned with their usage in conditionally compiled functions. The unconditional
OsmIngestSummaryimport supports the always-compiledIngestOutcomestruct.
74-107: LGTM!The
execute_ingestimplementation understore-sqlitecorrectly orchestrates the ingestion pipeline: OSM PBF parsing, POI persistence, Wikidata claim extraction, and spatial index writing. Error handling provides appropriate context for each failure point.
109-115: LGTM!The fallback
execute_ingestgracefully fails with a descriptiveMissingFeatureerror whenstore-sqliteis disabled, guiding users to enable the required feature.
117-148: LGTM!Helper functions are correctly feature-gated. The bz2 detection by extension and conditional decompression wrapping is appropriate for the use case.
200-258: LGTM!
IngestConfigandIngestOutcomeare correctly unconditional, allowing CLI parsing to succeed before feature-gated execution paths are invoked.wildside-cli/src/solve.rs (5)
8-24: LGTM!Import gating is correctly structured. The
OrtoolsSolverimport appropriately excludes the VRP feature to prevent conflicts, andHttpTravelTimeProviderConfigremains unconditional for use in configuration parsing.
161-178: LGTM!The
store-sqliteimplementation correctly wires up the store, scorer, and travel time provider, then delegates solver construction tobuild_solver_with_featuresfor feature-based selection.
180-188: LGTM!The fallback implementation returns a descriptive
MissingFeatureerror whenstore-sqliteis disabled.
190-226: LGTM!The
build_solver_with_featuresfunction correctly implements solver priority: VRP when available, OR-Tools as fallback, and a clear error when neither is enabled. The feature gate combinations ensure only one implementation compiles per feature set.
300-330: Tests verify compile-time feature selection.These tests serve as compile-time assertions that the feature gate logic is correctly configured. Whilst somewhat tautological (defining a constant then asserting its value), they document the expected solver priority and ensure each feature combination compiles.
wildside-core/src/store/sqlite.rs (9)
23-56: LGTM!The
SqlitePoiStoreErrorenum provides comprehensive error variants with clear messages and proper source attribution usingthiserror.
58-69: LGTM!
SqlitePoiStorecorrectly holds only the in-memory R-tree, and theDebugimplementation provides useful diagnostics without exposing internals.
71-96: LGTM!The
openmethod correctly opens the database read-only, loads the spatial index, and validates that all indexed POIs exist in the database. The R-tree is bulk-loaded for efficient spatial queries.
98-115: LGTM!The
PoiStoreimplementation correctly queries the R-tree and returns results sorted by ID for deterministic ordering.
117-131: LGTM!The
find_missing_poi_in_chunkfunction correctly identifies missing POIs using binary search. The caller deduplicates IDs beforehand, ensuring correct behaviour.
133-154: LGTM!The validation logic correctly handles ID deduplication and chunked queries to stay within SQLite's parameter limits.
156-159: LGTM!Using the conservative 999-parameter limit ensures compatibility across SQLite versions. The signature allows future enhancement to query runtime limits.
161-190: LGTM!The chunked POI loading correctly builds parameterised queries, parses JSON tags with proper error context, and returns sorted results for efficient lookup.
192-367: LGTM!Comprehensive test coverage including happy paths and error scenarios. The use of
rstestfixtures provides clean test setup, and error cases validate allSqlitePoiStoreErrorvariants.
…ctions The monolithic `wildside-engine-design.md` document has been restructured into multiple smaller markdown files, each representing a distinct section of the design. This improves maintainability and readability by keeping files under 400 lines, and allows sequential reading from introduction through the architecture, scoring, solver, implementation strategy, and works cited. The new files include: - 01-introduction.md - 02-data-foundation.md - 03-scoring-personalization.md - 04-library-architecture.md - 05-solver-implementation.md - 06-implementation-testing-deployment.md - 99-works-cited.md This also establishes a clear document structure and facilitates easier updates and navigation for contributors and reviewers. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
leynos
changed the title
There was a problem hiding this comment.
Actionable comments posted: 29
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
docs/wildside-engine-design.mddocs/wildside-engine-design/01-introduction.mddocs/wildside-engine-design/02-data-foundation.mddocs/wildside-engine-design/03-scoring-personalization.mddocs/wildside-engine-design/04-library-architecture.mddocs/wildside-engine-design/05-solver-implementation.mddocs/wildside-engine-design/06-implementation-testing-deployment.mddocs/wildside-engine-design/99-works-cited.md
🧰 Additional context used
📓 Path-based instructions (4)
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: Use the markdown files within thedocs/directory as a knowledge base and source of truth for project requirements, dependency choices, and architectural decisions.
When new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve, proactively update the relevant file(s) in thedocs/directory to reflect the latest state. Ensure the documentation remains accurate and current.
Documentation must use en-GB-oxendict spelling and grammar ("-ize" / "-yse" / "-our") with the exception of the LICENSE filename.
docs/**/*.md: Use British English based on the Oxford English Dictionary locale en-GB, including -ize suffixes (e.g., realize, organization), -lyse suffixes (e.g., analyse, paralyse), -our suffixes (e.g., colour, behaviour), -re suffixes (e.g., centre, calibre), double 'l' (e.g., cancelled, counsellor), maintained 'e' (e.g., likeable, rateable), -ogue suffixes (e.g., analogue, catalogue)
Use the Oxford comma in documentation: 'ships, planes, and hovercraft' where it aids comprehension
Treat company names as collective nouns, for example: 'Concordat Industries are expanding'
Write headings in sentence case
Use Markdown headings (#, ##, ###, and so on) in order without skipping levels
Follow markdownlint recommendations
Provide code blocks and lists using standard Markdown syntax
Always provide a language identifier for fenced code blocks; use 'plaintext' for non-code text
Use '-' as the first level bullet and renumber lists when items change
Prefer inline links using 'text' or angle brackets around the URL
Ensure blank lines before and after bulleted lists and fenced blocks
Ensure tables have a delimiter line below the header row
Expand any uncommon acronym on first use, for example, Continuous Integration (CI)
Wrap paragraphs at 80 columns
Do not wrap tables
Use GitHub-flavoured numeric footnotes referenced as '[^1]'
Footnotes must be numbered in order of appearance in the document
Caption ev...
Files:
docs/wildside-engine-design/04-library-architecture.mddocs/wildside-engine-design/05-solver-implementation.mddocs/wildside-engine-design/06-implementation-testing-deployment.mddocs/wildside-engine-design/01-introduction.mddocs/wildside-engine-design/03-scoring-personalization.mddocs/wildside-engine-design/02-data-foundation.mddocs/wildside-engine-design/99-works-cited.mddocs/wildside-engine-design.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Validate Markdown files usingmake markdownlint.
Runmake fmtafter any documentation changes to format all Markdown files and fix table markup.
Validate Mermaid diagrams in Markdown files by runningmake nixie.
Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks in Markdown must be wrapped at 120 columns.
Markdown tables and headings must not be wrapped.
Use dashes (-) for list bullets in Markdown.
Use GitHub-flavoured Markdown footnotes ([^1]) for references and footnotes.
Files:
docs/wildside-engine-design/04-library-architecture.mddocs/wildside-engine-design/05-solver-implementation.mddocs/wildside-engine-design/06-implementation-testing-deployment.mddocs/wildside-engine-design/01-introduction.mddocs/wildside-engine-design/03-scoring-personalization.mddocs/wildside-engine-design/02-data-foundation.mddocs/wildside-engine-design/99-works-cited.mddocs/wildside-engine-design.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/wildside-engine-design/04-library-architecture.mddocs/wildside-engine-design/05-solver-implementation.mddocs/wildside-engine-design/06-implementation-testing-deployment.mddocs/wildside-engine-design/01-introduction.mddocs/wildside-engine-design/03-scoring-personalization.mddocs/wildside-engine-design/02-data-foundation.mddocs/wildside-engine-design/99-works-cited.mddocs/wildside-engine-design.md
docs/**/!(README).md
📄 CodeRabbit inference engine (docs/documentation-style-guide.md)
Avoid first and second person personal pronouns outside the README.md file
Files:
docs/wildside-engine-design/04-library-architecture.mddocs/wildside-engine-design/05-solver-implementation.mddocs/wildside-engine-design/06-implementation-testing-deployment.mddocs/wildside-engine-design/01-introduction.mddocs/wildside-engine-design/03-scoring-personalization.mddocs/wildside-engine-design/02-data-foundation.mddocs/wildside-engine-design/99-works-cited.mddocs/wildside-engine-design.md
docs/**/*.{md,rs}
📄 CodeRabbit inference engine (docs/documentation-style-guide.md)
Wrap code at 120 columns
Files:
docs/wildside-engine-design/04-library-architecture.mddocs/wildside-engine-design/05-solver-implementation.mddocs/wildside-engine-design/06-implementation-testing-deployment.mddocs/wildside-engine-design/01-introduction.mddocs/wildside-engine-design/03-scoring-personalization.mddocs/wildside-engine-design/02-data-foundation.mddocs/wildside-engine-design/99-works-cited.mddocs/wildside-engine-design.md
🪛 LanguageTool
docs/wildside-engine-design/04-library-architecture.md
[uncategorized] ~26-~26: Loose punctuation mark.
Context: ...rates when necessary. - wildside-core: This crate is the heart of the engine. ...
(UNLIKELY_OPENING_PUNCTUATION)
[misspelling] ~38-~38: Did you mean “side effect” (=adverse effect, unintended consequence)? Open compounds are not hyphenated.
Context: ...`. - This crate is deterministic and side-effect free, making it easy to test rigoro...
(AFFECT_EFFECT)
[uncategorized] ~41-~41: Loose punctuation mark.
Context: ... testing and fuzzing. - wildside-data: Contains the ETL logic and data adapter...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~62-~62: Loose punctuation mark.
Context: ...ge the C++ dependency. - wildside-cli: A small command-line application for op...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~64-~64: Loose punctuation mark.
Context: ...ion for operational tasks. - ingest: Runs the full ETL pipeline from `wildsi...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~79-~79: Loose punctuation mark.
Context: ...obal popularity scores. - solve: Runs the route solver from the command ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~148-~148: Loose punctuation mark.
Context: ...zed, read-only artefacts: - pois.db: An SQLite (or RocksDB) file containing ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~152-~152: Loose punctuation mark.
Context: ...n a major release. - pois.rstar: A serialized R*-tree file for fast spa...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~156-~156: Loose punctuation mark.
Context: ...ross 0.x releases. - popularity.bin: A compact binary file of pre-calculated...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~164-~164: Possible missing comma found.
Context: ...rry a wikidata tag, the ETL is skipped but the claims schema is still initialized ...
(AI_HYDRA_LEO_MISSING_COMMA)
[misspelling] ~182-~182: Did you mean “side effect” (=adverse effect, unintended consequence)? Open compounds are not hyphenated.
Context: ...ign choice means the engine itself is side-effect free during a request. It allows applic...
(AFFECT_EFFECT)
docs/wildside-engine-design/05-solver-implementation.md
[typographical] ~1-~1: To join two clauses or introduce examples, consider using an em dash.
Context: # Section 4: The Solver Implementation - Abstracted and Replaceable This section...
(DASH_RULE)
[style] ~18-~18: Would you like to use the Oxford spelling “synchronization”? The spelling ‘synchronisation’ is also correct.
Context: ...void shared mutable state or use proper synchronisation to maintain thread safety. This abstrac...
(OXFORD_SPELLING_Z_NOT_S)
[grammar] ~36-~36: Please add a punctuation mark at the end of paragraph.
Context: ...ame.15 This implementation will live in the wildside-solver-vrp crate. ### 4.2....
(PUNCTUATION_PARAGRAPH_END)
[style] ~86-~86: The term ‘absolutely need’ is used quite frequently. Consider using a less common alternative to set your writing apart from others.
Context: ...o opt into this complexity only if they absolutely need it, without burdening the default setup...
(ABSOLUTELY_GREAT_RULE)
[misspelling] ~119-~119: This word is normally spelled as one.
Context: ...st. When called from within an existing multi-threaded Tokio runtime (detected via `Handle::...
(EN_COMPOUNDS_MULTI_THREADED)
docs/wildside-engine-design/06-implementation-testing-deployment.md
[locale-violation] ~11-~11: license must be spelled with a “c” when used as a noun in British English. Use “licence”.
Context: ...e licensed under the permissive ISC license, satisfying the project's legal requi...
(LICENCE_LICENSE_NOUN_SINGULAR)
[uncategorized] ~19-~19: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...e Flags:** The engine uses feature flags so consumers can select the solver and s...
(COMMA_COMPOUND_SENTENCE_2)
[uncategorized] ~23-~23: Loose punctuation mark.
Context: ...["solver-vrp", "store-sqlite", "serde"]`: The default build includes the nati...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~27-~27: Loose punctuation mark.
Context: ...equest/response types. - solver-vrp: Enables the native Rust solver backed b...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~30-~30: Loose punctuation mark.
Context: ...tures are enabled. - solver-ortools: Enables the optional OR-Tools solver ba...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~33-~33: Loose punctuation mark.
Context: ...T integration lands. - store-sqlite: Enables the SQLite-backed POI store and...
(UNLIKELY_OPENING_PUNCTUATION)
[style] ~69-~69: This word has been used in one of the immediately preceding sentences. Using a synonym could make your text more interesting to read, unless the repetition is intentional.
Context: ...ed as a JSON file containing a complete problem instance and its expected solution. The...
(EN_REPEATEDWORDS_PROBLEM)
[uncategorized] ~72-~72: Loose punctuation mark.
Context: ...solution. The schema captures: - pois: Array of POIs with id, coordinates, and...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~73-~73: Loose punctuation mark.
Context: ...and tags. - travel_time_matrix_seconds: Integer matrix of travel times (avoids ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~75-~75: Loose punctuation mark.
Context: ...s float precision issues). - request: A complete SolveRequest specification...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~77-~77: Loose punctuation mark.
Context: ...ed, and optional max_nodes. - expected: The expected route POI IDs in order, sc...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~82-~82: Loose punctuation mark.
Context: ...udes: - FixedMatrixTravelTimeProvider: A TravelTimeProvider implementation t...
(UNLIKELY_OPENING_PUNCTUATION)
[style] ~86-~86: Would you like to use the Oxford spelling “parameterized”? The spelling ‘parameterised’ is also correct.
Context: ...xternal routing dependencies. - rstest parameterised unit tests (golden_routes.rs): A sing...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~87-~87: This word has been used in one of the immediately preceding sentences. Using a synonym could make your text more interesting to read, unless the repetition is intentional.
Context: ...ates over all JSON fixtures, loads each problem instance, constructs the solver with ...
(EN_REPEATEDWORDS_PROBLEM)
[style] ~144-~144: Since ownership is already implied, this phrasing may be redundant.
Context: ...main application, it can be promoted to its own repository with no code churn, as the b...
(PRP_OWN)
docs/wildside-engine-design/01-introduction.md
[style] ~16-~16: This word has been used in one of the immediately preceding sentences. Using a synonym could make your text more interesting to read, unless the repetition is intentional.
Context: ...ithms, and solving NP-hard optimization problems all align with the language's strengths...
(EN_REPEATEDWORDS_PROBLEM)
[uncategorized] ~65-~65: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...`, and tag map) directly in the R*-tree so lookups can avoid secondary hash-map pr...
(COMMA_COMPOUND_SENTENCE_2)
[uncategorized] ~66-~66: Possible missing comma found.
Context: ...secondary hash-map probes. During start-up the store reads these entries, valida...
(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~85-~85: Would you like to use the Oxford spelling “normalized”? The spelling ‘normalised’ is also correct.
Context: ...non-negative values. Scores SHOULD be normalised to the [0.0, 1.0] range unless explic...
(OXFORD_SPELLING_Z_NOT_S)
[uncategorized] ~148-~148: Possible missing comma found.
Context: ...operate exclusively on these types. At runtime the SqlitePoiStore provides the fast-...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~152-~152: Possible missing comma found.
Context: ...malformed tag payloads. Once validation succeeds the database connection is released, an...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~174-~174: Possible missing comma found.
Context: ... the application queries the SQLite POI store which uses an R-tree to locate points w...
(AI_HYDRA_LEO_MISSING_COMMA)
docs/wildside-engine-design/03-scoring-personalization.md
[typographical] ~86-~86: Two consecutive dots
Context: ...ching themes and clamps the result to 0.0..=1.0. Combining popularity and relevanc...
(DOUBLE_PUNCTUATION)
docs/wildside-engine-design/02-data-foundation.md
[typographical] ~1-~1: To join two clauses or introduce examples, consider using an em dash.
Context: # Section 1: The Data Foundation - Ingesting and Integrating Open Data The...
(DASH_RULE)
[grammar] ~30-~30: Please add a punctuation mark at the end of paragraph.
Context: ...quence of independent "blobs," a design that osmpbf leverages to process these bl...
(PUNCTUATION_PARAGRAPH_END)
[locale-violation] ~38-~38: licenses must be spelled with a “c” when used as a noun in British English. Use “licences”.
Context: ...l-licensed under the Apache-2.0 and MIT licenses, which are standard, permissive license...
(LICENCE_LICENSE_NOUN_PLURAL)
[locale-violation] ~39-~39: licenses must be spelled with a “c” when used as a noun in British English. Use “licences”.
Context: ...icenses, which are standard, permissive licenses suitable for commercial software develo...
(LICENCE_LICENSE_NOUN_PLURAL)
[style] ~45-~45: This expression can be considered offensive.
Context: ...rate is licensed under the "Do What The Fuck You Want To Public License, Version 2" (WTF...
(PROFANITY)
[locale-violation] ~46-~46: License must be spelled with a “c” when used as a noun in British English. Use “licence”.
Context: ...he "Do What The Fuck You Want To Public License, Version 2" (WTFPLv2).3 While extremely...
(LICENCE_LICENSE_NOUN_SINGULAR)
[locale-violation] ~58-~58: License must be spelled with a “c” when used as a noun in British English. Use “licence”.
Context: ...Parser Crates | Crate Name | Primary License | Key Technical Features ...
(LICENCE_LICENSE_NOUN_SINGULAR)
[locale-violation] ~61-~61: license must be spelled with a “c” when used as a noun in British English. Use “licence”.
Context: ...hs ago. | Not Recommended. The WTFPLv2 license introduces unnecessary legal risk for a...
(LICENCE_LICENSE_NOUN_SINGULAR)
[style] ~72-~72: The serial comma (Oxford comma, Harvard comma) is missing.
Context: ...Reader::par_map_reduceto count nodes, ways and relations while computing ageo::Rect`...
(SERIAL_COMMA_ON)
[uncategorized] ~88-~88: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...resolved after both passes emit warnings so operators can investigate fixture gaps ...
(COMMA_COMPOUND_SENTENCE_2)
[grammar] ~102-~102: Please add a punctuation mark at the end of paragraph.
Context: ...built tool for this task.5 For storage, the wd2sql tool provides an excellent te...
(PUNCTUATION_PARAGRAPH_END)
[uncategorized] ~125-~125: The preposition ‘to’ seems more likely in this position.
Context: ...ous reqwest client, issues requests on the Tokio runtime, and always sends a d...
(AI_HYDRA_LEO_REPLACE_ON_TO)
[style] ~163-~163: You have used the passive voice repeatedly in nearby sentences. To make your writing clearer and easier to read, consider using active voice.
Context: ... referenced by the PoiEntityLinks set are processed further. For those entities, the parser...
(REP_PASSIVE_VOICE)
[uncategorized] ~219-~219: Do not mix variants of the same word (‘initialisation’ and ‘initialization’) within a single text.
Context: ...e_data::wikidata::storemodule. Schema initialisation is handled by theinitialise_schema` f...
(EN_WORD_COHERENCY)
[style] ~220-~220: Would you like to use the Oxford spelling “initialize”? The spelling ‘initialise’ is also correct.
Context: ...Schema initialisation is handled by the initialise_schema function, which enables foreign...
(OXFORD_SPELLING_Z_NOT_S)
[uncategorized] ~221-~221: Do not mix variants of the same word (‘normalise’ and ‘normalize’) within a single text.
Context: ...reign keys and creates a compact set of normalised tables: - wikidata_entities contains...
(EN_WORD_COHERENCY)
[uncategorized] ~232-~232: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ... poi_wikidata_claims joins both tables so the scoring pipeline can resolve a POI'...
(COMMA_COMPOUND_SENTENCE_2)
[uncategorized] ~244-~244: Possible missing preposition found.
Context: ... thousands of queries per user request. Will be rate-limited and result in extreme l...
(AI_HYDRA_LEO_MISSING_IT)
[grammar] ~273-~273: Please add a punctuation mark at the end of paragraph.
Context: ... to work seamlessly with the types from the geo crate. The practical implementat...
(PUNCTUATION_PARAGRAPH_END)
docs/wildside-engine-design/99-works-cited.md
[typographical] ~3-~3: To join two clauses or introduce examples, consider using an em dash.
Context: ...ks cited 1. Rust (programming language) - Wikipedia, accessed on August 13, 2025, ...
(DASH_RULE)
[inconsistency] ~3-~3: A new year has begun. Did you mean “August 13, 2026”?
Context: ...ming language) - Wikipedia, accessed on August 13, 2025, <https://en.wikipedia.org/wiki/Rust...
(DATE_NEW_YEAR)
[typographical] ~6-~6: To join two clauses or introduce examples, consider using an em dash.
Context: ...Rust_(programming_language)> 2. osmpbf - crates.io: Rust Pack...
(DASH_RULE)
[inconsistency] ~7-~7: A new year has begun. Did you mean “August 13, 2026”?
Context: ...: Rust Package Registry, accessed on August 13, 2025, https://crates.io/crates/osmpbf 3. ...
(DATE_NEW_YEAR)
[typographical] ~9-~9: To join two clauses or introduce examples, consider using an em dash.
Context: ...rates.io/crates/osmpbf> 3. osmpbfreader - crates.io: Rust Pack...
(DASH_RULE)
[inconsistency] ~10-~10: A new year has begun. Did you mean “August 13, 2026”?
Context: ...: Rust Package Registry, accessed on August 13, 2025, <https://crates.io/crates/osmpbfreader...
(DATE_NEW_YEAR)
[typographical] ~12-~12: To join two clauses or introduce examples, consider using an em dash.
Context: ...//crates.io/crates/osmpbfreader> 4. osm - Keywords - crates.io...
(DASH_RULE)
[typographical] ~12-~12: To join two clauses or introduce examples, consider using an em dash.
Context: .../crates/osmpbfreader> 4. osm - Keywords - crates.io: Rust Pack...
(DASH_RULE)
[inconsistency] ~13-~13: A new year has begun. Did you mean “August 13, 2026”?
Context: ...: Rust Package Registry, accessed on August 13, 2025, https://crates.io/keywords/osm 5. r...
(DATE_NEW_YEAR)
[typographical] ~15-~15: To join two clauses or introduce examples, consider using an em dash.
Context: ...at/vrp: A Vehicle Routing Problem solver - GitHub, accessed on August 13, 2025, ...
(DASH_RULE)
[inconsistency] ~16-~16: A new year has begun. Did you mean “August 13, 2026”?
Context: ...Problem solver - GitHub, accessed on August 13, 2025, <https://github.com/reinterpretcat/vrp...
(DATE_NEW_YEAR)
[typographical] ~18-~18: To join two clauses or introduce examples, consider using an em dash.
Context: ...retcat/vrp> 6. CP-SAT Solver | OR-Tools - Google for Developers, accessed on Augus...
(DASH_RULE)
[inconsistency] ~18-~18: A new year has begun. Did you mean “August 13, 2026”?
Context: ...ls - Google for Developers, accessed on August 13, 2025, <https://developers.google.com/optimiz...
(DATE_NEW_YEAR)
[typographical] ~21-~21: To join two clauses or introduce examples, consider using an em dash.
Context: ...om/optimization/cp/cp_solver> 7. cp_sat - Rust - Docs.rs, access...
(DASH_RULE)
[typographical] ~21-~21: To join two clauses or introduce examples, consider using an em dash.
Context: ...mization/cp/cp_solver> 7. cp_sat - Rust - Docs.rs, accessed on A...
(DASH_RULE)
[inconsistency] ~21-~21: A new year has begun. Did you mean “August 13, 2026”?
Context: ... Docs.rs, accessed on August 13, 2025, https://docs.rs/cp_sat 8. How to...
(DATE_NEW_YEAR)
[typographical] ~24-~24: Consider using an em dash in dialogues and enumerations.
Context: ... to use rust system binding API crates? - Reddit, accessed on August 13, 2025,...
(DASH_RULE)
[inconsistency] ~25-~25: A new year has begun. Did you mean “August 13, 2026”?
Context: ...ng API crates? - Reddit, accessed on August 13, 2025, <https://www.reddit.com/r/rust/comm...
(DATE_NEW_YEAR)
[typographical] ~28-~28: To join two clauses or introduce examples, consider using an em dash.
Context: ...system_binding_api_crates/> 9. wikidata - Rust - Docs.rs, access...
(DASH_RULE)
[typographical] ~28-~28: To join two clauses or introduce examples, consider using an em dash.
Context: ...binding_api_crates/> 9. wikidata - Rust - Docs.rs, accessed on A...
(DASH_RULE)
[inconsistency] ~28-~28: A new year has begun. Did you mean “August 13, 2026”?
Context: ... Docs.rs, accessed on August 13, 2025, https://docs.rs/wikidata 10. Geo...
(DATE_NEW_YEAR)
[typographical] ~31-~31: To join two clauses or introduce examples, consider using an em dash.
Context: ...ttps://docs.rs/wikidata> 10. Geospatial - Categories - [crates.io](http://crates.i...
(DASH_RULE)
[typographical] ~31-~31: To join two clauses or introduce examples, consider using an em dash.
Context: ...s/wikidata> 10. Geospatial - Categories - crates.io: Rust Pack...
(DASH_RULE)
[inconsistency] ~32-~32: A new year has begun. Did you mean “August 13, 2026”?
Context: ... Rust Package Registry, accessed on August 13, 2025, <https://crates.io/categories/scie...
(DATE_NEW_YEAR)
[typographical] ~35-~35: To join two clauses or introduce examples, consider using an em dash.
Context: ...o/categories/science::geo> 11. vrp_core - Rust - Docs.rs, access...
(DASH_RULE)
[typographical] ~35-~35: To join two clauses or introduce examples, consider using an em dash.
Context: ...ories/science::geo> 11. vrp_core - Rust - Docs.rs, accessed on A...
(DASH_RULE)
[inconsistency] ~35-~35: A new year has begun. Did you mean “August 13, 2026”?
Context: ... Docs.rs, accessed on August 13, 2025, <https://docs.rs/vrp-core/latest/v...
(DATE_NEW_YEAR)
[inconsistency] ~39-~39: A new year has begun. Did you mean “August 13, 2026”?
Context: ... Problems with Profits, accessed on August 13, 2025, <https://www.researchgate.net/publ...
(DATE_NEW_YEAR)
[typographical] ~42-~42: To join two clauses or introduce examples, consider using an em dash.
Context: ...ing_Problems_with_Profits> 13. vrp_core - Rust - Docs.rs, access...
(DASH_RULE)
[typographical] ~42-~42: To join two clauses or introduce examples, consider using an em dash.
Context: ...blems_with_Profits> 13. vrp_core - Rust - Docs.rs, accessed on A...
(DASH_RULE)
[inconsistency] ~42-~42: A new year has begun. Did you mean “August 13, 2026”?
Context: ... Docs.rs, accessed on August 13, 2025, https://docs.rs/vrp-core 14. VR...
(DATE_NEW_YEAR)
[typographical] ~45-~45: To join two clauses or introduce examples, consider using an em dash.
Context: ... https://docs.rs/vrp-core 14. VRP - Vehicle Routing Problem; ChatGPT-augment...
(DASH_RULE)
[typographical] ~45-~45: To join two clauses or introduce examples, consider using an em dash.
Context: ...hicle Routing Problem; ChatGPT-augmented - Stock/Inventory - Frappe Forum, acce...
(DASH_RULE)
[typographical] ~45-~45: To join two clauses or introduce examples, consider using an em dash.
Context: ...lem; ChatGPT-augmented - Stock/Inventory - Frappe Forum, accessed on August 13,...
(DASH_RULE)
[inconsistency] ~46-~46: A new year has begun. Did you mean “August 13, 2026”?
Context: ...ventory - Frappe Forum, accessed on August 13, 2025, <https://discuss.frappe.io/t/vrp-v...
(DATE_NEW_YEAR)
[typographical] ~49-~49: To join two clauses or introduce examples, consider using an em dash.
Context: ... Solving vehicle routing problem in Java - SoftwareMill, accessed on August 13,...
(DASH_RULE)
[inconsistency] ~49-~49: A new year has begun. Did you mean “August 13, 2026”?
Context: ...lem in Java - SoftwareMill, accessed on August 13, 2025, <https://softwaremill.com/solving-...
(DATE_NEW_YEAR)
[inconsistency] ~53-~53: A new year has begun. Did you mean “August 13, 2026”?
Context: ...// Lib.rs, accessed on August 13, 2025, https://lib.rs/crates/cp_sat
(DATE_NEW_YEAR)
docs/wildside-engine-design.md
[typographical] ~9-~9: To join two clauses or introduce examples, consider using an em dash.
Context: ...on.md) - [Section 1: The Data Foundation - Ingesting and Integrating Open Data](w...
(DASH_RULE)
[typographical] ~15-~15: To join two clauses or introduce examples, consider using an em dash.
Context: ... - [Section 4: The Solver Implementation - Abstracted and Replaceable](wildside-e...
(DASH_RULE)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Sourcery review
- GitHub Check: build
🔇 Additional comments (10)
docs/wildside-engine-design/03-scoring-personalization.md (1)
1-88: Documentation is well-structured and accurate.The scoring and personalisation design is clearly articulated. The offline/online boundary and data-artefact strategy align with the library-first architecture and feature-gated components described elsewhere in the design documentation.
docs/wildside-engine-design/99-works-cited.md (1)
53-54: Clarify the final bibliography entry: is it a duplicate or entry 16?The last entry ("CP-SAT — Rust math library // Lib.rs") is not numbered and may be a duplicate of entry 7 (cp_sat – Rust – Docs.rs). Confirm whether this should be:
- Removed as a duplicate.
- Numbered as entry 16 if it is a distinct source.
- Merged with entry 7 if it references the same resource.
docs/wildside-engine-design/01-introduction.md (6)
156-172: Validate sequence diagram and clarify notation.The first sequence diagram (lines 156–172) correctly illustrates the startup flow. Ensure the notation is validated with
make nixie.
177-190: Validate second sequence diagram syntax.The second sequence diagram (lines 177–190) correctly shows the query flow. Validate via
make nixie.
1-29: Introduction section complies with documentation standards.The introductory paragraphs are well-structured, employ en-GB-oxendict spelling correctly ("realised", "emphasise" would be consistent, though "emphasis" is neutral), avoid first/second person pronouns, and establish clear narrative direction. The section sets appropriate context for the Rust-based design and aligns with the library-first architecture described in the PR objectives.
30-93: Core domain model list is clear and comprehensive.The enumeration of
PointOfInterest,Theme,InterestProfile,Route,PoiStore,SqlitePoiStore,TravelTimeProvider,Scorer, and test utilities is well-organised. Inline links to source files and clear descriptions of responsibilities and constraints (e.g., Scorer determinism and finiteness guarantees) provide excellent guidance for implementers.
95-105: Spatial indexing section clearly explains design rationale.The explanation of
SpatialIndex, its wrapping ofrstar::RTree, and thebuild_spatial_indexhelper is lucid and well-motivated. The narrative about avoiding coupling by exposing read-only helpers aligns with library-first principles.
50-76: Verify link targets are valid and accessible.Lines 56, 64, and subsequent documentation references link to crate internals (e.g.,
../../wildside-core/src/store.rs). Ensure these relative paths resolve correctly from the published documentation context and that all referenced modules and types exist.Verification approach
Confirm that:
- The file
wildside-core/src/store.rsexportsPoiStoretrait.- The file
wildside-core/src/poi.rsexportsPointOfInterest.- Documentation builds without broken link warnings via
cargo doc.docs/wildside-engine-design.md (2)
1-2: Document structure is sound and navigation is clear.The restructuring from monolithic design to a focussed, section-based index aligns with documentation best practices. The heading hierarchy (level 1 for title, level 2 for sections) is correct. The bullet list uses dashes as per guidelines.
8-18: All linked section files exist and are complete.The index correctly references and links to all seven section files, each present with substantial content (54–282 lines). All files remain well under the 400-line guideline mentioned in the index. Markdown formatting, headings, and list structure conform to project standards. No issues detected.
- Implement matrix testing for various feature flag combinations in CI - Support testing with default, no-default, all-features, and specific features - Improve CI to ensure compatibility across solver and store feature permutations Additional changes: - Adapt Makefile to accept TEST_FLAGS for feature gating during test runs - Update solve.rs to select solver implementation based on enabled features - Add SolveError::NotImplemented variant for placeholder solver implementations - Refactor spatial index persistence format handling to explicit header+payload - Enhance documentation for feature flags and project licensing terminology - Guard CLI ingest and solve commands behind feature flags - Remove serde from store-sqlite feature to better express dependencies Overall, this enables rigorous testing and conditional compilation support for feature-flagged components to improve code reliability and CI coverage. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
leynos
changed the title
- Wrap ingest-related code and tests with #[cfg(feature = "store-sqlite")] to enable optional compilation based on the SQLite store feature. - Refactor imports and function definitions to conditionally include functionality only if the feature is enabled. - Remove dummy execute_ingest fallback, consolidating ingest implementation behind feature flag. - Update test modules and helpers with appropriate feature gating to isolate SQLite-dependent tests. This modularizes the ingest CLI to support compilation without SQLite, improving build flexibility. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
leynos
changed the title
There was a problem hiding this comment.
Actionable comments posted: 12
Fix all issues with AI Agents 🤖
In @.github/workflows/ci.yml:
- Line 71: Remove the explicit job dependency on build by deleting the "needs:
build" entry under the feature-matrix job so it can run in parallel with the
build job; only keep or reintroduce "needs: build" if the feature-matrix must be
gated on build's validation, otherwise ensure the feature-matrix job still
checks out code and uses its own rust-cache so it remains independent.
- Around line 75-77: Add an explicit test case for the intended default feature
combination by updating the CI matrix entry named "default-features": set its
test_flags to the explicit string "--no-default-features --features
solver-vrp,store-sqlite" so the workflow runs a job that disables default
features and enables the documented defaults (solver-vrp and store-sqlite),
ensuring the default configuration is verified and documented.
In @docs/wildside-engine-design/01-introduction.md:
- Line 85: Replace the British‑spelled word "normalised" with the en-GB-oxendict
preferred form "normalized" in the sentence fragment containing "Scores SHOULD
be normalised to the `[0.0, 1.0]` range..." so it reads "Scores SHOULD be
normalized to the `[0.0, 1.0]` range unless explicitly..."; update that single
token in the document (file: 01-introduction.md) to maintain the project's -ize
spelling convention.
In @docs/wildside-engine-design/02-data-foundation.md:
- Line 153: Replace the British-variant verb "normalises" with the
en-GB-oxendict-compliant spelling "normalizes" in the phrase that reads
"structure scans the ingested POIs, normalises `wikidata=*` tag values
(handling" so the line becomes "structure scans the ingested POIs, normalizes
`wikidata=*` tag values (handling"; update any other occurrences of "normalises"
in docs/wildside-engine-design/02-data-foundation.md accordingly to maintain
consistent -ize verb spelling.
- Around line 218-219: Change the British-English words in the documentation
sentence so they use the project's en-GB-oxendict -ize spelling: replace
"initialisation" with "initialization" and "normalised" with "normalized" in the
sentence that references the initialise_schema function (do not change the code
identifier `initialise_schema`).
In @docs/wildside-engine-design/05-solver-implementation.md:
- Around line 36-38: The sentence break before the crate reference is awkward
and missing terminal punctuation; edit the paragraph so the crate name
`wildside-solver-vrp` stays on the same line as the sentence and add the
appropriate punctuation (e.g., a period) at the end. Locate the sentence
mentioning the implementation and the crate name and merge the stray newline so
it reads as a single, complete sentence referencing `wildside-solver-vrp`.
In @docs/wildside-engine-design/06-implementation-testing-deployment.md:
- Around line 144-145: The sentence "it can be promoted to its a separate
repository" is malformed; edit the paragraph containing that phrase (the line
starting "different release cadence from the main application, it can be
promoted to its a separate repository") and remove the extraneous "its" so it
reads "...it can be promoted to a separate repository..." ensuring spacing and
punctuation remain correct.
- Line 86: Replace the British spelling "parameterised" with the Oxford/en-GB
"parameterized" in the document text where it appears in the phrase "rstest
parameterised unit tests (`golden_routes.rs`)" so the heading reads "rstest
parameterized unit tests (`golden_routes.rs`)" to conform to the requested -ize
convention.
In @Makefile:
- Line 20: The Makefile passes --features test-support globally but the root
package wildside-engine lacks that feature causing cargo to fail; either add a
test-support feature to the root Cargo.toml that simply re-exports/depends-on
the feature in wildside-core and wildside-solver-vrp, or change the Makefile
invocation so the feature is only enabled for the crates that define it (e.g.,
remove --features test-support from the global nextest call and instead run
nextest per-crate with --manifest-path pointing at wildside-core and
wildside-solver-vrp or use a workspace targeting mechanism that scopes features
to those crates).
In @wildside-core/src/store/sqlite.rs:
- Around line 78-79: The local bindings database_path and index_path are
unnecessarily allocating PathBufs with .as_ref().to_path_buf(); change them to
borrow the input paths instead (e.g., let database_path =
database_path.as_ref(); let index_path = index_path.as_ref();) and update any
subsequent uses to work with &Path rather than owned PathBuf (adjust function
signatures or clones where needed) so you avoid the extra allocations.
- Around line 117-131: The function find_missing_poi_in_chunk can hit a logic
path where the length check indicates a missing ID but the loop finds none;
replace the compile-time-only debug_assert! with a runtime panic by using
unreachable! to ensure this logic error is caught in release builds—update the
end of find_missing_poi_in_chunk to call unreachable!("chunk length mismatch
should reveal missing id") instead of debug_assert! and remove the trailing None
return so the function either returns Some(id) or panics.
- Around line 82-87: Remove the redundant clone of database_path when building
SqlitePoiStoreError::OpenDatabase: call Connection::open_with_flags with the
owned database_path (instead of &database_path) and in the map_err closure
construct the error using path: database_path.to_path_buf() (referencing
database_path, Connection::open_with_flags, and
SqlitePoiStoreError::OpenDatabase) so you don't clone the path for the error
case.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wildside-cli/src/lib.rs (1)
265-272:IngestOutcomeshould be feature-gated.
IngestOutcomeis only constructed within#[cfg(feature = "store-sqlite")]code. Gate the struct to prevent dead code warnings and clarify intent.🔎 Proposed fix
+#[cfg(feature = "store-sqlite")] #[derive(Debug, Clone, PartialEq)] struct IngestOutcome {
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (22)
.github/workflows/ci.ymlMakefiledocs/execplans/phase-4-feature-flags.mddocs/users-guide.mddocs/wildside-engine-design.mddocs/wildside-engine-design/01-introduction.mddocs/wildside-engine-design/02-data-foundation.mddocs/wildside-engine-design/04-library-architecture.mddocs/wildside-engine-design/05-solver-implementation.mddocs/wildside-engine-design/06-implementation-testing-deployment.mdwildside-cli/src/lib.rswildside-cli/src/solve.rswildside-cli/src/tests/feature_flags.rswildside-cli/src/tests/helpers.rswildside-cli/src/tests/steps.rswildside-cli/src/tests/unit.rswildside-core/Cargo.tomlwildside-core/src/solver.rswildside-core/src/store/spatial_index.rswildside-core/src/store/sqlite.rswildside-solver-ortools/src/lib.rswildside-solver-vrp/src/lib.rs
🧰 Additional context used
📓 Path-based instructions (5)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Runmake check-fmt,make lint, andmake testbefore committing in Rust projects.
make check-fmtexecutescargo fmt --workspace -- --checkto validate formatting across the entire workspace without modifying files.
make lintexecutescargo clippy --workspace --all-targets --all-features -- -D warningsto lint every target with all features enabled and deny all Clippy warnings.
make testexecutescargo test --workspaceto run the full workspace test suite.
Clippy warnings MUST be disallowed in Rust code.
Fix any warnings emitted during tests in the Rust code itself rather than silencing them.
Where a Rust function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a Rust function has too many parameters, group related parameters in meaningfully named structs.
Where a Rust function is returning a large error, consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new functionality in Rust. Run both before and after making any change.
Every Rust module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs in Rust using Rustdoc comments (///) so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessarymutbindings in Rust.
Handle errors with theResulttype instead of panicking in Rust where feasible.
Avoidunsafecode in Rust unless absolutely necessary, and document any usage clearly.
Place function attributes after doc comments in Rust.
Do not usereturnin single-line Rust functions.
Use predicate functions for conditional criteria in Rust with more than two branches.
Rust lints must not be silenced except as a last resort.
Rust lint rule suppressions must be tightly scoped and include a clear reason.
Preferexpectoverallowin Rust.
Userstestfixtures for shared setup in Rust tests.
Replace duplicated Rust tests with `#[rstest(...)...
Files:
wildside-core/src/solver.rswildside-cli/src/tests/unit.rswildside-cli/src/tests/feature_flags.rswildside-solver-ortools/src/lib.rswildside-solver-vrp/src/lib.rswildside-cli/src/solve.rswildside-core/src/store/spatial_index.rswildside-cli/src/tests/steps.rswildside-core/src/store/sqlite.rswildside-cli/src/lib.rswildside-cli/src/tests/helpers.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.
- Adhere to single responsibility and CQRS
- Place function attributes after doc comments.
- Do not use
returnin single-line functions.- Move conditionals with >2 branches into a predicate function.
- Avoid
unsafeunless absolutely necessary.- Every module must begin with a
//!doc comment that explains the module's purpose and utility.- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.- Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
- Use
rstestfixtures for shared setup and to avoid repetition between tests.- Replace duplicated tests with
#[rstest(...)]parameterised cases.- Prefer
mockallfor mocks/stubs.- Prefer
.expect()over.unwrap()in tests..expect()and.unwrap()are forbidden outside of tests. Errors must be propagated.- Ensure that any API or behavioural changes are reflected in the documentation in
docs/- Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/- Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
- Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and...
Files:
wildside-core/src/solver.rswildside-cli/src/tests/unit.rswildside-cli/src/tests/feature_flags.rswildside-solver-ortools/src/lib.rswildside-solver-vrp/src/lib.rswildside-cli/src/solve.rswildside-core/src/store/spatial_index.rswildside-cli/src/tests/steps.rswildside-core/src/store/sqlite.rswildside-cli/src/lib.rswildside-cli/src/tests/helpers.rs
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: Use the markdown files within thedocs/directory as a knowledge base and source of truth for project requirements, dependency choices, and architectural decisions.
When new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve, proactively update the relevant file(s) in thedocs/directory to reflect the latest state. Ensure the documentation remains accurate and current.
Documentation must use en-GB-oxendict spelling and grammar ("-ize" / "-yse" / "-our") with the exception of the LICENSE filename.
docs/**/*.md: Use British English based on the Oxford English Dictionary locale en-GB, including -ize suffixes (e.g., realize, organization), -lyse suffixes (e.g., analyse, paralyse), -our suffixes (e.g., colour, behaviour), -re suffixes (e.g., centre, calibre), double 'l' (e.g., cancelled, counsellor), maintained 'e' (e.g., likeable, rateable), -ogue suffixes (e.g., analogue, catalogue)
Use the Oxford comma in documentation: 'ships, planes, and hovercraft' where it aids comprehension
Treat company names as collective nouns, for example: 'Concordat Industries are expanding'
Write headings in sentence case
Use Markdown headings (#, ##, ###, and so on) in order without skipping levels
Follow markdownlint recommendations
Provide code blocks and lists using standard Markdown syntax
Always provide a language identifier for fenced code blocks; use 'plaintext' for non-code text
Use '-' as the first level bullet and renumber lists when items change
Prefer inline links using 'text' or angle brackets around the URL
Ensure blank lines before and after bulleted lists and fenced blocks
Ensure tables have a delimiter line below the header row
Expand any uncommon acronym on first use, for example, Continuous Integration (CI)
Wrap paragraphs at 80 columns
Do not wrap tables
Use GitHub-flavoured numeric footnotes referenced as '[^1]'
Footnotes must be numbered in order of appearance in the document
Caption ev...
Files:
docs/wildside-engine-design.mddocs/wildside-engine-design/01-introduction.mddocs/execplans/phase-4-feature-flags.mddocs/users-guide.mddocs/wildside-engine-design/04-library-architecture.mddocs/wildside-engine-design/02-data-foundation.mddocs/wildside-engine-design/06-implementation-testing-deployment.mddocs/wildside-engine-design/05-solver-implementation.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Validate Markdown files usingmake markdownlint.
Runmake fmtafter any documentation changes to format all Markdown files and fix table markup.
Validate Mermaid diagrams in Markdown files by runningmake nixie.
Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks in Markdown must be wrapped at 120 columns.
Markdown tables and headings must not be wrapped.
Use dashes (-) for list bullets in Markdown.
Use GitHub-flavoured Markdown footnotes ([^1]) for references and footnotes.
Files:
docs/wildside-engine-design.mddocs/wildside-engine-design/01-introduction.mddocs/execplans/phase-4-feature-flags.mddocs/users-guide.mddocs/wildside-engine-design/04-library-architecture.mddocs/wildside-engine-design/02-data-foundation.mddocs/wildside-engine-design/06-implementation-testing-deployment.mddocs/wildside-engine-design/05-solver-implementation.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/wildside-engine-design.mddocs/wildside-engine-design/01-introduction.mddocs/execplans/phase-4-feature-flags.mddocs/users-guide.mddocs/wildside-engine-design/04-library-architecture.mddocs/wildside-engine-design/02-data-foundation.mddocs/wildside-engine-design/06-implementation-testing-deployment.mddocs/wildside-engine-design/05-solver-implementation.md
docs/**/!(README).md
📄 CodeRabbit inference engine (docs/documentation-style-guide.md)
Avoid first and second person personal pronouns outside the README.md file
Files:
docs/wildside-engine-design.mddocs/wildside-engine-design/01-introduction.mddocs/execplans/phase-4-feature-flags.mddocs/users-guide.mddocs/wildside-engine-design/04-library-architecture.mddocs/wildside-engine-design/02-data-foundation.mddocs/wildside-engine-design/06-implementation-testing-deployment.mddocs/wildside-engine-design/05-solver-implementation.md
docs/**/*.{md,rs}
📄 CodeRabbit inference engine (docs/documentation-style-guide.md)
Wrap code at 120 columns
Files:
docs/wildside-engine-design.mddocs/wildside-engine-design/01-introduction.mddocs/execplans/phase-4-feature-flags.mddocs/users-guide.mddocs/wildside-engine-design/04-library-architecture.mddocs/wildside-engine-design/02-data-foundation.mddocs/wildside-engine-design/06-implementation-testing-deployment.mddocs/wildside-engine-design/05-solver-implementation.md
🧠 Learnings (2)
📚 Learning: 2026-01-03T14:22:19.441Z
Learnt from: leynos
Repo: leynos/wildside-engine PR: 81
File: docs/wildside-engine-design/02-data-foundation.md:0-0
Timestamp: 2026-01-03T14:22:19.441Z
Learning: Apply en-GB Oxford English spelling in Markdown files by preferring -ize (e.g., normalize, initialize, organize) over -ise, -yse over -yze (e.g., analyse, paralyse), and -our over -or (e.g., colour, favour). The Oxford British English convention favors -ize in these contexts. This guidance should be applied across Markdown documentation in the docs directory to maintain consistent spelling.
Applied to files:
docs/wildside-engine-design.mddocs/wildside-engine-design/01-introduction.mddocs/execplans/phase-4-feature-flags.mddocs/users-guide.mddocs/wildside-engine-design/04-library-architecture.mddocs/wildside-engine-design/02-data-foundation.mddocs/wildside-engine-design/06-implementation-testing-deployment.mddocs/wildside-engine-design/05-solver-implementation.md
📚 Learning: 2026-01-04T04:50:10.691Z
Learnt from: leynos
Repo: leynos/rstest-bdd PR: 384
File: crates/rstest-bdd/tests/async_step_infrastructure.rs:4-4
Timestamp: 2026-01-04T04:50:10.691Z
Learning: In en-GB-oxendict (Oxford English) spelling convention, verbs use the -ize ending (e.g., "normalizes," "organizes," "realizes"), not -ise. The convention is specifically -ize / -yse / -our, so "normalizes" is correct in rstest-bdd documentation and comments.
Applied to files:
docs/wildside-engine-design/02-data-foundation.md
🧬 Code graph analysis (6)
wildside-cli/src/tests/feature_flags.rs (1)
wildside-cli/src/lib.rs (1)
run_ingest(69-83)
wildside-solver-ortools/src/lib.rs (1)
wildside-core/src/solver.rs (1)
solve(179-179)
wildside-cli/src/solve.rs (2)
wildside-fs/src/lib.rs (1)
open_utf8_file(10-12)wildside-solver-ortools/src/lib.rs (1)
new(32-38)
wildside-core/src/store/spatial_index.rs (1)
wildside-core/src/store/sqlite.rs (2)
open(73-95)poi(204-210)
wildside-core/src/store/sqlite.rs (5)
wildside-core/src/store/spatial_index.rs (2)
load_index_entries(129-166)poi(182-188)wildside-core/src/store/mod.rs (1)
get_pois_in_bbox(71-74)wildside-core/src/test_support.rs (2)
get_pois_in_bbox(52-64)write_sqlite_database(69-92)wildside-core/tests/sqlite_poi_store_behaviour.rs (2)
bbox(78-80)new(29-38)wildside-core/src/poi.rs (1)
envelope(46-48)
wildside-cli/src/lib.rs (8)
wildside-core/tests/poi_store_behaviour.rs (1)
store(19-22)wildside-core/src/store/spatial_index.rs (1)
write_spatial_index(90-95)wildside-cli/src/tests/helpers.rs (1)
wikidata(87-89)wildside-data/src/wikidata/etl/mod.rs (1)
extract_linked_entity_claims(153-198)wildside-data/src/wikidata/store/persistence.rs (1)
persist_claims_to_path(285-295)wildside-data/src/ingest/mod.rs (1)
ingest_osm_pbf_report(149-202)wildside-data/src/ingest/sqlite.rs (1)
persist_pois_to_sqlite(97-122)wildside-cli/src/solve.rs (2)
into_config(105-108)try_from(160-187)
🪛 LanguageTool
docs/wildside-engine-design.md
[typographical] ~10-~10: To join two clauses or introduce examples, consider using an em dash.
Context: ...on.md) - [Section 1: The Data Foundation - Ingesting and Integrating Open Data](w...
(DASH_RULE)
[typographical] ~16-~16: To join two clauses or introduce examples, consider using an em dash.
Context: ... - [Section 4: The Solver Implementation - Abstracted and Replaceable](wildside-e...
(DASH_RULE)
docs/wildside-engine-design/01-introduction.md
[style] ~16-~16: This word has been used in one of the immediately preceding sentences. Using a synonym could make your text more interesting to read, unless the repetition is intentional.
Context: ...ithms, and solving NP-hard optimization problems all align with the Rust ecosystem's str...
(EN_REPEATEDWORDS_PROBLEM)
[uncategorized] ~65-~65: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...`, and tag map) directly in the R*-tree so lookups can avoid secondary hash-map pr...
(COMMA_COMPOUND_SENTENCE_2)
[style] ~85-~85: Would you like to use the Oxford spelling “normalized”? The spelling ‘normalised’ is also correct.
Context: ...non-negative values. Scores SHOULD be normalised to the [0.0, 1.0] range unless explic...
(OXFORD_SPELLING_Z_NOT_S)
docs/execplans/phase-4-feature-flags.md
[uncategorized] ~13-~13: The hyphen in fully-tested is redundant.
Context: ...le keeping the default build stable and fully-tested. Success is observable when the workspa...
(ADVERB_LY_HYPHEN_FIX)
[style] ~87-~87: Consider using the typographical ellipsis character here instead.
Context: ...lite, with conditional compilation via #[cfg(feature = "...")]. Testing guidance is in docs/rust...
(ELLIPSIS)
[style] ~122-~122: Consider using the typographical ellipsis character here instead.
Context: ...y re-export the SQLite store and to add doc(cfg(...)) hints where appropriate. Adjust exis...
(ELLIPSIS)
[style] ~125-~125: Consider using a more formal replacement.
Context: ... compiled when the feature is enabled. After that, update wildside-cli to respect the f...
(AFTER_THAT)
[style] ~141-~141: Consider using the typographical ellipsis character here instead.
Context: ...and scenarios are guarded with the same #[cfg(feature = "...")] selectors so they only compile unde...
(ELLIPSIS)
[style] ~183-~183: Consider removing “of” to be more concise
Context: ...ceptance The change is acceptable when all of the following are true: - make check-fmt...
(ALL_OF_THE)
[uncategorized] ~185-~185: Loose punctuation mark.
Context: ... following are true: - make check-fmt, make lint, and make test succeed wi...
(UNLIKELY_OPENING_PUNCTUATION)
[style] ~198-~198: Consider using the typographical ellipsis character here instead.
Context: ... that the module is behind the intended #[cfg(feature = "...")] guard and that its dependencies are...
(ELLIPSIS)
[uncategorized] ~216-~216: Loose punctuation mark.
Context: ...nifest with these names: - solver-vrp: enables wildside-solver-vrp. - `solve...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~217-~217: Loose punctuation mark.
Context: ...wildside-solver-vrp. - solver-ortools: enables wildside-solver-ortools. - s...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~218-~218: Loose punctuation mark.
Context: ...ldside-solver-ortools. - store-sqlite`: enables the SQLite-backed POI store. T...
(UNLIKELY_OPENING_PUNCTUATION)
docs/wildside-engine-design/04-library-architecture.md
[uncategorized] ~26-~26: Loose punctuation mark.
Context: ...rates when necessary. - wildside-core: This crate is the heart of the engine. ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~41-~41: Loose punctuation mark.
Context: ... testing and fuzzing. - wildside-data: Contains the ETL logic and data adapter...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~62-~62: Loose punctuation mark.
Context: ...ge the C++ dependency. - wildside-cli: A small command-line application for op...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~64-~64: Loose punctuation mark.
Context: ...ion for operational tasks. - ingest: Runs the full ETL pipeline from `wildsi...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~79-~79: Loose punctuation mark.
Context: ...obal popularity scores. - solve: Runs the route solver from the command ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~148-~148: Loose punctuation mark.
Context: ...zed, read-only artefacts: - pois.db: An SQLite (or RocksDB) file containing ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~152-~152: Loose punctuation mark.
Context: ...n a major release. - pois.rstar: A serialized R*-tree file for fast spa...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~156-~156: Loose punctuation mark.
Context: ...ross 0.x releases. - popularity.bin: A compact binary file of pre-calculated...
(UNLIKELY_OPENING_PUNCTUATION)
docs/wildside-engine-design/02-data-foundation.md
[typographical] ~1-~1: To join two clauses or introduce examples, consider using an em dash.
Context: # Section 1: The Data Foundation - Ingesting and Integrating Open Data The...
(DASH_RULE)
[grammar] ~30-~30: Please add a punctuation mark at the end of paragraph.
Context: ...quence of independent "blobs," a design that osmpbf leverages to process these bl...
(PUNCTUATION_PARAGRAPH_END)
[style] ~45-~45: This expression can be considered offensive.
Context: ...rate is licensed under the "Do What The Fuck You Want To Public Licence, Version 2" (WTF...
(PROFANITY)
[uncategorized] ~123-~123: The preposition ‘to’ seems more likely in this position.
Context: ...ous reqwest client, issues requests for the Tokio runtime, and always sends a d...
(AI_HYDRA_LEO_REPLACE_FOR_TO)
[style] ~153-~153: Would you like to use the Oxford spelling “normalizes”? The spelling ‘normalises’ is also correct.
Context: ...nksstructure scans the ingested POIs, normaliseswikidata=*` tag values (handling commo...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~161-~161: You have used the passive voice repeatedly in nearby sentences. To make your writing clearer and easier to read, consider using active voice.
Context: ... referenced by the PoiEntityLinks set are processed further. For those entities, the parser...
(REP_PASSIVE_VOICE)
[style] ~218-~218: Would you like to use the Oxford spelling “initialize”? The spelling ‘initialise’ is also correct.
Context: ...Schema initialization is handled by the initialise_schema function, which enables foreign...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~219-~219: Would you like to use the Oxford spelling “normalized”? The spelling ‘normalised’ is also correct.
Context: ...reign keys and creates a compact set of normalised tables: - wikidata_entities contains...
(OXFORD_SPELLING_Z_NOT_S)
[uncategorized] ~242-~242: Possible missing preposition found.
Context: ... thousands of queries per user request. Will be rate-limited and result in extreme l...
(AI_HYDRA_LEO_MISSING_IT)
[grammar] ~271-~271: Please add a punctuation mark at the end of paragraph.
Context: ... to work seamlessly with the types from the geo crate. The practical implementat...
(PUNCTUATION_PARAGRAPH_END)
docs/wildside-engine-design/06-implementation-testing-deployment.md
[style] ~69-~69: This word has been used in one of the immediately preceding sentences. Using a synonym could make your text more interesting to read, unless the repetition is intentional.
Context: ...ed as a JSON file containing a complete problem instance and its expected solution. The...
(EN_REPEATEDWORDS_PROBLEM)
[style] ~86-~86: Would you like to use the Oxford spelling “parameterized”? The spelling ‘parameterised’ is also correct.
Context: ...ernal routing dependencies. - rstest parameterised unit tests (golden_routes.rs): A si...
(OXFORD_SPELLING_Z_NOT_S)
docs/wildside-engine-design/05-solver-implementation.md
[grammar] ~36-~36: Please add a punctuation mark at the end of paragraph.
Context: ...ame.15 This implementation will live in the wildside-solver-vrp crate. ### 4.2....
(PUNCTUATION_PARAGRAPH_END)
[style] ~86-~86: The term ‘absolutely need’ is used quite frequently. Consider using a less common alternative to set your writing apart from others.
Context: ...o opt into this complexity only if they absolutely need it, without burdening the default setup...
(ABSOLUTELY_GREAT_RULE)
🔍 Remote MCP Context7
Let me search for information about the testing framework and feature flag patterns being used:Now let me search for information about GitHub Actions feature matrix testing and spatial indexing patterns:
Summary of Relevant Context for PR Review
Testing Framework (rstest-bdd)
rstest-bdd brings the collaborative clarity of BDD to Rust without asking you to adopt a bespoke runner or a monolithic "world" object, instead building on the excellent rstest fixture and parametrisation model. The PR upgrades this from 0.2.0 to 0.3.0. Key point: In version 0.3.0, the #[scenario] attribute now requires a path argument pointing to the .feature file; index-only usage is no longer supported. This means any BDD tests in the PR using rstest-bdd 0.3.0 must reference feature files with explicit paths (as seen in wildside-cli/src/tests/feature_flag_steps.rs).
Feature Flag Implementation Best Practices
The PR's approach to feature flags aligns with Rust/Cargo conventions. Dependencies automatically enable default features unless default-features = false is specified, which can make it difficult to ensure that the default features are not enabled, especially for a dependency that appears multiple times in the dependency graph. The PR correctly handles this by:
- Declaring optional dependencies with
optional = true - Creating explicit feature definitions (e.g.,
solver-vrp,solver-ortools,store-sqlite) - Using
#[cfg(feature = "store-sqlite")]guards in code
GitHub Actions Matrix Strategy
The matrix strategy runs jobs across multiple configurations - different OS versions, language versions, or other parameters, testing your application across environments without duplicating workflows. The PR's new feature-matrix job in .github/workflows/ci.yml uses this pattern to test multiple feature combinations. Matrix strategies improve continuous integration practices by executing numerous job combinations simultaneously, and this parallel execution reduces runtime, allowing developers to receive test feedback more swiftly.
Spatial Indexing (R-tree/rstar)
rstar is a flexible, n-dimensional r*-tree implementation for the Rust ecosystem suitable for use as a spatial index, with primitives provided for point, line, and rectangle geometries. R-trees are suited best if many queries and only few insertions are made, and rstar supports bulk loading which cuts down the constant factors when creating an r-tree significantly compared to sequential insertions. The PR's approach to persist R-tree indices using bincode serialization and a header format (WSPI magic + version) aligns with the library's design for read-heavy, indexed spatial data access.
Key Validation Points for Review
- rstest-bdd 0.3.0 compatibility: Verify that all
#[scenario]attributes includepath = "..."parameters pointing to.featurefiles - Feature flag consistency: Confirm that all public API re-exports in
src/lib.rsand CLI modules are consistently gated behind matching feature flags across crates - Cargo resolver behavior: With resolver = "2",
--no-default-featuresdisables defaults for all workspace members, not just the root - Spatial index persistence: The custom binary format (magic + version + bincode payload) correctly supports versioning for future migrations and validation
[::web_search::], [::Deepwiki::],
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Sourcery review
- GitHub Check: build
🔇 Additional comments (48)
Makefile (1)
9-9: LGTM!The TEST_FLAGS variable declaration follows Makefile conventions and integrates cleanly with the CI feature-matrix job.
.github/workflows/ci.yml (2)
93-106: LGTM!The job steps correctly set up the Rust toolchain, caching, and nextest, then execute tests with the appropriate TEST_FLAGS. The matrix strategy with
fail-fast: falseensures all feature combinations are tested independently.
69-92: The placeholder solver-ortools crate is properly designed to handle gracefully in matrix tests. TheOrtoolsSolver::solve()method returnsSolveError::NotImplementedrather than panicking, allowing matrix entriessolver-ortools-only,solvers-only, andsolver-ortools-and-storeto compile and execute without test failures.wildside-core/Cargo.toml (1)
23-26: Feature flag configuration is well-structured.The dependency chain is logical:
serdeprovides serialization primitives for core data structures.store-sqlitedepends onserdeand addsbincodefor spatial index persistence andrusqlitefor database access.- Defaults enable both, allowing consumers to opt out selectively via
default-features = false.This aligns with the PR's goal of conditionally exposing SQLite store behind feature gates.
wildside-cli/src/tests/steps.rs (2)
3-4: Feature gating is correctly applied.The module-level
#![cfg(feature = "store-sqlite")]gate ensures these BDD steps compile only when the SQLite store is available, aligning with the CLI's feature-gated ingestion logic.
185-199: Scenario registration syntax for rstest-bdd 0.3.0 is correct.The macro correctly specifies
path = "tests/features/ingest_command.feature"for all three scenarios, as required by rstest-bdd 0.3.0. The referenced .feature file exists at the expected path.wildside-cli/src/tests/unit.rs (1)
3-4: Feature gating and test structure are sound.The module-level
#![cfg(feature = "store-sqlite")]correctly gates these unit tests, ensuring they compile only when SQLite store support is enabled. The tests properly validateIngestConfigerror paths using rstest parameterization and appropriate assertions.wildside-solver-vrp/src/lib.rs (1)
12-12: Documentation improvement enhances clarity.The reference to
SolveError::InvalidRequestis more precise than the previous generic "modelling errors" wording, making the error-handling contract explicit for consumers.wildside-core/src/solver.rs (2)
163-165: NotImplemented variant is well-integrated.This error variant provides a clear signal when a solver backend is not available, supporting the PR's feature-gated architecture. The implementation correctly derives
Errorvia thiserror and is properly documented.
171-172: Trait documentation accurately reflects placeholder behaviour.The updated documentation correctly guides implementers to return
NotImplementedwhen a concrete backend is unavailable, which aligns with the feature-flagged solver backends introduced in this phase.docs/wildside-engine-design.md (1)
3-20: LGTM! Clear structure and rationale.The index file successfully delegates content to modular sections whilst providing a clear rationale for the 400-line guideline. The sequential reading flow is well-signposted.
wildside-cli/src/tests/feature_flags.rs (1)
1-31: LGTM! Efficient feature-flag test.The test correctly verifies that
run_ingestreturnsMissingFeaturewhenstore-sqliteis disabled. The module-level cfg gate is appropriate, and the test avoids unnecessary file I/O since the function returns early when the feature is absent.docs/execplans/phase-4-feature-flags.md (1)
1-235: LGTM! Comprehensive ExecPlan documentation.The document thoroughly captures Phase 4 feature-flag implementation with clear progress tracking, decision rationales, validation criteria, and concrete steps. The completed status and retrospective provide valuable context for future reference.
docs/wildside-engine-design/04-library-architecture.md (1)
1-185: LGTM! Comprehensive architecture documentation.The library-first design is clearly articulated with well-defined crate boundaries, a stable API surface, and explicit offline/online separation. The artefact versioning strategy with migration guidance provides robust operational support.
docs/wildside-engine-design/05-solver-implementation.md (1)
1-150: Documentation structure and spelling are sound.The solver implementation design document follows the decomposed structure well. Spelling adheres to en-GB-oxendict conventions (e.g., "synchronization", "modelling"). The markdownlint directive at line 99 is appropriate for the long trait signature.
docs/users-guide.md (3)
64-66: Documentation accurately reflects the newNotImplementederror variant.The addition of
SolveError::NotImplementedis correctly documented, aligning with the placeholder solver implementation inwildside-solver-ortools.
73-79: Spelling correction applied and feature-gating documented.The "serialized" spelling now conforms to en-GB-oxendict. The
store-sqlitefeature-gating forSqlitePoiStoreis accurately described.
107-108: Error handling summary correctly expanded.The
SolveErrordescription now covers bothInvalidRequestandNotImplementedvariants, matching the updated API surface.wildside-solver-ortools/src/lib.rs (1)
1-50: Well-structured placeholder solver crate.The
OrtoolsSolverplaceholder reserves the API surface for future CP-SAT integration. The use ofSolveError::NotImplementedcorrectly signals that the backend is not yet available, addressing the prior review feedback. Theconst fnconstructor andforbid(unsafe_code)are appropriate choices.wildside-cli/src/lib.rs (3)
69-83: Feature-gated ingest dispatch is well-structured.The
run_ingestfunction cleanly handles both feature states: returningMissingFeaturewhenstore-sqliteis disabled and delegating toexecute_ingestwhen enabled. Thelet _ = argssuppresses the unused variable warning appropriately.
92-125: Ingest pipeline implementation is correct.The
execute_ingestfunction properly sequences: OSM PBF ingestion → SQLite persistence → Wikidata claims extraction → claims persistence → spatial index writing. Error handling uses the?operator with appropriateCliErrormappings.
40-42: Do not gate these constants.
ARG_OSM_PBF,ARG_WIKIDATA_DUMP, andARG_OUTPUT_DIRare used within the#[arg(...)]attributes of theIngestArgsstruct definition (lines 156, 160, 164). TheIngestArgsstruct is part of the ungatedCommandenum and must be parsable regardless of whether thestore-sqlitefeature is enabled. Gating the constants would cause a compile error when the feature is disabled, as the attributes would reference undefined constants at compile time.Likely an incorrect or invalid review comment.
docs/wildside-engine-design/06-implementation-testing-deployment.md (1)
1-167: Documentation structure is comprehensive.The implementation, testing, and deployment strategy document covers packaging, versioning, feature flags, testing disciplines (golden tests, property tests, benchmarks, fuzz testing, API contract tests), and migration strategy. The tables are well-formatted and provide clear rationale for design decisions.
wildside-core/src/store/spatial_index.rs (3)
97-126: Read/write format consistency verified.The header (magic + version) is now written as raw bytes separately from the bincode payload, matching the read logic in
load_index_entries. This addresses the prior critical review feedback about format mismatch.
168-301: Comprehensive test coverage for spatial index persistence.The test suite covers:
- Round-trip serialization/deserialization
- Missing file I/O errors
- Invalid magic header detection
- Decode failure handling
- Unsupported and legacy version rejection
- Payload inspection after write
The use of
rstestfixtures (temp_index_path,sample_pois) follows project conventions.
23-60: Error types are well-designed.
SpatialIndexErrorandSpatialIndexWriteErrorprovide semantic variants with contextual paths and source errors. The#[source]attribute enables proper error chaining.wildside-cli/src/tests/helpers.rs (4)
128-135:let-elsepattern is appropriate.The
merge_fieldfunction usesif letwithlet Some(value)binding, which is valid Rust 2024 syntax. The logic correctly prioritises env over file layer values.
38-102:DatasetFilesprovides clean test fixture management.The struct manages
TempDirlifetime correctly via_dirfield. The helper methods expose paths without ownership transfer, enabling reuse across test scenarios.
166-175: Hardcoded Wikidata payload is adequate for unit tests.The minimal JSON payload contains sufficient structure to exercise claim extraction without requiring external fixtures. Mark as test data if this grows to warrant extraction.
145-148: Remove this suggestion. The fixture directory path is compile-time validated through the workspace configuration in Cargo.toml, which declares wildside-data as a workspace member. The env!("CARGO_MANIFEST_DIR") macro guarantees the path resolution at compile time, not runtime, making it inherently safe. No additional checks or documentation are necessary.Likely an incorrect or invalid review comment.
wildside-core/src/store/sqlite.rs (7)
1-21: LGTM: Module documentation and imports are well-structured.The module-level doc comment clearly describes the purpose, imports are organized logically, and the
SQLITE_MAX_VARIABLE_NUMBERconstant includes helpful context about the chunking strategy.
23-56: LGTM: Error types follow best practices.The
SqlitePoiStoreErrorenum provides semantic error variants with clear documentation. Proper use of#[source]attributes enables error chaining, and the structured variants allow callers to inspect and handle specific failure modes.
58-69: LGTM: Custom Debug implementation is appropriate.The custom
Debugimplementation shows the index size rather than materializing the entire RTree, which is sensible for a large spatial index. The use offinish_non_exhaustive()correctly indicates that internal details are omitted.
98-115: LGTM: Spatial query implementation is correct.The bbox query correctly converts from
geo::Recttorstar::AABB, queries the spatial index, sorts results by id for deterministic ordering, and returns a boxed iterator. Materializing all results into a Vec is consistent with the in-memory design of this store.
156-159: LGTM: Variable limit function is correctly structured.The function is designed for future extensibility (e.g., querying SQLite's runtime limit), so the unused
connectionparameter and placeholder comment are appropriate.
161-190: LGTM: POI loading logic is correct and safe.The dynamic SQL construction on line 170 is safe because placeholders are derived from
ids.len(), not user input. Parameter binding viaparams_from_iterprevents SQL injection. JSON tag parsing includes proper error handling withInvalidTags, and results are sorted for deterministic behaviour.
192-365: LGTM: Comprehensive test coverage with appropriate fixture design.The test suite covers both successful queries (bbox retrieval, sorting, empty results) and error conditions (missing POI, corrupted magic, unsupported version, invalid tags). Proper use of
rstestfixtures with composition (e.g.,sqlite_store_fixturedepends ontemp_artifactsandsample_pois) promotes reusability. The use of.expect()in tests follows the project's guidelines.docs/wildside-engine-design/02-data-foundation.md (5)
1-67: LGTM: Introduction and parser analysis are well-structured.The opening section clearly establishes the importance of the data foundation, and the comparative analysis of OSM PBF parsers is thorough and well-reasoned. The recommendation for
osmpbfbased on licensing and technical merits is sound.
68-88: LGTM: Ingestion summary accurately describes the implementation.The section clearly explains the
OsmIngestReportfunctionality, including the POI emission rules, coordinate indexing strategy, and handling of unresolved geometry. The technical details align with the implementation in the codebase.
90-213: LGTM: Wikidata ETL design is comprehensive and well-documented.The sections clearly describe the dump acquisition implementation, linked entity extraction, and processing flow. The Mermaid sequence diagram effectively illustrates the line-by-line parsing logic with error handling. The technical approach is sound and aligns with the implemented functionality.
214-250: LGTM: Schema design and comparison are well-documented.The SQLite schema section clearly describes the table structure, indexing strategy, and versioning approach. The comparative analysis table effectively contrasts live SPARQL queries with offline dump processing, providing clear justification for the architectural decision.
251-280: LGTM: Geospatial library recommendations are well-justified.The section clearly explains the rationale for adopting the GeoRust ecosystem, specifically
geofor geometric types andrstarfor spatial indexing. The recommendation aligns with the implementation choices made throughout the codebase.wildside-cli/src/solve.rs (6)
8-24: LGTM: Feature-gated imports are correctly structured.The imports are properly gated behind their respective features. The
store-sqlitefeature gates the store, travel-time provider, and scorer. Solver imports are correctly gated withVrpSolvertaking priority when both solver features are enabled.
31-66: LGTM: Type aliases centralize solver selection logic.The type aliases and constants effectively centralize the feature-based solver selection. The precedence order (VRP over OR-Tools) is clear and consistent. The test-only
SelectedSolverKindenum andSELECTED_SOLVER_KINDconstant provide a clean way to verify feature flag behaviour in tests.
197-202: LGTM: Builder delegates to centralized helpers.The
DefaultSolveSolverBuilderimplementation now cleanly delegates tomake_store_and_depsandbuild_solver_with_features, eliminating the duplicated feature-gate logic that was present in the previous version. This aligns with the past review feedback.
210-235: LGTM: Store dependency construction is correctly gated.The
make_store_and_depshelper properly constructs the store, scorer, and provider whenstore-sqliteis enabled, and returns a clearMissingFeatureerror otherwise. The unused variable suppression on line 229 correctly prevents warnings in the disabled feature path.
237-262: LGTM: Solver construction centralizes feature selection.The
build_solver_with_featureshelper uses theSelectedSolvertype alias to construct the appropriate solver based on enabled features. The two-level feature gating (store-sqlite outer, solver inner) ensures clear error messages for missing features. Unused variable suppressions on lines 247 and 256 are appropriate.
336-353: LGTM: Feature flag test validates solver selection logic.The test verifies that the compile-time
SELECTED_SOLVER_KINDconstant matches the expected value based on enabled features. This effectively validates that the type alias selection logic is consistent with the feature flags, addressing the past review feedback about testing the selection logic.
- Use path references instead of cloned PathBufs for database and index paths - Replace debug_assert with unreachable! in find_missing_poi_in_chunk - Minor code clarity and efficiency improvements Additionally, enable conditional compilation of IngestOutcome struct based on 'store-sqlite' feature, update Cargo.toml dependencies, and adjust CI test flags to include 'solver-vrp' and 'store-sqlite' features in default-features matrix. Minor documentation typo fixes (normalise -> normalize, parameterised -> parameterized, normalized consistent) included. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
leynos
changed the title
There was a problem hiding this comment.
Actionable comments posted: 10
Fix all issues with AI Agents 🤖
In @.github/workflows/ci.yml:
- Around line 91-101: The CI workflow duplicates the checkout, Rust toolchain,
cache, and cargo-nextest steps (the uses entries for actions/checkout,
dtolnay/rust-toolchain, Swatinem/rust-cache, and taiki-e/install-action for
cargo-nextest) between jobs; either document the intentional duplication in a
comment near those steps explaining the trade-off (small stable setup vs.
complexity of composite actions) or extract these repeated steps into a reusable
composite action or a reusable workflow and replace the duplicated blocks with a
single reusable call to reduce maintenance burden.
- Around line 69-91: The feature-matrix job runs unconditionally; add a
dependency so it only runs after the build job succeeds by inserting needs:
build under the feature-matrix job (i.e., add needs: build to the feature-matrix
job definition just before the existing strategy: block) so the feature-matrix
job is gated on the build job completing successfully.
- Around line 75-76: The matrix entry named "default-features" is misleading
because test_flags uses "--no-default-features --features
solver-vrp,store-sqlite"; rename the matrix key "default-features" to
"explicit-defaults" (or "default-explicit") to reflect that features are being
specified explicitly, and update any workflow references that consume this
matrix value to use the new name; ensure the test_flags line remains unchanged.
In @docs/wildside-engine-design/02-data-foundation.md:
- Around line 271-273: Remove the erroneous blank line splitting the sentence
that begins "It is designed to work seamlessly with the types from the" so that
it reads continuously with the following "`geo` crate." — ensure the phrase
flows into the next clause about building an `rstar::RTree` without a paragraph
break.
- Around line 30-32: Remove the stray blank line splitting the sentence: join "a
design that" and "`osmpbf` leverages to process these blobs in parallel across
multiple CPU" into a single continuous sentence so the phrase reads without the
paragraph break (edit the sentence containing "a design that" and the following
line starting with "`osmpbf`").
- Around line 100-101: Replace the incorrect "task.5." sequence with proper
markdown footnote syntax and a single terminal period: change the fragment so
the sentence reads with the `wikidata-rust` crate referenced and the footnote as
`[^5]`, e.g. "... The `wikidata-rust` crate is a purpose-built tool for this
task[^5]." Ensure only one period appears at the end and keep the
`wikidata-rust` identifier unchanged.
In @docs/wildside-engine-design/05-solver-implementation.md:
- Around line 84-86: The phrase "absolutely need" in the sentence that reads
"consumers of the Wildside engine can choose to opt into this complexity only if
they absolutely need it" is flagged for being overused; replace it with a
stronger, more precise alternative (e.g., "require it," "have a clear need for
it," or "must use it") in the same paragraph so the sentence instead reads that
consumers can opt into the feature-flagged implementation only if they require
it or have a clear need, preserving meaning but avoiding the cliché.
- Around line 4-6: Replace the first-person phrase "allows us to abstract the
solver behind a trait" with a neutral phrasing such as "allows abstraction of
the solver behind a trait" or "enables the solver to be abstracted behind a
trait" so the sentence no longer uses "us" and complies with documentation
guidelines; ensure the rest of the sentence ("making the specific implementation
a configurable choice") remains unchanged.
- Around line 98-101: The paragraph continuation immediately follows the
code-formatted signature `fn get_travel_time_matrix(&self, pois:
&[PointOfInterest]) -> Result<TravelTimeMatrix, TravelTimeError>` without a
separating blank line; insert a single blank line after that code line (or
alternatively merge the continuation sentence into the same paragraph) so the
Markdown renders correctly and the sentence "Keeping the solver synchronous
preserves object safety and makes the core embeddable." is separated from the
code block.
- Around line 24-37: Replace the bare numeric footnotes "13", "15", and "16"
with GitHub-flavoured footnote references [^13], [^15], and [^16] wherever they
appear, and then either add matching footnote definitions at the end of this
document or convert those references into explicit cross-references to the
existing definitions in the users-guide (so the tokens resolve); ensure the
added references use the exact labels [^13], [^15], [^16] to match the
replacements.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
wildside-cli/src/lib.rs (2)
232-240: Error conflation may obscure root cause.
require_existingtreatsErr(_)identically toOk(false), discarding the underlying error. Permission failures, symlink issues, or I/O errors will present as "missing file" to the user, hindering diagnosis.Proposed fix preserving the original error
fn require_existing(path: &Utf8Path, field: &'static str) -> Result<(), CliError> { match wildside_fs::file_is_file(path) { Ok(true) => Ok(()), - Ok(false) | Err(_) => Err(CliError::MissingSourceFile { + Ok(false) => Err(CliError::MissingSourceFile { field, path: path.to_path_buf(), }), + Err(e) => Err(CliError::SourceFileAccess { + field, + path: path.to_path_buf(), + source: e, + }), } }This requires adding a
SourceFileAccessvariant toCliError. If the current behaviour is intentional (treating inaccessible as missing), add a code comment explaining the rationale.
265-276: Type alias to()maintains API surface when feature disabled.Using
type IngestOutcome = ();whenstore-sqliteis absent is a pragmatic approach to keep the function signature consistent across feature permutations. Document this pattern in a comment so future maintainers understand the intent.Proposed documentation
+/// When `store-sqlite` is disabled, `IngestOutcome` is a unit type because +/// ingestion always fails with `MissingFeature`. This preserves the function +/// signature across feature permutations. #[cfg(not(feature = "store-sqlite"))] type IngestOutcome = ();
♻️ Duplicate comments (1)
docs/wildside-engine-design/01-introduction.md (1)
66-67: Add comma before "so" to separate independent clauses.Line 66–67 reads: "...directly in the R*-tree so lookups can avoid secondary hash-map probes." These are two independent clauses (main clause + result clause) joined by "so", requiring a comma between them.
- the full POI records (id, `geo::Coord`, and tag map) directly in the R\*-tree + the full POI records (id, `geo::Coord`, and tag map) directly in the R\*-tree, so lookups can avoid secondary hash-map probes. During start-up, the store
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/ci.ymlCargo.tomldocs/wildside-engine-design/01-introduction.mddocs/wildside-engine-design/02-data-foundation.mddocs/wildside-engine-design/05-solver-implementation.mddocs/wildside-engine-design/06-implementation-testing-deployment.mdwildside-cli/src/lib.rswildside-core/src/store/sqlite.rs
🧰 Additional context used
📓 Path-based instructions (6)
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: Use the markdown files within thedocs/directory as a knowledge base and source of truth for project requirements, dependency choices, and architectural decisions.
When new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve, proactively update the relevant file(s) in thedocs/directory to reflect the latest state. Ensure the documentation remains accurate and current.
Documentation must use en-GB-oxendict spelling and grammar ("-ize" / "-yse" / "-our") with the exception of the LICENSE filename.
docs/**/*.md: Use British English based on the Oxford English Dictionary locale en-GB, including -ize suffixes (e.g., realize, organization), -lyse suffixes (e.g., analyse, paralyse), -our suffixes (e.g., colour, behaviour), -re suffixes (e.g., centre, calibre), double 'l' (e.g., cancelled, counsellor), maintained 'e' (e.g., likeable, rateable), -ogue suffixes (e.g., analogue, catalogue)
Use the Oxford comma in documentation: 'ships, planes, and hovercraft' where it aids comprehension
Treat company names as collective nouns, for example: 'Concordat Industries are expanding'
Write headings in sentence case
Use Markdown headings (#, ##, ###, and so on) in order without skipping levels
Follow markdownlint recommendations
Provide code blocks and lists using standard Markdown syntax
Always provide a language identifier for fenced code blocks; use 'plaintext' for non-code text
Use '-' as the first level bullet and renumber lists when items change
Prefer inline links using 'text' or angle brackets around the URL
Ensure blank lines before and after bulleted lists and fenced blocks
Ensure tables have a delimiter line below the header row
Expand any uncommon acronym on first use, for example, Continuous Integration (CI)
Wrap paragraphs at 80 columns
Do not wrap tables
Use GitHub-flavoured numeric footnotes referenced as '[^1]'
Footnotes must be numbered in order of appearance in the document
Caption ev...
Files:
docs/wildside-engine-design/05-solver-implementation.mddocs/wildside-engine-design/06-implementation-testing-deployment.mddocs/wildside-engine-design/01-introduction.mddocs/wildside-engine-design/02-data-foundation.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Validate Markdown files usingmake markdownlint.
Runmake fmtafter any documentation changes to format all Markdown files and fix table markup.
Validate Mermaid diagrams in Markdown files by runningmake nixie.
Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks in Markdown must be wrapped at 120 columns.
Markdown tables and headings must not be wrapped.
Use dashes (-) for list bullets in Markdown.
Use GitHub-flavoured Markdown footnotes ([^1]) for references and footnotes.
Files:
docs/wildside-engine-design/05-solver-implementation.mddocs/wildside-engine-design/06-implementation-testing-deployment.mddocs/wildside-engine-design/01-introduction.mddocs/wildside-engine-design/02-data-foundation.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/wildside-engine-design/05-solver-implementation.mddocs/wildside-engine-design/06-implementation-testing-deployment.mddocs/wildside-engine-design/01-introduction.mddocs/wildside-engine-design/02-data-foundation.md
docs/**/!(README).md
📄 CodeRabbit inference engine (docs/documentation-style-guide.md)
Avoid first and second person personal pronouns outside the README.md file
Files:
docs/wildside-engine-design/05-solver-implementation.mddocs/wildside-engine-design/06-implementation-testing-deployment.mddocs/wildside-engine-design/01-introduction.mddocs/wildside-engine-design/02-data-foundation.md
docs/**/*.{md,rs}
📄 CodeRabbit inference engine (docs/documentation-style-guide.md)
Wrap code at 120 columns
Files:
docs/wildside-engine-design/05-solver-implementation.mddocs/wildside-engine-design/06-implementation-testing-deployment.mddocs/wildside-engine-design/01-introduction.mddocs/wildside-engine-design/02-data-foundation.md
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Runmake check-fmt,make lint, andmake testbefore committing in Rust projects.
make check-fmtexecutescargo fmt --workspace -- --checkto validate formatting across the entire workspace without modifying files.
make lintexecutescargo clippy --workspace --all-targets --all-features -- -D warningsto lint every target with all features enabled and deny all Clippy warnings.
make testexecutescargo test --workspaceto run the full workspace test suite.
Clippy warnings MUST be disallowed in Rust code.
Fix any warnings emitted during tests in the Rust code itself rather than silencing them.
Where a Rust function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a Rust function has too many parameters, group related parameters in meaningfully named structs.
Where a Rust function is returning a large error, consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new functionality in Rust. Run both before and after making any change.
Every Rust module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs in Rust using Rustdoc comments (///) so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessarymutbindings in Rust.
Handle errors with theResulttype instead of panicking in Rust where feasible.
Avoidunsafecode in Rust unless absolutely necessary, and document any usage clearly.
Place function attributes after doc comments in Rust.
Do not usereturnin single-line Rust functions.
Use predicate functions for conditional criteria in Rust with more than two branches.
Rust lints must not be silenced except as a last resort.
Rust lint rule suppressions must be tightly scoped and include a clear reason.
Preferexpectoverallowin Rust.
Userstestfixtures for shared setup in Rust tests.
Replace duplicated Rust tests with `#[rstest(...)...
Files:
wildside-cli/src/lib.rswildside-core/src/store/sqlite.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.
- Adhere to single responsibility and CQRS
- Place function attributes after doc comments.
- Do not use
returnin single-line functions.- Move conditionals with >2 branches into a predicate function.
- Avoid
unsafeunless absolutely necessary.- Every module must begin with a
//!doc comment that explains the module's purpose and utility.- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.- Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
- Use
rstestfixtures for shared setup and to avoid repetition between tests.- Replace duplicated tests with
#[rstest(...)]parameterised cases.- Prefer
mockallfor mocks/stubs.- Prefer
.expect()over.unwrap()in tests..expect()and.unwrap()are forbidden outside of tests. Errors must be propagated.- Ensure that any API or behavioural changes are reflected in the documentation in
docs/- Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/- Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
- Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and...
Files:
wildside-cli/src/lib.rswildside-core/src/store/sqlite.rs
Cargo.toml
📄 CodeRabbit inference engine (AGENTS.md)
Cargo.toml: Use explicit version ranges inCargo.tomland keep dependencies up-to-date.
All crate versions specified inCargo.tomlmust use SemVer-compatible caret requirements (e.g.,some-crate = "1.2.3"). Wildcard (*) or open-ended inequality (>=) version requirements are strictly forbidden.
Tilde requirements (~) in Rust dependencies should only be used where a dependency must be locked to patch-level updates for a specific, documented reason.
Files:
Cargo.toml
🧠 Learnings (2)
📚 Learning: 2026-01-03T14:22:19.441Z
Learnt from: leynos
Repo: leynos/wildside-engine PR: 81
File: docs/wildside-engine-design/02-data-foundation.md:0-0
Timestamp: 2026-01-03T14:22:19.441Z
Learning: Apply en-GB Oxford English spelling in Markdown files by preferring -ize (e.g., normalize, initialize, organize) over -ise, -yse over -yze (e.g., analyse, paralyse), and -our over -or (e.g., colour, favour). The Oxford British English convention favors -ize in these contexts. This guidance should be applied across Markdown documentation in the docs directory to maintain consistent spelling.
Applied to files:
docs/wildside-engine-design/05-solver-implementation.mddocs/wildside-engine-design/06-implementation-testing-deployment.mddocs/wildside-engine-design/01-introduction.mddocs/wildside-engine-design/02-data-foundation.md
📚 Learning: 2026-01-04T04:50:10.691Z
Learnt from: leynos
Repo: leynos/rstest-bdd PR: 384
File: crates/rstest-bdd/tests/async_step_infrastructure.rs:4-4
Timestamp: 2026-01-04T04:50:10.691Z
Learning: In en-GB-oxendict (Oxford English) spelling convention, verbs use the -ize ending (e.g., "normalizes," "organizes," "realizes"), not -ise. The convention is specifically -ize / -yse / -our, so "normalizes" is correct in rstest-bdd documentation and comments.
Applied to files:
docs/wildside-engine-design/02-data-foundation.md
🪛 LanguageTool
docs/wildside-engine-design/05-solver-implementation.md
[style] ~85-~85: The term ‘absolutely need’ is used quite frequently. Consider using a less common alternative to set your writing apart from others.
Context: ...o opt into this complexity only if they absolutely need it, without burdening the default setup...
(ABSOLUTELY_GREAT_RULE)
docs/wildside-engine-design/06-implementation-testing-deployment.md
[style] ~69-~69: This word has been used in one of the immediately preceding sentences. Using a synonym could make your text more interesting to read, unless the repetition is intentional.
Context: ...ed as a JSON file containing a complete problem instance and its expected solution. The...
(EN_REPEATEDWORDS_PROBLEM)
docs/wildside-engine-design/01-introduction.md
[style] ~16-~16: This word has been used in one of the immediately preceding sentences. Using a synonym could make your text more interesting to read, unless the repetition is intentional.
Context: ...ithms, and solving NP-hard optimization problems all align with the Rust ecosystem's str...
(EN_REPEATEDWORDS_PROBLEM)
[uncategorized] ~65-~65: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...`, and tag map) directly in the R*-tree so lookups can avoid secondary hash-map pr...
(COMMA_COMPOUND_SENTENCE_2)
docs/wildside-engine-design/02-data-foundation.md
[typographical] ~1-~1: To join two clauses or introduce examples, consider using an em dash.
Context: # Section 1: The Data Foundation - Ingesting and Integrating Open Data The...
(DASH_RULE)
[grammar] ~30-~30: Please add a punctuation mark at the end of paragraph.
Context: ...quence of independent "blobs," a design that osmpbf leverages to process these bl...
(PUNCTUATION_PARAGRAPH_END)
[style] ~45-~45: This expression can be considered offensive.
Context: ...rate is licensed under the "Do What The Fuck You Want To Public Licence, Version 2" (WTF...
(PROFANITY)
[uncategorized] ~123-~123: The preposition ‘to’ seems more likely in this position.
Context: ...ous reqwest client, issues requests for the Tokio runtime, and always sends a d...
(AI_HYDRA_LEO_REPLACE_FOR_TO)
[style] ~161-~161: You have used the passive voice repeatedly in nearby sentences. To make your writing clearer and easier to read, consider using active voice.
Context: ... referenced by the PoiEntityLinks set are processed further. For those entities, the parser...
(REP_PASSIVE_VOICE)
[style] ~218-~218: Would you like to use the Oxford spelling “initialize”? The spelling ‘initialise’ is also correct.
Context: ...Schema initialization is handled by the initialise_schema function, which enables foreign...
(OXFORD_SPELLING_Z_NOT_S)
[uncategorized] ~242-~242: Possible missing preposition found.
Context: ... thousands of queries per user request. Will be rate-limited and result in extreme l...
(AI_HYDRA_LEO_MISSING_IT)
[grammar] ~271-~271: Please add a punctuation mark at the end of paragraph.
Context: ... to work seamlessly with the types from the geo crate. The practical implementat...
(PUNCTUATION_PARAGRAPH_END)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Sourcery review
- GitHub Check: build
🔇 Additional comments (26)
docs/wildside-engine-design/06-implementation-testing-deployment.md (1)
1-166: Approve file with notation on static analysis.All past review comments have been correctly addressed:
- British spelling ("licence," "parameterized," "artefacts," "behavioural") ✓
- Comma before "so" at line 19 ✓
- Bold formatting for feature flags, schema fields, and infrastructure components ✓
- Redundant phrasing removed ("a separate repository" at line 144) ✓
- Extraneous "its" removed ✓
The file follows en-GB-oxendict conventions, maintains 80-column paragraph wrapping, and properly formats tables, headings, and lists. Content aligns with PR Phase 4 objectives and provides clear guidance on testing discipline, deployment strategy, and repository organisation.
Note on static analysis: LanguageTool flags line 69 for word repetition ("expected"), but this is a false positive—the uses at line 70 ("expected solution") and line 77 ("expected route") are semantically distinct and separated by multiple lines.
wildside-core/src/store/sqlite.rs (12)
1-17: LGTM!Module doc comment is present per guidelines, and imports are appropriate for the functionality.
19-21: LGTM!The SQLite variable limit constant is correctly documented and uses the well-known default value.
23-56: LGTM!The error enum is well-structured with semantic variants, proper
thiserrorderivation, and thorough documentation for each variant.
58-69: LGTM!The struct is appropriately documented, and the custom
Debugimplementation avoids dumping the entire index whilst indicating additional hidden state.
71-96: LGTM!The
openmethod correctly uses path references, maps errors appropriately, and validates index integrity against the SQLite database before bulk-loading the in-memory RTree.
98-115: LGTM!The
PoiStoretrait implementation correctly queries the in-memory RTree, clones results as required, and sorts for deterministic output order.
117-129: LGTM!The helper correctly uses
unreachable!for the invariant violation, and the binary search logic is sound given thatload_pois_chunkguarantees sorted output.
131-152: LGTM!The function correctly handles empty inputs, deduplicates IDs, and respects SQLite's parameter limit through chunking.
154-157: LGTM!The helper returns the safe default limit. Retaining the connection parameter for future tunability is a reasonable forward-looking pattern.
159-188: LGTM!The function safely constructs parameterised queries, properly maps JSON parsing errors to domain errors, and sorts results for downstream binary search.
190-231: LGTM!Test fixtures follow
rstestpatterns per coding guidelines, and the helper function provides clean test data construction.
233-363: LGTM!Test coverage is comprehensive, covering happy paths (bbox queries, sorting), edge cases (empty results), and all error variants (missing POI, corrupted magic, unsupported version, invalid tags). The tests correctly use
expect()as permitted in test code per coding guidelines.Cargo.toml (7)
1-8: LGTM!Package metadata correctly inherits workspace fields and disables publishing. This is a clean facade crate setup.
10-14: LGTM!Optional workspace dependencies for solver backends and explicit
default-features = falseon the core crate enable proper feature-flag composition.
23-25: LGTM!Workspace lint inheritance keeps the facade crate consistent with the rest of the workspace.
34-34: LGTM!New
wildside-solver-ortoolsmember correctly added to the workspace.
49-55: Workspace dependencies comply with SemVer caret requirements. All versions in this segment use proper explicit version ranges as mandated by the coding guidelines. The rstest-bdd and rstest-bdd-macros 0.3.0 releases are published.
21-21: No action required.wildside-solver-ortoolsdoes not expose atest-supportfeature, so the conditional concern does not apply. The current implementation correctly enablestest-supportonly for the crates that provide it.
38-42: No action required.The
rust-version = "1.92"is the correct, current stable Rust version (released December 11, 2025) and is appropriate for theedition = "2024"specification. The configuration is up-to-date and valid.docs/wildside-engine-design/05-solver-implementation.md (1)
1-149: Documentation structure and content are sound.The solver implementation design is well-structured, covering the trait interface, native Rust and OR-Tools backends, travel-time provider boundary, and implementation details. The technical content aligns with the feature-flag architecture described in the PR objectives.
wildside-cli/src/lib.rs (4)
69-83: Feature-gated ingestion flow is correctly implemented.The
#[cfg]blocks ensure exactly one code path compiles based on thestore-sqlitefeature. Thelet _ = args;pattern correctly silences the unused variable warning when the feature is disabled.
4-26: Feature-gated imports are well-organised.The conditional imports correctly align with their usage sites, avoiding dead-code warnings when
store-sqliteis disabled. This follows the coding guideline to conditionally compile feature-specific code.
140-158: Wikidata dump handling is robust.
open_wikidata_dumpcorrectly detects.bz2extension case-insensitively and wraps the file inMultiBzDecoderas appropriate. TheBufReaderwrapper improves I/O efficiency for both compressed and uncompressed paths.
243-263:TryFromimplementation correctly validates required fields.The implementation uses
ok_orto convertOptiontoResultwith descriptiveMissingArgumenterrors, and sensibly defaultsoutput_dirto the current directory. Error propagation follows guidelines..github/workflows/ci.yml (1)
69-105: No SQLite system dependency installation required.The codebase already correctly configures rusqlite with the
bundledfeature across all packages (wildside-core, wildside-cli, wildside-scorer, and wildside-data). The bundled feature statically links SQLite, eliminating the need for system packages like libsqlite3-dev. The CI workflow requires no changes.Likely an incorrect or invalid review comment.
Introduced `SourceFileAccess` error for IO errors encountered while accessing input paths in the CLI ingestion flow. Updated `require_existing` to distinguish between missing files and other IO errors, returning the new error variant accordingly. Also includes minor docs formatting fixes. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
leynos
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @docs/wildside-engine-design/01-introduction.md:
- Around line 1-188: The document contains a dangling footnote marker
"computationally demanding.1" (the inline marker intended as [^1]) but lacks the
corresponding footnote definition; add a footnote definition line `[^1]: Rust's
performance, memory-safety, and concurrency guarantees make it particularly
well-suited for data-intensive, concurrent workloads such as parsing large
binary files and running complex algorithms.` at the end of the file so the
reference resolves and follows GitHub-flavoured Markdown footnote formatting.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
docs/wildside-engine-design/01-introduction.mdwildside-cli/src/error.rswildside-cli/src/lib.rs
🧰 Additional context used
📓 Path-based instructions (5)
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: Use the markdown files within thedocs/directory as a knowledge base and source of truth for project requirements, dependency choices, and architectural decisions.
When new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve, proactively update the relevant file(s) in thedocs/directory to reflect the latest state. Ensure the documentation remains accurate and current.
Documentation must use en-GB-oxendict spelling and grammar ("-ize" / "-yse" / "-our") with the exception of the LICENSE filename.
docs/**/*.md: Use British English based on the Oxford English Dictionary locale en-GB, including -ize suffixes (e.g., realize, organization), -lyse suffixes (e.g., analyse, paralyse), -our suffixes (e.g., colour, behaviour), -re suffixes (e.g., centre, calibre), double 'l' (e.g., cancelled, counsellor), maintained 'e' (e.g., likeable, rateable), -ogue suffixes (e.g., analogue, catalogue)
Use the Oxford comma in documentation: 'ships, planes, and hovercraft' where it aids comprehension
Treat company names as collective nouns, for example: 'Concordat Industries are expanding'
Write headings in sentence case
Use Markdown headings (#, ##, ###, and so on) in order without skipping levels
Follow markdownlint recommendations
Provide code blocks and lists using standard Markdown syntax
Always provide a language identifier for fenced code blocks; use 'plaintext' for non-code text
Use '-' as the first level bullet and renumber lists when items change
Prefer inline links using 'text' or angle brackets around the URL
Ensure blank lines before and after bulleted lists and fenced blocks
Ensure tables have a delimiter line below the header row
Expand any uncommon acronym on first use, for example, Continuous Integration (CI)
Wrap paragraphs at 80 columns
Do not wrap tables
Use GitHub-flavoured numeric footnotes referenced as '[^1]'
Footnotes must be numbered in order of appearance in the document
Caption ev...
Files:
docs/wildside-engine-design/01-introduction.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Validate Markdown files usingmake markdownlint.
Runmake fmtafter any documentation changes to format all Markdown files and fix table markup.
Validate Mermaid diagrams in Markdown files by runningmake nixie.
Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks in Markdown must be wrapped at 120 columns.
Markdown tables and headings must not be wrapped.
Use dashes (-) for list bullets in Markdown.
Use GitHub-flavoured Markdown footnotes ([^1]) for references and footnotes.
Files:
docs/wildside-engine-design/01-introduction.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/wildside-engine-design/01-introduction.md
docs/**/!(README).md
📄 CodeRabbit inference engine (docs/documentation-style-guide.md)
Avoid first and second person personal pronouns outside the README.md file
Files:
docs/wildside-engine-design/01-introduction.md
docs/**/*.{md,rs}
📄 CodeRabbit inference engine (docs/documentation-style-guide.md)
Wrap code at 120 columns
Files:
docs/wildside-engine-design/01-introduction.md
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Runmake check-fmt,make lint, andmake testbefore committing in Rust projects.
make check-fmtexecutescargo fmt --workspace -- --checkto validate formatting across the entire workspace without modifying files.
make lintexecutescargo clippy --workspace --all-targets --all-features -- -D warningsto lint every target with all features enabled and deny all Clippy warnings.
make testexecutescargo test --workspaceto run the full workspace test suite.
Clippy warnings MUST be disallowed in Rust code.
Fix any warnings emitted during tests in the Rust code itself rather than silencing them.
Where a Rust function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a Rust function has too many parameters, group related parameters in meaningfully named structs.
Where a Rust function is returning a large error, consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new functionality in Rust. Run both before and after making any change.
Every Rust module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs in Rust using Rustdoc comments (///) so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessarymutbindings in Rust.
Handle errors with theResulttype instead of panicking in Rust where feasible.
Avoidunsafecode in Rust unless absolutely necessary, and document any usage clearly.
Place function attributes after doc comments in Rust.
Do not usereturnin single-line Rust functions.
Use predicate functions for conditional criteria in Rust with more than two branches.
Rust lints must not be silenced except as a last resort.
Rust lint rule suppressions must be tightly scoped and include a clear reason.
Preferexpectoverallowin Rust.
Userstestfixtures for shared setup in Rust tests.
Replace duplicated Rust tests with `#[rstest(...)...
Files:
wildside-cli/src/lib.rswildside-cli/src/error.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.
- Adhere to single responsibility and CQRS
- Place function attributes after doc comments.
- Do not use
returnin single-line functions.- Move conditionals with >2 branches into a predicate function.
- Avoid
unsafeunless absolutely necessary.- Every module must begin with a
//!doc comment that explains the module's purpose and utility.- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.- Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
- Use
rstestfixtures for shared setup and to avoid repetition between tests.- Replace duplicated tests with
#[rstest(...)]parameterised cases.- Prefer
mockallfor mocks/stubs.- Prefer
.expect()over.unwrap()in tests..expect()and.unwrap()are forbidden outside of tests. Errors must be propagated.- Ensure that any API or behavioural changes are reflected in the documentation in
docs/- Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/- Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
- Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and...
Files:
wildside-cli/src/lib.rswildside-cli/src/error.rs
🧠 Learnings (1)
📚 Learning: 2026-01-03T14:22:19.441Z
Learnt from: leynos
Repo: leynos/wildside-engine PR: 81
File: docs/wildside-engine-design/02-data-foundation.md:0-0
Timestamp: 2026-01-03T14:22:19.441Z
Learning: Apply en-GB Oxford English spelling in Markdown files by preferring -ize (e.g., normalize, initialize, organize) over -ise, -yse over -yze (e.g., analyse, paralyse), and -our over -or (e.g., colour, favour). The Oxford British English convention favors -ize in these contexts. This guidance should be applied across Markdown documentation in the docs directory to maintain consistent spelling.
Applied to files:
docs/wildside-engine-design/01-introduction.md
🧬 Code graph analysis (1)
wildside-cli/src/lib.rs (4)
wildside-core/src/store/spatial_index.rs (1)
write_spatial_index(90-95)wildside-data/src/wikidata/store/persistence.rs (1)
persist_claims_to_path(285-295)wildside-data/src/ingest/mod.rs (1)
ingest_osm_pbf_report(149-202)wildside-data/src/ingest/sqlite.rs (1)
persist_pois_to_sqlite(97-122)
🪛 LanguageTool
docs/wildside-engine-design/01-introduction.md
[style] ~16-~16: This word has been used in one of the immediately preceding sentences. Using a synonym could make your text more interesting to read, unless the repetition is intentional.
Context: ...ithms, and solving NP-hard optimization problems all align with the Rust ecosystem's str...
(EN_REPEATEDWORDS_PROBLEM)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Sourcery review
- GitHub Check: build
🔇 Additional comments (19)
wildside-cli/src/lib.rs (14)
4-26: LGTM: Import gating is correct.The conditional compilation correctly gates all ingestion implementation dependencies behind
store-sqlitewhilst keepingUtf8PathBufunconditional for the always-availableIngestArgsstruct.
43-46: LGTM: Environment constants correctly gated.The constants are only used within the feature-gated
TryFrom<IngestArgs>implementation.
69-83: LGTM: Feature gating pattern is correct.The dual implementation correctly returns
MissingFeaturewhenstore-sqliteis disabled whilst preserving the function signature for consistent CLI dispatch.
85-90: LGTM: Configuration resolution is clean.
127-138: LGTM: Wikidata claims extraction is correct.The early return for empty links avoids unnecessary processing.
140-151: LGTM: Wikidata dump opening handles BZ2 decompression.
153-158: LGTM: Extension check is correct.
204-209: LGTM: Configuration merging follows OrthoConfig pattern.
211-217: LGTM: IngestConfig struct is well-defined.
232-251: LGTM: File existence validation correctly distinguishes error cases.The three-way error handling (file exists, file missing, IO error) aligns with the new
SourceFileAccessvariant.
254-274: LGTM: TryFrom conversion handles missing arguments correctly.
276-284: LGTM: IngestOutcome captures ingestion results.
286-290: LGTM: Unit type preserves signature across feature permutations.The phantom unit type is a pragmatic solution for maintaining a consistent return type when ingestion always fails with
MissingFeature.
92-125: Add parent directory creation towrite_spatial_index.The
write_indexfunction directly callsFile::create(path)without ensuring the parent directory exists. This differs frompersist_pois_to_sqlite, which callsensure_parent_dir(path)?before opening the connection. Updatewrite_indexto callensure_parent_dirbefore file creation to prevent failures when output_dir lacks the required hierarchy.wildside-cli/src/error.rs (5)
12-13: LGTM: Import gating matches usage.The
SpatialIndexWriteErrorimport is consistently gated with its usage in theWriteSpatialIndexvariant.
35-40: LGTM: MissingFeature variant is well-designed.The variant correctly uses static string slices and provides clear error messages for missing compile-time features.
53-60: LGTM: SourceFileAccess variant correctly distinguishes IO errors.The new variant properly separates IO errors from missing file errors, as described in the commit message.
100-106: LGTM: WriteSpatialIndex variant correctly feature-gated.The variant is only available when
store-sqliteis enabled, matching the availability of the underlying error type.
129-131: LGTM: OpenPoiStore variant correctly feature-gated.The transparent delegation to
SqlitePoiStoreErroris appropriate and matches the feature availability.
…otation to import Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
leynos
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @wildside-cli/src/lib.rs:
- Around line 70-84: The unused args suppression should use a narrow #[expect]
on the parameter instead of a blanket let _ = args;—update the run_ingest
signature (parameter name args in fn run_ingest(args: IngestArgs)) to apply
#[expect(unused_variables, reason = "feature-gated; preserved for when
store-sqlite is enabled")] to that parameter and remove the let _ = args; line
so the lint is explicitly documented and scoped.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wildside-cli/src/lib.rs (1)
56-56: Add doc comment to public function.Document
pub fn run()with a///doc comment. The guidelines require public APIs to use Rustdoc comments.Suggested addition
+/// Run the Wildside CLI with the current process arguments and environment. +/// +/// Parses CLI arguments and dispatches to the appropriate subcommand handler. +/// Returns a [`CliError`] if argument parsing fails or the subcommand encounters an error. pub fn run() -> Result<(), CliError> {As per coding guidelines: "Document public APIs in Rust using Rustdoc comments (
///) so documentation can be generated with cargo doc."
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
wildside-cli/src/lib.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Runmake check-fmt,make lint, andmake testbefore committing in Rust projects.
make check-fmtexecutescargo fmt --workspace -- --checkto validate formatting across the entire workspace without modifying files.
make lintexecutescargo clippy --workspace --all-targets --all-features -- -D warningsto lint every target with all features enabled and deny all Clippy warnings.
make testexecutescargo test --workspaceto run the full workspace test suite.
Clippy warnings MUST be disallowed in Rust code.
Fix any warnings emitted during tests in the Rust code itself rather than silencing them.
Where a Rust function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a Rust function has too many parameters, group related parameters in meaningfully named structs.
Where a Rust function is returning a large error, consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new functionality in Rust. Run both before and after making any change.
Every Rust module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs in Rust using Rustdoc comments (///) so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessarymutbindings in Rust.
Handle errors with theResulttype instead of panicking in Rust where feasible.
Avoidunsafecode in Rust unless absolutely necessary, and document any usage clearly.
Place function attributes after doc comments in Rust.
Do not usereturnin single-line Rust functions.
Use predicate functions for conditional criteria in Rust with more than two branches.
Rust lints must not be silenced except as a last resort.
Rust lint rule suppressions must be tightly scoped and include a clear reason.
Preferexpectoverallowin Rust.
Userstestfixtures for shared setup in Rust tests.
Replace duplicated Rust tests with `#[rstest(...)...
Files:
wildside-cli/src/lib.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.
- Adhere to single responsibility and CQRS
- Place function attributes after doc comments.
- Do not use
returnin single-line functions.- Move conditionals with >2 branches into a predicate function.
- Avoid
unsafeunless absolutely necessary.- Every module must begin with a
//!doc comment that explains the module's purpose and utility.- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.- Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
- Use
rstestfixtures for shared setup and to avoid repetition between tests.- Replace duplicated tests with
#[rstest(...)]parameterised cases.- Prefer
mockallfor mocks/stubs.- Prefer
.expect()over.unwrap()in tests..expect()and.unwrap()are forbidden outside of tests. Errors must be propagated.- Ensure that any API or behavioural changes are reflected in the documentation in
docs/- Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/- Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
- Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and...
Files:
wildside-cli/src/lib.rs
🧬 Code graph analysis (1)
wildside-cli/src/lib.rs (5)
wildside-core/src/store/spatial_index.rs (1)
write_spatial_index(90-95)wildside-data/src/wikidata/etl/mod.rs (1)
extract_linked_entity_claims(153-198)wildside-data/src/wikidata/store/persistence.rs (1)
persist_claims_to_path(285-295)wildside-data/src/ingest/mod.rs (1)
ingest_osm_pbf_report(149-202)wildside-data/src/ingest/sqlite.rs (1)
persist_pois_to_sqlite(97-122)
🔍 Remote MCP
The web search didn't find the specific Wildside project. Let me search more specifically for Wildside and feature-flagging patterns:Excellent! Now let me search for information about testing strategies with conditional features:## Summary of Additional Context Found
Based on my search findings, here is relevant context that would help in reviewing this PR:
Rust Feature Flagging Best Practices
Conditional compilation in Rust is determined statically during compilation, allowing code to be compiled only under certain conditions using #[cfg] and #[cfg_attr] attributes. The PR's approach of using #[cfg(feature = "store-sqlite")] and similar gates aligns with standard Rust conventions.
Testing Strategy for Feature Combinations
Because features require an exponential number of configurations and test cases to be 100% covered, by default tests and documentation will only run with the default set of features. The PR's CI workflow addition with 8 feature combinations (visible in .github/workflows/ci.yml) appropriately addresses this challenge. Documentation recommends considering a testing strategy based on time, resources, and cost-benefit of covering specific scenarios in relation to feature combinations.
Feature Testing Tools
For projects utilizing Rust feature flags, it's common to set up a test matrix in CI to individually test all feature flags, though this setup can be difficult to maintain and easy to forget to update. The PR's manual feature matrix in CI (8 configurations) represents a pragmatic approach for a workspace with 3 main features (solver-vrp, solver-ortools, store-sqlite).
Key Design Pattern in PR
A recommended approach is to have a public interface with documentation calling an internal method implemented in conditionally compiled modules. The PR implements this pattern with:
- Public re-exports in
lib.rs(wildside-engine facade) - Feature-gated internal modules (e.g.,
#[cfg(feature = "store-sqlite")]onwildside-core/src/store/sqlite.rs) - Fallback error types (
MissingFeature) for disabled features
Workspace Feature Coordination
In workspaces with multiple packages, package features can depend on features from other crates, and the plugin can check required features and suggest quick-fixes, though features that depend on each other need manual management. The PR's use of workspace-level feature dependencies (default = ["serde", "store-sqlite"] in root Cargo.toml) coordinates these across multiple crates.
[::web_search::]
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Sourcery review
- GitHub Check: build
🔇 Additional comments (2)
wildside-cli/src/lib.rs (2)
93-126: LGTM – ingestion pipeline is well-structured.The
execute_ingestfunction clearly orchestrates the ingestion workflow with proper error wrapping. Each step provides contextual information (path, source error) that will aid debugging. The feature gating ensures this code is only compiled whenstore-sqliteis enabled.
287-291: LGTM – type alias pattern preserves function signature across features.The conditional
IngestOutcometype (struct vs.()) is an elegant solution to maintainrun_ingest's signature across feature permutations. Whenstore-sqliteis disabled, the function returns early withMissingFeature, so the outcome type is never constructed. The doc comment clearly explains this intent.
…erences - Added missing dependency ordering in GitHub Actions CI workflow. - Enhanced Markdown formatting in multiple design documents. - Converted inline numeric citations to Markdown footnotes for clarity. - Updated tables with footnote references and improved text formatting. - Added extensive documentation references across several wildside-engine-design docs. - Minor typo fixes and consistency improvements in code comments and docs. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 4
Fix all issues with AI Agents 🤖
In @.github/workflows/ci.yml:
- Around line 105-108: Add an explicit echo of the TEST_FLAGS before running
tests so CI logs show active feature flags: modify the test step run command
(which currently just executes "make test") to a multi-line shell script that
first runs echo "Testing with flags: $TEST_FLAGS" and then runs "make test",
keeping the existing env mapping for TEST_FLAGS unchanged; ensure the quotes and
variable expansion are correct so the value prints in the workflow logs.
- Around line 69-91: The feature-matrix job is missing a job-level timeout; add
a timeout-minutes entry (e.g., timeout-minutes: 30) to the feature-matrix job
definition so the job fails fast on hangs—insert the line with two-space
indentation under the feature-matrix job (i.e., directly before the existing
strategy: block) so the job 'feature-matrix' has a timeout configured.
In @docs/wildside-engine-design/02-data-foundation.md:
- Around line 278-293: Update the access dates in the footnote citations for
markers [^2], [^3], [^4], [^5], [^6], [^8], [^9], [^10], and [^12] by replacing
"August 13, 2025" with a current, consistent access date (e.g., "January 5,
2026" or another chosen date), ensuring all cited lines in the block use the
same updated date for consistency.
In @docs/wildside-engine-design/05-solver-implementation.md:
- Around line 152-158: Update the three footnote access dates referencing
"August 13, 2025" to "August 13, 2026" in the document; specifically edit the
footnote entries for [^13], [^15], and [^16] in 05-solver-implementation.md so
each occurrence of "August 13, 2025" becomes "August 13, 2026".
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/ci.ymldocs/wildside-engine-design/01-introduction.mddocs/wildside-engine-design/02-data-foundation.mddocs/wildside-engine-design/05-solver-implementation.md
🧰 Additional context used
📓 Path-based instructions (4)
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: Use the markdown files within thedocs/directory as a knowledge base and source of truth for project requirements, dependency choices, and architectural decisions.
When new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve, proactively update the relevant file(s) in thedocs/directory to reflect the latest state. Ensure the documentation remains accurate and current.
Documentation must use en-GB-oxendict spelling and grammar ("-ize" / "-yse" / "-our") with the exception of the LICENSE filename.
docs/**/*.md: Use British English based on the Oxford English Dictionary locale en-GB, including -ize suffixes (e.g., realize, organization), -lyse suffixes (e.g., analyse, paralyse), -our suffixes (e.g., colour, behaviour), -re suffixes (e.g., centre, calibre), double 'l' (e.g., cancelled, counsellor), maintained 'e' (e.g., likeable, rateable), -ogue suffixes (e.g., analogue, catalogue)
Use the Oxford comma in documentation: 'ships, planes, and hovercraft' where it aids comprehension
Treat company names as collective nouns, for example: 'Concordat Industries are expanding'
Write headings in sentence case
Use Markdown headings (#, ##, ###, and so on) in order without skipping levels
Follow markdownlint recommendations
Provide code blocks and lists using standard Markdown syntax
Always provide a language identifier for fenced code blocks; use 'plaintext' for non-code text
Use '-' as the first level bullet and renumber lists when items change
Prefer inline links using 'text' or angle brackets around the URL
Ensure blank lines before and after bulleted lists and fenced blocks
Ensure tables have a delimiter line below the header row
Expand any uncommon acronym on first use, for example, Continuous Integration (CI)
Wrap paragraphs at 80 columns
Do not wrap tables
Use GitHub-flavoured numeric footnotes referenced as '[^1]'
Footnotes must be numbered in order of appearance in the document
Caption ev...
Files:
docs/wildside-engine-design/01-introduction.mddocs/wildside-engine-design/05-solver-implementation.mddocs/wildside-engine-design/02-data-foundation.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Validate Markdown files usingmake markdownlint.
Runmake fmtafter any documentation changes to format all Markdown files and fix table markup.
Validate Mermaid diagrams in Markdown files by runningmake nixie.
Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks in Markdown must be wrapped at 120 columns.
Markdown tables and headings must not be wrapped.
Use dashes (-) for list bullets in Markdown.
Use GitHub-flavoured Markdown footnotes ([^1]) for references and footnotes.
Files:
docs/wildside-engine-design/01-introduction.mddocs/wildside-engine-design/05-solver-implementation.mddocs/wildside-engine-design/02-data-foundation.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/wildside-engine-design/01-introduction.mddocs/wildside-engine-design/05-solver-implementation.mddocs/wildside-engine-design/02-data-foundation.md
docs/**/!(README).md
📄 CodeRabbit inference engine (docs/documentation-style-guide.md)
Avoid first and second person personal pronouns outside the README.md file
Files:
docs/wildside-engine-design/01-introduction.mddocs/wildside-engine-design/05-solver-implementation.mddocs/wildside-engine-design/02-data-foundation.md
docs/**/*.{md,rs}
📄 CodeRabbit inference engine (docs/documentation-style-guide.md)
Wrap code at 120 columns
Files:
docs/wildside-engine-design/01-introduction.mddocs/wildside-engine-design/05-solver-implementation.mddocs/wildside-engine-design/02-data-foundation.md
🧠 Learnings (2)
📚 Learning: 2026-01-03T14:22:19.441Z
Learnt from: leynos
Repo: leynos/wildside-engine PR: 81
File: docs/wildside-engine-design/02-data-foundation.md:0-0
Timestamp: 2026-01-03T14:22:19.441Z
Learning: Apply en-GB Oxford English spelling in Markdown files by preferring -ize (e.g., normalize, initialize, organize) over -ise, -yse over -yze (e.g., analyse, paralyse), and -our over -or (e.g., colour, favour). The Oxford British English convention favors -ize in these contexts. This guidance should be applied across Markdown documentation in the docs directory to maintain consistent spelling.
Applied to files:
docs/wildside-engine-design/01-introduction.mddocs/wildside-engine-design/05-solver-implementation.mddocs/wildside-engine-design/02-data-foundation.md
📚 Learning: 2026-01-04T04:50:10.691Z
Learnt from: leynos
Repo: leynos/rstest-bdd PR: 384
File: crates/rstest-bdd/tests/async_step_infrastructure.rs:4-4
Timestamp: 2026-01-04T04:50:10.691Z
Learning: In en-GB-oxendict (Oxford English) spelling convention, verbs use the -ize ending (e.g., "normalizes," "organizes," "realizes"), not -ise. The convention is specifically -ize / -yse / -our, so "normalizes" is correct in rstest-bdd documentation and comments.
Applied to files:
docs/wildside-engine-design/02-data-foundation.md
🪛 LanguageTool
docs/wildside-engine-design/01-introduction.md
[style] ~16-~16: This word has been used in one of the immediately preceding sentences. Using a synonym could make your text more interesting to read, unless the repetition is intentional.
Context: ...ithms, and solving NP-hard optimization problems all align with the Rust ecosystem's str...
(EN_REPEATEDWORDS_PROBLEM)
docs/wildside-engine-design/05-solver-implementation.md
[inconsistency] ~152-~152: A new year has begun. Did you mean “August 13, 2026”?
Context: ... vrp-core crate on docs.rs, accessed on August 13, 2025, https://docs.rs/vrp-core [^15]: So...
(DATE_NEW_YEAR)
[inconsistency] ~155-~155: A new year has begun. Did you mean “August 13, 2026”?
Context: ...routing problem in Java", accessed on August 13, 2025, <https://softwaremill.com/solving-ve...
(DATE_NEW_YEAR)
[inconsistency] ~157-~157: A new year has begun. Did you mean “August 13, 2026”?
Context: ...P-SAT solver documentation, accessed on August 13, 2025, <https://developers.google.com/optim...
(DATE_NEW_YEAR)
docs/wildside-engine-design/02-data-foundation.md
[typographical] ~1-~1: To join two clauses or introduce examples, consider using an em dash.
Context: # Section 1: The Data Foundation - Ingesting and Integrating Open Data The...
(DASH_RULE)
[style] ~42-~42: This expression can be considered offensive.
Context: ...rate is licensed under the "Do What The Fuck You Want To Public Licence, Version 2" (WTF...
(PROFANITY)
[uncategorized] ~120-~120: The preposition ‘to’ seems more likely in this position.
Context: ...ous reqwest client, issues requests for the Tokio runtime, and always sends a d...
(AI_HYDRA_LEO_REPLACE_FOR_TO)
[style] ~158-~158: You have used the passive voice repeatedly in nearby sentences. To make your writing clearer and easier to read, consider using active voice.
Context: ... referenced by the PoiEntityLinks set are processed further. For those entities, the parser...
(REP_PASSIVE_VOICE)
[style] ~215-~215: Would you like to use the Oxford spelling “initialize”? The spelling ‘initialise’ is also correct.
Context: ...Schema initialization is handled by the initialise_schema function, which enables foreign...
(OXFORD_SPELLING_Z_NOT_S)
[uncategorized] ~239-~239: Possible missing preposition found.
Context: ... thousands of queries per user request. Will be rate-limited and result in extreme l...
(AI_HYDRA_LEO_MISSING_IT)
[inconsistency] ~278-~278: A new year has begun. Did you mean “August 13, 2026”?
Context: ... osmpbf crate on crates.io, accessed on August 13, 2025, https://crates.io/crates/osmpbf [^...
(DATE_NEW_YEAR)
[inconsistency] ~280-~280: A new year has begun. Did you mean “August 13, 2026”?
Context: ...freader crate on crates.io, accessed on August 13, 2025, <https://crates.io/crates/osmpbfread...
(DATE_NEW_YEAR)
[inconsistency] ~282-~282: A new year has begun. Did you mean “August 13, 2026”?
Context: ...yword listing on crates.io, accessed on August 13, 2025, https://crates.io/keywords/osm [^5...
(DATE_NEW_YEAR)
[inconsistency] ~284-~284: A new year has begun. Did you mean “August 13, 2026”?
Context: ... wikidata crate on docs.rs, accessed on August 13, 2025, https://docs.rs/wikidata [^6]: spa...
(DATE_NEW_YEAR)
[inconsistency] ~286-~286: A new year has begun. Did you mean “August 13, 2026”?
Context: ...-client crate on crates.io, accessed on August 13, 2025, <https://crates.io/crates/sparql-cli...
(DATE_NEW_YEAR)
[inconsistency] ~288-~288: A new year has begun. Did you mean “August 13, 2026”?
Context: ...al categories on crates.io, accessed on August 13, 2025, <https://crates.io/categories/scienc...
(DATE_NEW_YEAR)
[inconsistency] ~290-~290: A new year has begun. Did you mean “August 13, 2026”?
Context: ...[^9]: geo crate on docs.rs, accessed on August 13, 2025, https://docs.rs/geo [^10]: rstar cra...
(DATE_NEW_YEAR)
[inconsistency] ~291-~291: A new year has begun. Did you mean “August 13, 2026”?
Context: ...0]: rstar crate on docs.rs, accessed on August 13, 2025, https://docs.rs/rstar [^12]: rstar...
(DATE_NEW_YEAR)
[inconsistency] ~293-~293: A new year has begun. Did you mean “August 13, 2026”?
Context: ...2]: rstar crate on docs.rs, accessed on August 13, 2025, https://docs.rs/rstar
(DATE_NEW_YEAR)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Sourcery review
- GitHub Check: build
🔇 Additional comments (5)
docs/wildside-engine-design/01-introduction.md (1)
1-190: Excellent documentation foundation.All prior issues have been resolved. The file is well-structured, uses proper Markdown footnote syntax, contains valid Mermaid diagrams, adheres to en-GB-oxendict spelling conventions, and follows the coding guidelines for paragraph wrapping and punctuation. The domain model, spatial indexing design, and runtime architecture are clearly explained with supporting diagrams and sequence flows.
docs/wildside-engine-design/02-data-foundation.md (4)
120-120: Preposition usage is correct; skip flagged suggestion.LanguageTool suggests changing "for the Tokio runtime" to "to the Tokio runtime," but the current phrasing is semantically and idiomatically correct. The
reqwestclient issues requests for execution on the Tokio runtime, not to it. No change required.
214-215: Verb spelling is correct per project conventions.The use of "initialization" (with -ize suffix) complies with the project's en-GB-oxendict standard. The code identifier
initialise_schemaretains its original spelling. No change required.
239-239: Sentence structure is grammatically correct.LanguageTool flags a possible missing preposition, but the sentence "Will be rate-limited and result in extreme latency" is complete and idiomatic. Both clauses are properly formed. No change required.
1-277: Document structure and formatting comply with guidelines.The document follows proper Markdown conventions, maintains consistent British English (en-GB-oxendict) spelling, uses correct heading hierarchy, and properly formats tables and footnotes. The technical content aligns well with the PR objectives regarding ingestion architecture and SQLite-backed data storage. The only outstanding issue is the outdated footnote dates (lines 278–293), which have already been flagged separately.
- Set 30 minute timeout for feature-matrix job in CI - Add echo of TEST_FLAGS environment variable before running tests - Annotate unused variable warning for args in run_ingest when store-sqlite feature disabled Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wildside-cli/src/lib.rs (1)
56-68: Document the public API function.The public function
run()lacks a doc comment. Document all public APIs using Rustdoc comments (///) forcargo docgeneration.🔎 Proposed fix
+/// Run the Wildside CLI with the current process arguments and environment. +/// +/// Parses command-line arguments, dispatches to the appropriate subcommand handler, +/// and returns an error if argument parsing or command execution fails. +/// +/// # Errors +/// +/// Returns [`CliError`] if argument parsing fails or if the selected subcommand +/// encounters an error during execution. pub fn run() -> Result<(), CliError> {
♻️ Duplicate comments (1)
wildside-cli/src/lib.rs (1)
73-77: Complete the past review suggestion for parameter-level lint suppression.The past review comment suggested using
#[cfg_attr]on the parameter itself rather than a let binding. The current code has improved by adding a proper#[expect]with reason, but the let binding can still be eliminated by applying the attribute directly to the parameter.🔎 Complete the suggested refactor
Apply the attribute to the parameter and remove the let binding:
-fn run_ingest(args: IngestArgs) -> Result<IngestOutcome, CliError> { +fn run_ingest( + #[cfg_attr(not(feature = "store-sqlite"), expect(unused_variables, reason = "args not used when store-sqlite disabled"))] + args: IngestArgs +) -> Result<IngestOutcome, CliError> { #[cfg(not(feature = "store-sqlite"))] { - #[expect( - unused_variables, - reason = "args only used when store-sqlite is enabled" - )] - let _args = args; return Err(CliError::MissingFeature {🤖 Prompt for AI Agents
In @wildside-cli/src/lib.rs at line 70, update the run_ingest function signature to apply #[cfg_attr(not(feature = "store-sqlite"), expect(unused_variables, reason = "args not used when store-sqlite disabled"))] to the args parameter, and remove lines 73-77 (the #[expect] attribute and let _args = args; statement). The updated signature should span lines 70-73, with the opening brace on line 74.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/ci.ymlwildside-cli/src/lib.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Runmake check-fmt,make lint, andmake testbefore committing in Rust projects.
make check-fmtexecutescargo fmt --workspace -- --checkto validate formatting across the entire workspace without modifying files.
make lintexecutescargo clippy --workspace --all-targets --all-features -- -D warningsto lint every target with all features enabled and deny all Clippy warnings.
make testexecutescargo test --workspaceto run the full workspace test suite.
Clippy warnings MUST be disallowed in Rust code.
Fix any warnings emitted during tests in the Rust code itself rather than silencing them.
Where a Rust function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a Rust function has too many parameters, group related parameters in meaningfully named structs.
Where a Rust function is returning a large error, consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new functionality in Rust. Run both before and after making any change.
Every Rust module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs in Rust using Rustdoc comments (///) so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessarymutbindings in Rust.
Handle errors with theResulttype instead of panicking in Rust where feasible.
Avoidunsafecode in Rust unless absolutely necessary, and document any usage clearly.
Place function attributes after doc comments in Rust.
Do not usereturnin single-line Rust functions.
Use predicate functions for conditional criteria in Rust with more than two branches.
Rust lints must not be silenced except as a last resort.
Rust lint rule suppressions must be tightly scoped and include a clear reason.
Preferexpectoverallowin Rust.
Userstestfixtures for shared setup in Rust tests.
Replace duplicated Rust tests with `#[rstest(...)...
Files:
wildside-cli/src/lib.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.
- Adhere to single responsibility and CQRS
- Place function attributes after doc comments.
- Do not use
returnin single-line functions.- Move conditionals with >2 branches into a predicate function.
- Avoid
unsafeunless absolutely necessary.- Every module must begin with a
//!doc comment that explains the module's purpose and utility.- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.- Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
- Use
rstestfixtures for shared setup and to avoid repetition between tests.- Replace duplicated tests with
#[rstest(...)]parameterised cases.- Prefer
mockallfor mocks/stubs.- Prefer
.expect()over.unwrap()in tests..expect()and.unwrap()are forbidden outside of tests. Errors must be propagated.- Ensure that any API or behavioural changes are reflected in the documentation in
docs/- Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/- Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
- Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and...
Files:
wildside-cli/src/lib.rs
🧬 Code graph analysis (1)
wildside-cli/src/lib.rs (5)
wildside-core/src/store/spatial_index.rs (1)
write_spatial_index(90-95)wildside-data/src/wikidata/etl/mod.rs (1)
extract_linked_entity_claims(153-198)wildside-data/src/wikidata/store/persistence.rs (1)
persist_claims_to_path(285-295)wildside-data/src/ingest/mod.rs (1)
ingest_osm_pbf_report(149-202)wildside-data/src/ingest/sqlite.rs (1)
persist_pois_to_sqlite(97-122)
🔍 Remote MCP
I'll search for relevant information to help you review this PR effectively. Given the focus on feature flags, workspace dependencies, and conditional compilation, let me gather some context.
Summary of Relevant Context for PR Review
Based on comprehensive research into Rust feature flag patterns, workspace management, and conditional compilation best practices, here's the relevant context for effectively reviewing this Phase 4 PR:
Feature Flags & Optional Dependencies
Every package must ensure that default-features = false is specified to avoid enabling default features automatically, since dependencies enable default features unless default-features = false is specified. Optional dependencies can be marked "optional" to exclude them from compilation by default, and implicitly define a feature with the same name, which can be referenced with cfg(feature = "...") syntax.
Key insight for review: This PR's approach of marking wildside-solver-ortools and wildside-solver-vrp as optional dependencies and gating them behind feature flags follows best practices.
Feature Unification in Workspaces
When building multiple packages in a workspace, the features of the dependencies of all packages are unified, and if you want to avoid that unification for different workspace members, you need to build them via separate cargo invocations. If a crate has N independent features, then all 2^N possible build combinations can occur in practice, and it's important to ensure CI covers all these combinations.
Key insight for review: The PR's feature-matrix CI job testing 8 distinct feature flag combinations is an excellent defensive measure against feature unification issues.
Workspace Dependencies Best Practices
Workspace dependencies can inherit base definitions from [workspace.dependencies], and features declared inline are additive with features from the workspace definition. Optional dependencies can be combined with the workspace = true key, and features specified inline are unioned with features in the workspace table.
Key insight for review: The PR's use of workspace.dependencies to centralize dependency management while allowing member crates to selectively mark dependencies as optional is architecturally sound.
Conditional Compilation Patterns
The goal should be to have readable code without #[cfg] everywhere in confusing combinations, and maintain scalable conditional compilation in a maintainable manner. Off-by-default code should be trivially correct (e.g., dummy shims that do nothing or return errors), and if off-by-default code is nontrivial, it should have dedicated CI jobs to ensure conditional parts are tested.
Key insight for review: The PR's approach of returning MissingFeature errors for disabled runtime paths, with dedicated test coverage in wildside-cli/src/tests/feature_flag_steps.rs and feature_flags.rs, is a good implementation of this pattern.
Re-export Facade Pattern
The PR implements a root-crate re-export facade (src/lib.rs) that conditionally exports solver/store types without changing public APIs in breaking ways, since enabling a feature should not introduce SemVer-incompatible changes.
Key insight for review: The approach of re-exporting core types unconditionally while gating implementation types (SqlitePoiStore, VrpSolver, OrtoolsSolver) maintains API stability while allowing pluggable implementations.
Areas to Focus Review
- Feature flag consistency: Verify all feature-gated conditional compilation uses the same feature names across Cargo.toml and source code
- Default features impact: The change from
default = ["serde"]todefault = ["serde", "store-sqlite"]inwildside-coreis a significant default set change—confirm this doesn't break downstream consumers - Error handling for missing features: Validate that
MissingFeatureerrors provide clear guidance on which features must be enabled for given operations - Test coverage breadth: The feature-matrix CI job is good; ensure it covers critical user workflows under each combination
- Documentation of feature-gating: The design doc updates should clearly explain feature flag purposes and trade-offs
[::web_search::]
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (5)
.github/workflows/ci.yml (4)
69-72: Excellent job setup with appropriate safeguards.The
needs: builddependency andtimeout-minutes: 30constraint provide good CI hygiene—feature tests won't run if basic validation fails, and hung tests will fail fast rather than consuming resources indefinitely. The structure is sound.
73-92: Comprehensive feature matrix coverage.The eight configurations provide excellent coverage of feature combinations: all three features in isolation, key pairwise combinations, no-features baseline, and all-features maximum. The
fail-fast: falsestrategy ensures you'll see failures across all combinations rather than stopping at the first, which is valuable for diagnosing feature-specific issues. This aligns with best practices for testing workspace feature unification.
106-111: Test execution with good observability.The explicit echo of
TEST_FLAGSat line 108 provides valuable debugging context in CI logs when investigating feature-specific failures. The environment variable plumbing through to the Makefile is correct.
69-111: Verify that all feature-matrix tests pass.The PR summary notes "Remaining work noted: formatting, linting, and full test validation across feature permutations." Confirm that all eight feature combinations in this matrix now pass successfully before merging, as the matrix is only valuable if the underlying code handles all these configurations correctly.
Run this workflow manually or verify in CI that all matrix entries complete successfully:
#!/bin/bash # Description: Check the status of feature-matrix tests in recent CI runs # Query recent workflow runs for this PR gh run list --workflow=ci.yml --limit 5 --json conclusion,status,displayTitle,url # For detailed logs of the most recent run: # gh run view --logwildside-cli/src/lib.rs (1)
90-295: Feature-gated design is well implemented.The feature-gated ingestion implementation demonstrates several strengths:
- Consistent feature gating: All store-sqlite-dependent code (imports, functions, types) is properly conditionally compiled.
- Dual-type IngestOutcome approach: The use of a struct definition when
store-sqliteis enabled and a unit type fallback when disabled (lines 291-295) cleverly preserves the function signature across feature permutations while preventing dead code compilation. The doc comment clearly explains the rationale.- Improved error handling: The
require_existingfunction (lines 237-256) appropriately distinguishesMissingSourceFile(file not found or not a file) fromSourceFileAccess(IO errors), providing clear diagnostics with field names.- Clean orchestration: The
execute_ingestworkflow (lines 97-130) cleanly sequences OSM PBF ingestion, Wikidata claims extraction, and spatial index generation with proper error context.This aligns well with the PR objectives for Phase 4 feature-flagging.
…isabled Replace unused variable annotation with drop() for args in non-store-sqlite builds. This removes unnecessary attributes and clarifies intent when the feature is not enabled. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
leynos
changed the title
leynos
deleted the
terragon/feature-flags-implementation-phase-4-dk5292
branch
1 participant
Summary
docs/wildside-engine-designwith a dedicated Phase-4 feature-flags ExecPlan artifact atdocs/execplans/phase-4-feature-flags.md.wildside-engine, which re-exports core types and conditionally exposes solver/store implementations via feature flags.--all-featuresbuilds.store-sqlitefeature and adjust exports accordingly, soSqlitePoiStoreis only available when that feature is enabled.Changes
wildside-engine, re-exporting core types and conditionally exposing solver/store implementations via feature flags.solver-vrp,solver-ortools,store-sqlite.wildside-solver-ortoolscrate as a placeholder backend and wired into the workspace so--all-featuresbuilds succeed.store-sqlitefeature, with dedicated store module layout and spatial index helpers.wildside-corenow exportsPoiStoreunconditionally andSqlitePoiStore/SqlitePoiStoreErroronly whenstore-sqliteis enabled.MissingFeatureerror variant for unsupported paths, and guards usage ofSqlitePoiStorebehind#[cfg(feature = "store-sqlite")].wildside-solver-ortoolscrate added to satisfy--all-featuresbuilds and provide a documented surface for future integration.docs/wildside-engine-designand a dedicateddocs/execplansPhase-4 feature-flags page.Rationale
Plan of Work (highlights)
solver-vrp,solver-ortools, andstore-sqliteand propagate through workspace crates.wildside-solver-ortoolscrate behind thesolver-ortoolsfeature and wire it through the workspace.wildside-core. Introduce a feature such asstore-sqlite(orsqliteif you want a shorter internal name) and makerusqlite,serde,serde_json, andbincodeoptional dependencies that are activated by that feature. Move SQLite-specific types (SqlitePoiStore,SqlitePoiStoreError, spatial index helpers) into a module compiled only when that feature is enabled, and keepPoiStoreand any in-memory/test stores available without it. Updatewildside-core/src/lib.rsto conditionally re-export the SQLite store and to adddoc(cfg(...))hints where appropriate. Adjust existing unit tests inwildside-core/src/store.rsso SQLite tests are only compiled when the feature is enabled.wildside-clito respect the feature flags. Use#[cfg(feature = "solver-vrp")]and#[cfg(feature = "solver-ortools")]to select the solver implementation inwildside-cli/src/solve.rs(a small factory module can keep the conditional code contained). GateSqlitePoiStoreusage on the store feature and introduce an explicit error variant inwildside_cli/src/error.rsfor missing feature support, so the CLI fails gracefully when built without a required feature. Add feature forwarding inwildside-cli/Cargo.tomlso its features map to the root features viadep:dependencies, keeping the root manifest as the source of truth.rstestfor unit-level tests that exercise solver/store selection, andrstest-bddfor behavioural tests that assert the user-visible errors when a feature is disabled. Because the feature combinations are compile-time, plan to run targeted test commands for specific feature sets (for example,--no-default-featureswith solver-only or store-only). Ensure all test modules and scenarios are guarded with the same#[cfg(feature = "...")]selectors so they only compile under the matching feature set.docs/wildside-engine-design.md, update any testing notes affected by the rstest-bdd version bump, and mark the Phase 4 feature flag entry indocs/roadmap.mdas done. Run formatting, linting, and tests via the Makefile and keep logs for review.Validation & Acceptance Criteria
How to Review
solver-vrp,solver-ortools,store-sqlite) and how they affect CLI/core wiring.Artifacts & Notes
Interfaces & Dependencies
solver-vrp: enableswildside-solver-vrp.solver-ortools: enableswildside-solver-ortools.store-sqlite: enables the SQLite-backed POI store.pub use wildside_core::{SolveRequest, SolveResponse, Solver};#[cfg(feature = "store-sqlite")] pub use wildside_core::SqlitePoiStore;wildside-coremust compile withoutstore-sqlite, and only exposeSqlitePoiStoreand its errors when the feature is enabled.wildside-clishould compile under any feature combination, returning a dedicatedCliErrorwhen a solver or store feature is missing.Revision note
Task