feat(logging): add durable lifecycle logging foundation - #1174
Conversation
|
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:
📝 WalkthroughWalkthroughThe PR adds structured audit and lifecycle logging, SQLite-backed persistence, privacy-safe artifact capture, runtime configuration apply support, Windows ACL coverage, workspace integration, and invitation presentation changes. ChangesLogging and persistence stack
Invitation presentation and CI
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
This pull request is currently a draft. Reviews will not take place until the PR is marked as ready for review. |
e84d1d9 to
65eb641
Compare
65eb641 to
d458ec6
Compare
d458ec6 to
e605e92
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
crates/mesh-llm-tui/src/output/rendering/join_token.rs (1)
146-168: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAllocate enough height for the invite message.
Lines 146-168 return a one-row area in normal mode.
join_token_messagerenders the mesh label and token on separate lines.Paragraphclips the token line.This also causes
tui_invite_event_exposes_token_in_dashboardto fail because it expects the rendered token. Use the panel inner area, or reserve at least the wrapped message height, for normal-mode rendering.Proposed fix
Rect { x: inner_x, - y: panel_area.y.saturating_add(panel_area.height / 2), + y: panel_area.y.saturating_add(1), width: inner_right.saturating_sub(inner_x), - height: 1, + height: panel_area.height.saturating_sub(2), }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/mesh-llm-tui/src/output/rendering/join_token.rs` around lines 146 - 168, Update join_token_text_area to reserve enough vertical space for both the mesh label and token rendered by join_token_message, rather than returning a one-row Rect in normal mode. Use the panel’s inner area or the required wrapped message height while preserving the existing empty-area behavior for insufficient panel dimensions.crates/mesh-llm-host-runtime/src/runtime/run_auto.rs (1)
256-262: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAudit logging never starts for
--local-model-onlyand--plugin.
run_runtime_clireturns at Line 257 forlocal_model_onlyand at Line 261 forplugin. Both returns happen before the config load at Line 278 and beforeinit_audit_loggingat Line 290. A user who passes--audit-log-pathtogether with--local-model-onlygets no audit sink and no error. The local-model-only path still serves the OpenAI API, so the requested audit records are lost.Initialize audit logging before these early returns, or reject the flag combination explicitly.
Also applies to: 289-297
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/mesh-llm-host-runtime/src/runtime/run_auto.rs` around lines 256 - 262, Update run_runtime_cli so audit logging is initialized before the local_model_only and plugin early returns, ensuring --audit-log-path is honored for both paths. Reuse the existing configuration loading and init_audit_logging flow where possible, or explicitly reject incompatible flag combinations if initialization cannot occur before dispatch.crates/mesh-llm-host-runtime/src/mesh/owner_control/mod.rs (1)
761-768: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winKeep synchronous persistence outside the configuration-state lock.
apply_with_live_loggingperforms only short in-memory logging updates, butapplywrites the configuration and revision sidecar whileconfig_stateremains locked. These filesystem operations can stall configuration readers; narrow the lock scope.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/mesh-llm-host-runtime/src/mesh/owner_control/mod.rs` around lines 761 - 768, Update the closure around config_state in the apply flow so apply_with_live_logging runs while holding the lock only for its in-memory state update, then release the lock before performing synchronous persistence of the configuration and revision sidecar. Preserve the returned result, revision, and config hash by reacquiring the state as needed, and use the existing apply-related symbols without changing unrelated behavior.crates/mesh-llm-host-runtime/src/lib.rs (1)
137-144: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDo not make plugin startup depend on host config loading.
When
options.pluginis set,run_runtime_clistartsplugin::run_plugin_processbefore loading host config. This?can now prevent plugin startup when the host config is malformed or unreadable. Bypass config loading for plugin mode, or handle the error fail-open and skip logging initialization.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/mesh-llm-host-runtime/src/lib.rs` around lines 137 - 144, Update initialize_host_runtime_for_options so plugin mode does not depend on plugin::load_config succeeding: detect options.plugin before loading host configuration, and continue plugin startup without logging initialization when that config is malformed or unreadable. Preserve the existing config loading and logging initialization behavior for non-plugin runtimes.
🟡 Minor comments (21)
crates/mesh-llm-tui/src/output/EVENTS.md-8-8 (1)
8-8: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winCorrect the invitation-token privacy statement.
Line 8 says that the dashboard does not retain or display the raw token. Lines 17 and 32 document the opposite behavior.
DashboardState::apply_output_event()also retains the token for the join-token panel.State that the TUI removes token copy controls instead.
Proposed fix
-- Pretty mode consumes the same events to update the dashboard, event history, endpoint cards, model progress, and process rows. Invitation readiness is represented without retaining or displaying a raw token. +- Pretty mode consumes the same events to update the dashboard, event history, endpoint cards, model progress, and process rows. Invitation readiness is represented without a token copy control.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/mesh-llm-tui/src/output/EVENTS.md` at line 8, Update the invitation-readiness statement in EVENTS.md to remove the inaccurate claim that the TUI does not retain or display the raw token; state instead that the TUI removes token copy controls, consistent with DashboardState::apply_output_event() and the behavior documented elsewhere.crates/mesh-llm-ui/src/features/app-shell/components/AppHeader.tsx-164-166 (1)
164-166: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHide the local-endpoint instruction for private meshes.
When
isPublicMeshis false andapiDirectUrlis empty, the next message still tells the user thathttp://127.0.0.1:9337/v1is available locally. The private-mesh branch does not establish that endpoint.Render the local-endpoint instruction only for the public-mesh branch, or replace it with private connection guidance.
Proposed fix
- <div className="text-xs text-muted-foreground"> - This gives you <code className="text-[0.7rem]">http://127.0.0.1:9337/v1</code> locally — point any - OpenAI-compatible app at it. - </div> + {isPublicMesh ? ( + <div className="text-xs text-muted-foreground"> + This gives you <code className="text-[0.7rem]">http://127.0.0.1:9337/v1</code> locally — point any + OpenAI-compatible app at it. + </div> + ) : null}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/mesh-llm-ui/src/features/app-shell/components/AppHeader.tsx` around lines 164 - 166, Update the conditional message in the AppHeader render to avoid presenting a local endpoint for private meshes. When isPublicMesh is false and apiDirectUrl is empty, render only trusted operator-channel connection guidance or no local-endpoint instruction; retain the existing public-mesh guidance for isPublicMesh true.crates/mesh-llm-config/src/validate.rs-965-982 (1)
965-982: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse valid configuration keys in the regression fixture.
Neither
runtime.bind_portnorowner_control.bind_addrbelongs to the corresponding configuration struct. Unknown nested keys are discarded during deserialization. Use valid fields, such asruntime.listen_allandowner_control.bind = "0.0.0.0:9337", so the assertion covers a realistic v1 configuration.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/mesh-llm-config/src/validate.rs` around lines 965 - 982, Update the regression fixture used with validate_config_diagnostics to use valid MeshConfig fields: replace runtime.bind_port with runtime.listen_all and owner_control.bind_addr with owner_control.bind set to "0.0.0.0:9337". Keep the deserialization and zero-diagnostics assertions unchanged.crates/mesh-llm-log-store/src/tests.rs-955-991 (1)
955-991: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe comments misdescribe these assertions, and the stated constraint is not exercised.
Lines 956-989 insert
aud-3,aud-5, andaud-4, all withSome("req-1"). The comment at Line 967 says "different request + different entry should work", but the call passes the samerequest_id. The three inserts differ only byentry_id, so they test one behavior three times. No insert uses a secondrequest_id, soUNIQUE(request_id, entry_id)is never exercised across distinct requests.Insert a second summary and use its
request_idfor one of the entries, then correct the comments.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/mesh-llm-log-store/src/tests.rs` around lines 955 - 991, Update the audit-entry test around insert_audit_entry to create a second summary with a distinct request_id, use that request_id for one entry such as aud-5, and correct the nearby comments to accurately describe same-request/different-entry and different-request/different-entry cases. Preserve the expected count and existing unique-entry behavior.crates/mesh-llm-log-store/src/repositories/cleanup.rs-73-91 (1)
73-91: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe queued count under-reports deleted artifact pointers.
queue_artifact_deletionsreturns the row count fromINSERT OR IGNORE, and the caller adds it tototal.INSERT OR IGNOREskips rows whose(artifact_id, request_id)pair is already present inpending_artifact_deletions, which happens whenever a previous cleanup run queued a pointer that the artifact worker has not yet acknowledged. The followingDELETE FROM artifact_pointersstill removes those rows. The returnedtotaltherefore drifts below the real number of deleted rows, andinsert_cleanup_runpersists that inaccurate figure.Return the
DELETErow count instead.🐛 Proposed fix
) -> Result<i64, LogStoreError> { - let queued = transaction + transaction .execute( "INSERT OR IGNORE INTO pending_artifact_deletions (artifact_id, request_id) \ SELECT artifact_id, request_id FROM artifact_pointers WHERE occurred_at < ?", rusqlite::params![cutoff], ) - .map_err(LogStoreError::Sqlite)? as i64; - transaction + .map_err(LogStoreError::Sqlite)?; + let deleted = transaction .execute( "DELETE FROM artifact_pointers WHERE occurred_at < ?", rusqlite::params![cutoff], ) - .map_err(LogStoreError::Sqlite)?; - Ok(queued) + .map_err(LogStoreError::Sqlite)? as i64; + Ok(deleted) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/mesh-llm-log-store/src/repositories/cleanup.rs` around lines 73 - 91, Update queue_artifact_deletions to return the row count from the DELETE FROM artifact_pointers execution rather than the INSERT OR IGNORE count. Preserve both SQL operations and their error handling, while ensuring the returned value reflects every deleted pointer so the caller’s total and insert_cleanup_run remain accurate.crates/mesh-llm-events/src/audit/sanitization.rs-213-236 (1)
213-236: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThis test fails on Windows.
Line 215 uses
std::env::var("HOME").expect(...). Windows normally setsUSERPROFILEand notHOME, so the test panics there. The stack adds Windows CI coverage for this workspace, so the failure is reachable.Read whichever variable is present, or gate the test to Unix.
💚 Proposed fix
- let home = std::env::var("HOME").expect("HOME should be available to the test"); + let home = ["HOME", "USERPROFILE"] + .into_iter() + .find_map(|variable| std::env::var(variable).ok()) + .filter(|home| !home.is_empty()) + .expect("HOME or USERPROFILE should be available to the test");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/mesh-llm-events/src/audit/sanitization.rs` around lines 213 - 236, Update the test metadata_strings_are_recursively_secret_path_and_length_safe to obtain the home directory from an environment variable available across platforms, preferring HOME and falling back to USERPROFILE before asserting. Preserve the existing redaction and truncation assertions.crates/mesh-llm-events/src/audit.rs-336-363 (1)
336-363: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winA failed reopen after rotation leaves a stale file handle.
If
open_private_append_fileat Line 359 returns an error, the rename chain has already run.state.filestill refers to the old inode, which is now the.1rotated file, andstate.sizekeeps its pre-rotation value. The error propagates once, but the nextemit_auditcall writes through the stale descriptor into the rotated file and retries rotation. Those events are then removed by a later rotation, so audit records are lost without a signal.Record the failure in the state and refuse further writes until a reopen succeeds.
🛡️ Proposed fix sketch
- state.file = open_private_append_file(&self.config.path)?; - state.size = 0; + match open_private_append_file(&self.config.path) { + Ok(file) => { + state.file = file; + state.size = 0; + } + Err(error) => { + state.reopen_failed = true; + return Err(error); + } + }Add
reopen_failed: booltoAuditFileState, and return an error early fromemit_auditwhile the flag is set. Retry the reopen on the next emit and clear the flag when it succeeds.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/mesh-llm-events/src/audit.rs` around lines 336 - 363, Update AuditFileState, rotate_if_needed, and emit_audit to track reopen_failed after rotation. When open_private_append_file fails, set the flag and prevent subsequent writes through the stale state.file; on a later emit, retry reopening the active audit path, clear the flag and reset state.size only after success, while preserving the error propagation on failed attempts.crates/mesh-llm-log-store/src/tests.rs-665-682 (1)
665-682: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAssert the specific error variant.
foreign_keys_enforcedchecks onlyresult.is_err(). The test passes even if the insert fails for an unrelated reason, such as a malformed payload or a missing column, so it would not detectPRAGMA foreign_keysbeing turned off if some other error appeared first. Other tests in this file match a variant, for exampleLogStoreError::AlreadyExists.💚 Proposed fix
- assert!( - result.is_err(), - "orphan insert should fail — foreign_keys=ON" - ); + let error = result.expect_err("orphan insert should fail — foreign_keys=ON"); + assert!( + matches!(error, LogStoreError::Sqlite(_)), + "expected a SQLite constraint error, got: {error:?}" + );Match the variant that the store actually maps a foreign-key violation to.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/mesh-llm-log-store/src/tests.rs` around lines 665 - 682, Update the foreign_keys_enforced test to match the specific LogStoreError variant returned for SQLite foreign-key violations instead of only asserting result.is_err(). Reuse the existing error-matching style from nearby tests, while preserving the orphan insert setup and ensuring unrelated errors fail the test.crates/mesh-llm-events/src/audit/sanitization.rs-57-59 (1)
57-59: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winThe invite-token exception also bypasses length bounding.
Line 57 returns before
redact_audit_textruns. An invite-token string therefore skips theMAX_AUDIT_TEXT_LENtruncation and the home-path normalization. The module header states that this code bounds untrusted audit metadata before emission, so an oversized value written under an*_invite_tokenkey defeats that bound and inflates the rotating audit file.Keep the exception from the secret-marker redaction, but still bound the length.
🛡️ Proposed fix
- if matches!(value, Value::String(_)) && key.is_some_and(is_invite_token_key) { - return; - } + if let Value::String(token) = value + && key.is_some_and(is_invite_token_key) + { + if token.chars().count() > MAX_AUDIT_TEXT_LEN { + let prefix: String = token.chars().take(MAX_AUDIT_TEXT_LEN).collect(); + *token = format!("{prefix}... [TRUNCATED]"); + } + return; + }Add a regression test for an oversized invite-token value.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/mesh-llm-events/src/audit/sanitization.rs` around lines 57 - 59, Update the sanitization flow around the invite-token check so invite-token strings bypass only secret-marker redaction while still passing through redact_audit_text for length bounding and home-path normalization. Add a regression test covering an oversized invite-token value and verify the emitted result is truncated to MAX_AUDIT_TEXT_LEN.crates/mesh-llm-events/src/logging/artifacts.rs-30-97 (1)
30-97: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove unapproved
dead_codesuppressions.The change suppresses warnings for staged public API instead of documenting an approved exception or keeping the API warning-clean.
crates/mesh-llm-events/src/logging/artifacts.rs#L30-L97: remove the#[allow(dead_code)]attributes, or document an approved and narrow exception.crates/mesh-llm-events/src/logging/identifiers.rs#L28-L28: remove the#[allow(dead_code)]attribute, or document an approved and narrow exception.crates/mesh-llm-events/src/logging/replay.rs#L26-L26: remove the#[allow(dead_code)]attribute, or document an approved and narrow exception.As per coding guidelines, do not use
#[allow(...)]without a clear reason and developer approval.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/mesh-llm-events/src/logging/artifacts.rs` around lines 30 - 97, Remove the unapproved #[allow(dead_code)] attributes from the artifact metadata fields and methods in crates/mesh-llm-events/src/logging/artifacts.rs:30-97, the affected declaration in crates/mesh-llm-events/src/logging/identifiers.rs:28-28, and the affected declaration in crates/mesh-llm-events/src/logging/replay.rs:26-26. Only retain an attribute if you add a clear, narrow, developer-approved exception.Source: Coding guidelines
crates/mesh-llm-events/src/logging/artifacts.rs-136-145 (1)
136-145: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDeserialize the fixture in this test.
Line 138 creates a JSON fixture with omitted boolean fields. The test then creates a new value instead of deserializing that fixture. This does not verify the
#[serde(default)]compatibility contract.Proposed fix
- let _json = r#"{"artifact_id":"00000000-0000-4000-a000-00000000001","kind":"chunk","bytes":128,"checksum":"","version":1}"#; - // This would fail if the artifact_id format is invalid UUID. Let's test with a valid one. - let meta = ArtifactMetadata::new(ArtifactKind::Chunk, 128); + let json = r#"{"artifact_id":"00000000-0000-4000-a000-000000000001","kind":"chunk","bytes":128,"checksum":"","version":1}"#; + let meta: ArtifactMetadata = serde_json::from_str(json).unwrap();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/mesh-llm-events/src/logging/artifacts.rs` around lines 136 - 145, Update test_defaults_for_boolean_flags to deserialize the minimal JSON fixture into ArtifactMetadata instead of constructing it with ArtifactMetadata::new. Keep the fixture’s omitted boolean fields and assert that deserialization populates redacted, truncated, missing, and corrupt as false.crates/mesh-llm-log-store/src/store.rs-19-24 (1)
19-24: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winUse sub-second precision in
mesh_llm_log_store::SystemClock.
LogStoreSink::persist_audit_entryand runtime health-audit paths use this clock. Second-level timestamps make audit ordering depend on ID tie-breakers. Use one fixed-width fractional format before persisting rows. Lifecycle timestamps andRequestRegistryalready use the host runtime clock with millisecond precision.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/mesh-llm-log-store/src/store.rs` around lines 19 - 24, Update SystemClock::now to format UTC timestamps with fixed-width millisecond precision before persisting them, matching the host runtime clock’s precision and preserving the existing timestamp structure.crates/mesh-llm-host-runtime/src/logging/writer.rs-9-14 (1)
9-14: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
recursion_blocksconflates real recursion with cross-thread contention.
in_error_pathis one process-wideAtomicBool, not a thread-local. Lines 10-13 document that choice, andconcurrent_error_path_is_suppressed_and_restored(line 228) encodes it. The suppression behaviour is sound for a fail-open path.The accounting is not. When thread A holds the error path and thread B calls
try_record_errorfor an unrelated request, line 84 incrementsrecursion_blocks. An operator reading that counter cannot distinguish two very different conditions:
- A self-logging loop was prevented. That indicates a defect and needs investigation.
- Two independent audits raced and one was discarded. That indicates load, and the discarded audit is lost data.
service.rscompounds this:record_fallback_suppressed(line 107) increments the same counter for a third condition, a fallback whose own persistence failed. Three causes share one number.Split the counter so each cause is separately observable.
♻️ Proposed split
pub struct FailOpenWriter { recursion_guard: Arc<RecursionGuard>, pub write_drops: Arc<AtomicU64>, - /// Number of times the error-fallback path was blocked by recursion detection. + /// Number of times the error-fallback path was blocked because another + /// fallback was already in flight. This covers both true recursion on one + /// thread and contention between threads. pub recursion_blocks: Arc<AtomicU64>, + + /// Number of times a fallback audit's own persistence failure was + /// deliberately not re-reported. + pub fallback_suppressions: Arc<AtomicU64>, }pub fn record_fallback_suppressed(&self) { - self.recursion_blocks.fetch_add(1, Ordering::Relaxed); + self.fallback_suppressions.fetch_add(1, Ordering::Relaxed); }
delivery_shutdown.rsline 30 andservice_tests.rsline 1706 assert onrecursion_blocksand need updating to the correct counter.Also applies to: 83-86
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/mesh-llm-host-runtime/src/logging/writer.rs` around lines 9 - 14, Split the shared recursion_blocks accounting into distinct counters for self-recursion prevention, cross-thread error-path contention, and fallback persistence failure. Update try_record_error and record_fallback_suppressed to increment their respective counters, expose the new counters through the existing reporting interface, and update delivery_shutdown.rs and service_tests.rs assertions to use the appropriate counter while preserving the current suppression behavior.crates/mesh-llm-log-store/src/artifacts.rs-449-455 (1)
449-455: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAlign the "never written" predicate between
read_artifactandstatus.
read_artifactrequires all three ofmedia_kind.is_none(),checksum.is_none(), andbytes == 0to reportArtifactMissing.statusrequires onlychecksum.is_none()andbytes == 0. A pointer row that has amedia_kindbut no checksum and zero bytes therefore reportsArtifactStatus::Missingfromstatus, whileread_artifactcontinues to the filesystem and can returnOk. Operators comparing a status check against a read see contradictory results for the same artifact.Extract one predicate and call it from both paths.
♻️ Proposed shared predicate
// near the other helpers fn pointer_has_no_stored_content(row: &crate::repositories::ArtifactPointerRow) -> bool { row.checksum.is_none() && row.bytes == 0 }- if row.media_kind.is_none() && row.checksum.is_none() && row.bytes == 0 { + if pointer_has_no_stored_content(&row) {Also applies to: 570-573
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/mesh-llm-log-store/src/artifacts.rs` around lines 449 - 455, Extract a shared `pointer_has_no_stored_content` predicate near the existing helpers that checks for a missing checksum and zero bytes, then use it in both `read_artifact` and `status`. Remove the additional `media_kind.is_none()` requirement from the `read_artifact` missing-artifact check so both paths consistently classify such pointers as `ArtifactMissing`/`Missing`.crates/mesh-llm-host-runtime/src/logging/writer.rs-78-96 (1)
78-96: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe documented meaning of the return value does not match the behaviour.
Line 78 states that
try_record_errorreturns "falseif blocked by recursion guard (caller should proceed silently)". Line 96 returnsresult.is_ok(), so it also returnsfalsewhen the recorder panics.try_record_error_catches_panic(line 167) confirms this.The two cases need different operator responses. A recursion block is expected under load. A recorder panic is a defect.
LoggingService::write_error_audit(service.rs line 907) forwards this boolean to its caller as "was written", so both cases report identically.Correct the doc comment, or return an enum that names the three outcomes.
♻️ Minimal doc correction
- /// Attempt to record an error/audit entry. Returns `true` if the fallback path was entered successfully, `false` if blocked by recursion guard (caller should proceed silently). This method is designed to never panic — it absorbs all internal failures. + /// Attempt to record an error/audit entry. Returns `true` when the + /// fallback path was entered and the recorder completed. Returns `false` + /// in two cases: the recursion guard blocked entry, or the recorder + /// panicked and the panic was absorbed. The caller proceeds silently in + /// both cases. This method never panics.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/mesh-llm-host-runtime/src/logging/writer.rs` around lines 78 - 96, Update the documentation for try_record_error to accurately state that it returns false both when recursion blocks entry and when the recorder panics, while true indicates successful recorder completion. Keep the existing boolean behavior and implementation unchanged, including the silent handling of both failure cases.crates/mesh-llm-log-store/src/repositories.rs-166-182 (1)
166-182: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
row.get(N).ok()hides column type errors asNone.
rusqlitemaps a SQLNULLintoOption<T>directly, sorow.get(3)?already yieldsNonefor a nullterminal_at. Using.ok()additionally swallowsInvalidColumnTypeandFromSqlConversionFailure. If a migration changesstatus_codefromINTEGERtoTEXT,get_summaryreturnsstatus_code: Nonefor every row instead of failing, and callers report a summary with no status rather than an error.Replace
.ok()with?on theOption<T>columns. The same pattern appears inlist_summaries(lines 204-213) and in the threeArtifactPointerRowmappers (lines 631-632, 667-668, 740-741).🐛 Proposed fix
Ok(SummaryRow { request_id: row.get(0)?, state: row.get(1)?, created_at: row.get(2)?, - terminal_at: row.get(3).ok(), - route: row.get(4).ok(), - model: row.get(5).ok(), - provider: row.get(6).ok(), - engine: row.get(7).ok(), - status_code: row.get(8).ok(), - error_msg: row.get(9).ok(), - tenant_id: row.get(10).ok(), - account_id: row.get(11).ok(), - user_id: row.get(12).ok(), + terminal_at: row.get(3)?, + route: row.get(4)?, + model: row.get(5)?, + provider: row.get(6)?, + engine: row.get(7)?, + status_code: row.get(8)?, + error_msg: row.get(9)?, + tenant_id: row.get(10)?, + account_id: row.get(11)?, + user_id: row.get(12)?, })Also applies to: 625-638
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/mesh-llm-log-store/src/repositories.rs` around lines 166 - 182, Replace .ok() with ? for nullable column mappings in the SummaryRow mapper used by get_summary, preserving Option<T> handling for SQL NULL while propagating conversion errors. Apply the same change in list_summaries and all three ArtifactPointerRow mappers, including the mappings near their corresponding query results.crates/mesh-llm-host-runtime/src/logging/service_tests.rs-1011-1030 (1)
1011-1030: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe channel-independence assertion cannot fail.
Line 1020 reads
other_current <= 101 || other_ch == ch. The enclosingif other_ch != chon line 1017 already guarantees the second operand isfalse, so it contributes nothing. The first operand compares against the magic bound101, which no channel can exceed in this test: each channel receives at most 100 explicit events plus the one admitted event.The assertion therefore holds for any implementation, including one where all three channels share a single counter. The stated intent — "other channels weren't affected by events on this channel" — is not checked.
Capture each channel's sequence before the loop and assert it is unchanged after.
💚 Proposed fix
- // Verify other channels weren't affected by events on this channel. - for other_ch in [ - ReplayChannel::Requests, - ReplayChannel::Operations, - ReplayChannel::System, - ] { - if other_ch != ch { - let other_current = svc.sequences_ref().current(other_ch); - assert!( - other_current <= 101 || other_ch == ch, - "channel {:?} should not have advanced beyond its own events (got {})", - other_ch, - other_current - ); - } - } + // Verify other channels were not advanced by events on this channel. + for (other_ch, before) in &untouched { + assert_eq!( + svc.sequences_ref().current(*other_ch), + *before, + "channel {other_ch:?} must not advance from {ch:?} events" + ); + }Build
untouchedimmediately before the 100-event loop, holding the current value of each channel other thanch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/mesh-llm-host-runtime/src/logging/service_tests.rs` around lines 1011 - 1030, Update the test around the per-channel event loop to capture each non-target channel’s sequence value immediately before emitting the 100 events, storing these values in an `untouched` collection keyed by channel. Replace the ineffective `other_current <= 101 || other_ch == ch` assertion in the verification loop with a comparison against the corresponding pre-loop value, ensuring every channel other than `ch` is unchanged.crates/mesh-llm-host-runtime/src/logging/service_tests.rs-332-337 (1)
332-337: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
TestClockrepeats timestamps after 60 calls.
nowcomputes(n % 60)and formats it as the seconds field, so call 60 returns the same string as call 0. Several tests advance the clock far past that bound:test_monotonic_channel_sequencesperforms 300 enqueues andtest_queue_never_exceeds_capacityperforms 10,000, and every enqueue callsclock.now().Duplicate
occurred_atvalues matter because the log store orders and paginates on(occurred_at, id).durable_sink_reopens_one_summary_with_ordered_retry_eventsreads back rows withORDER BY occurred_at ASC, event_id ASC(line 760) and asserts an exact event sequence. That test stays under 60 ticks today, so it passes. Any added event or added setup call pushes it over and makes the ordering depend on random UUID comparison.Format the full counter instead of wrapping it.
🐛 Proposed fix
impl Clock for TestClock { fn now(&self) -> String { let n = self.counter.fetch_add(1, AtomicOrdering::Relaxed); - format!("2025-01-01T00:00:{:02}Z", (n % 60) as u32) + // Never wrap: the log store orders and paginates on occurred_at, so a + // repeated timestamp makes read-back ordering nondeterministic. + format!("2025-01-01T00:00:00.{:09}Z", n) } }
test_manual_pump_persists_exact_entry_without_consuming_replayasserts the literal"2025-01-01T00:00:00Z"on line 1564 and needs updating with this change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/mesh-llm-host-runtime/src/logging/service_tests.rs` around lines 332 - 337, Update TestClock::now to format the full atomic counter value instead of applying modulo 60, preserving the existing timestamp prefix and zero-padded seconds format. Also update test_manual_pump_persists_exact_entry_without_consuming_replay to expect the resulting timestamp representation.crates/mesh-llm-host-runtime/src/logging/service.rs-428-437 (1)
428-437: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winA manual queue with capacity 0 counts a drop before anything is queued.
spawnclamps the worker channel withself.config.queue_capacity.max(1)on line 655, because Tokio rejects a zero-capacity channel.LoggingService::new(line 517) andnew_disabled(line 567) do not apply the same clamp toDeliveryMode::Manual { capacity }.With
queue_capacity: 0, the firstoffer_persistence_tocall evaluatespending.len() >= *capacityas0 >= 0, which is true.pending.pop_front()then returnsNonebecause the queue is empty, yet line 432 still incrementspersistence_queue_drops. One drop is recorded although no entry was discarded. The queue then holds exactly one entry rather than zero.
test_zero_capacity_service_can_spawn_without_panicking(service_tests.rs line 1444) covers the spawn path only, so this manual path is untested.Apply the same clamp for consistency.
🐛 Proposed fix
let delivery = Arc::new(Mutex::new(DeliveryMode::Manual { pending: VecDeque::new(), - capacity: config.queue_capacity, + // Match the worker channel clamp in `spawn`, so manual and worker + // ownership account for drops identically. + capacity: config.queue_capacity.max(1), }));Apply the same change in
new_disabled(line 567) and in thepump_syncrestore at line 980.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/mesh-llm-host-runtime/src/logging/service.rs` around lines 428 - 437, Clamp the manual delivery capacity to at least 1 wherever it is initialized or restored, including LoggingService::new, new_disabled, and pump_sync. Use the same queue_capacity.max(1) behavior as spawn so zero-capacity services neither count a drop for an empty queue nor retain more entries than the effective capacity.crates/mesh-llm-host-runtime/src/logging/service_tests/delivery_shutdown.rs-63-109 (1)
63-109: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winA regression in this test hangs the suite instead of failing it.
The hook at lines 75-82 performs a blocking
recv()on astd::sync::mpscreceiver. The test asserts on line 103 that the write task has not finished, which proves the SQLite call ran on the blocking pool.If a future change moves that call back onto the async task, the hook's blocking
recv()runs on thecurrent_threadexecutor. That executor is then parked and can never poll thespawn_blockingjoin future on line 98, sostarted_rx.recv()never returns to the test. The test hangs until the CI job timeout rather than reporting the regression.The condition the test guards against is exactly the condition that produces the hang. Bound the wait.
💚 Proposed bounded wait
- tokio::task::spawn_blocking(move || started_rx.recv()) - .await - .expect("start observer joins") - .expect("blocking worker started"); + tokio::task::spawn_blocking(move || { + started_rx.recv_timeout(Duration::from_secs(5)) + }) + .await + .expect("start observer joins") + .expect("the SQLite operation must reach the blocking pool");
release_tx.send(())on line 107 then still unblocks the hook on the success path, and a regression reports a failed expectation within five seconds.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/mesh-llm-host-runtime/src/logging/service_tests/delivery_shutdown.rs` around lines 63 - 109, Bound the `started_rx.recv()` wait in `log_store_sqlite_busy_seam_never_blocks_the_shared_tokio_executor` to five seconds so a regression cannot hang the current-thread executor indefinitely. Preserve the existing `spawn_blocking` observation and success-path `release_tx.send(())` behavior, while making timeout failure report through the test expectation.crates/mesh-llm-log-store/src/artifacts_tests.rs-641-696 (1)
641-696: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winGate the whole test with
#[cfg(unix)].On Windows,
store,target_dir, andlink_pathare created but never used, because every use sits inside#[cfg(unix)]blocks. This produces unused-variable warnings in the Windows test build. The neighbouring symlink tests already gate at the test level.♻️ Proposed change
+#[cfg(unix)] #[test] fn artifact_symlink_rejected() { + use std::os::unix::fs::symlink; + let tmp = tempfile::tempdir().expect("create temp dir");Then remove the inner
#[cfg(unix)]attributes and thestd::os::unix::fs::path prefix.As per coding guidelines: "Do not leave warnings, unused code, or dead code introduced by a change."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/mesh-llm-log-store/src/artifacts_tests.rs` around lines 641 - 696, Gate the entire artifact_symlink_rejected test with #[cfg(unix)] so its Unix-only setup and assertions are excluded on Windows. Remove the inner #[cfg(unix)] blocks and use the platform-appropriate symlink call without the Unix-specific path prefix.Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 61518d01-0884-4460-a539-681bb8cde0e4
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (97)
.agents/skills/manage-ci/references/current-inventory.md.github/workflows/pr_builds.ymlCargo.tomlci/ci.mdcrates/mesh-llm-cli/src/parser/commands.rscrates/mesh-llm-config/src/lib.rscrates/mesh-llm-config/src/model.rscrates/mesh-llm-config/src/model/built_in_schema.rscrates/mesh-llm-config/src/validate.rscrates/mesh-llm-events/Cargo.tomlcrates/mesh-llm-events/src/audit.rscrates/mesh-llm-events/src/audit/sanitization.rscrates/mesh-llm-events/src/lib.rscrates/mesh-llm-events/src/logging/artifacts.rscrates/mesh-llm-events/src/logging/envelope.rscrates/mesh-llm-events/src/logging/events.rscrates/mesh-llm-events/src/logging/identifiers.rscrates/mesh-llm-events/src/logging/lifecycle.rscrates/mesh-llm-events/src/logging/mod.rscrates/mesh-llm-events/src/logging/proxy.rscrates/mesh-llm-events/src/logging/replay.rscrates/mesh-llm-events/src/logging/summaries.rscrates/mesh-llm-events/src/logging/tests.rscrates/mesh-llm-host-runtime/Cargo.tomlcrates/mesh-llm-host-runtime/src/config_schema.rscrates/mesh-llm-host-runtime/src/lib.rscrates/mesh-llm-host-runtime/src/logging/bus.rscrates/mesh-llm-host-runtime/src/logging/foundation.rscrates/mesh-llm-host-runtime/src/logging/lifecycle.rscrates/mesh-llm-host-runtime/src/logging/limits.rscrates/mesh-llm-host-runtime/src/logging/mod.rscrates/mesh-llm-host-runtime/src/logging/persistence.rscrates/mesh-llm-host-runtime/src/logging/policy.rscrates/mesh-llm-host-runtime/src/logging/registry.rscrates/mesh-llm-host-runtime/src/logging/runtime_state.rscrates/mesh-llm-host-runtime/src/logging/sequences.rscrates/mesh-llm-host-runtime/src/logging/service.rscrates/mesh-llm-host-runtime/src/logging/service_tests.rscrates/mesh-llm-host-runtime/src/logging/service_tests/delivery_shutdown.rscrates/mesh-llm-host-runtime/src/logging/writer.rscrates/mesh-llm-host-runtime/src/mesh/owner_control/mod.rscrates/mesh-llm-host-runtime/src/mesh/owner_control_response.rscrates/mesh-llm-host-runtime/src/mesh/plugin_config.rscrates/mesh-llm-host-runtime/src/protocol/config_tests.rscrates/mesh-llm-host-runtime/src/protocol/convert.rscrates/mesh-llm-host-runtime/src/protocol/tests/config.rscrates/mesh-llm-host-runtime/src/runtime/config_state.rscrates/mesh-llm-host-runtime/src/runtime/options.rscrates/mesh-llm-host-runtime/src/runtime/run_auto.rscrates/mesh-llm-host-runtime/src/runtime/tests/startup_models.rscrates/mesh-llm-host-runtime/src/runtime/tracing_writer.rscrates/mesh-llm-host-runtime/src/sdk.rscrates/mesh-llm-host-runtime/src/sdk/embedded_logging.rscrates/mesh-llm-host-runtime/tests/audit_test.rscrates/mesh-llm-log-store/Cargo.tomlcrates/mesh-llm-log-store/README.mdcrates/mesh-llm-log-store/src/artifact_privacy.rscrates/mesh-llm-log-store/src/artifacts.rscrates/mesh-llm-log-store/src/artifacts_tests.rscrates/mesh-llm-log-store/src/capture.rscrates/mesh-llm-log-store/src/capture_tests.rscrates/mesh-llm-log-store/src/cursor.rscrates/mesh-llm-log-store/src/error.rscrates/mesh-llm-log-store/src/lib.rscrates/mesh-llm-log-store/src/migrations.rscrates/mesh-llm-log-store/src/repositories.rscrates/mesh-llm-log-store/src/repositories/cleanup.rscrates/mesh-llm-log-store/src/store.rscrates/mesh-llm-log-store/src/tests.rscrates/mesh-llm-tui/src/output/EVENTS.mdcrates/mesh-llm-tui/src/output/dashboard.rscrates/mesh-llm-tui/src/output/formatting.rscrates/mesh-llm-tui/src/output/mod.rscrates/mesh-llm-tui/src/output/rendering/join_token.rscrates/mesh-llm-tui/src/output/rendering/layout.rscrates/mesh-llm-tui/src/output/rendering/mod.rscrates/mesh-llm-tui/src/output/rendering/tui.rscrates/mesh-llm-tui/src/output/state.rscrates/mesh-llm-tui/src/output/tests/formatting.rscrates/mesh-llm-tui/src/output/tests/mod.rscrates/mesh-llm-tui/src/output/tests/rendering.rscrates/mesh-llm-ui/e2e/smoke/live-parity.spec.tscrates/mesh-llm-ui/src/App.test.tsxcrates/mesh-llm-ui/src/app/layout/RootLayout.test.tsxcrates/mesh-llm-ui/src/features/app-shell/components/AppHeader.test.tsxcrates/mesh-llm-ui/src/features/app-shell/components/AppHeader.tsxcrates/mesh-llm-ui/src/features/chat/components/ChatPage.tsxcrates/mesh-llm/src/commands/runtime.rscrates/mesh-llm/src/lib.rsscripts/affected-crates.shscripts/console-format.jsscripts/console-format.test.jsscripts/plan-clippy-batches.shscripts/plan-pr-build-jobs.pyscripts/publish-crates.shscripts/tests/test_plan_pr_build_jobs.pyscripts/tests/test_pr_workflow_artifacts.py
💤 Files with no reviewable changes (3)
- crates/mesh-llm-tui/src/output/formatting.rs
- crates/mesh-llm-tui/src/output/mod.rs
- crates/mesh-llm-tui/src/output/state.rs
1a71f5d to
4983851
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
crates/mesh-llm-tui/src/output/tests/rendering.rs (2)
596-605: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert that the copy-control removal remains enforced.
The test checks token retention and rendering. It does not check that the copy control stays absent.
tui_layoutstill exposesjoin_token_copy_button, and the layout sets it to a zero area. Add a zero-area assertion so a later copy-control regression cannot pass this test unnoticed.Suggested assertion
let areas = tui_layout(Rect::new(0, 0, 120, 24), &state); + assert_eq!(areas.join_token_copy_button.width, 0); + assert_eq!(areas.join_token_copy_button.height, 0);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/mesh-llm-tui/src/output/tests/rendering.rs` around lines 596 - 605, Extend the rendering test around tui_layout to assert that areas.join_token_copy_button has zero width and height, preserving the enforced removal of the copy control while keeping the existing token rendering assertions unchanged.
607-618: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRetain constrained-terminal coverage for the token.
The new snapshot uses
120x24, so the token fits without wrapping. The change removes the full-screen wrapping coverage. Becauseinvite_tokenremains displayed, keep a narrow or short-frame test that verifies wrapping and redraw behavior. Small-terminal regressions can otherwise pass.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/mesh-llm-tui/src/output/tests/rendering.rs` around lines 607 - 618, Extend the rendering tests around tui_join_token_text_area to retain constrained-terminal coverage for invite_token: add or restore a narrow or short-frame case that forces token wrapping and verifies the resulting redraw behavior. Keep the existing two-row height assertions and ensure the test exercises the wrapped layout rather than only the 120x24 snapshot dimensions.crates/mesh-llm-host-runtime/src/runtime/config_state.rs (1)
463-502: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe live-logging apply sequence is duplicated in
owner_control.
apply_with_live_loggingandapply_owner_control_config_with_persistenceincrates/mesh-llm-host-runtime/src/mesh/owner_control/mod.rs(lines 152-189) implement the same five steps: prepare,apply_live_logging, persist,restore_live_loggingonPersistError, and theApplied→Liveremap. Two copies of the rollback rule can diverge.Extract the shared sequence into one function that accepts the persist step as a parameter, then call it from both sites.
As per coding guidelines: "Do not add Rust methods or functions exceeding configured Clippy line-count or cognitive-complexity limits; use small, semantically named helpers and clear control-flow phases."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/mesh-llm-host-runtime/src/runtime/config_state.rs` around lines 463 - 502, The live-logging apply sequence is duplicated between ConfigState::apply_with_live_logging and apply_owner_control_config_with_persistence. Extract the shared prepare, live apply, persistence callback, rollback-on-PersistError, finish, and Applied-to-Live remapping flow into a small semantically named helper that accepts the persistence operation as a parameter, then have both methods delegate to it while preserving existing behavior and keeping complexity within Clippy limits.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/mesh-llm-host-runtime/src/runtime/config_state.rs`:
- Around line 431-438: Guard the prepare/commit gap in config_state.rs: in
finish_apply, replace the debug_assert_eq! with a runtime revision check that
returns ApplyResult::RevisionConflict { current_revision: self.revision } when
self.revision + 1 differs from pending.revision. In
crates/mesh-llm-host-runtime/src/mesh/owner_control/mod.rs lines 152-194, verify
every shared config_state writer acquires apply_serialization_lock and route any
bypassing writer through that lock.
In `@crates/mesh-llm-log-store/src/tests.rs`:
- Around line 721-727: Update the constraint-error mapping around
is_unique_constraint_error to detect SQLITE_CONSTRAINT_FOREIGNKEY separately and
return the appropriate distinct LogStoreError variant for foreign-key failures.
Keep primary-key and unique constraint violations mapped to AlreadyExists, and
update the orphan-insert assertion in the test to expect the foreign-key
variant.
---
Nitpick comments:
In `@crates/mesh-llm-host-runtime/src/runtime/config_state.rs`:
- Around line 463-502: The live-logging apply sequence is duplicated between
ConfigState::apply_with_live_logging and
apply_owner_control_config_with_persistence. Extract the shared prepare, live
apply, persistence callback, rollback-on-PersistError, finish, and
Applied-to-Live remapping flow into a small semantically named helper that
accepts the persistence operation as a parameter, then have both methods
delegate to it while preserving existing behavior and keeping complexity within
Clippy limits.
In `@crates/mesh-llm-tui/src/output/tests/rendering.rs`:
- Around line 596-605: Extend the rendering test around tui_layout to assert
that areas.join_token_copy_button has zero width and height, preserving the
enforced removal of the copy control while keeping the existing token rendering
assertions unchanged.
- Around line 607-618: Extend the rendering tests around
tui_join_token_text_area to retain constrained-terminal coverage for
invite_token: add or restore a narrow or short-frame case that forces token
wrapping and verifies the resulting redraw behavior. Keep the existing two-row
height assertions and ensure the test exercises the wrapped layout rather than
only the 120x24 snapshot dimensions.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 15e116f5-54b5-46d6-8df1-4e04cc4eadfe
📒 Files selected for processing (28)
crates/mesh-llm-config/src/validate.rscrates/mesh-llm-events/src/audit.rscrates/mesh-llm-events/src/audit/sanitization.rscrates/mesh-llm-events/src/logging/artifacts.rscrates/mesh-llm-events/src/logging/identifiers.rscrates/mesh-llm-events/src/logging/replay.rscrates/mesh-llm-host-runtime/src/exact_test_wrappers.rscrates/mesh-llm-host-runtime/src/lib.rscrates/mesh-llm-host-runtime/src/logging/service.rscrates/mesh-llm-host-runtime/src/logging/service_tests.rscrates/mesh-llm-host-runtime/src/logging/service_tests/delivery_shutdown.rscrates/mesh-llm-host-runtime/src/logging/writer.rscrates/mesh-llm-host-runtime/src/mesh/owner_control/mod.rscrates/mesh-llm-host-runtime/src/mesh/owner_control_response.rscrates/mesh-llm-host-runtime/src/runtime/config_state.rscrates/mesh-llm-host-runtime/src/runtime/run_auto.rscrates/mesh-llm-host-runtime/src/runtime/tests/startup_models.rscrates/mesh-llm-log-store/src/artifacts.rscrates/mesh-llm-log-store/src/artifacts_tests.rscrates/mesh-llm-log-store/src/repositories.rscrates/mesh-llm-log-store/src/repositories/cleanup.rscrates/mesh-llm-log-store/src/store.rscrates/mesh-llm-log-store/src/tests.rscrates/mesh-llm-tui/src/output/EVENTS.mdcrates/mesh-llm-tui/src/output/rendering/join_token.rscrates/mesh-llm-tui/src/output/tests/rendering.rscrates/mesh-llm-ui/src/features/app-shell/components/AppHeader.test.tsxcrates/mesh-llm-ui/src/features/app-shell/components/AppHeader.tsx
💤 Files with no reviewable changes (2)
- crates/mesh-llm-events/src/logging/identifiers.rs
- crates/mesh-llm-events/src/logging/replay.rs
🚧 Files skipped from review as they are similar to previous changes (15)
- crates/mesh-llm-host-runtime/src/mesh/owner_control_response.rs
- crates/mesh-llm-host-runtime/src/logging/service_tests/delivery_shutdown.rs
- crates/mesh-llm-ui/src/features/app-shell/components/AppHeader.test.tsx
- crates/mesh-llm-log-store/src/store.rs
- crates/mesh-llm-events/src/audit/sanitization.rs
- crates/mesh-llm-tui/src/output/rendering/join_token.rs
- crates/mesh-llm-config/src/validate.rs
- crates/mesh-llm-ui/src/features/app-shell/components/AppHeader.tsx
- crates/mesh-llm-events/src/audit.rs
- crates/mesh-llm-log-store/src/artifacts.rs
- crates/mesh-llm-log-store/src/repositories/cleanup.rs
- crates/mesh-llm-log-store/src/artifacts_tests.rs
- crates/mesh-llm-events/src/logging/artifacts.rs
- crates/mesh-llm-log-store/src/repositories.rs
- crates/mesh-llm-host-runtime/src/logging/service.rs
c8803a1 to
a849461
Compare
c86ec11 to
76d7ca8
Compare
1 participant
Summary
First PR in the logging stack. This branch is based directly on
mainand establishes the durable, privacy-safe logging foundation consumed by the API and console layers.Typed logging contracts
mesh-llm-events.Durable storage and retention
mesh-llm-log-storecrate with SQLite migrations, transactional schema upgrades, future-schema rejection, repositories, cursors, and typed store errors.Runtime logging foundation
Privacy and audit hardening
[logging.audit]nesting is completed by feat(console): add typed logs ledger and maintenance UI #1176 without a deprecated compatibility layer.Repository integration
Validation
mesh-llm-events,mesh-llm-log-store, config, TUI, and host logging tests passed locally, along with formatting and warnings-denied Clippy.Stack
mainGuardrails
This PR does not add operator HTTP endpoints or the logs console. It establishes the contracts, privacy policy, storage, configuration, and runtime primitives those layers use. No mesh wire schema, ALPN label, native ABI, OTLP log API, or generated website artifact is changed.
Summary by CodeRabbit
New Features
Bug Fixes