Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 5 minutes and 47 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (35)
📝 WalkthroughWalkthroughAdds DB/Redis test toggles and serialized nextest profiles; implements GraphQL feature gating and derive relation attributes; introduces DataLoader batching, cursor pagination, server-side streaming, FK-aware migration ordering, and a may-channel logging/tracing bridge; extensive docs, CI, and test-infra updates. Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant SQ as SelectQuery<E>
participant DB as Postgres
participant Parser as RowParser
participant RL as RelationLoader<E,R>
App->>SQ: .load(relation) / .all(&executor)
SQ->>SQ: register RelationLoader into loaders
SQ->>DB: execute SELECT query
DB-->>SQ: rows
loop parse rows
SQ->>Parser: parse row -> Model
Parser-->>SQ: Model
end
SQ->>SQ: collect Vec<Model>
SQ->>RL: invoke execute(&mut results, executor)
RL->>DB: batch SELECT WHERE fk IN (...)
DB-->>RL: related rows
RL->>RL: group by fk, build relation values
RL->>SQ: inject relations into results
SQ-->>App: Vec<Model> with populated relations
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant enhancements to the Lifeguard ORM, including a DataLoader for N+1 query resolution, cursor-based pagination, and server-side coroutine streaming. It also refactors the integration test infrastructure into a unified suite with automated Docker cleanup and adds optional GraphQL support. The review feedback highlights several improvement opportunities: removing an unnecessary unsafe block in the streaming implementation, eliminating redundant configuration in nextest.toml, and replacing panics with proper syn::Error reporting in procedural macros. Additionally, the documentation numbering in the gap analysis should be corrected for clarity, and the nt-unit recipe in the justfile should be updated to accurately reflect its intended scope.
| let local_exec = MayPostgresExecutor::new(executor.client().clone()); | ||
|
|
||
| // SAFETY: We own all local bindings natively passing lifetime boundary. | ||
| unsafe { |
There was a problem hiding this comment.
The unsafe block here seems unnecessary and could be a potential source of confusion or bugs. The closure passed to may::coroutine::spawn appears to capture only Send + 'static types, which should make it safe to spawn without unsafe.
If unsafe is truly required, a detailed // SAFETY: comment explaining why the compiler cannot prove the safety of this operation is essential for maintainability. Otherwise, this block should be removed.
Comment on lines
+63
to
+65
| [[profile.db-serial.overrides]] | ||
| filter = 'binary(db_integration_suite)' | ||
| test-threads = 1 |
There was a problem hiding this comment.
The test-threads = 1 setting in this override is redundant, as the db-serial profile already defines test-threads = 1 on line 55. While this doesn't cause an issue, removing the redundant line would make the configuration cleaner and easier to maintain.
Suggested change
| [[profile.db-serial.overrides]] | |
| filter = 'binary(db_integration_suite)' | |
| test-threads = 1 | |
| [[profile.db-serial.overrides]] | |
| filter = 'binary(db_integration_suite)' |
|
|
||
| --- | ||
|
|
||
| ## 4.1 Row-Level Security Entity Enforcement |
There was a problem hiding this comment.
There appears to be a duplicate section number. This section is numbered 4.1, but it follows section 4. This could be confusing for readers. Consider renumbering this to 5 and updating the subsequent section numbers accordingly for better clarity and document structure.
Comment on lines
+123
to
+126
| # Run unit tests only with nextest (same selection as nextest-test) | ||
| nt-unit: | ||
| @echo "🧪 Running unit tests with nextest..." | ||
| @DATABASE_URL={{DATABASE_URL}} cargo nextest run --workspace --all-features --test-group unit | ||
| @echo "🧪 Running tests with nextest (excluding DB-heavy integration binaries)..." | ||
| @DATABASE_URL={{DATABASE_URL}} cargo nextest run --workspace --all-features --fail-fast --retries 1 --exclude lifeguard-integration-tests -E 'not binary(db_integration_suite)' |
There was a problem hiding this comment.
The nt-unit recipe is now identical to nextest-test, but its name implies it only runs unit tests. This is misleading as it runs a broader set of tests. The previous implementation used --test-group unit, which was more specific.
To improve clarity, consider one of the following:
- Restore the previous behavior of running only unit tests.
- Rename the recipe to reflect its new function.
- Make
nt-unitan alias ofnextest-testif they are intended to be identical.
Comment on lines
+386
to
+388
| let entity_path: syn::Path = syn::parse_str(&rel.entity).expect("Invalid entity path in #[has_many]"); | ||
| let from_col = rel.from.as_deref().unwrap_or("id"); | ||
| let to_col = rel.to.as_deref().expect("to column required for has_many"); |
There was a problem hiding this comment.
Using .expect() in a procedural macro can lead to unhelpful panic messages for the user if parsing fails. It's better to return a syn::Error to provide a clear and actionable compile-time error pointing to the source of the issue.
This applies to parsing the entity path and expecting the to column in has_many, belongs_to, and has_one attribute handlers.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/db_integration/active_model_crud.rs (1)
1811-1827:⚠️ Potential issue | 🟡 MinorDispatch
save()from the mutated clone.
before_save()runs onrecord, but the branch still callsself.insert()/self.update(). Any field or PK changes made inbefore_save()are discarded, so these helpers won’t catch regressions in save-hook propagation.🧪 Proposed fix
- let is_insert = self.get(test_hook_user::Column::Id).is_none(); - let model = if is_insert { - self.insert(executor)? - } else { - self.update(executor)? - }; + let is_insert = record.get(test_hook_user::Column::Id).is_none(); + let model = if is_insert { + record.insert(executor)? + } else { + record.update(executor)? + };Also applies to: 2397-2413, 2577-2593
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/db_integration/active_model_crud.rs` around lines 1811 - 1827, The save() implementation calls before_save() on the cloned mutable record but still dispatches insert()/update() on self, discarding any mutations; change the branch to call insert(executor) / update(executor) on record (the mutated clone) instead of self, then set record.inner = TestHookUserRecord::from_model(&model) and call record.after_save(&model) before returning model so hook mutations (including PK changes) are preserved; update the same pattern in the other affected save() occurrences (around lines 2397-2413 and 2577-2593).
🧹 Nitpick comments (3)
lifeguard-derive/tests/test_minimal.rs (1)
1426-1455: These assertions don't prove the column mapping is correct.Both tests only check
is_some(), so they still pass if"email"returns the wrong field or if"email"is incorrectly accepted as an alias foremail_address. Since relation/query code consumes the returned value, please assert the exactsea_query::Valueand the negative case for every renamed Rust field.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lifeguard-derive/tests/test_minimal.rs` around lines 1426 - 1455, Update the two tests (test_user_model_get_by_column_name_covers_all_sql_columns and test_renamed_columns_model_get_by_column_name_uses_sql_names) to assert exact returned sea_query::Value variants/contents from model.get_by_column_name instead of only is_some/is_none; for each SQL column name call model.get_by_column_name("...") and assert it equals the expected sea_query::Value (e.g., Value::Int or Value::String with the exact contents) and also assert that calling with the Rust field name (e.g., "firstName") returns None for renamed fields on UserWithRenamedColumnsModel so you cover both positive exact-value checks and negative cases for every renamed Rust field.src/query/execution.rs (1)
53-63: Short-circuit loaders when no parent rows were returned.
all()now calls every loader even when the base query produced zero models. Guarding that case here avoids pointless follow-up work and keeps batch loaders from having to special-case empty slices themselves.♻️ Suggested change
let mut results = Vec::new(); for row in rows { let model = <E::Model as FromRow>::from_row(&row) .map_err(|e| LifeError::ParseError(format!("Failed to parse row: {e}")))?; results.push(model); } - for loader in &loaders { - loader.execute(&mut results, executor)?; + if !results.is_empty() { + for loader in &loaders { + loader.execute(&mut results, executor)?; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/query/execution.rs` around lines 53 - 63, The loop always invokes each loader via loader.execute(&mut results, executor) even when results is empty; modify the logic in the all()/execution path to short-circuit before iterating loaders when results.is_empty() (i.e., after building results from rows, if results.is_empty() return early or skip the loaders loop) so batch loaders are not called with an empty slice and unnecessary work is avoided; update the code around the results vector and the loaders loop (references: results, rows, loaders, loader.execute) to implement this guard.src/query/stream.rs (1)
15-19: Consider using a more unique cursor identifier.The current implementation uses a simple incrementing counter for cursor names (
lifeguard_stream_{counter}). While this works for sequential operations within a single process, consider adding a random component or timestamp to prevent potential conflicts in long-running applications or if the counter wraps.🔧 Optional: Add randomness to cursor name
+use std::time::{SystemTime, UNIX_EPOCH}; + static UUID_COUNTER: AtomicUsize = AtomicUsize::new(0); fn next_cursor_id() -> usize { UUID_COUNTER.fetch_add(1, Ordering::SeqCst) } + +fn cursor_name() -> String { + let counter = next_cursor_id(); + let ts = SystemTime::now() + .duration_since(UNIX_EPOCH) + .map(|d| d.as_nanos() % 1_000_000) + .unwrap_or(0); + format!("lifeguard_stream_{}_{}", counter, ts) +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/query/stream.rs` around lines 15 - 19, The cursor ID generator uses a simple incrementing counter (UUID_COUNTER and next_cursor_id) which can collide if it wraps or across long-running instances; change the cursor naming to include a random/timestamp component (e.g., append a high-resolution timestamp or a short random hex/UUID) so lifeguard_stream_{counter} becomes lifeguard_stream_{counter}_{rand} or lifeguard_stream_{counter}_{ts}, update next_cursor_id to either return/produce that combined string or add a new function (e.g., next_cursor_name) that composes UUID_COUNTER plus the random/timestamp suffix, and ensure any callers that expect numeric IDs are updated to use the new string ID.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yaml:
- Around line 16-18: The CI job currently only sets DATABASE_URL and
TEST_DATABASE_URL but tests expect a Redis instance; add environment variables
TEST_REDIS_URL and REDIS_URL pointing to the local Redis endpoint (e.g.,
redis://127.0.0.1:6379) to the job's env block so the env-backed integration
path in tests/context.rs picks up the correct Redis address and the shared Redis
assumption in tests/db_integration_suite.rs is satisfied.
- Around line 38-41: Replace the floating action reference
"dtolnay/rust-toolchain@master" with a pinned commit SHA to avoid CI drift;
locate the workflow step that currently uses the action (the line containing
dtolnay/rust-toolchain@master) and change the ref to a specific commit (for
example dtolnay/rust-toolchain@315e265) so the toolchain and components
(rustfmt, clippy) are resolved deterministically.
In `@docs/TEST_INFRASTRUCTURE.md`:
- Around line 100-104: The example cargo test command is invalid because cargo
accepts only a single positional test filter; replace the single combined line
with two separate commands that run the same package and test binary but pass
one filter each (e.g., run cargo test -p lifeguard --test db_integration_suite
related_trait:: -- --test-threads=1 and a second command cargo test -p lifeguard
--test db_integration_suite dataloader_n_plus_one:: -- --test-threads=1), and
apply this change wherever the snippet is duplicated (references: the filters
related_trait:: and dataloader_n_plus_one:: and the test binary
db_integration_suite).
In `@lifeguard-derive/src/attributes.rs`:
- Around line 139-163: The parse_relation_attr function currently only ensures
entity is non-empty and leaves typos/unknown keys and path parsing errors to
cause panics later; update parse_relation_attr (and the RelationAttribute
creation) to (1) accept only the known keys "entity", "from", and "to" and
return a syn::Error for any unknown key, (2) parse the entity string into a
syn::Path immediately (e.g., parse_str::<syn::Path>(&s.value()) and store the
Path or return a syn::Error if it fails) instead of keeping a raw string, and
(3) validate required relation-specific keys up-front (e.g., require at least
one of from/to or both if your macro semantics expect them) and return a
descriptive syn::Error when missing; this avoids later expect(...) panics in the
macro expansion and surfaces user errors from parse_relation_attr.
In `@lifeguard-derive/src/macros/life_model.rs`:
- Around line 330-333: The code currently always collects field-level
#[graphql(...)] attributes into the graphql_attrs vector which leaves orphaned
attributes when the SimpleObject derive (graphql_derive) is not enabled; wrap
the collection of these attributes in the same feature gate used for the GraphQL
derive (e.g., cfg(feature = "graphql")) so graphql_attrs is only built when the
feature is enabled (adjust the block that creates graphql_attrs and the
analogous blocks around the other occurrences where graphql_attrs is
collected/used such as the similar code paths later in the file), and ensure any
references to graphql_attrs are likewise guarded or given an empty/default when
the feature is disabled to avoid compilation errors.
In `@lifeguard-migrate/src/sql_dependency_order.rs`:
- Around line 53-61: extract_created_table_from_migration_sql currently returns
a single Option<String>, causing write_apply_order_file and FileMeta.created to
only record the first CREATE TABLE per migration; change
extract_created_table_from_migration_sql to return Vec<String> (or an iterator)
that finds all matches with the existing regex (use re.captures_iter or
find_iter) and normalize each via normalize_sql_table_name, update
FileMeta.created to be Vec<String> and adjust write_apply_order_file and any
caller logic to iterate all created table names and record per-table creator
metadata so every created table in a file is tracked (also update related logic
around lines referenced: 139-145 and 166-179 to consume the Vec and map each
table -> creator).
- Around line 88-97: When building TableInfo dependencies before calling
topological_sort, ignore self-referential foreign keys so a table referencing
itself (e.g., employees -> employees) doesn't create a false cycle: in the loop
that constructs deps for each (name, sql) (which uses
extract_referenced_tables_from_migration_sql and names.contains), filter out any
dependency equal to the current table name (compare d to name) so
TableInfo.dependencies only contains other-table deps; this change affects the
for (name, sql) block that populates tables and prevents self-references from
being handed to topological_sort().
In `@src/query/cursor.rs`:
- Around line 19-29: Cursor pagination currently orders/filters only by the
chosen column (CursorPaginator, fields column/order/after_val/before_val), which
is non-deterministic when values repeat; update the implementation so
cursor_by() either enforces the supplied column is unique or (preferred) adds a
stable tie-breaker: include the primary key (or another unique key) as a
secondary ORDER BY term and carry its value in the cursor payload (extend
after_val/before_val to hold the PK), then change the WHERE logic to compare
tuples (primary sort column, pk) or equivalent conditional (e.g., (col > value)
OR (col = value AND pk > pk_value)) for both forward and backward paging, and
update any methods that encode/decode cursor payloads (the cursor
creation/parsing code around CursorPaginator) to include/extract the PK value.
In `@src/query/loader.rs`:
- Around line 12-14: The current implementation uses
grouped_children.remove(&key) which consumes the Vec for the first parent only;
change the logic to borrow the grouped entry (e.g., grouped_children.get(&key))
and clone or clone the Vec of children for each parent so every parent with the
same FK receives the related rows; update the generic bounds to require
R::Model: Clone on RelationLoader (and anywhere needed) so you can call cloned()
or clone() when passing the batch to RelationInjector::inject, and keep the
RelationInjector::inject signature the same.
- Around line 63-75: The loader is rejecting composite-key relations by only
handling Identity::Unary; update load() to accept all Identity variants returned
by E::to() by extracting column names into a Vec<String> for rel_def.from_col
and rel_def.to_col instead of forcing a single string and returning an error;
specifically, replace the Unary-only match on Identity (used in the rel_def
handling in load()) with logic that maps Identity::Unary(iden) ->
vec![iden.to_string()] and Identity::Composite(idens) (or the enum variant
representing multi-column identities) -> idens.iter().map(|i|
i.to_string()).collect(), and remove the hard error so SelectQuery::load() and
DataLoader batch fetching consume vectors of column names consistently. Ensure
you reference rel_def.from_col, rel_def.to_col, E::to(), and the Identity enum
when making the change so the loader works with binary/ternary/multi-column
relations.
In `@src/query/traits.rs`:
- Around line 182-183: The doctests use `executor: &dyn LifeExecutor` but the
`LifeModelTrait` wrapper methods (e.g., `insert` and the other wrapper methods
that take `&E` where `E: LifeExecutor`) require a concrete generic `&E`; either
update the doctests to construct and pass a concrete executor type that
implements `LifeExecutor` (define a simple `struct TestExecutor; impl
LifeExecutor for TestExecutor { ... }` and use `&TestExecutor`) or change the
wrapper method signatures on `LifeModelTrait` to accept trait objects
(`executor: &dyn LifeExecutor`) and adjust bounds accordingly so trait objects
are allowed; apply the same fix to the other wrapper examples referenced (the
same pattern around the other wrapper methods).
In `@src/relation/lazy.rs`:
- Around line 169-171: LazyLoader::load currently calls build_where_condition
which creates WHERE clauses with Expr::cust("{table}.{column}") producing
unquoted, unqualified SQL; update build_where_condition (and any call sites in
LazyLoader::load) to construct identifiers using the relation definition's
resolved schema/table/column names (e.g., rel_def.to_tbl, rel_def.to_col,
rel_def.from_col) and emit them via the query builder's proper identifier APIs
(not Expr::cust) so schema_name, table_name and column_name derive attributes
are respected and identifiers are correctly quoted/qualified.
In `@tests/context.rs`:
- Around line 53-56: The helper function postgres_url_from_env should not fall
back to DATABASE_URL because clean_db() issues destructive DROP TABLE ...
CASCADE; change postgres_url_from_env to require only TEST_DATABASE_URL (i.e.,
return non_empty_env("TEST_DATABASE_URL") and remove or_else fallback) or, if
you prefer a safety-guard, detect and reject any DATABASE_URL that does not
clearly point to a test instance (e.g., require "test" in the DB name or allow
only localhost/127.0.0.1 with a test DB) and return None otherwise; update
references to postgres_url_from_env and ensure clean_db() will only run when a
safe TEST_DATABASE_URL is present.
- Around line 89-99: TEST_CONTEXT currently couples Postgres and Redis
resolution by matching only on postgres_url_from_env(), causing TEST_REDIS_URL
to be ignored when Postgres comes from env; fix by resolving Postgres and Redis
independently: call postgres_url_from_env() and redis_url_from_env() separately
to populate pg_url and redis_url, and only invoke
start_pg_and_redis_containers() (or individual
start_pg_container()/start_redis_container() helpers) for the missing
service(s); update the TEST_CONTEXT initializer (referencing TEST_CONTEXT,
postgres_url_from_env(), redis_url_from_env(), start_pg_and_redis_containers(),
LifeguardTestContext) so each URL falls back independently to either env value
or a started container.
---
Outside diff comments:
In `@tests/db_integration/active_model_crud.rs`:
- Around line 1811-1827: The save() implementation calls before_save() on the
cloned mutable record but still dispatches insert()/update() on self, discarding
any mutations; change the branch to call insert(executor) / update(executor) on
record (the mutated clone) instead of self, then set record.inner =
TestHookUserRecord::from_model(&model) and call record.after_save(&model) before
returning model so hook mutations (including PK changes) are preserved; update
the same pattern in the other affected save() occurrences (around lines
2397-2413 and 2577-2593).
---
Nitpick comments:
In `@lifeguard-derive/tests/test_minimal.rs`:
- Around line 1426-1455: Update the two tests
(test_user_model_get_by_column_name_covers_all_sql_columns and
test_renamed_columns_model_get_by_column_name_uses_sql_names) to assert exact
returned sea_query::Value variants/contents from model.get_by_column_name
instead of only is_some/is_none; for each SQL column name call
model.get_by_column_name("...") and assert it equals the expected
sea_query::Value (e.g., Value::Int or Value::String with the exact contents) and
also assert that calling with the Rust field name (e.g., "firstName") returns
None for renamed fields on UserWithRenamedColumnsModel so you cover both
positive exact-value checks and negative cases for every renamed Rust field.
In `@src/query/execution.rs`:
- Around line 53-63: The loop always invokes each loader via loader.execute(&mut
results, executor) even when results is empty; modify the logic in the
all()/execution path to short-circuit before iterating loaders when
results.is_empty() (i.e., after building results from rows, if
results.is_empty() return early or skip the loaders loop) so batch loaders are
not called with an empty slice and unnecessary work is avoided; update the code
around the results vector and the loaders loop (references: results, rows,
loaders, loader.execute) to implement this guard.
In `@src/query/stream.rs`:
- Around line 15-19: The cursor ID generator uses a simple incrementing counter
(UUID_COUNTER and next_cursor_id) which can collide if it wraps or across
long-running instances; change the cursor naming to include a random/timestamp
component (e.g., append a high-resolution timestamp or a short random hex/UUID)
so lifeguard_stream_{counter} becomes lifeguard_stream_{counter}_{rand} or
lifeguard_stream_{counter}_{ts}, update next_cursor_id to either return/produce
that combined string or add a new function (e.g., next_cursor_name) that
composes UUID_COUNTER plus the random/timestamp suffix, and ensure any callers
that expect numeric IDs are updated to use the new string ID.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4fbae89e-4829-4894-9b42-2ef22957affd
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (40)
.agent/memory-bank/progress.md.config/nextest.toml.github/workflows/ci.yamlCargo.tomlLIFEGUARD_GAP_ANALYSIS.mddocs/TEST_INFRASTRUCTURE.mddocs/planning/audits/LIFEGUARD_FOUNDATION_CONTINUATION.mddocs/planning/audits/RELATION_WHERE_EXPR_DECISION.mddocs/planning/lifeguard-derive/AUTHORING_MODEL_TRAIT.mdjustfilelifeguard-derive/Cargo.tomllifeguard-derive/src/attributes.rslifeguard-derive/src/lib.rslifeguard-derive/src/macros/life_model.rslifeguard-derive/tests/test_minimal.rslifeguard-migrate/Cargo.tomllifeguard-migrate/src/build_script.rslifeguard-migrate/src/lib.rslifeguard-migrate/src/sql_dependency_order.rssrc/active_model/traits.rssrc/lib.rssrc/query/cursor.rssrc/query/execution.rssrc/query/loader.rssrc/query/mod.rssrc/query/select.rssrc/query/stream.rssrc/query/traits.rssrc/relation/def/condition.rssrc/relation/def/struct_def.rssrc/relation/lazy.rssrc/relation/traits.rstests-integration/Cargo.tomltests/context.rstests/db_integration/active_model_crud.rstests/db_integration/active_model_graph.rstests/db_integration/dataloader_n_plus_one.rstests/db_integration/related_trait.rstests/db_integration/stream_and_cursor.rstests/db_integration_suite.rs
Comment on lines
+38
to
41
| - uses: dtolnay/rust-toolchain@master | ||
| with: | ||
| toolchain: nightly-2025-06-30 | ||
| override: true | ||
| components: rustfmt, clippy |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the CI workflow file
find . -name "ci.yaml" -o -name "ci.yml" | head -20Repository: microscaler/lifeguard
Length of output: 92
🏁 Script executed:
# Once we locate it, let's examine the specific lines mentioned
if [ -f ".github/workflows/ci.yaml" ]; then
cat -n .github/workflows/ci.yaml | sed -n '30,50p'
fiRepository: microscaler/lifeguard
Length of output: 743
🌐 Web query:
dtolnay/rust-toolchain GitHub action best practices pinning version immutable reference
💡 Result:
Best practices for using dtolnay/rust-toolchain in GitHub Actions emphasize security through pinning to immutable references and reproducibility via explicit toolchain specification. ## Pinning the Action Version (Security) GitHub recommends pinning third-party actions to full Git commit SHAs for security, as tags like @master or @stable can be updated or rebased, potentially introducing breaking changes or supply chain risks. This aligns with OSSF Scorecard requirements. For dtolnay/rust-toolchain: - Select a commit SHA from the master branch history (e.g., dtolnay/rust-toolchain@315e265). SHAs not on master will eventually garbage-collect and fail workflows. - Review recent commits before updating your pin, or trail master by months for stability. All tags (@stable, @1.89.0) rebases onto master. - Example: - uses: dtolnay/rust-toolchain@315e265 with: toolchain: stable dtolnay confirms this is supported and recommends @master for default usage, but pinning SHAs for production CI. ## Specifying Rust Toolchain Version (Reproducibility) Use a repo-root rust-toolchain.toml to pin the exact Rust version, components, and targets. This ensures consistent builds across local dev, CI, and contributors; rustup automatically installs it. Example rust-toolchain.toml: [toolchain] channel = "1.84.0" profile = "minimal" components = ["clippy", "rustfmt"] targets = ["wasm32-unknown-unknown"] In the action, omit toolchain input to auto-use the file, or specify explicitly: - uses: dtolnay/rust-toolchain@ with: toolchain: 1.84.0 # or 'stable', 'nightly-2026-03-01', 'stable minus 8 releases' Update intentionally (e.g., via Dependabot/PR) to avoid CI breaks from new lints/deprecations. ## Additional Tips - Pair with Swatinem/rust-cache@v2 for fast CI. - Use outputs like ${{ steps.toolchain.outputs.cachekey }} for cache keys. - For full reproducibility, pin rustup version if needed (emerging support). This approach balances security, speed, and maintainability.
Citations:
- 1: Support hash-based pinning of action version dtolnay/rust-toolchain#126
- 2: https://github.com/dtolnay/rust-toolchain
- 3: https://swatinem.de/blog/rust-toolchain/
- 4: https://github.com/actions-rust-lang/setup-rust-toolchain
- 5: https://corrode.dev/blog/tips-for-faster-ci-builds/
- 6: Support pinning the rustup version dtolnay/rust-toolchain#154
Pin dtolnay/rust-toolchain to a commit SHA.
Using @master makes CI behavior drift with upstream commits and weakens the workflow's supply-chain guarantees. Pin to a specific commit SHA (e.g., dtolnay/rust-toolchain@315e265) instead of @master or version tags.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yaml around lines 38 - 41, Replace the floating action
reference "dtolnay/rust-toolchain@master" with a pinned commit SHA to avoid CI
drift; locate the workflow step that currently uses the action (the line
containing dtolnay/rust-toolchain@master) and change the ref to a specific
commit (for example dtolnay/rust-toolchain@315e265) so the toolchain and
components (rustfmt, clippy) are resolved deterministically.
Comment on lines
+100
to
+104
| Targeted modules (faster feedback): | ||
|
|
||
| ```bash | ||
| cargo test -p lifeguard --test db_integration_suite related_trait:: dataloader_n_plus_one:: -- --test-threads=1 | ||
| ``` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the mentioned files exist and find the snippet
find . -name "TEST_INFRASTRUCTURE.md" -o -name "LIFEGUARD_FOUNDATION_CONTINUATION.md" | head -20Repository: microscaler/lifeguard
Length of output: 154
🏁 Script executed:
# Search for the cargo test command in both files
rg "cargo test.*related_trait.*dataloader" --type mdRepository: microscaler/lifeguard
Length of output: 487
🌐 Web query:
cargo test positional test filter syntax how many filters
💡 Result:
The cargo test command supports positional test filter arguments with the syntax cargo test [options] [testname] [-- test-options], where [testname] is a single optional positional argument that acts as a filter string passed to libtest. Tests whose names contain the filter string as a substring are run. Only 1 positional filter is supported directly. Multiple positional arguments before -- are not standard; the first is treated as the filter, and additional ones may cause errors or unexpected behavior as they are passed to libtest as extra filters or options. To run tests matching multiple specific filters, use a common substring if possible (e.g., cargo test common_part), run separately (e.g., cargo test filter1 && cargo test filter2), or use tools like cargo-nextest which support multiple filters natively. After --, multiple arguments are passed to libtest as options (e.g., --skip filter1 --skip filter2) or positional filters, where libtest's filters: Vec collects non-flag positional args as AND filters (test name must contain all strings), but Cargo's [testname] is singular.
Citations:
- 1: https://doc.rust-lang.org/cargo/commands/cargo-test.html
- 2: https://rust-book.cs.brown.edu/ch11-02-running-tests.html
- 3: https://doc.rust-lang.org/stable/cargo/commands/cargo-test.html
- 4: https://doc.rust-lang.org/1.73.0/cargo/commands/cargo-test.html
- 5: https://doc.rust-lang.org/1.62.1/cargo/commands/cargo-test.html
- 6: Bring multiple test filtering to test/bench rust-lang/cargo#6697
- 7: https://doc.rust-lang.org/book/ch11-02-running-tests.html
- 8: https://doc.rust-lang.org/stable/test/struct.TestOpts.html
This cargo test example is invalid and appears in two files.
cargo test accepts only one positional test filter before --. In the documented command, related_trait:: is the filter and dataloader_n_plus_one:: is extra argv that will be rejected. Split this into two separate commands instead.
The same incorrect snippet is duplicated in:
docs/TEST_INFRASTRUCTURE.md(lines 100-104)docs/planning/audits/LIFEGUARD_FOUNDATION_CONTINUATION.md(A3.1 row)
Suggested fix
-cargo test -p lifeguard --test db_integration_suite related_trait:: dataloader_n_plus_one:: -- --test-threads=1
+cargo test -p lifeguard --test db_integration_suite related_trait:: -- --test-threads=1
+cargo test -p lifeguard --test db_integration_suite dataloader_n_plus_one:: -- --test-threads=1📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Targeted modules (faster feedback): | |
| ```bash | |
| cargo test -p lifeguard --test db_integration_suite related_trait:: dataloader_n_plus_one:: -- --test-threads=1 | |
| ``` | |
| Targeted modules (faster feedback): | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/TEST_INFRASTRUCTURE.md` around lines 100 - 104, The example cargo test
command is invalid because cargo accepts only a single positional test filter;
replace the single combined line with two separate commands that run the same
package and test binary but pass one filter each (e.g., run cargo test -p
lifeguard --test db_integration_suite related_trait:: -- --test-threads=1 and a
second command cargo test -p lifeguard --test db_integration_suite
dataloader_n_plus_one:: -- --test-threads=1), and apply this change wherever the
snippet is duplicated (references: the filters related_trait:: and
dataloader_n_plus_one:: and the test binary db_integration_suite).
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
src/query/loader.rs (2)
136-147:⚠️ Potential issue | 🔴 Critical
grouped_children.remove()breaksbelongs_towhen multiple parents share the same FK.Using
.remove(&key)consumes the Vec for the first matching parent only. Subsequent parents with the same foreign key receive empty vectors instead of the related models.This is a correctness bug for
belongs_torelationships where multiple child models point to the same parent entity (e.g., multiplePostrecords belonging to the sameUser).Proposed fix
for model in models.iter_mut() { if let Some(parent_fk_val) = model.get_by_column_name(&from_col_name) { let key = format!("{parent_fk_val:?}"); - if let Some(group) = grouped_children.remove(&key) { - model.inject(group); + if let Some(group) = grouped_children.get(&key) { + model.inject(group.clone()); } else { model.inject(Vec::new()); }This requires adding
Clonebound onR::Modelin theRelationLoaderwhere clause.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/query/loader.rs` around lines 136 - 147, The loop currently uses grouped_children.remove(&key) which consumes the Vec for the first parent and leaves subsequent parents with the same FK empty; change the logic to borrow the grouped vector instead of removing it and inject a cloned Vec so multiple parents can receive the same children. Add a Clone bound for R::Model on the RelationLoader where-clause so you can call .clone() on the stored Vec items, and replace grouped_children.remove(&key) with a lookup (e.g., get/entry) that clones the Vec when calling model.inject(...) while preserving the original in grouped_children.
79-87:⚠️ Potential issue | 🟠 MajorComposite key rejection limits loader utility.
The loader returns an error for
Binary,Ternary, andManyidentity variants, but the rest of the query layer (per context snippet 2) supports these composite keys. This creates an inconsistency whereSelectQuery::load()fails at runtime for relation shapes that work elsewhere.While the comment acknowledges this is a POC limitation, consider:
- Documenting this limitation in
SelectQuery::load()'s doc comment- Adding a tracking TODO with issue reference
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/query/loader.rs` around lines 79 - 87, The loader currently rejects composite Identity variants (Binary, Ternary, Many) in loader.rs by returning LifeError::Other for rel_def.from_col/to_col; update SelectQuery::load()'s doc comment to explicitly document this POC limitation (mentioning that Binary/Ternary/Many composite keys are not supported by DataLoader batch fetching) and add a TODO with a tracking issue reference (e.g., TODO: support composite keys - issue `#NNN`) near the match arms that check Identity::Unary to make the limitation discoverable; keep the existing runtime check (matching Identity::Unary) but ensure the documentation and TODO reference the Identity enum and the LifeError::Other rejection so future implementers can find and remove the restriction.tests/context.rs (2)
52-56:⚠️ Potential issue | 🔴 CriticalDon't let destructive test helpers fall back to
DATABASE_URL.
tests/context.rs::clean_db()still issuesDROP TABLE ... CASCADE, so this fallback can target a developer or shared app database when onlyDATABASE_URLis set. Please requireTEST_DATABASE_URLhere, or hard-reject non-test targets before using destructive helpers.Suggested fix
fn postgres_url_from_env() -> Option<String> { - non_empty_env("TEST_DATABASE_URL").or_else(|| non_empty_env("DATABASE_URL")) + non_empty_env("TEST_DATABASE_URL") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/context.rs` around lines 52 - 56, The postgres_url_from_env() fallback to DATABASE_URL is unsafe for destructive helpers like clean_db(); change behavior so postgres_url_from_env() only returns TEST_DATABASE_URL (remove or disable the or_else fallback) or, alternatively, add a guard in clean_db() that refuses to run unless the resolved URL exactly comes from TEST_DATABASE_URL (panic or return an error if only DATABASE_URL is present) so destructive operations never target a non-test database; update references to postgres_url_from_env() and the clean_db() function accordingly.
88-95:⚠️ Potential issue | 🟠 MajorResolve Postgres and Redis independently.
This still ignores
TEST_REDIS_URLwhenever Postgres comes from a container, and it silently targets a real localhost Redis when only Postgres is configured from env. Mixed env/container setups stay broken and can bleed test state into a developer machine.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/context.rs` around lines 88 - 95, TEST_CONTEXT currently chooses between using env for Postgres or starting both containers, which causes TEST_REDIS_URL to be ignored when Postgres comes from env; update the closure that builds TEST_CONTEXT so Postgres and Redis are resolved independently: call postgres_url_from_env() and redis_url_from_env() separately and only invoke start_pg_and_redis_containers() (or split into start_pg_container() and start_redis_container()) for the services that are missing, then construct LifeguardTestContext { pg_url, redis_url } from those resolved values; refer to the symbols TEST_CONTEXT, postgres_url_from_env, redis_url_from_env, start_pg_and_redis_containers, and LifeguardTestContext to locate and modify the logic.
🧹 Nitpick comments (2)
src/query/execution.rs (1)
92-115: Defensive error on line 110-114 is unreachable.The
ok_or_elseerror at lines 110-114 can never trigger sinceresultsis initialized with exactly one element at line 104 (vec![model]), and loaders only mutate the models in place without removing elements. This is dead code.Consider simplifying:
Simplification suggestion
- let mut results = vec![model]; - for loader in &loaders { - // To support executors that accept slices/arrays - loader.execute(&mut results, executor)?; - } - - results.into_iter().next().ok_or_else(|| { - LifeError::Other( - "internal error: expected one model after query (empty iterator)".to_string(), - ) - }) + let mut results = vec![model]; + for loader in &loaders { + loader.execute(&mut results, executor)?; + } + + // SAFETY: results was initialized with exactly one element and loaders only mutate in-place + Ok(results.into_iter().next().expect("results contains exactly one element"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/query/execution.rs` around lines 92 - 115, The unreachable defensive error comes from returning ok_or_else on results after creating results with exactly one element (vec![model]) and only in-place loader mutations; update the end of the with_converted_params closure in the one method to directly return the single model instead of the dead ok_or_else branch — e.g., after running loader.execute on &loaders, return the model via results.remove(0) (or results.pop().unwrap()) wrapped in Ok, replacing the current results.into_iter().next().ok_or_else(...) call; this targets the one function, the results vec, loader.execute calls, and the LifeError::Other branch to remove.src/query/loader.rs (1)
112-133: Stringified key grouping is pragmatic but has edge cases.The
format!("{child_fk_val:?}")approach for HashMap keys is well-documented and reasonable for avoidingHashissues with floats. However:
Debugformatting may not be stable across Rust versions for all types- Two semantically equal values might format differently (e.g.,
"foo"vsString::from("foo"))The inline documentation is excellent. Consider adding a unit test that verifies key consistency for common FK types (i32, i64, String, Uuid).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/query/loader.rs` around lines 112 - 133, The grouping uses format!("{child_fk_val:?}") in the grouped_children creation (see grouped_children, child.get_by_column_name) which can produce unstable Debug output or different strings for semantically-equal inputs; add a unit test in the loader test module that constructs representative FK Value variants (e.g., i32, i64, String, Uuid) and asserts that equivalent values produce identical string keys via the same formatting path used by the loop (i.e., call whatever helper/logic calls format!("{...:?}") or exercise child.get_by_column_name then the grouping code) to lock in expected key equality and catch regressions across Rust/toolchain changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/query/loader.rs`:
- Around line 89-99: The current loop pushes sea_query::Value instances into
parent_ids but only filters Value::String(None) and Value::Int(None), which
misses many nullable variants; update the conditional to only push when the
Value contains a concrete Some value by matching all non-null variants (e.g.,
Value::Bool(Some(_)), Value::TinyInt(Some(_)), Value::SmallInt(Some(_)),
Value::BigInt(Some(_)), Value::UInt(Some(_)), Value::UTinyInt(Some(_)),
Value::USmallInt(Some(_)), Value::UBigInt(Some(_)), Value::Float(Some(_)),
Value::Double(Some(_)), Value::Char(Some(_)), Value::Bytes(Some(_)),
Value::Json(Some(_)), and all chrono/date-time variants) — or invert the pattern
to explicitly skip every Variant(None); modify the loop around
model.get_by_column_name(&from_col_name) to perform this exhaustive match before
calling parent_ids.push(val) so no nullable Value::...None variants are added.
In `@src/relation/def/condition.rs`:
- Around line 315-333: The loop in build_where_condition currently panics when
model.get_by_column_name(&from_name) yields None for partially loaded models;
instead, change build_where_condition to return a Result and replace the
unwrap_or_else panic with an early Err that surfaces a relation/query error
(e.g., a RelationKeyMissing or QueryError) including from_name, rel_def (or
relation name) and context so callers (like LazyLoader::load) can handle it;
update call sites of build_where_condition to propagate or map that Result
accordingly.
---
Duplicate comments:
In `@src/query/loader.rs`:
- Around line 136-147: The loop currently uses grouped_children.remove(&key)
which consumes the Vec for the first parent and leaves subsequent parents with
the same FK empty; change the logic to borrow the grouped vector instead of
removing it and inject a cloned Vec so multiple parents can receive the same
children. Add a Clone bound for R::Model on the RelationLoader where-clause so
you can call .clone() on the stored Vec items, and replace
grouped_children.remove(&key) with a lookup (e.g., get/entry) that clones the
Vec when calling model.inject(...) while preserving the original in
grouped_children.
- Around line 79-87: The loader currently rejects composite Identity variants
(Binary, Ternary, Many) in loader.rs by returning LifeError::Other for
rel_def.from_col/to_col; update SelectQuery::load()'s doc comment to explicitly
document this POC limitation (mentioning that Binary/Ternary/Many composite keys
are not supported by DataLoader batch fetching) and add a TODO with a tracking
issue reference (e.g., TODO: support composite keys - issue `#NNN`) near the match
arms that check Identity::Unary to make the limitation discoverable; keep the
existing runtime check (matching Identity::Unary) but ensure the documentation
and TODO reference the Identity enum and the LifeError::Other rejection so
future implementers can find and remove the restriction.
In `@tests/context.rs`:
- Around line 52-56: The postgres_url_from_env() fallback to DATABASE_URL is
unsafe for destructive helpers like clean_db(); change behavior so
postgres_url_from_env() only returns TEST_DATABASE_URL (remove or disable the
or_else fallback) or, alternatively, add a guard in clean_db() that refuses to
run unless the resolved URL exactly comes from TEST_DATABASE_URL (panic or
return an error if only DATABASE_URL is present) so destructive operations never
target a non-test database; update references to postgres_url_from_env() and the
clean_db() function accordingly.
- Around line 88-95: TEST_CONTEXT currently chooses between using env for
Postgres or starting both containers, which causes TEST_REDIS_URL to be ignored
when Postgres comes from env; update the closure that builds TEST_CONTEXT so
Postgres and Redis are resolved independently: call postgres_url_from_env() and
redis_url_from_env() separately and only invoke start_pg_and_redis_containers()
(or split into start_pg_container() and start_redis_container()) for the
services that are missing, then construct LifeguardTestContext { pg_url,
redis_url } from those resolved values; refer to the symbols TEST_CONTEXT,
postgres_url_from_env, redis_url_from_env, start_pg_and_redis_containers, and
LifeguardTestContext to locate and modify the logic.
---
Nitpick comments:
In `@src/query/execution.rs`:
- Around line 92-115: The unreachable defensive error comes from returning
ok_or_else on results after creating results with exactly one element
(vec![model]) and only in-place loader mutations; update the end of the
with_converted_params closure in the one method to directly return the single
model instead of the dead ok_or_else branch — e.g., after running loader.execute
on &loaders, return the model via results.remove(0) (or results.pop().unwrap())
wrapped in Ok, replacing the current results.into_iter().next().ok_or_else(...)
call; this targets the one function, the results vec, loader.execute calls, and
the LifeError::Other branch to remove.
In `@src/query/loader.rs`:
- Around line 112-133: The grouping uses format!("{child_fk_val:?}") in the
grouped_children creation (see grouped_children, child.get_by_column_name) which
can produce unstable Debug output or different strings for semantically-equal
inputs; add a unit test in the loader test module that constructs representative
FK Value variants (e.g., i32, i64, String, Uuid) and asserts that equivalent
values produce identical string keys via the same formatting path used by the
loop (i.e., call whatever helper/logic calls format!("{...:?}") or exercise
child.get_by_column_name then the grouping code) to lock in expected key
equality and catch regressions across Rust/toolchain changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 33e4e629-0aed-4613-b851-d21070326060
📒 Files selected for processing (47)
.agent/memory-bank/progress.mdlifeguard-codegen/src/main.rslifeguard-derive/src/attributes.rslifeguard-derive/src/type_conversion.rslifeguard-derive/tests/test_column_attributes.rslifeguard-derive/tests/test_column_attributes_parsing.rslifeguard-derive/tests/test_derive_linked.rslifeguard-derive/tests/test_derive_partial_model.rslifeguard-derive/tests/test_derive_partial_model_codegen.rslifeguard-derive/tests/test_derive_relation.rslifeguard-derive/tests/test_index_validation.rslifeguard-derive/tests/test_minimal.rslifeguard-derive/tests/test_partial_model_column_name_equals.rslifeguard-derive/tests/test_try_into_model.rslifeguard-derive/tests/ui.rslifeguard-migrate/src/lib.rslifeguard-migrate/src/main.rslifeguard-migrate/tests/test_build_script.rslifeguard-migrate/tests/test_dependency_ordering.rslifeguard-migrate/tests/test_entity_loader.rslifeguard-migrate/tests/test_minimal_attribute.rslifeguard-migrate/tests/test_registry_loader.rslifeguard-migrate/tests/test_sql_generation.rssrc/active_model/graph.rssrc/active_model/traits.rssrc/cache/mod.rssrc/cache/provider.rssrc/cache/redis_provider.rssrc/cache/reflector.rssrc/executor.rssrc/lib.rssrc/query/aggregate.rssrc/query/execution.rssrc/query/loader.rssrc/query/select.rssrc/query/stream.rssrc/query/traits.rssrc/relation/def/condition.rssrc/relation/def/struct_def.rstests-integration/migration_test.rstests/context.rstests/db_integration/active_model_crud.rstests/db_integration/active_model_graph.rstests/db_integration/dataloader_n_plus_one.rstests/db_integration/related_trait.rstests/db_integration/stream_and_cursor.rstests/db_integration_suite.rs
✅ Files skipped from review due to trivial changes (26)
- lifeguard-derive/tests/test_column_attributes_parsing.rs
- lifeguard-derive/tests/test_index_validation.rs
- tests-integration/migration_test.rs
- src/cache/mod.rs
- src/cache/provider.rs
- lifeguard-derive/tests/test_derive_partial_model.rs
- lifeguard-migrate/tests/test_minimal_attribute.rs
- lifeguard-derive/tests/ui.rs
- lifeguard-derive/tests/test_try_into_model.rs
- lifeguard-derive/tests/test_derive_partial_model_codegen.rs
- lifeguard-migrate/tests/test_registry_loader.rs
- lifeguard-migrate/tests/test_sql_generation.rs
- lifeguard-migrate/tests/test_dependency_ordering.rs
- src/cache/redis_provider.rs
- lifeguard-migrate/tests/test_entity_loader.rs
- lifeguard-derive/tests/test_column_attributes.rs
- lifeguard-codegen/src/main.rs
- src/query/aggregate.rs
- lifeguard-migrate/src/main.rs
- tests/db_integration_suite.rs
- lifeguard-migrate/tests/test_build_script.rs
- src/active_model/graph.rs
- lifeguard-derive/tests/test_derive_relation.rs
- lifeguard-derive/tests/test_derive_linked.rs
- src/relation/def/struct_def.rs
- lifeguard-derive/tests/test_partial_model_column_name_equals.rs
🚧 Files skipped from review as they are similar to previous changes (8)
- lifeguard-migrate/src/lib.rs
- src/query/traits.rs
- src/active_model/traits.rs
- tests/db_integration/dataloader_n_plus_one.rs
- lifeguard-derive/src/attributes.rs
- src/query/stream.rs
- lifeguard-derive/tests/test_minimal.rs
- tests/db_integration/stream_and_cursor.rs
There was a problem hiding this comment.
♻️ Duplicate comments (3)
.github/workflows/ci.yaml (2)
38-41:⚠️ Potential issue | 🟠 MajorPin
dtolnay/rust-toolchainto a commit SHA.Using
@mastermakes CI drift with upstream updates and weakens supply-chain reproducibility. Pin to an audited commit SHA.Suggested workflow fix
- - uses: dtolnay/rust-toolchain@master + - uses: dtolnay/rust-toolchain@315e265cd78dad1e1dcf3a5074f6d6c47029d5aa with: toolchain: nightly-2025-06-30 components: rustfmt, clippy🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yaml around lines 38 - 41, Replace the floating reference dtolnay/rust-toolchain@master with an audited commit SHA (e.g., dtolnay/rust-toolchain@<commit-sha>) to avoid CI drift; update the uses line that currently contains "dtolnay/rust-toolchain@master" (the block that also specifies toolchain: nightly-2025-06-30 and components: rustfmt, clippy) to the chosen commit SHA and commit that pinned reference so the workflow is reproducible.
16-18:⚠️ Potential issue | 🟠 MajorProvision Redis for the env-backed integration suite.
Line 18 configures env-backed DB usage, but the job does not provide Redis env vars/service. Redis-dependent modules in
db_integration_suitecan fail againstredis://127.0.0.1:6379when nothing is listening.Suggested workflow fix
env: DATABASE_URL: postgres://postgres:postgres@localhost:5432/postgres TEST_DATABASE_URL: postgres://postgres:postgres@localhost:5432/postgres + REDIS_URL: redis://127.0.0.1:6379 + TEST_REDIS_URL: redis://127.0.0.1:6379 services: postgres: @@ --health-interval 10s --health-timeout 5s --health-retries 5 + redis: + image: redis:7-alpine + ports: + - 6379:6379 + options: >- + --health-cmd "redis-cli ping" + --health-interval 10s + --health-timeout 5s + --health-retries 5Also applies to: 20-33
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yaml around lines 16 - 18, The CI job sets DATABASE_URL/TEST_DATABASE_URL for env-backed DB tests but doesn't provision Redis, causing redis-dependent tests in db_integration_suite to fail; update the workflow to add a Redis service (e.g., services.redis using an official Redis image) and export matching env vars such as REDIS_URL (and TEST_REDIS_URL or REDIS_HOST/REDIS_PORT) so modules expecting redis://127.0.0.1:6379 can connect; ensure these env names align with the code that reads Redis configuration (look for references to REDIS_URL, TEST_REDIS_URL, or redis host/port in the test setup).docs/TEST_INFRASTRUCTURE.md (1)
113-114:⚠️ Potential issue | 🟡 MinorSplit the
cargo testfiltered example into two commands.The command at Line 113 uses two positional filters before
--, which is not valid forcargo test. Please document these as separate invocations.Suggested doc fix
-cargo test -p lifeguard --test db_integration_suite related_trait:: dataloader_n_plus_one:: -- --test-threads=1 +cargo test -p lifeguard --test db_integration_suite related_trait:: -- --test-threads=1 +cargo test -p lifeguard --test db_integration_suite dataloader_n_plus_one:: -- --test-threads=1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/TEST_INFRASTRUCTURE.md` around lines 113 - 114, The example uses a single cargo test invocation with two positional filters (related_trait:: and dataloader_n_plus_one::) which is invalid; update the docs to show two separate commands: run cargo test -p lifeguard --test db_integration_suite with the first positional filter related_trait:: and the flags (e.g., -- --test-threads=1), and then run a second cargo test -p lifeguard --test db_integration_suite with the positional filter dataloader_n_plus_one:: and the same flags, replacing the single combined line in TEST_INFRASTRUCTURE.md.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/ci.yaml:
- Around line 38-41: Replace the floating reference
dtolnay/rust-toolchain@master with an audited commit SHA (e.g.,
dtolnay/rust-toolchain@<commit-sha>) to avoid CI drift; update the uses line
that currently contains "dtolnay/rust-toolchain@master" (the block that also
specifies toolchain: nightly-2025-06-30 and components: rustfmt, clippy) to the
chosen commit SHA and commit that pinned reference so the workflow is
reproducible.
- Around line 16-18: The CI job sets DATABASE_URL/TEST_DATABASE_URL for
env-backed DB tests but doesn't provision Redis, causing redis-dependent tests
in db_integration_suite to fail; update the workflow to add a Redis service
(e.g., services.redis using an official Redis image) and export matching env
vars such as REDIS_URL (and TEST_REDIS_URL or REDIS_HOST/REDIS_PORT) so modules
expecting redis://127.0.0.1:6379 can connect; ensure these env names align with
the code that reads Redis configuration (look for references to REDIS_URL,
TEST_REDIS_URL, or redis host/port in the test setup).
In `@docs/TEST_INFRASTRUCTURE.md`:
- Around line 113-114: The example uses a single cargo test invocation with two
positional filters (related_trait:: and dataloader_n_plus_one::) which is
invalid; update the docs to show two separate commands: run cargo test -p
lifeguard --test db_integration_suite with the first positional filter
related_trait:: and the flags (e.g., -- --test-threads=1), and then run a second
cargo test -p lifeguard --test db_integration_suite with the positional filter
dataloader_n_plus_one:: and the same flags, replacing the single combined line
in TEST_INFRASTRUCTURE.md.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 396d8450-e3e2-40f2-a5c7-f18e020bc872
📒 Files selected for processing (2)
.github/workflows/ci.yamldocs/TEST_INFRASTRUCTURE.md
…docs; fix TestDatabase connect reuse
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 21245408 | Triggered | Generic Password | 8144cae | .github/workflows/ci.yaml | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
docs/TEST_INFRASTRUCTURE.md (1)
120-124:⚠️ Potential issue | 🟡 MinorSplit this targeted
cargo testexample into two commands.
cargo testonly accepts one positional filter before--, so the second module filter makes the documented command invalid.How many positional test filters does `cargo test` accept before `--`, and is `cargo test -p lifeguard --test db_integration_suite related_trait:: dataloader_n_plus_one:: -- --test-threads=1` valid syntax?💡 Suggested fix
-cargo test -p lifeguard --test db_integration_suite related_trait:: dataloader_n_plus_one:: -- --test-threads=1 +cargo test -p lifeguard --test db_integration_suite related_trait:: -- --test-threads=1 +cargo test -p lifeguard --test db_integration_suite dataloader_n_plus_one:: -- --test-threads=1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/TEST_INFRASTRUCTURE.md` around lines 120 - 124, The documented single cargo test command is invalid because cargo test accepts only one positional test filter before --; split the example into two separate invocations that each include a single positional filter: keep the -p lifeguard package selector and the --test db_integration_suite test target in both invocations, use related_trait:: as the positional filter in the first invocation and dataloader_n_plus_one:: as the positional filter in the second, and place the shared -- --test-threads=1 runtime flag after -- in both commands..github/workflows/ci.yaml (1)
38-41:⚠️ Potential issue | 🟠 MajorPin
dtolnay/rust-toolchainto an immutable SHA.Using
@masterlets upstream changes alter CI behavior without a repo change and weakens the workflow's supply-chain guarantees.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yaml around lines 38 - 41, Replace the floating Git ref in the GitHub Actions step that currently reads uses: dtolnay/rust-toolchain@master with an immutable commit SHA; locate a known-good commit in the dtolnay/rust-toolchain repository (ideally the commit that corresponds to the desired behavior for toolchain: nightly-2025-06-30), update the uses value to uses: dtolnay/rust-toolchain@<commit-sha>, and keep the existing inputs (toolchain and components) intact; verify the workflow runs with the pinned SHA and update any documentation or comments to note why the SHA is pinned.
🧹 Nitpick comments (1)
src/logging/mod.rs (1)
148-160: Consider adding backpressure to the global queue.
mpsc::channel()is unbounded, so a stalled stderr drain or a burst of trace logs can grow memory without limit. A bounded queue plus a dropped-record counter would make overload behavior predictable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/logging/mod.rs` around lines 148 - 160, Replace the unbounded mpsc::channel() in start_global_drain with a bounded channel and add a dropped-record counter to make overload behavior predictable: change the creation in start_global_drain to use a bounded channel (e.g., mpsc sync/bounded API accepted by your mpsc import) and keep GLOBAL_TX as the Sender<LogRecord>, update call sites that send logs to use try_send (or non-blocking send) and, on Err(Full), increment a shared AtomicUsize (e.g., DROPPED_LOGS) to track dropped records; in the drain goroutine (the loop over rx in start_global_drain) periodically read and log the DROPPED_LOGS counter (and reset it) so operators can see how many records were dropped, and ensure the new counter and bounded capacity are documented in comments near GLOBAL_TX and start_global_drain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yaml:
- Around line 62-76: The CI currently applies SQL files by lexicographic sort
but the generator provides a FK-safe ordering via write_apply_order_file() in
lifeguard-migrate/src/sql_dependency_order.rs; update the generate-migrations
binary to call write_apply_order_file() after generating SQL into
migrations/generated and write apply_order.txt (one filename per line, relative
paths) into that same output dir, then change the CI step to prefer reading
migrations/generated/apply_order.txt into sql_files (preserving order) and only
fall back to the existing find | sort approach if apply_order.txt is missing or
empty so migrations are applied in the FK-safe order.
In `@src/logging/log_bridge.rs`:
- Around line 19-29: ChannelLogger::flush is currently a no-op causing buffered
records to be lost; implement flush to synchronously wait until pending channel
records are drained by the background task started in start_global_drain().
Change the drain setup so start_global_drain retains a global drain handle or
supports a flush signal, then implement ChannelLogger::flush to send a special
flush marker (or a LogRecord variant) via enqueue with a oneshot responder and
block on that responder until the background task has processed all records up
to that marker; reference enqueue, ChannelLogger::flush, start_global_drain, and
LogRecord when adding the flush marker/oneshot handshake so flush reliably waits
for the drain to complete.
In `@src/logging/mod.rs`:
- Around line 133-145: format_line currently writes message and active_span
verbatim allowing embedded newlines to create forged log lines; update
format_line to sanitize both self.message and the displayed active_span by
replacing CR and LF characters with their escaped forms (e.g. "\r" -> "\\r" and
"\n" -> "\\n") before formatting so emitted records cannot contain physical line
breaks. Locate the format_line method and perform the sanitization on
self.message and any span string used (active_span) prior to building the final
line, then use the sanitized values in the format! and push_str calls.
---
Duplicate comments:
In @.github/workflows/ci.yaml:
- Around line 38-41: Replace the floating Git ref in the GitHub Actions step
that currently reads uses: dtolnay/rust-toolchain@master with an immutable
commit SHA; locate a known-good commit in the dtolnay/rust-toolchain repository
(ideally the commit that corresponds to the desired behavior for toolchain:
nightly-2025-06-30), update the uses value to uses:
dtolnay/rust-toolchain@<commit-sha>, and keep the existing inputs (toolchain and
components) intact; verify the workflow runs with the pinned SHA and update any
documentation or comments to note why the SHA is pinned.
In `@docs/TEST_INFRASTRUCTURE.md`:
- Around line 120-124: The documented single cargo test command is invalid
because cargo test accepts only one positional test filter before --; split the
example into two separate invocations that each include a single positional
filter: keep the -p lifeguard package selector and the --test
db_integration_suite test target in both invocations, use related_trait:: as the
positional filter in the first invocation and dataloader_n_plus_one:: as the
positional filter in the second, and place the shared -- --test-threads=1
runtime flag after -- in both commands.
---
Nitpick comments:
In `@src/logging/mod.rs`:
- Around line 148-160: Replace the unbounded mpsc::channel() in
start_global_drain with a bounded channel and add a dropped-record counter to
make overload behavior predictable: change the creation in start_global_drain to
use a bounded channel (e.g., mpsc sync/bounded API accepted by your mpsc import)
and keep GLOBAL_TX as the Sender<LogRecord>, update call sites that send logs to
use try_send (or non-blocking send) and, on Err(Full), increment a shared
AtomicUsize (e.g., DROPPED_LOGS) to track dropped records; in the drain
goroutine (the loop over rx in start_global_drain) periodically read and log the
DROPPED_LOGS counter (and reset it) so operators can see how many records were
dropped, and ensure the new counter and bounded capacity are documented in
comments near GLOBAL_TX and start_global_drain.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 589a5a5c-d05d-4bba-b929-ef304680ed61
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockmigrations/generated/inventory/20260127141142_generated_from_entities.sqlis excluded by!**/generated/**migrations/generated/inventory/20260127143606_generated_from_entities.sqlis excluded by!**/generated/**
📒 Files selected for processing (16)
.agent/memory-bank/progress.md.github/workflows/ci.yaml.gitignore.pre-commit-config.yamlCargo.tomlDEVELOPMENT.mdREADME.mddocs/OBSERVABILITY_APP_INTEGRATION.mddocs/TEST_INFRASTRUCTURE.mdlifeguard-migrate/src/build_script.rsmigrations/README.mdsrc/lib.rssrc/logging/log_bridge.rssrc/logging/mod.rssrc/logging/tracing_layer.rssrc/test_helpers.rs
✅ Files skipped from review due to trivial changes (5)
- migrations/README.md
- .pre-commit-config.yaml
- README.md
- docs/OBSERVABILITY_APP_INTEGRATION.md
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib.rs
- lifeguard-migrate/src/build_script.rs
…tion FK order, CI - Return LifeError from build_where_condition; propagate in find_related, find_linked, LazyLoader - LogMsg flush barrier for ChannelLogger; escape CR/LF in format_line - ALTER TABLE IF EXISTS/ONLY parsing in sql_dependency_order; generate-migrations writes apply_order.txt - CI: apply SQL via apply_order.txt; use PGPASSWORD secret for Postgres - Cursor, loader, derive, and db integration test updates
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
docs/TEST_INFRASTRUCTURE.md (1)
122-126:⚠️ Potential issue | 🟡 MinorInvalid
cargo testcommand with multiple filters.
cargo testaccepts only one positional filter before--. The commandcargo test ... related_trait:: dataloader_n_plus_one::passes two filters, which won't work as intended. Split into separate commands or use a common substring.📝 Suggested fix
Targeted modules (faster feedback): ```bash -cargo test -p lifeguard --test db_integration_suite related_trait:: dataloader_n_plus_one:: -- --test-threads=1 +# Run specific module tests separately: +cargo test -p lifeguard --test db_integration_suite related_trait:: -- --test-threads=1 +cargo test -p lifeguard --test db_integration_suite dataloader_n_plus_one:: -- --test-threads=1</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/TEST_INFRASTRUCTURE.mdaround lines 122 - 126, The cargo test
invocation currently uses two positional filters ("related_trait::" and
"dataloader_n_plus_one::") which is invalid; update the example in
TEST_INFRASTRUCTURE.md to run those module tests separately (e.g., two
invocations of the cargo test shown) or use a single shared substring filter
instead so the command accepts only one positional filter before "--"; adjust
the "Targeted modules" block to show either two separate commands for
db_integration_suite with filters "related_trait::" and
"dataloader_n_plus_one::" or a single combined filter that matches both.</details> </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (2)</summary><blockquote> <details> <summary>src/query/stream.rs (1)</summary><blockquote> `81-85`: **Consider validating cursor name to prevent SQL injection.** While the cursor name is generated internally, the `sql` variable comes from user-constructed queries. If `sql` ever contained malicious content (unlikely in normal use), the `DECLARE {cursor_name} CURSOR FOR {sql}` could be problematic. The current implementation is safe because `sql` is built from `sea_query`, but a defensive assertion or comment would help future maintainers. <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@src/query/stream.rsaround lines 81 - 85, The DECLARE statement currently
interpolates cursor_name and sql into declare_statement before txn.execute; add
a defensive validation for cursor_name (the identifier you inject) to ensure it
matches a safe SQL identifier pattern (e.g. ^[A-Za-z_][A-Za-z0-9_]*$) or
otherwise quote/escape it properly, and assert or return an error if validation
fails; update the code around with_converted_params / declare_statement /
txn.execute to perform this check on cursor_name before formatting the DECLARE
string so sql remains untouched but the injected identifier cannot be abused.</details> </blockquote></details> <details> <summary>lifeguard-derive/src/macros/life_model.rs (1)</summary><blockquote> `459-482`: **Inconsistent error handling in `#[belongs_to]` compared to `#[has_many]`/`#[has_one]`.** Lines 460-461 use `.expect()` which panics at macro expansion time on invalid input. The `#[has_many]` and `#[has_one]` branches use `parse_relation_entity_path` and explicit `syn::Error` handling for graceful compile-time diagnostics. Apply the same pattern here for consistency. <details> <summary>♻️ Proposed refactor</summary> ```diff if let Some(rel) = &col_attrs.belongs_to { - let entity_path: syn::Path = syn::parse_str(&rel.entity).expect("Invalid entity path in #[belongs_to]"); - let from_col = rel.from.as_deref().expect("from column required for belongs_to"); + let rel_attr = relation_attr_on_field(field, "belongs_to"); + let entity_path = match parse_relation_entity_path( + &rel.entity, + rel_attr, + field_name, + "#[belongs_to]", + ) { + Ok(p) => p, + Err(e) => return e.to_compile_error().into(), + }; + let Some(from_col) = rel.from.as_deref() else { + let msg = "#[belongs_to] requires `from = \"column_name\"` (foreign key column on this entity)"; + let e = if let Some(a) = rel_attr { + syn::Error::new_spanned(a, msg) + } else { + syn::Error::new_spanned(field_name, msg) + }; + return e.to_compile_error().into(); + }; let to_col = rel.to.as_deref().unwrap_or("id");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lifeguard-derive/src/macros/life_model.rs` around lines 459 - 482, The belongs_to branch currently uses syn::parse_str and .expect() on rel.entity and rel.from which panics at macro expansion; replace those with the same safe parsing and error propagation used by the #[has_many]/#[has_one] branches: call parse_relation_entity_path(&rel.entity) and propagate any syn::Error via return Err(syn::Error::new_spanned(...)) (or the helper used elsewhere), validate rel.from using rel.from.as_deref().ok_or_else(|| syn::Error::new_spanned(...)) instead of expect(), and keep rel.to defaulting to "id" only after validation; update the block that builds the RelationDef and the impls for lifeguard::Related, lifeguard::query::loader::RelationInjector, model_name and field_name to use the safely-parsed entity_path and validated from_col/to_col.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yaml:
- Around line 20-24: CI uses the PGPASSWORD secret to build
DATABASE_URL/TEST_DATABASE_URL which will be empty for fork PRs; update the
workflow to detect a missing PGPASSWORD and either (a) set a safe fallback
password for CI or (b) short-circuit/skip database-dependent jobs/tests for fork
PRs by adding a conditional that checks env.PGPASSWORD (or use
github.event.pull_request.head.repo.fork) before using
DATABASE_URL/TEST_DATABASE_URL; modify the job steps that reference PGPASSWORD,
DATABASE_URL, and TEST_DATABASE_URL to use this check and ensure tests that
require Postgres/Redis are skipped when the secret is unavailable.
In `@lifeguard-derive/src/attributes.rs`:
- Line 94: Update the doc comment for the cursor_tiebreak attribute in
attributes.rs so the symbol DeriveEntity is wrapped in backticks; specifically
edit the comment containing `#[cursor_tiebreak = "ColumnVariant"]` (the
description that ends with "(DeriveEntity)") to use `(DeriveEntity)` →
``(`DeriveEntity`)`` so Clippy's doc-markdown lint is satisfied.
In `@lifeguard-derive/src/macros/entity.rs`:
- Around line 117-124: Extract the large LifeModelTrait impl block out of
derive_entity into a helper function named e.g. generate_life_model_trait_impl
that returns TokenStream2; have it accept the symbols used in the block
(struct_name, model_name, column_name, cursor_tiebreak_impl, find_impl) and
return the quoted impl for lifeguard::LifeModelTrait including type aliases,
all_columns, the cursor_tiebreak_impl and find_impl inserts. Replace the inline
impl generation in derive_entity with a call to
generate_life_model_trait_impl(struct_name, model_name, column_name,
cursor_tiebreak_impl, find_impl) to reduce derive_entity's line count below the
limit.
In `@lifeguard-derive/src/macros/life_model.rs`:
- Around line 495-510: The pattern matching for extracting rel.to should use
Rust's let...else to satisfy clippy's manual-let-else lint: replace the match on
rel.to.as_deref() with "let Some(to_col) = rel.to.as_deref() else { ... }" and
move the existing error construction/return into the else block preserving the
syn::Error::new_spanned usage for rel_attr and field_name; update the symbols
rel, rel_attr, field_name and the variable name to to_col so subsequent code
continues to compile.
- Around line 423-438: Replace the manual match on rel.to.as_deref() with the
let...else pattern to satisfy clippy: bind `t` using `let Some(t) =
rel.to.as_deref() else { ... }`, move the existing error construction and the
`return e.to_compile_error().into();` into the else block, and keep the same
logic/spans that use `rel_attr`, `field_name`, and `syn::Error::new_spanned` so
`to_col` is set to `t` when present and the original error path is preserved.
In `@src/query/stream.rs`:
- Around line 68-121: The coroutine spawned with may::go! that calls
local_exec.begin() may leave txn open on panic; ensure the transaction is always
finalized by introducing a rollback-on-drop or panic handling around txn (e.g.,
create a guard that holds txn and calls txn.rollback() in Drop unless explicitly
committed) and/or wrap the body in std::panic::catch_unwind to rollback on
panics; adjust the flow around with_converted_params, FromRow::from_row
processing and tx.send handling so that on any Err or panic the guard triggers
rollback, and only call txn.commit() after successful completion to guarantee no
transaction remains open.
In `@tests/context.rs`:
- Around line 56-63: Adjust redis_url_from_env so it never falls back to a
potentially non-test REDIS_URL when tests are configured: detect whether a test
DB was requested via postgres_url_from_env/non_empty_env("TEST_DATABASE_URL")
and, if so, require TEST_REDIS_URL (i.e., do not call non_empty_env("REDIS_URL")
as a fallback); otherwise, if no test-specific redis env is set at all, return a
safe default like "redis://127.0.0.1:6379". Update the logic in the
redis_url_from_env function to reference non_empty_env("TEST_REDIS_URL") and to
conditionally use the localhost default only when no test envs are present.
---
Duplicate comments:
In `@docs/TEST_INFRASTRUCTURE.md`:
- Around line 122-126: The cargo test invocation currently uses two positional
filters ("related_trait::" and "dataloader_n_plus_one::") which is invalid;
update the example in TEST_INFRASTRUCTURE.md to run those module tests
separately (e.g., two invocations of the cargo test shown) or use a single
shared substring filter instead so the command accepts only one positional
filter before "--"; adjust the "Targeted modules" block to show either two
separate commands for db_integration_suite with filters "related_trait::" and
"dataloader_n_plus_one::" or a single combined filter that matches both.
---
Nitpick comments:
In `@lifeguard-derive/src/macros/life_model.rs`:
- Around line 459-482: The belongs_to branch currently uses syn::parse_str and
.expect() on rel.entity and rel.from which panics at macro expansion; replace
those with the same safe parsing and error propagation used by the
#[has_many]/#[has_one] branches: call parse_relation_entity_path(&rel.entity)
and propagate any syn::Error via return Err(syn::Error::new_spanned(...)) (or
the helper used elsewhere), validate rel.from using
rel.from.as_deref().ok_or_else(|| syn::Error::new_spanned(...)) instead of
expect(), and keep rel.to defaulting to "id" only after validation; update the
block that builds the RelationDef and the impls for lifeguard::Related,
lifeguard::query::loader::RelationInjector, model_name and field_name to use the
safely-parsed entity_path and validated from_col/to_col.
In `@src/query/stream.rs`:
- Around line 81-85: The DECLARE statement currently interpolates cursor_name
and sql into declare_statement before txn.execute; add a defensive validation
for cursor_name (the identifier you inject) to ensure it matches a safe SQL
identifier pattern (e.g. ^[A-Za-z_][A-Za-z0-9_]*$) or otherwise quote/escape it
properly, and assert or return an error if validation fails; update the code
around with_converted_params / declare_statement / txn.execute to perform this
check on cursor_name before formatting the DECLARE string so sql remains
untouched but the injected identifier cannot be abused.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c1bdf01c-501d-43eb-b175-3919e9d024f0
⛔ Files ignored due to path filters (2)
migrations/generated/apply_order.txtis excluded by!**/generated/**migrations/generated/inventory/20260328192153_generated_from_entities.sqlis excluded by!**/generated/**
📒 Files selected for processing (31)
.agent/memory-bank/activeContext.md.agent/memory-bank/progress.md.config/nextest.toml.github/workflows/ci.yamldocs/TEST_INFRASTRUCTURE.mddocs/planning/audits/RELATION_WHERE_EXPR_DECISION.mddocs/planning/lifeguard-derive/AUTHORING_MODEL_TRAIT.mdexamples/entities/src/bin/generate_migrations.rsjustfilelifeguard-derive/src/attributes.rslifeguard-derive/src/lib.rslifeguard-derive/src/macros/entity.rslifeguard-derive/src/macros/life_model.rslifeguard-migrate/src/sql_dependency_order.rssrc/lib.rssrc/logging/log_bridge.rssrc/logging/mod.rssrc/query/cursor.rssrc/query/loader.rssrc/query/select.rssrc/query/stream.rssrc/query/traits.rssrc/relation/def/condition.rssrc/relation/eager.rssrc/relation/lazy.rssrc/relation/traits.rstests/context.rstests/db_integration/dataloader_n_plus_one.rstests/db_integration/related_trait.rstests/db_integration/stream_and_cursor.rstests/db_integration_suite.rs
✅ Files skipped from review due to trivial changes (3)
- src/logging/log_bridge.rs
- docs/planning/lifeguard-derive/AUTHORING_MODEL_TRAIT.md
- justfile
🚧 Files skipped from review as they are similar to previous changes (6)
- .config/nextest.toml
- src/query/traits.rs
- tests/db_integration/related_trait.rs
- tests/db_integration/dataloader_n_plus_one.rs
- src/relation/lazy.rs
- lifeguard-migrate/src/sql_dependency_order.rs
Comment on lines
+56
to
+63
| /// Only **`TEST_DATABASE_URL`** (see module docs — no `DATABASE_URL` fallback). | ||
| fn postgres_url_from_env() -> Option<String> { | ||
| non_empty_env("TEST_DATABASE_URL") | ||
| } | ||
|
|
||
| fn redis_url_from_env() -> Option<String> { | ||
| non_empty_env("TEST_REDIS_URL").or_else(|| non_empty_env("REDIS_URL")) | ||
| } |
There was a problem hiding this comment.
Redis fallback to REDIS_URL may connect to non-test instance.
When TEST_DATABASE_URL is set but TEST_REDIS_URL is not, line 62 falls back to REDIS_URL (which might point to a developer or production Redis). While less destructive than a database fallback, this could cause test state to leak into other environments.
Consider requiring TEST_REDIS_URL explicitly when TEST_DATABASE_URL is provided, or at minimum default to a clearly localhost URL only when no Redis env is set.
💡 Suggested defensive approach
fn redis_url_from_env() -> Option<String> {
- non_empty_env("TEST_REDIS_URL").or_else(|| non_empty_env("REDIS_URL"))
+ non_empty_env("TEST_REDIS_URL")
}Then at line 93-94, the fallback to redis://127.0.0.1:6379 would only apply when no test-specific URL is configured, avoiding accidental connection to REDIS_URL.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// Only **`TEST_DATABASE_URL`** (see module docs — no `DATABASE_URL` fallback). | |
| fn postgres_url_from_env() -> Option<String> { | |
| non_empty_env("TEST_DATABASE_URL") | |
| } | |
| fn redis_url_from_env() -> Option<String> { | |
| non_empty_env("TEST_REDIS_URL").or_else(|| non_empty_env("REDIS_URL")) | |
| } | |
| /// Only **`TEST_REDIS_URL`** (see module docs — no `REDIS_URL` fallback). | |
| fn postgres_url_from_env() -> Option<String> { | |
| non_empty_env("TEST_DATABASE_URL") | |
| } | |
| fn redis_url_from_env() -> Option<String> { | |
| non_empty_env("TEST_REDIS_URL") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/context.rs` around lines 56 - 63, Adjust redis_url_from_env so it never
falls back to a potentially non-test REDIS_URL when tests are configured: detect
whether a test DB was requested via
postgres_url_from_env/non_empty_env("TEST_DATABASE_URL") and, if so, require
TEST_REDIS_URL (i.e., do not call non_empty_env("REDIS_URL") as a fallback);
otherwise, if no test-specific redis env is set at all, return a safe default
like "redis://127.0.0.1:6379". Update the logic in the redis_url_from_env
function to reference non_empty_env("TEST_REDIS_URL") and to conditionally use
the localhost default only when no test envs are present.
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lifeguard-derive/src/macros/entity.rs (1)
15-175:⚠️ Potential issue | 🟡 MinorPipeline failure: Function exceeds 100 lines (clippy::too_many_lines).
The
derive_entityfunction has 102 lines, exceeding clippy's default limit. Consider extracting helper functions for readability and to resolve the CI failure.♻️ Suggested refactor: Extract impl generation helpers
+fn generate_schema_name_impl(schema_name: &Option<String>, span: proc_macro2::Span) -> TokenStream2 { + if let Some(ref schema) = schema_name { + let schema_lit = syn::LitStr::new(schema, span); + quote! { + fn schema_name(&self) -> Option<&'static str> { + Some(`#schema_lit`) + } + } + } else { + quote! { + fn schema_name(&self) -> Option<&'static str> { + None + } + } + } +} + +fn generate_cursor_tiebreak_impl(attrs: &[syn::Attribute], column_name: &syn::Ident) -> TokenStream2 { + match attributes::extract_cursor_tiebreak(attrs) { + Some(variant) => quote! { + fn cursor_tiebreak_column() -> Option<Self::Column> { + Some(`#column_name`::`#variant`) + } + }, + None => quote! {}, + } +} + pub fn derive_entity(input: TokenStream) -> TokenStream { // ... use helpers instead of inline logic - let schema_name_impl = if let Some(ref schema) = schema_name { - // ... - }; + let schema_name_impl = generate_schema_name_impl(&schema_name, struct_name.span()); - let cursor_tiebreak_impl = match attributes::extract_cursor_tiebreak(&input.attrs) { - // ... - }; + let cursor_tiebreak_impl = generate_cursor_tiebreak_impl(&input.attrs, &column_name);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lifeguard-derive/src/macros/entity.rs` around lines 15 - 175, The derive_entity function is over 100 lines (clippy::too_many_lines); split it into small helpers to reduce its length. Extract the large quote!-block pieces and attribute-handling blocks into private helper functions (e.g., fn schema_name_impl_for(struct_name, schema_name) -> TokenStream2, fn find_impl_for(soft_delete) -> TokenStream2, fn cursor_tiebreak_impl_for(column_name, attrs) -> TokenStream2, and fn lifemodel_impl_for(struct_name, model_name, column_name, cursor_tiebreak_impl, find_impl) -> TokenStream2) and call those from derive_entity, keeping derive_entity to input parsing and final assembly only (returning TokenStream::from(expanded)). Ensure you preserve the same identifiers (`#struct_name`, `#model_name`, `#column_name`) when assembling the final TokenStream.
🧹 Nitpick comments (5)
.config/nextest.toml (1)
63-65: Minor: Redundanttest-threadsoverride.The
[[profile.db-serial.overrides]]setstest-threads = 1, but the parent[profile.db-serial]already setstest-threads = 1at line 55. This override is redundant unless you intend to apply different settings todb_integration_suitespecifically (which currently matches the profile default).♻️ Optional: Remove redundant override or add a distinguishing setting
[[profile.db-serial.overrides]] filter = 'binary(db_integration_suite)' -test-threads = 1 +# test-threads inherited from profile.db-serial (already 1) +# Add specific overrides here if needed (e.g., different slow-timeout)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.config/nextest.toml around lines 63 - 65, The override block [[profile.db-serial.overrides]] that matches filter = 'binary(db_integration_suite)' redundantly sets test-threads = 1 because the parent profile [profile.db-serial] already sets test-threads = 1; either remove this override block entirely or change it to include a distinct setting you need for db_integration_suite (e.g., a different test-threads value or another override key) so the override is meaningful.src/query/stream.rs (1)
46-54: Document that loaders are not executed during streaming.The regular
SelectQuery::all()method (seesrc/query/execution.rslines 43-61) dispatches registered loaders after fetching results.stream_allbypasses this, so eager-loaded relations won't be populated. This is likely intentional for performance, but should be documented.📝 Suggested doc addition
impl<E: LifeModelTrait + 'static> SelectQueryStreamEx<E> for SelectQuery<E> where E::Model: FromRow + Send + Sync + 'static, { + /// Stream results using a server-side cursor. + /// + /// **Note:** Unlike `SelectQuery::all()`, this method does **not** execute + /// registered loaders (eager loading). Use this for large result sets where + /// relations will be loaded separately or are not needed. fn stream_all(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/query/stream.rs` around lines 46 - 54, Update the docs/comments for the streaming API to explicitly state that registered loaders are not executed when using SelectQuery::stream_all (SelectQueryStreamEx::stream_all) and thus eager-loaded relations will not be populated; add a brief note near the SelectQueryStreamEx/SelectQuery::stream_all declaration (and optionally cross-reference SelectQuery::all()) explaining this is intentional for performance and recommend callers use SelectQuery::all() when they need loaders to run.lifeguard-derive/src/macros/entity.rs (1)
117-124: Consider adding variant validation to#[cursor_tiebreak]attribute in a follow-up.The
extract_cursor_tiebreakfunction inlifeguard-derive/src/attributes.rs(lines 95-110) parses the string literal into anIdentwithout validating it exists on theColumnenum. A typo like#[cursor_tiebreak = "InvalidColumn"]will compile through the attribute parsing but produce a compile error pointing at the generated code rather than the attribute site.Since invalid variants still fail at compile time, this is acceptable for now, but validation at macro expansion time would improve developer experience by catching errors at the attribute declaration.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lifeguard-derive/src/macros/entity.rs` around lines 117 - 124, The attribute parser extract_cursor_tiebreak currently converts the #[cursor_tiebreak = "..."] string into an Ident without checking that the Ident is a real variant of the generated Column enum; update extract_cursor_tiebreak in lifeguard-derive/src/attributes.rs to validate the parsed Ident against the actual Column variants (derive input) and emit a clear compile_error! (or return a syn::Error::new_spanned) referencing the attribute token when the variant is missing so the error points to the attribute site instead of the generated cursor_tiebreak_column function; use the same enum variant names you use to generate Column (the Column enum parsing logic) to perform the membership check.src/query/cursor.rs (1)
154-157: Silent discard of PK bounds may surprise callers.When
cursor_tiebreak_column()returnsNone(composite PK or no PK), suppliedafter_pk/before_pkvalues are silently ignored. Consider logging a warning or documenting this behavior in theafter_pk/before_pkdoc comments so users know their bounds won't take effect.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/query/cursor.rs` around lines 154 - 157, The code silently drops supplied PK bounds by calling self.after_pk_val.take() and self.before_pk_val.take() when cursor_tiebreak_column() is None; update the behavior so callers are informed: either add a warning log in that else branch (use the module/logger used elsewhere) that clearly states "after_pk/before_pk ignored because cursor_tiebreak_column() returned None (composite or no PK)" referencing cursor_tiebreak_column(), after_pk_val, and before_pk_val, or update the public doc comments for the methods/properties that accept after_pk/before_pk to state that these bounds will be ignored when cursor_tiebreak_column() returns None; choose one approach and apply it consistently.src/logging/mod.rs (1)
236-246: UnreachableFlushbranch intry_enqueueerror handling.Since
try_enqueuealways sendsLogMsg::Record(record), theLogMsg::Flusharm in themap_errclosure is unreachable. This branch exists only for exhaustive matching. Consider addingunreachable!()or a comment to clarify.💡 Optional: Make unreachability explicit
.map_err(|SendError(msg)| match msg { LogMsg::Record(r) => SendError(r), - LogMsg::Flush(done_tx) => { - drop(done_tx); - SendError(LogRecord::new( - LogLevel::Error, - "lifeguard::logging", - "internal logging channel state error", - )) - } + LogMsg::Flush(_) => unreachable!("try_enqueue only sends Record variants"), })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/logging/mod.rs` around lines 236 - 246, The error handler in the try_enqueue flow matches LogMsg::Flush even though try_enqueue only sends LogMsg::Record, making the Flush arm unreachable; update the map_err closure in try_enqueue to make unreachability explicit by replacing the LogMsg::Flush match arm with unreachable!() (or a clear comment plus unreachable!()) while keeping the SendError(LogRecord::...) for the Record branch; reference try_enqueue, the map_err closure, LogMsg::Flush, and SendError when locating and applying this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lifeguard-derive/src/attributes.rs`:
- Around line 94-95: The doc comment for the function extract_cursor_tiebreak
contains bare item names causing clippy::doc_markdown to fail; edit the
triple-slash doc line above pub fn extract_cursor_tiebreak(attrs: &[Attribute])
-> Option<syn::Ident> to wrap the identifiers Entity and DeriveEntity in
backticks (e.g., `Entity`, `DeriveEntity`) and any other bare item names so the
documentation uses inline code formatting.
In `@lifeguard-derive/src/macros/life_model.rs`:
- Around line 495-510: Refactor the manual match that computes to_col into a
let-else using the `rel.to.as_deref()` pattern: replace `let to_col = match
rel.to.as_deref() { Some(t) => t, None => { ... } }` with `let Some(to_col) =
rel.to.as_deref() else { ... };` and keep the existing error construction logic
that uses `rel_attr` or `field_name` to create the `syn::Error::new_spanned(...
)` and return `e.to_compile_error().into()` so behavior is unchanged but the
manual-let-else lints are resolved.
- Around line 459-462: The belongs_to branch currently calls
syn::parse_str(...).expect and .expect/.unwrap on rel.from/rel.to which can
panic; replace these with the same graceful compile-time error handling used by
parse_relation_entity_path: call parse_relation_entity_path(&rel.entity) to
parse the entity path and propagate or return the resulting syn::Error on
failure, and replace rel.from.as_deref().expect(...) with a check that returns a
syn::Error (e.g., using syn::Error::new_spanned or similar) when missing; keep
rel.to.as_deref().unwrap_or("id") as a non-panicking default but avoid unwraps
altogether by using as_deref().unwrap_or("id") without expect. Update the
belongs_to handling in life_model.rs to mirror the error-return pattern used by
has_many/has_one.
- Around line 423-438: Replace the outer match that extracts to_col with a
let-else: use `let Some(t) = rel.to.as_deref() else { ... }` and inside the else
build the syn::Error by using an if-let on rel_attr (e.g. `if let Some(a) =
rel_attr { syn::Error::new_spanned(a, "...") } else {
syn::Error::new_spanned(field_name, "...") }`) and then return its
compile_error().into(); this removes the manual match and satisfies the clippy
`manual-let-else` / `single-match-else` warnings while preserving the existing
error messages and control flow for variables rel, rel_attr, field_name and the
resulting to_col.
In `@src/query/stream.rs`:
- Around line 119-121: The current code swallows errors from txn.commit() in the
closure (let _ = txn.commit()), which can hide commit failures; change this to
handle the Result returned by txn.commit() from the Transaction type used in
stream.rs (location: the closure that closes the transaction and collapses
cursors). Either propagate the error by returning/propagating it to the caller
(convert the closure to return a Result and use ? or map_err) or at minimum log
the error with context using the existing logger (include the transaction/cursor
id and error), so commit failures are not silently ignored.
- Around line 60-61: The stream function currently calls
self.query.build(PostgresQueryBuilder) directly which bypasses soft-delete
filtering; before building, call self.apply_soft_delete() (as SelectQuery::all()
does) so soft-deleted entities are filtered out—adjust the method to take &mut
self or have apply_soft_delete consume self if needed, and then call
apply_soft_delete() on the same query instance prior to invoking
self.query.build(PostgresQueryBuilder) (referencing SelectQuery::all,
apply_soft_delete, and the self.query.build call to locate the change).
In `@src/relation/def/condition.rs`:
- Around line 340-358: The fallback currently matches primary key values by the
zipped index which fails when rel_def.from_col order differs from the model
primary key order; update the fallback in the loop (use rel_def.from_col /
rel_def.to_col, model.get_by_column_name, pk_names, pk_values) to look up the
position of from_name in pk_names (e.g. find pk_names.iter().position(|pk| pk ==
&from_name)) and, if found, pull the corresponding value from
pk_values.get(position).cloned() instead of using the loop index; then keep the
same ok_or_else LifeError if no value is found.
- Around line 54-71: join predicates built in join_tbl_on_condition() and
join_tbl_on_expr() use raw Expr::cust("{table}.{column}") which breaks for
aliases, schema-qualified tables, or quoted identifiers; update those functions
to use the same column-expression construction as expr_for_related_table_column
(i.e., use Expr::col with ColumnName or alias tuple derived from the TableRef)
for both left and right sides of the JOIN predicate, or extract the shared logic
into a helper (reuse expr_for_related_table_column or rename it to
expr_for_table_column) and call it from join_tbl_on_condition() and
join_tbl_on_expr() so all JOIN predicates are created with Expr::col/ColumnName
and not Expr::cust.
In `@src/relation/traits.rs`:
- Around line 611-614: The branch in Linked::via() that currently returns
SelectQuery::new() when path.is_empty() must not produce an unrestricted query;
change it to return an Err (propagate a meaningful error) or construct a
SelectQuery with an impossible predicate so no rows are matched. Replace the
current return Ok(SelectQuery::new()) in the path.is_empty() block with either
Err(...) using your function's error type (so callers must handle the empty
linked path) or build a SelectQuery that includes a guaranteed-false condition
(e.g., a WHERE 1=0 equivalent) so the API doesn't silently drop scoping.
---
Outside diff comments:
In `@lifeguard-derive/src/macros/entity.rs`:
- Around line 15-175: The derive_entity function is over 100 lines
(clippy::too_many_lines); split it into small helpers to reduce its length.
Extract the large quote!-block pieces and attribute-handling blocks into private
helper functions (e.g., fn schema_name_impl_for(struct_name, schema_name) ->
TokenStream2, fn find_impl_for(soft_delete) -> TokenStream2, fn
cursor_tiebreak_impl_for(column_name, attrs) -> TokenStream2, and fn
lifemodel_impl_for(struct_name, model_name, column_name, cursor_tiebreak_impl,
find_impl) -> TokenStream2) and call those from derive_entity, keeping
derive_entity to input parsing and final assembly only (returning
TokenStream::from(expanded)). Ensure you preserve the same identifiers
(`#struct_name`, `#model_name`, `#column_name`) when assembling the final TokenStream.
---
Nitpick comments:
In @.config/nextest.toml:
- Around line 63-65: The override block [[profile.db-serial.overrides]] that
matches filter = 'binary(db_integration_suite)' redundantly sets test-threads =
1 because the parent profile [profile.db-serial] already sets test-threads = 1;
either remove this override block entirely or change it to include a distinct
setting you need for db_integration_suite (e.g., a different test-threads value
or another override key) so the override is meaningful.
In `@lifeguard-derive/src/macros/entity.rs`:
- Around line 117-124: The attribute parser extract_cursor_tiebreak currently
converts the #[cursor_tiebreak = "..."] string into an Ident without checking
that the Ident is a real variant of the generated Column enum; update
extract_cursor_tiebreak in lifeguard-derive/src/attributes.rs to validate the
parsed Ident against the actual Column variants (derive input) and emit a clear
compile_error! (or return a syn::Error::new_spanned) referencing the attribute
token when the variant is missing so the error points to the attribute site
instead of the generated cursor_tiebreak_column function; use the same enum
variant names you use to generate Column (the Column enum parsing logic) to
perform the membership check.
In `@src/logging/mod.rs`:
- Around line 236-246: The error handler in the try_enqueue flow matches
LogMsg::Flush even though try_enqueue only sends LogMsg::Record, making the
Flush arm unreachable; update the map_err closure in try_enqueue to make
unreachability explicit by replacing the LogMsg::Flush match arm with
unreachable!() (or a clear comment plus unreachable!()) while keeping the
SendError(LogRecord::...) for the Record branch; reference try_enqueue, the
map_err closure, LogMsg::Flush, and SendError when locating and applying this
change.
In `@src/query/cursor.rs`:
- Around line 154-157: The code silently drops supplied PK bounds by calling
self.after_pk_val.take() and self.before_pk_val.take() when
cursor_tiebreak_column() is None; update the behavior so callers are informed:
either add a warning log in that else branch (use the module/logger used
elsewhere) that clearly states "after_pk/before_pk ignored because
cursor_tiebreak_column() returned None (composite or no PK)" referencing
cursor_tiebreak_column(), after_pk_val, and before_pk_val, or update the public
doc comments for the methods/properties that accept after_pk/before_pk to state
that these bounds will be ignored when cursor_tiebreak_column() returns None;
choose one approach and apply it consistently.
In `@src/query/stream.rs`:
- Around line 46-54: Update the docs/comments for the streaming API to
explicitly state that registered loaders are not executed when using
SelectQuery::stream_all (SelectQueryStreamEx::stream_all) and thus eager-loaded
relations will not be populated; add a brief note near the
SelectQueryStreamEx/SelectQuery::stream_all declaration (and optionally
cross-reference SelectQuery::all()) explaining this is intentional for
performance and recommend callers use SelectQuery::all() when they need loaders
to run.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 29a67a7c-2827-447f-8cce-c9d6fbb46e92
⛔ Files ignored due to path filters (2)
migrations/generated/apply_order.txtis excluded by!**/generated/**migrations/generated/inventory/20260328192153_generated_from_entities.sqlis excluded by!**/generated/**
📒 Files selected for processing (33)
.agent/memory-bank/activeContext.md.agent/memory-bank/progress.md.config/nextest.toml.github/workflows/ci.yaml.pre-commit-config.yamlDEVELOPMENT.mddocs/TEST_INFRASTRUCTURE.mddocs/planning/audits/RELATION_WHERE_EXPR_DECISION.mddocs/planning/lifeguard-derive/AUTHORING_MODEL_TRAIT.mdexamples/entities/src/bin/generate_migrations.rsjustfilelifeguard-derive/src/attributes.rslifeguard-derive/src/lib.rslifeguard-derive/src/macros/entity.rslifeguard-derive/src/macros/life_model.rslifeguard-migrate/src/sql_dependency_order.rssrc/lib.rssrc/logging/log_bridge.rssrc/logging/mod.rssrc/query/cursor.rssrc/query/loader.rssrc/query/select.rssrc/query/stream.rssrc/query/traits.rssrc/relation/def/condition.rssrc/relation/eager.rssrc/relation/lazy.rssrc/relation/traits.rstests/context.rstests/db_integration/dataloader_n_plus_one.rstests/db_integration/related_trait.rstests/db_integration/stream_and_cursor.rstests/db_integration_suite.rs
✅ Files skipped from review due to trivial changes (4)
- .pre-commit-config.yaml
- tests/db_integration_suite.rs
- DEVELOPMENT.md
- docs/planning/lifeguard-derive/AUTHORING_MODEL_TRAIT.md
🚧 Files skipped from review as they are similar to previous changes (9)
- docs/planning/audits/RELATION_WHERE_EXPR_DECISION.md
- src/query/traits.rs
- src/lib.rs
- tests/db_integration/dataloader_n_plus_one.rs
- lifeguard-migrate/src/sql_dependency_order.rs
- justfile
- src/relation/lazy.rs
- tests/db_integration/related_trait.rs
- tests/context.rs
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
This was referenced Mar 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note
Medium Risk
Touches core query/streaming, relation filtering, and derive-macro codegen, which can affect correctness of pagination and data loading across the ORM. CI/test infrastructure and logging changes are mostly low risk but broad and could mask environment-specific issues if misconfigured.
Overview
Core ORM correctness fixes and hardening.
SelectQuery::stream_allnow applies soft-delete filters consistently, cleans up transactions safely (rollback on panic/error), and surfaces commit failures instead of dropping them. Cursor pagination gains an opt-in#[cursor_tiebreak]for stable keyset ordering using(cursor_col, pk)with updated integration coverage.Relations + loaders now fail safely and build better SQL.
find_linkedrejects empty paths,build_where_conditionreturnsLifeErrorwhen FK values are missing, and related-table predicates switch from stringExpr::custto structuredExpr::colto support schema/alias quoting.RelationLoaderis updated for composite keys and duplicate parent FKs, and derive-generatedRelationDefusesConditionType::Allfor composite join semantics.Macro + tooling + CI improvements.
lifeguard-deriveadds parsing for field-level#[has_many]/#[belongs_to]/#[has_one](with propersyn::Errorreporting), generatessoft_delete_column()and forwardscursor_tiebreakonly when explicitly set, and gates GraphQL attribute passthrough behind agraphqlfeature. CI now uses secret-driven Postgres passwords, adds Redis service/env, regenerates and applies entity migrations viaapply_order.txt, pinscargo-nextest, and introduces a serial DB test profile plus updatedjustrecipes.Observability/logging robustness. Channel logging now escapes CR/LF to keep logs single-line and implements a flush handshake (
LogMsg::Flush) solog::Log::flushblocks until prior records are drained; new docs clarify host-owned OpenTelemetry/tracing setup.Written by Cursor Bugbot for commit 8340ddf. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation