Feat/schema validators session and scopes 2#57
Merged
Conversation
…3, F() update test - Add ValidateOp::Delete and run_validators on derive delete path - SelectQuery::scope_or and scope_any with unit tests - infer-schema table filter integration test (SI-3); migrate README touch-ups - db_integration column_f_update for ColumnTrait::f_add UPDATE - #[validate(custom = path)] on LifeRecord fields (V-5) - DESIGN_FIND_RELATED_SCOPES, PRD and mapping docs, progress memory bank
…ttern - Add db_integration column_f_where (Expr::expr + f_add, ExprTrait, order_by_expr) - Extend ColumnTrait::f_add rustdoc for WHERE/ORDER BY - PRD §8.7 Phase D + memory bank progress
- Add __update_exprs map and generated set_<field>_expr on LifeRecord - UPDATE SET prefers expression over literal/save_as; setters clear expr - dirty_fields / reset / set / take integrate with __update_exprs - column_f_update: mutex + shared setup; record_set_n_expr_update_increments_on_postgres - PRD §8.7, SEAORM mapping, derive rustdoc, progress memory bank
…EADME - ModelIdentityMap::mark_dirty_key; session rustdoc + unit test - Derived identity_map_key() via ActiveModelTrait::get (UFCS) - insert() rejects non-empty __update_exprs (already in macro) - README competitive table + bullets; PRD §7.7/§8.7/§9.7; DESIGN_SESSION_UOW - DEVELOPMENT db_integration notes; SEAORM session row; F-3 rustdoc note on f_add - Tests: insert_rejects_when_set_expr_pending, user_record_identity_map_key_*
- Session/ModelIdentityMap: register_pending_insert, flush_dirty_with_map_key, promote_pending_to_loaded; pooled/txn map-key flush; integration tests - lifeguard-migrate: compare-schema vs merged generated SQL; infer-schema CLI subprocess e2e; smoke tests use dedicated PG schema - Docs: README/SEAORM/PRD/DESIGN validators G6, F() PostgreSQL typing, Phase A closure; ColumnTrait f_add rustdoc; memory bank
…ntityModelCell - Add Send wrapper for Rc<RefCell<Model>> and attach_session_with_model plumbing - Update README, PRD §9, DESIGN_SESSION_UOW, integration test, memory bank
…oledLifeExecutor - Add ReadPreference Default|Primary, read_pool_for, with_read_preference - Integration test pooled_read_preference_primary_forces_primary_tier - README, DESIGN_FIND_RELATED_SCOPES, memory bank
- Unit struct -> snake_case MIGRATION_NAME + MigrationName impl - Re-export DeriveMigrationName from lifeguard::migration - Tests and SEAORM mapping + README row
- Wrap SeaQuery with_cte so CTEs work with all/one (vs raw WithQuery) - join_subquery; window; expr_window*; docs and README roadmap
- query/select: correct SelectQuery::all/one links; compile-safe sea_query examples for CTEs, joins, windows, window_function_cust; expr_window docs - pool/pooled: ReadPreference when-to-use, PooledLifeExecutor routing, with_read_preference example - lib.rs and query/mod.rs: explicit opt-in APIs and default vs advanced SQL - memory-bank: rustdoc API surface audit snapshot
…pes vs find_related - lifeguard-derive: backtick snake_case in DeriveMigrationName rustdoc (clippy::doc_markdown) - lifeguard-migrate: composite PK emits #[primary_key] per column; column_map_from_merged_baseline; compare-schema column name drift for shared tables; PRD §5.7/§5.7a; DESIGN_SCHEMA; README; golden infer_composite_pk - query::scope + FindRelated: document find_related does not inherit parent scopes; Related/LazyLoader links - DESIGN_FIND_RELATED_SCOPES, PRD §7.7, SEAORM mapping; memory bank
![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 a unit-of-work Session with identity mapping, a compare-schema utility for migration reconciliation, and expanded SelectQuery support for CTEs and window functions. The review identifies a critical unsoundness in the Send implementation for identity map cells and potential performance issues related to OS-thread blocking mutexes in the may coroutine runtime. Further feedback suggests optimizing session notification hooks to reduce cloning overhead and improving the efficiency of string validation predicates by avoiding redundant character counting.
Comment on lines
+16
to
+23
| match value { | ||
| Value::String(Some(s)) if s.chars().count() > max => Err(format!( | ||
| "must be at most {max} characters (got {})", | ||
| s.chars().count() | ||
| )), | ||
| Value::String(Some(_)) | Value::String(None) => Ok(()), | ||
| _ => Ok(()), | ||
| } |
There was a problem hiding this comment.
In string_utf8_chars_max, s.chars().count() is called twice: once in the match guard and once in the error message formatting. Since this is an
if let Value::String(Some(s)) = value {
let count = s.chars().count();
if count > max {
return Err(format!("must be at most {max} characters (got {count})"));
}
}
Ok(())
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR introduces substantial feature additions across validation orchestration, session/unit-of-work patterns, F-style expressions, query builders, pool executor enhancements, schema migration tooling, and comprehensive documentation reorganization. Key additions include Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Rec as LifeRecord
participant Sess as Session
participant Map as Identity Map
participant Exec as LifeExecutor
participant DB as PostgreSQL
App->>Rec: mutate field
Rec->>Rec: mark __update_exprs or literal
Rec->>Sess: notify_identity_map_dirty
Sess->>Map: mark_dirty_key or enqueue pending
Note over App,Sess: Dirty tracking queued
App->>Sess: flush_dirty(executor, closure)
Sess->>Map: drain pending keys→mark_dirty_key
Map->>Map: iterate dirty by map-key order
loop Each dirty entry
Map->>Exec: closure(executor, rc, key)
Exec->>Rec: derive from model, call update()
Rec->>Exec: SET literals + __update_exprs
Exec->>DB: UPDATE with F-expressions
DB-->>Exec: row updated
end
Exec-->>Map: Result
Map->>Map: clear_dirty
Map-->>Sess: Ok
Sess-->>App: flush complete
sequenceDiagram
participant App as Application
participant Rec as LifeRecord
participant Val as Validation
participant VF as validate_fields
participant VM as validate_model
App->>Rec: set field value
Rec->>Rec: accumulate validation errors
Note over Rec: insert/update/delete triggered
Rec->>Val: run_validators(record, op)
Val->>Val: record.validation_strategy(op)
alt FailFast
Val->>VF: validate_fields
VF-->>Val: first error?
alt Error found
Val-->>Rec: Err(ValidationError)
else OK
Val->>VM: validate_model
VM-->>Val: result
Val-->>Rec: result
end
else Aggregate
par
Val->>VF: validate_fields
VF-->>Val: Vec of errors
and
Val->>VM: validate_model
VM-->>Val: Vec of errors
end
Note over Val: Collect all errors
Val-->>Rec: Err(Validation(combined))
end
Rec-->>App: validation result
sequenceDiagram
participant App as Application
participant Pool as LifeguardPool
participant Exec as PooledLifeExecutor
participant Dispatch as Dispatcher
participant Primary as Primary Tier
participant Replica as Replica Tier
App->>Exec: with_read_preference(ReadPreference::Primary)
Exec->>Exec: read_preference = Primary
Note over Exec: New executor with Primary preference
App->>Exec: query_all_values (read operation)
Exec->>Dispatch: dispatch_read_with_preference
alt ReadPreference::Primary
Dispatch->>Primary: route to primary
Primary-->>Dispatch: result
else ReadPreference::Default
Dispatch->>Replica: route to replica (if healthy)
Replica-->>Dispatch: result
end
Dispatch-->>Exec: rows
Exec-->>App: result set
Note over App,Replica: Writes always route to primary tier
App->>Exec: execute (write operation)
Exec->>Primary: route to primary (always)
Primary-->>Exec: rows affected
Exec-->>App: result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (12)
lifeguard-migrate/src/main.rs (1)
237-242: Use named format string syntax per coding guidelines.The format string uses a positional argument
{}instead of named format syntax.♻️ Suggested fix
if !generated_dir.is_dir() { return Err(MigrationError::InvalidFormat(format!( - "compare-schema: not a directory: {}", - generated_dir.display() + "compare-schema: not a directory: {path}", + path = generated_dir.display() ))); }As per coding guidelines: "Use named format string syntax instead of positional arguments in format! and write! macros"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lifeguard-migrate/src/main.rs` around lines 237 - 242, Replace the positional format! usage with named-format syntax when constructing the MigrationError for a non-directory generated_dir: update the MigrationError::InvalidFormat(format!(...)) call so the message uses a named placeholder like "{path}" and pass generated_dir.display() as path=generated_dir.display(); locate the block checking generated_dir.is_dir() in main.rs and change the format invocation accordingly to follow the coding guideline.lifeguard-migrate/tests/infer_schema_cli_subprocess.rs (1)
35-39: Use named format string syntax per coding guidelines.♻️ Suggested fix
assert!( out.status.success(), - "infer-schema failed: stderr={}", - String::from_utf8_lossy(&out.stderr) + "infer-schema failed: stderr={stderr}", + stderr = String::from_utf8_lossy(&out.stderr) );As per coding guidelines: "Use named format string syntax instead of positional arguments in format! and write! macros"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lifeguard-migrate/tests/infer_schema_cli_subprocess.rs` around lines 35 - 39, The assert! uses a positional format argument; change it to named format syntax per guidelines by updating the assertion message to use a named placeholder like "infer-schema failed: stderr={stderr}" and pass stderr = String::from_utf8_lossy(&out.stderr) as the named argument, referencing the existing variable out in the assertion.DEVELOPMENT.md (1)
42-42: Consider breaking up this documentation block for readability.This line is extremely dense with test files, environment variables, and coverage details. Consider using a bullet list or sub-sections to improve scannability.
📝 Suggested restructuring
-- **Live Postgres tests (optional):** `infer_schema_postgres_smoke.rs` (**`infer_schema_rust`** on `public`); `infer_schema_table_filter_si3.rs` (SI-3); **`infer_schema_cli_subprocess.rs`** (full **`infer-schema`** binary via `CARGO_BIN_EXE_lifeguard-migrate`); **`migration_db_compare_smoke.rs`** (`compare_generated_dir_to_live_db` + **`compare-schema`** CLI). All require **`TEST_DATABASE_URL`**, **`DATABASE_URL`**, or **`LIFEGUARD_DATABASE_URL`** (and subprocess tests need the Cargo-built binary); otherwise they skip. **`db_integration_suite`** includes **`column_f_update`** (F-style `UPDATE SET`, derived `set_*_expr` / `update()`, insert guard for `__update_exprs`), **`column_f_where`** (`WHERE` / `ORDER BY` with `Expr::expr` + `ColumnTrait::f_*`), and **`session_identity_flush`** (`ModelIdentityMap` / `Session`, `attach_session`, `flush_dirty` / `flush_dirty_in_transaction`, `register_pending_insert` / `flush_dirty_with_map_key` / `flush_dirty_in_transaction_with_map_key` / `flush_dirty_in_transaction_pooled_with_map_key` / `promote_pending_to_loaded`, `LifeRecord::update` / `insert`, PRD §9). +- **Live Postgres tests (optional):** All require `TEST_DATABASE_URL`, `DATABASE_URL`, or `LIFEGUARD_DATABASE_URL`; otherwise they skip. + - `infer_schema_postgres_smoke.rs` — `infer_schema_rust` on `public` + - `infer_schema_table_filter_si3.rs` — SI-3 + - `infer_schema_cli_subprocess.rs` — full `infer-schema` binary via `CARGO_BIN_EXE_lifeguard-migrate` + - `migration_db_compare_smoke.rs` — `compare_generated_dir_to_live_db` + `compare-schema` CLI +- **`db_integration_suite`** includes: + - `column_f_update` — F-style `UPDATE SET`, derived `set_*_expr` / `update()`, insert guard for `__update_exprs` + - `column_f_where` — `WHERE` / `ORDER BY` with `Expr::expr` + `ColumnTrait::f_*` + - `session_identity_flush` — `ModelIdentityMap` / `Session`, PRD §9🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@DEVELOPMENT.md` at line 42, The single dense sentence in DEVELOPMENT.md should be split into a short intro plus clear subsections or a bullet list: create a "Live Postgres tests (optional)" bullet listing the test files (infer_schema_postgres_smoke.rs, infer_schema_table_filter_si3.rs, infer_schema_cli_subprocess.rs, migration_db_compare_smoke.rs) and the required env vars (TEST_DATABASE_URL, DATABASE_URL, LIFEGUARD_DATABASE_URL) and note the subprocess/CARGO_BIN_EXE_lifeguard-migrate requirement; then create a "db_integration_suite" subsection or bullets that enumerate the included tests/features (column_f_update with set_*_expr/update()/__update_exprs, column_f_where using Expr::expr and ColumnTrait::f_*, and session_identity_flush with ModelIdentityMap/Session, attach_session, flush_dirty*, register_pending_insert, promote_pending_to_loaded, LifeRecord::update/insert). Ensure each symbol from the diff (file names and function/type names) appears verbatim to preserve searchability and break lines so each item is one bullet for readability.src/session/identity_model_cell.rs (1)
32-34:replace_withmay panic if already borrowed.The
borrow_mut()call will panic if theRefCellis already borrowed. In a session identity-map context where multiple record references might exist, this could be triggered unexpectedly.Consider using
try_borrow_mut()and returning aResultto allow callers to handle borrow conflicts gracefully:♻️ Proposed defensive change
- pub fn replace_with(&self, model: M) { - *self.rc.borrow_mut() = model; + pub fn replace_with(&self, model: M) -> Result<(), std::cell::BorrowMutError> { + *self.rc.try_borrow_mut()? = model; + Ok(()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/session/identity_model_cell.rs` around lines 32 - 34, The replace_with method currently calls self.rc.borrow_mut() which will panic if the RefCell is already borrowed; change replace_with to attempt a non-panicking borrow using self.rc.try_borrow_mut() and return a Result<(), BorrowError> (or a crate-appropriate error) so callers can handle borrow conflicts; update the signature of replace_with(M) to return Result<(), _>, map the TryBorrowError to your chosen error type and avoid unwraps or panicking operations in replace_with, leaving callers responsible for retrying or propagating the error.lifeguard-migrate/tests/migration_db_compare_smoke.rs (1)
24-25: Use named formatting here.Line 25 uses positional formatting, and the repo guideline asks for named arguments in
format!.♻️ Proposed fix
fn scratch_schema_name() -> String { - format!("lg_cmp_{}", Uuid::new_v4().simple()) + let schema_id = Uuid::new_v4().simple(); + format!("lg_cmp_{schema_id}") }As per coding guidelines,
**/*.rs: Use named format string syntax instead of positional arguments in format! and write! macros🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lifeguard-migrate/tests/migration_db_compare_smoke.rs` around lines 24 - 25, The function scratch_schema_name uses positional formatting; update the format! call in scratch_schema_name() to use named formatting per Rust guidelines (e.g., provide a named argument like id and pass Uuid::new_v4().simple() as its value) so the format string reads with a named placeholder instead of positional formatting.src/active_model/predicates.rs (1)
15-24: Consider caching the character count to avoid redundant computation.On the error path,
s.chars().count()is called twice (once for the condition check at line 17, once for the error message at line 19). While this is only on the error path, for very long strings this could be noticeable.♻️ Optional refactor to cache character count
pub fn string_utf8_chars_max(value: &Value, max: usize) -> Result<(), String> { match value { - Value::String(Some(s)) if s.chars().count() > max => Err(format!( - "must be at most {max} characters (got {})", - s.chars().count() - )), - Value::String(Some(_)) | Value::String(None) => Ok(()), + Value::String(Some(s)) => { + let count = s.chars().count(); + if count > max { + Err(format!("must be at most {max} characters (got {count})")) + } else { + Ok(()) + } + } + Value::String(None) => Ok(()), _ => Ok(()), } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/active_model/predicates.rs` around lines 15 - 24, In string_utf8_chars_max, avoid calling s.chars().count() twice by computing the character count once into a local variable (e.g., let len = s.chars().count()) when Value::String(Some(s)) is matched; then use that len for both the comparison and the error message in the Err branch of the function to eliminate redundant iteration over the string.src/session/uow.rs (4)
258-274: Same format string guideline applies here.♻️ Proposed fix
let tx = executor.begin().map_err(|e| { - ActiveModelError::DatabaseError(format!("begin transaction: {e}")) + ActiveModelError::DatabaseError(format!("begin transaction: {err}", err = e)) })?; let ex: &dyn LifeExecutor = &tx; match self.flush_dirty_with_map_key(ex, |e, m, k| f(e, m, k)) { Ok(()) => tx.commit().map_err(|e| { - ActiveModelError::DatabaseError(format!("commit: {e}")) + ActiveModelError::DatabaseError(format!("commit: {err}", err = e)) }), Err(e) => { if let Err(rb_err) = tx.rollback() { return Err(ActiveModelError::DatabaseError(format!( - "flush failed: {e}; rollback failed: {rb_err}" + "flush failed: {err}; rollback failed: {rollback_err}", + err = e, + rollback_err = rb_err ))); } Err(e) } }As per coding guidelines: "Use named format string syntax instead of positional arguments in format! and write! macros".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/session/uow.rs` around lines 258 - 274, The format! calls inside the transaction handling (the ActiveModelError::DatabaseError constructions in the begin() map_err, commit() map_err, and the rollback error formatting in the Err branch) use positional interpolation; change them to named format string syntax (e.g., format!("begin transaction: {err}", err = e)) for each occurrence (references: executor.begin() map_err closure, tx.commit() map_err closure, and the rollback error formatting where "flush failed: {e}; rollback failed: {rb_err}" is built) so they follow the named-argument guideline.
291-294: Same format string guideline applies to exclusive_primary_write_executor error handling.♻️ Proposed fix
- let exec = pool.exclusive_primary_write_executor().map_err(|e| { - ActiveModelError::DatabaseError(format!("exclusive primary executor: {e}")) + let exec = pool.exclusive_primary_write_executor().map_err(|err| { + ActiveModelError::DatabaseError(format!("exclusive primary executor: {err}", err = err)) })?;As per coding guidelines: "Use named format string syntax instead of positional arguments in format! and write! macros".
Also applies to: 306-309
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/session/uow.rs` around lines 291 - 294, Update the error formatting in the closure that maps pool.exclusive_primary_write_executor() to ActiveModelError::DatabaseError to use named format arguments instead of the positional {e}; e.g., change format!("exclusive primary executor: {e}") to format!("exclusive primary executor: {err}", err = e). Apply the same named-argument change to the similar error formatting occurrences around lines 306-309 (the other ActiveModelError::DatabaseError constructions) and keep references to exclusive_primary_write_executor and flush_dirty_with_begin_commit_sql unchanged.
230-246: Use named format string arguments per coding guidelines.The
format!calls use positional arguments like{e}and{rb_err}. Per coding guidelines, use named format string syntax.♻️ Proposed fix
let tx = executor.begin().map_err(|e| { - ActiveModelError::DatabaseError(format!("begin transaction: {e}")) + ActiveModelError::DatabaseError(format!("begin transaction: {err}", err = e)) })?; let ex: &dyn LifeExecutor = &tx; match self.flush_dirty(ex, |e, m| f(e, m)) { Ok(()) => tx.commit().map_err(|e| { - ActiveModelError::DatabaseError(format!("commit: {e}")) + ActiveModelError::DatabaseError(format!("commit: {err}", err = e)) }), Err(e) => { if let Err(rb_err) = tx.rollback() { return Err(ActiveModelError::DatabaseError(format!( - "flush failed: {e}; rollback failed: {rb_err}" + "flush failed: {err}; rollback failed: {rollback_err}", + err = e, + rollback_err = rb_err ))); } Err(e) } }As per coding guidelines: "Use named format string syntax instead of positional arguments in format! and write! macros".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/session/uow.rs` around lines 230 - 246, The format! calls in the transaction handling block should use named format arguments rather than positional ones; update the three ActiveModelError::DatabaseError uses inside the begin/commit/rollback logic (the map_err after executor.begin(), the map_err after tx.commit(), and the rollback error formatting inside the Err(e) arm) to use named placeholders like {err} or {rb_err} and pass those names in the format! call so the messages include the same values but follow the named format string syntax; locate this in the function that calls executor.begin(), creates tx (let tx = executor.begin()), calls self.flush_dirty(...), and invokes tx.commit() or tx.rollback() to apply the changes.
335-353: Same format string guideline applies to the pooled transaction helper.♻️ Proposed fix
- executor.execute("BEGIN", &[]).map_err(|e| { - ActiveModelError::DatabaseError(format!("begin transaction: {e}")) + executor.execute("BEGIN", &[]).map_err(|err| { + ActiveModelError::DatabaseError(format!("begin transaction: {err}", err = err)) })?; match session.flush_dirty_with_map_key(executor, |e, m, k| f(e, m, k)) { Ok(()) => { - executor.execute("COMMIT", &[]).map_err(|e| { - ActiveModelError::DatabaseError(format!("commit: {e}")) + executor.execute("COMMIT", &[]).map_err(|err| { + ActiveModelError::DatabaseError(format!("commit: {err}", err = err)) })?; Ok(()) } Err(e) => { if let Err(rb_err) = executor.execute("ROLLBACK", &[]) { return Err(ActiveModelError::DatabaseError(format!( - "flush failed: {e}; rollback failed: {rb_err}" + "flush failed: {err}; rollback failed: {rollback_err}", + err = e, + rollback_err = rb_err ))); } Err(e) } }As per coding guidelines: "Use named format string syntax instead of positional arguments in format! and write! macros".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/session/uow.rs` around lines 335 - 353, Replace positional-format usage in the pooled transaction helper by using named format parameters: change format!("begin transaction: {e}") inside the map_err closure on executor.execute to a named format like format!("begin transaction: {err}", err = e); do the same for the commit map_err (format!("commit: {e}" -> format!("commit: {err}", err = e))); and update the rollback-error composition to use named names (format!("flush failed: {e}; rollback failed: {rb_err}" -> format!("flush failed: {err}; rollback failed: {rb_err}", err = e, rb_err = rb_err)). Target the calls around executor.execute, the map_err closures, and the Err branch that constructs ActiveModelError::DatabaseError and the match over session.flush_dirty_with_map_key.lifeguard-derive/src/macros/life_record.rs (1)
903-914: Consider documenting theto_model()call cost in__lg_session_notify_dirty.Each notifying mutation calls
to_model()when__lg_session_modelis set, which clones all fields. While this is documented inattach_session_with_model's rustdoc, a brief inline comment at the call site would help maintainers understand the performance tradeoff.📝 Suggested inline comment
#[doc(hidden)] #[inline] fn __lg_session_notify_dirty(&self) { if let Some(ref n) = self.__lg_session_notifier { n.notify_identity_map_dirty(self.identity_map_key()); } if let Some(ref cell) = self.__lg_session_model { + // Sync literal field values to the identity-map Rc; this calls to_model() + // which clones all fields. F-style expressions are not represented on Model. if let Ok(m) = self.to_model() { cell.replace_with(m); } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lifeguard-derive/src/macros/life_record.rs` around lines 903 - 914, The call site in __lg_session_notify_dirty currently invokes to_model() when __lg_session_model is set, which can be expensive because to_model() clones all fields (as documented in attach_session_with_model); add a brief inline comment immediately above the to_model() call in __lg_session_notify_dirty explaining that to_model() performs a full clone of the record and may be costly on hot paths, so maintainers know the performance tradeoff and can consider alternatives (e.g., avoiding __lg_session_model, using a cheaper diff/partial update, or caching) when modifying notification behavior.src/session/mod.rs (1)
109-117: Consider documenting the theoretical u64 wraparound behavior.
wrapping_add(1)onnext_pending_idis correct, but after 2^64 registrations without promotion/removal, a key collision could occur. This is practically unreachable in any real workload, but a brief doc comment noting this would help future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/session/mod.rs` around lines 109 - 117, Add a short doc comment on register_pending_insert explaining that next_pending_id uses wrapping_add(1) and therefore can theoretically wrap after 2^64 increments, which would cause key reuse/collision of keys generated with PENDING_INSERT_KEY_PREFIX; mention this is practically unreachable but intentional behavior and that callers should promote/remove entries to avoid exhausting the counter if running an extremely long-lived/high-throughput process. Reference register_pending_insert, next_pending_id, and PENDING_INSERT_KEY_PREFIX in the comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/db_integration/pool_read_replica.rs`:
- Around line 323-325: The test is colliding with other replica tests because
setup_schema_on_primary drops/recreates pool_replica_test.t_pool_replica_smoke;
change the test to isolate its schema or serialize the module. Either (A)
generate a unique per-test schema/table suffix and pass it through
TestDatabase::with_url (or replace the TestDatabase::with_url call with a helper
that returns a DB bound to a unique schema) and update setup_schema_on_primary
to accept that schema name and create/target
pool_replica_test_<suffix>.t_pool_replica_smoke_<suffix>, or (B) add a
module-wide serial guard (e.g., a static Mutex/serial_test attribute) around
tests that use setup_schema_on_primary so they run one at a time; update
references to the table/schema in this test and in setup_schema_on_primary to
use the chosen schema identifier (TestDatabase::with_url,
setup_schema_on_primary, and pool_replica_test.t_pool_replica_smoke are the
symbols to modify).
---
Nitpick comments:
In `@DEVELOPMENT.md`:
- Line 42: The single dense sentence in DEVELOPMENT.md should be split into a
short intro plus clear subsections or a bullet list: create a "Live Postgres
tests (optional)" bullet listing the test files (infer_schema_postgres_smoke.rs,
infer_schema_table_filter_si3.rs, infer_schema_cli_subprocess.rs,
migration_db_compare_smoke.rs) and the required env vars (TEST_DATABASE_URL,
DATABASE_URL, LIFEGUARD_DATABASE_URL) and note the
subprocess/CARGO_BIN_EXE_lifeguard-migrate requirement; then create a
"db_integration_suite" subsection or bullets that enumerate the included
tests/features (column_f_update with set_*_expr/update()/__update_exprs,
column_f_where using Expr::expr and ColumnTrait::f_*, and session_identity_flush
with ModelIdentityMap/Session, attach_session, flush_dirty*,
register_pending_insert, promote_pending_to_loaded, LifeRecord::update/insert).
Ensure each symbol from the diff (file names and function/type names) appears
verbatim to preserve searchability and break lines so each item is one bullet
for readability.
In `@lifeguard-derive/src/macros/life_record.rs`:
- Around line 903-914: The call site in __lg_session_notify_dirty currently
invokes to_model() when __lg_session_model is set, which can be expensive
because to_model() clones all fields (as documented in
attach_session_with_model); add a brief inline comment immediately above the
to_model() call in __lg_session_notify_dirty explaining that to_model() performs
a full clone of the record and may be costly on hot paths, so maintainers know
the performance tradeoff and can consider alternatives (e.g., avoiding
__lg_session_model, using a cheaper diff/partial update, or caching) when
modifying notification behavior.
In `@lifeguard-migrate/src/main.rs`:
- Around line 237-242: Replace the positional format! usage with named-format
syntax when constructing the MigrationError for a non-directory generated_dir:
update the MigrationError::InvalidFormat(format!(...)) call so the message uses
a named placeholder like "{path}" and pass generated_dir.display() as
path=generated_dir.display(); locate the block checking generated_dir.is_dir()
in main.rs and change the format invocation accordingly to follow the coding
guideline.
In `@lifeguard-migrate/tests/infer_schema_cli_subprocess.rs`:
- Around line 35-39: The assert! uses a positional format argument; change it to
named format syntax per guidelines by updating the assertion message to use a
named placeholder like "infer-schema failed: stderr={stderr}" and pass stderr =
String::from_utf8_lossy(&out.stderr) as the named argument, referencing the
existing variable out in the assertion.
In `@lifeguard-migrate/tests/migration_db_compare_smoke.rs`:
- Around line 24-25: The function scratch_schema_name uses positional
formatting; update the format! call in scratch_schema_name() to use named
formatting per Rust guidelines (e.g., provide a named argument like id and pass
Uuid::new_v4().simple() as its value) so the format string reads with a named
placeholder instead of positional formatting.
In `@src/active_model/predicates.rs`:
- Around line 15-24: In string_utf8_chars_max, avoid calling s.chars().count()
twice by computing the character count once into a local variable (e.g., let len
= s.chars().count()) when Value::String(Some(s)) is matched; then use that len
for both the comparison and the error message in the Err branch of the function
to eliminate redundant iteration over the string.
In `@src/session/identity_model_cell.rs`:
- Around line 32-34: The replace_with method currently calls
self.rc.borrow_mut() which will panic if the RefCell is already borrowed; change
replace_with to attempt a non-panicking borrow using self.rc.try_borrow_mut()
and return a Result<(), BorrowError> (or a crate-appropriate error) so callers
can handle borrow conflicts; update the signature of replace_with(M) to return
Result<(), _>, map the TryBorrowError to your chosen error type and avoid
unwraps or panicking operations in replace_with, leaving callers responsible for
retrying or propagating the error.
In `@src/session/mod.rs`:
- Around line 109-117: Add a short doc comment on register_pending_insert
explaining that next_pending_id uses wrapping_add(1) and therefore can
theoretically wrap after 2^64 increments, which would cause key reuse/collision
of keys generated with PENDING_INSERT_KEY_PREFIX; mention this is practically
unreachable but intentional behavior and that callers should promote/remove
entries to avoid exhausting the counter if running an extremely
long-lived/high-throughput process. Reference register_pending_insert,
next_pending_id, and PENDING_INSERT_KEY_PREFIX in the comment.
In `@src/session/uow.rs`:
- Around line 258-274: The format! calls inside the transaction handling (the
ActiveModelError::DatabaseError constructions in the begin() map_err, commit()
map_err, and the rollback error formatting in the Err branch) use positional
interpolation; change them to named format string syntax (e.g., format!("begin
transaction: {err}", err = e)) for each occurrence (references: executor.begin()
map_err closure, tx.commit() map_err closure, and the rollback error formatting
where "flush failed: {e}; rollback failed: {rb_err}" is built) so they follow
the named-argument guideline.
- Around line 291-294: Update the error formatting in the closure that maps
pool.exclusive_primary_write_executor() to ActiveModelError::DatabaseError to
use named format arguments instead of the positional {e}; e.g., change
format!("exclusive primary executor: {e}") to format!("exclusive primary
executor: {err}", err = e). Apply the same named-argument change to the similar
error formatting occurrences around lines 306-309 (the other
ActiveModelError::DatabaseError constructions) and keep references to
exclusive_primary_write_executor and flush_dirty_with_begin_commit_sql
unchanged.
- Around line 230-246: The format! calls in the transaction handling block
should use named format arguments rather than positional ones; update the three
ActiveModelError::DatabaseError uses inside the begin/commit/rollback logic (the
map_err after executor.begin(), the map_err after tx.commit(), and the rollback
error formatting inside the Err(e) arm) to use named placeholders like {err} or
{rb_err} and pass those names in the format! call so the messages include the
same values but follow the named format string syntax; locate this in the
function that calls executor.begin(), creates tx (let tx = executor.begin()),
calls self.flush_dirty(...), and invokes tx.commit() or tx.rollback() to apply
the changes.
- Around line 335-353: Replace positional-format usage in the pooled transaction
helper by using named format parameters: change format!("begin transaction:
{e}") inside the map_err closure on executor.execute to a named format like
format!("begin transaction: {err}", err = e); do the same for the commit map_err
(format!("commit: {e}" -> format!("commit: {err}", err = e))); and update the
rollback-error composition to use named names (format!("flush failed: {e};
rollback failed: {rb_err}" -> format!("flush failed: {err}; rollback failed:
{rb_err}", err = e, rb_err = rb_err)). Target the calls around executor.execute,
the map_err closures, and the Err branch that constructs
ActiveModelError::DatabaseError and the match over
session.flush_dirty_with_map_key.
🪄 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: 07c0d58e-f837-4161-a559-d4a4cec1d982
📒 Files selected for processing (52)
.agent/memory-bank/activeContext.md.agent/memory-bank/progress.mdDEVELOPMENT.mdREADME.mddocs/planning/DESIGN_FIND_RELATED_SCOPES.mddocs/planning/DESIGN_SCHEMA_INFERENCE_CLI_CODEGEN.mddocs/planning/DESIGN_SESSION_UOW.mddocs/planning/PRD_SCHEMA_VALIDATORS_SESSION_AND_SCOPES.mddocs/planning/README.mddocs/planning/lifeguard-derive/SEAORM_LIFEGUARD_MAPPING.mdlifeguard-derive/src/attributes.rslifeguard-derive/src/lib.rslifeguard-derive/src/macros/life_record.rslifeguard-derive/src/macros/migration_name_derive.rslifeguard-derive/src/macros/mod.rslifeguard-derive/src/macros/scope_attr.rslifeguard-derive/tests/test_derive_migration_name.rslifeguard-derive/tests/test_minimal.rslifeguard-migrate/README.mdlifeguard-migrate/src/generated_migration_diff.rslifeguard-migrate/src/lib.rslifeguard-migrate/src/main.rslifeguard-migrate/src/schema_infer.rslifeguard-migrate/src/schema_migration_compare.rslifeguard-migrate/tests/golden/infer_composite_pk.expected.rslifeguard-migrate/tests/infer_schema_cli_subprocess.rslifeguard-migrate/tests/infer_schema_postgres_smoke.rslifeguard-migrate/tests/infer_schema_table_filter_si3.rslifeguard-migrate/tests/migration_db_compare_smoke.rssrc/active_model/mod.rssrc/active_model/predicates.rssrc/active_model/traits.rssrc/active_model/validate_op.rssrc/active_model/validation.rssrc/lib.rssrc/migration/migration_name.rssrc/migration/mod.rssrc/pool/mod.rssrc/pool/pooled.rssrc/query/column/column_trait.rssrc/query/mod.rssrc/query/scope.rssrc/query/select.rssrc/relation/traits.rssrc/session/identity_model_cell.rssrc/session/mod.rssrc/session/uow.rstests/db_integration/column_f_update.rstests/db_integration/column_f_where.rstests/db_integration/pool_read_replica.rstests/db_integration/session_identity_flush.rstests/db_integration_suite.rs
…c fixes - Add ARCHITECTURE.md with full diagrams; README summary + drill-down link - README: competitive matrix and current status updates (prior session) - Rustdoc: session/active_model/logging/predicates/query trait links and hygiene - Memory bank: progress entries
Relocate competitive matrix, roadmap, and repository-truth bullets from README. Update ARCHITECTURE, PRD, SEAORM mapping, and memory bank cross-links.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.agent/memory-bank/progress.md (1)
193-200:⚠️ Potential issue | 🟡 MinorStale metrics conflict with newer entries in this same file.
Lines 193-200 report outdated totals (for example test counts) that contradict newer completed entries above, so this section can no longer be treated as accurate status. Please either recompute these metrics or label them as historical snapshot only.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agent/memory-bank/progress.md around lines 193 - 200, Update the "Metrics" section entries so they no longer conflict with newer entries: recompute and correct the values for "Edge case coverage", "Test coverage", "Tests passing", "Memory Bank files", "CRUD operations", "ActiveModel/Record Operations", and "Query Builder Methods" to match the latest data elsewhere in the file, or else prepend a clear "Historical snapshot" label to the entire Metrics block indicating the values are outdated; ensure you update the specific lines containing the bullet keys "Tests passing" and "CRUD operations: Fixed critical bug in get() method" (and related bullets) so readers can unambiguously tell whether those numbers are current or historical.
🧹 Nitpick comments (1)
docs/planning/PRD_SCHEMA_VALIDATORS_SESSION_AND_SCOPES.md (1)
140-143: Schema reconciliation scope should explicitly include index-column validation follow-up.This section documents table/column-name reconciliation only. Add a follow-on requirement/check for index definitions where every indexed column must map to an existing struct field (and migration generation validates
CREATE INDEXsucceeds), so schema parity catches index drift too.Based on learnings: In lifeguard-derive and lifeguard-migrate code, consider implementing compile-time or migration-time validation to ensure every index column matches a struct field.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/planning/PRD_SCHEMA_VALIDATORS_SESSION_AND_SCOPES.md` around lines 140 - 143, Add index-column reconciliation: extend lifeguard_migrate::schema_migration_compare (and the compare-schema CLI path) to fetch index definitions (e.g., from information_schema/statistics or pg_indexes), parse each index's column list and verify every indexed column exists in the column_map_from_merged_baseline mapping and in the database baseline; surface mismatches as failures/warnings and ensure migration generation includes and validates CREATE INDEX operations (i.e., generated migration should attempt to create the index and the test harness in tests/migration_db_compare_smoke.rs should assert success/failure accordingly); also add a follow-up validation hook in lifeguard-derive/migration-time checks to ensure compile-time/migration-time mapping from struct fields to indexed columns is enforced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@COMPARISON.md`:
- Around line 137-140: Update COMPARISON.md to replace absolute language and
reflect precise compatibility: change the “You cannot mix and match” paragraph
to state factual compatibility statuses (e.g., “unsupported” vs “technically
incompatible”) and remove “must”/“fundamentally incompatible” wording; in the
comparison table split Diesel into “Diesel (sync/core)” and “Diesel-async”, mark
SeaORM as requiring Tokio runtime feature flags (e.g.,
runtime-tokio-native-tls), mark SQLx as unsupported/not recommended for
non-Tokio runtimes (may panic if no Tokio/async-std feature enabled) rather than
strictly incompatible, and mark Diesel (sync/core) as runtime-agnostic when used
via blocking calls while Diesel-async remains Tokio-focused—ensure you reference
BRRTRouter, Lifeguard, may, Tokio, async/await, SeaORM, SQLx, Diesel, and
Diesel-async in the updated text/table.
In `@docs/planning/PRD_SCHEMA_VALIDATORS_SESSION_AND_SCOPES.md`:
- Around line 200-205: The documentation is inconsistent: the Types line lists
ValidateOp as `Insert | Update` while the Integration/Deletes bullets mention
`ValidateOp::Delete`; update the Types entry for `ValidateOp` to include
`Delete` (i.e., `Insert | Update | Delete`) and verify the Integration/“Delete”
bullet and V-3/other references (e.g., lifeguard-derive generated hooks,
ValidateOp, before_delete) remain consistent with that change so all bullets
reference the same set of operations.
---
Outside diff comments:
In @.agent/memory-bank/progress.md:
- Around line 193-200: Update the "Metrics" section entries so they no longer
conflict with newer entries: recompute and correct the values for "Edge case
coverage", "Test coverage", "Tests passing", "Memory Bank files", "CRUD
operations", "ActiveModel/Record Operations", and "Query Builder Methods" to
match the latest data elsewhere in the file, or else prepend a clear "Historical
snapshot" label to the entire Metrics block indicating the values are outdated;
ensure you update the specific lines containing the bullet keys "Tests passing"
and "CRUD operations: Fixed critical bug in get() method" (and related bullets)
so readers can unambiguously tell whether those numbers are current or
historical.
---
Nitpick comments:
In `@docs/planning/PRD_SCHEMA_VALIDATORS_SESSION_AND_SCOPES.md`:
- Around line 140-143: Add index-column reconciliation: extend
lifeguard_migrate::schema_migration_compare (and the compare-schema CLI path) to
fetch index definitions (e.g., from information_schema/statistics or
pg_indexes), parse each index's column list and verify every indexed column
exists in the column_map_from_merged_baseline mapping and in the database
baseline; surface mismatches as failures/warnings and ensure migration
generation includes and validates CREATE INDEX operations (i.e., generated
migration should attempt to create the index and the test harness in
tests/migration_db_compare_smoke.rs should assert success/failure accordingly);
also add a follow-up validation hook in lifeguard-derive/migration-time checks
to ensure compile-time/migration-time mapping from struct fields to indexed
columns is enforced.
🪄 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: 5cf416b3-15c8-490c-bb19-f2b486e92902
📒 Files selected for processing (12)
.agent/memory-bank/activeContext.md.agent/memory-bank/progress.mdARCHITECTURE.mdCOMPARISON.mdOBSERVABILITY.mdREADME.mdROADMAP.mdSTATUS.mdVISION.mddocs/OBSERVABILITY.mddocs/planning/PRD_SCHEMA_VALIDATORS_SESSION_AND_SCOPES.mddocs/planning/lifeguard-derive/SEAORM_LIFEGUARD_MAPPING.md
✅ Files skipped from review due to trivial changes (8)
- docs/OBSERVABILITY.md
- ROADMAP.md
- OBSERVABILITY.md
- STATUS.md
- ARCHITECTURE.md
- VISION.md
- README.md
- docs/planning/lifeguard-derive/SEAORM_LIFEGUARD_MAPPING.md
🚧 Files skipped from review as they are similar to previous changes (1)
- .agent/memory-bank/activeContext.md
casibbald
force-pushed
the
feat/schema_validators_session_and_scopes_2
branch
from
d5994d1 to
7f9d826
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 5 total unresolved issues (including 4 from previous reviews).
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
…ttribute - Derive collects all #[validate(custom)] field errors when ValidationStrategy::Aggregate applies; add struct #[validation_strategy = "aggregate"|"fail_fast"] and register on LifeModel/LifeRecord derives. - Tests for multi-field FailFast vs Aggregate; shared validators in test_minimal. - Docs: STATUS merged into COMPARISON; README/ROADMAP/ARCHITECTURE/DEVELOPMENT/VISION updates; SECURITY_PROMPT; PRD validator section. - Session identity cell docs, pool metrics, predicates, migrate CLI/tests, pool_read_replica isolation; memory bank update.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
COMPARISON.md (1)
65-65:⚠️ Potential issue | 🟡 MinorKeep runtime-compatibility labels consistent across the doc.
Line 65 still marks Diesel and SQLx as “Incompatible,” but Lines 148-159 already describe a more precise split (runtime-bound async crates vs runtime-agnostic sync Diesel). Align this row with the nuanced wording to avoid contradictory guidance.
📝 Suggested wording update
-| **Coroutine Runtime** | ✅✅✅✅ **Native support (UNIQUE)** | ✅ **Implemented** | ❌ Incompatible | ❌ Incompatible | ❌ Incompatible | +| **Coroutine Runtime** | ✅✅✅✅ **Native support (UNIQUE)** | ✅ **Implemented** | ⚠️ Async runtime-bound (`runtime-*` features; not `may`-native) | ⚠️ Sync Diesel is runtime-agnostic (blocking API) | ⚠️ Async runtime-bound (Tokio/async-std features; not `may`-native) |According to official docs, what are the runtime requirements/support statements for SeaORM, SQLx, Diesel (sync/core), and diesel-async (Tokio/async-std/runtime-agnostic distinctions)?Also applies to: 148-159
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@COMPARISON.md` at line 65, Update the runtime-compatibility labels in the COMPARISON.md table row that currently shows Diesel and SQLx as “Incompatible” so they match the more nuanced descriptions later in the doc (lines describing runtime-bound async crates vs sync Diesel and diesel-async). Specifically, change the labels for SeaORM/SQLx/Diesel/diesel-async in the row under the "Coroutine Runtime" column to reflect: SeaORM and SQLx as runtime-bound async (needs an async runtime), Diesel (sync/core) as runtime-agnostic (works without an async runtime), and diesel-async as runtime-specific (requires Tokio/async-std or runtime adapter) so the table aligns with the detailed text in the later section.
🧹 Nitpick comments (2)
src/session/mod.rs (1)
129-147: Clarify potential Rc orphaning on PK collision.When
promote_pending_to_loadedis called, if the model's PK already exists in the map (registered via another path),register_loadedreturns the existingRc, not a new one. Callers holding the originalRcfromregister_pending_insertwould then have an orphaned reference not in the map.This matches the documented "first load wins" semantics (line 70), but consider adding a note in the doc comment that the returned
Rcmay differ from the one originally returned byregister_pending_insertin case of PK collision.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/session/mod.rs` around lines 129 - 147, Add a short doc-note to promote_pending_to_loaded explaining that because register_loaded implements "first load wins" semantics, if the model's primary key already exists in the identity map the Rc returned by promote_pending_to_loaded may differ from the Rc originally returned by register_pending_insert (callers could therefore hold an orphaned Rc); reference the functions promote_pending_to_loaded, register_loaded, and register_pending_insert and mention the is_pending_insert_key/map collision scenario so callers know to compare identity map entries instead of relying on the original Rc.lifeguard-derive/src/macros/life_record.rs (1)
991-996: Silentreplace_withfailure may mask borrow conflicts.When
cell.replace_with(m)returnsErr(BorrowMutError)(concurrent borrow), the error is silently discarded withlet _ =. While documenting the single-threaded contract makes this "shouldn't happen" in correct usage, a debug-mode warning ordebug_assert!could help catch contract violations during development.🔧 Optional: Log or assert on unexpected borrow error
if let Ok(m) = self.to_model() { - let _ = cell.replace_with(m); + if let Err(_e) = cell.replace_with(m) { + #[cfg(debug_assertions)] + eprintln!("lifeguard: SessionIdentityModelCell replace_with failed (concurrent borrow?)"); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lifeguard-derive/src/macros/life_record.rs` around lines 991 - 996, The current silent discard of replace_with errors hides BorrowMutError borrow conflicts on self.__lg_session_model; update the block that calls self.to_model() and cell.replace_with(m) to detect Err(e) and surface it in debug builds (e.g., debug_assert!(false, ... ) or debug_warn/log) so development catches unexpected concurrent-borrow violations: after calling self.to_model() in the branch that checks self.__lg_session_model, match the Result from cell.replace_with(m) and on Err(BorrowMutError) emit a debug_assert or a debug-level log including the error and context (mention Self::to_model and __lg_session_model) rather than silently ignoring it.
🤖 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-migrate/tests/migration_db_compare_smoke.rs`:
- Around line 120-129: The test currently forces UTF-8 with
dir.path().to_str().expect(...) when building the Command in the 'out' creation,
which can panic on non-UTF8 paths; replace that coerced string with the
Path/OsStr directly by removing the to_str().expect(...) and passing dir.path()
via .arg(dir.path()) (or using .arg(dir.path().as_os_str())) instead of
including it in the &str array passed to .args([...]) so the Command receives a
Path/OsStr and won't panic on non-UTF8 temp paths.
---
Duplicate comments:
In `@COMPARISON.md`:
- Line 65: Update the runtime-compatibility labels in the COMPARISON.md table
row that currently shows Diesel and SQLx as “Incompatible” so they match the
more nuanced descriptions later in the doc (lines describing runtime-bound async
crates vs sync Diesel and diesel-async). Specifically, change the labels for
SeaORM/SQLx/Diesel/diesel-async in the row under the "Coroutine Runtime" column
to reflect: SeaORM and SQLx as runtime-bound async (needs an async runtime),
Diesel (sync/core) as runtime-agnostic (works without an async runtime), and
diesel-async as runtime-specific (requires Tokio/async-std or runtime adapter)
so the table aligns with the detailed text in the later section.
---
Nitpick comments:
In `@lifeguard-derive/src/macros/life_record.rs`:
- Around line 991-996: The current silent discard of replace_with errors hides
BorrowMutError borrow conflicts on self.__lg_session_model; update the block
that calls self.to_model() and cell.replace_with(m) to detect Err(e) and surface
it in debug builds (e.g., debug_assert!(false, ... ) or debug_warn/log) so
development catches unexpected concurrent-borrow violations: after calling
self.to_model() in the branch that checks self.__lg_session_model, match the
Result from cell.replace_with(m) and on Err(BorrowMutError) emit a debug_assert
or a debug-level log including the error and context (mention Self::to_model and
__lg_session_model) rather than silently ignoring it.
In `@src/session/mod.rs`:
- Around line 129-147: Add a short doc-note to promote_pending_to_loaded
explaining that because register_loaded implements "first load wins" semantics,
if the model's primary key already exists in the identity map the Rc returned by
promote_pending_to_loaded may differ from the Rc originally returned by
register_pending_insert (callers could therefore hold an orphaned Rc); reference
the functions promote_pending_to_loaded, register_loaded, and
register_pending_insert and mention the is_pending_insert_key/map collision
scenario so callers know to compare identity map entries instead of relying on
the original Rc.
🪄 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: fee69f06-2830-471b-bb08-6185fbbaf986
📒 Files selected for processing (23)
.agent/memory-bank/activeContext.md.agent/memory-bank/progress.mdARCHITECTURE.mdCOMPARISON.mdDEVELOPMENT.mdREADME.mdROADMAP.mdSECURITY_PROMPT.mdVISION.mddocs/planning/PRD_SCHEMA_VALIDATORS_SESSION_AND_SCOPES.mdlifeguard-derive/src/attributes.rslifeguard-derive/src/lib.rslifeguard-derive/src/macros/life_record.rslifeguard-derive/tests/test_minimal.rslifeguard-migrate/src/main.rslifeguard-migrate/tests/infer_schema_cli_subprocess.rslifeguard-migrate/tests/migration_db_compare_smoke.rssrc/active_model/predicates.rssrc/pool/pooled.rssrc/session/identity_model_cell.rssrc/session/mod.rssrc/session/uow.rstests/db_integration/pool_read_replica.rs
✅ Files skipped from review due to trivial changes (7)
- lifeguard-migrate/tests/infer_schema_cli_subprocess.rs
- SECURITY_PROMPT.md
- ROADMAP.md
- ARCHITECTURE.md
- VISION.md
- DEVELOPMENT.md
- lifeguard-derive/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (7)
- lifeguard-derive/src/attributes.rs
- lifeguard-migrate/src/main.rs
- tests/db_integration/pool_read_replica.rs
- lifeguard-derive/tests/test_minimal.rs
- src/active_model/predicates.rs
- src/pool/pooled.rs
- docs/planning/PRD_SCHEMA_VALIDATORS_SESSION_AND_SCOPES.md
Comment on lines
+120
to
+129
| let out = std::process::Command::new(&bin) | ||
| .args([ | ||
| "compare-schema", | ||
| "--database-url", | ||
| &url, | ||
| "--schema", | ||
| &schema, | ||
| "--generated-dir", | ||
| dir.path().to_str().expect("utf8 temp path"), | ||
| ]) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify there are no remaining UTF-8 coercions for process args in this test file.
rg -n 'to_str\(\)\.expect\("utf8 temp path"\)|--generated-dir' --type=rustRepository: microscaler/lifeguard
Length of output: 269
🏁 Script executed:
# Read the actual file to verify lines 120-129
sed -n '115,135p' lifeguard-migrate/tests/migration_db_compare_smoke.rsRepository: microscaler/lifeguard
Length of output: 654
🏁 Script executed:
# Search for other instances of similar UTF-8 coercions in the file
rg -n 'to_str\(\)\.expect' lifeguard-migrate/tests/migration_db_compare_smoke.rsRepository: microscaler/lifeguard
Length of output: 126
Remove unnecessary UTF-8 coercion for temp path argument.
Line 128 uses dir.path().to_str().expect("utf8 temp path"), which can panic on paths containing non-UTF8 sequences. Since Command::arg() accepts &Path directly via AsRef<OsStr>, pass the path without coercion:
Proposed fix
let out = std::process::Command::new(&bin)
- .args([
- "compare-schema",
- "--database-url",
- &url,
- "--schema",
- &schema,
- "--generated-dir",
- dir.path().to_str().expect("utf8 temp path"),
- ])
+ .arg("compare-schema")
+ .arg("--database-url")
+ .arg(&url)
+ .arg("--schema")
+ .arg(&schema)
+ .arg("--generated-dir")
+ .arg(dir.path())
.output()
.expect("spawn compare-schema");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lifeguard-migrate/tests/migration_db_compare_smoke.rs` around lines 120 -
129, The test currently forces UTF-8 with dir.path().to_str().expect(...) when
building the Command in the 'out' creation, which can panic on non-UTF8 paths;
replace that coerced string with the Path/OsStr directly by removing the
to_str().expect(...) and passing dir.path() via .arg(dir.path()) (or using
.arg(dir.path().as_os_str())) instead of including it in the &str array passed
to .args([...]) so the Command receives a Path/OsStr and won't panic on non-UTF8
temp paths.
This was referenced May 4, 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
Medium risk because it introduces new
lifeguard-deriveattribute parsing and proc-macro exports (validate,validation_strategy,scope,DeriveMigrationName) that affect generated code and could break consumers if edge cases are missed; the rest is largely documentation reshaping.Overview
Restructures top-level documentation by slimming
README.mdinto a pitch + quick-start + doc index and moving detailed material into new/expanded docs:COMPARISON.md(repository truth + competitive matrix),VISION.md,ARCHITECTURE.md(full diagrams),OBSERVABILITY.md, andROADMAP.md, with cross-links updated across planning docs.Extends
lifeguard-derivevalidation/scope support by parsing#[validate(custom = path)]on fields and a struct-level#[validation_strategy = "aggregate"|"fail_fast"], and by exposing new proc-macro entrypoints#[scope]and#[derive(DeriveMigrationName)](plus updating derive attribute allow-lists to acceptvalidate/validation_strategy).Written by Cursor Bugbot for commit 79951ff. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
Release Notes
New Features
scope_or,scope_any) for complex queriesDocumentation