feat: finish normalized log follow and reconnect - #169
Conversation
|
@codex review |
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughThe change adds cursor-based ChangesLogs and storage
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant User
participant LogsCommand
participant FollowRuntime
participant TransportClient
participant HostLogStorage
User->>LogsCommand: invoke logs --follow
LogsCommand->>FollowRuntime: create follow request
FollowRuntime->>TransportClient: open transport
TransportClient->>HostLogStorage: read entries after cursor
HostLogStorage-->>FollowRuntime: return page and cursor
FollowRuntime-->>User: write log entries
TransportClient--xFollowRuntime: transient transport loss
FollowRuntime->>TransportClient: reconnect and resume cursor
TransportClient->>HostLogStorage: read entries after cursor
HostLogStorage-->>User: stream resumed entries
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 14c9e7c56f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/satelle-host/src/storage.rs (1)
2979-3025: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winExtract the duplicated readiness-summary log block.
begin_session(lines 3002-3025) andbegin_follow_up(lines 3138-3161) contain the same logic: insertNativeReadinessSummarywhencontext.readiness_refisSome, then insertProviderSmokeSummarywhenreadiness.provider_result_id().is_some(). The two blocks differ only insessionvs&session.This is normalized-log-contract logic. If one admission path changes and the other does not, the two paths silently diverge in what gets logged for the same readiness data. Extract a shared helper, for example
insert_readiness_summary_logs(&transaction, &context.readiness_ref, &session, &turn_id)?, and call it from bothbegin_sessionandbegin_follow_up.♻️ Proposed refactor sketch
+fn insert_readiness_summary_logs( + transaction: &rusqlite::Transaction<'_>, + readiness_ref: &Option<AdmissionReadinessRef>, + session: &Session, + turn_id: &TurnId, +) -> Result<(), StorageError> { + let Some(readiness) = readiness_ref else { + return Ok(()); + }; + insert_safe_log( + transaction, + &canonical_log( + LogEvent::NativeReadinessSummary, + LogSeverity::Info, + session, + turn_id, + session.updated_at(), + )?, + )?; + if readiness.provider_result_id().is_some() { + insert_safe_log( + transaction, + &canonical_log( + LogEvent::ProviderSmokeSummary, + LogSeverity::Info, + session, + turn_id, + session.updated_at(), + )?, + )?; + } + Ok(()) +}Then replace both inline blocks with
insert_readiness_summary_logs(&transaction, &context.readiness_ref, &session, &turn_id)?;.Also applies to: 3138-3161
🤖 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/satelle-host/src/storage.rs` around lines 2979 - 3025, Extract the duplicated readiness logging logic into a shared helper such as insert_readiness_summary_logs, preserving the conditional NativeReadinessSummary and ProviderSmokeSummary behavior based on readiness_ref and provider_result_id(). Replace both inline blocks in begin_session and begin_follow_up with calls to the helper, passing the transaction, readiness reference, session, and turn ID.
🧹 Nitpick comments (3)
crates/satelle-cli/src/logs.rs (3)
124-148: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueQuote every interpolated value, not only the host alias.
shell_argumentprotects--host, butsession, eachsource, andlevelgo in raw. TodayLogReadPlan::resolveruns first and restricts those three values to a parsedSessionIdand closed literal sets, so the emittedrecovery_commandis safe. The mixed quoting is still a hazard: this method reads rawLogReadRequeststrings, not the validated plan, so any future relaxation of those validators silently produces an unquoted recovery command.Proposed change
if let Some(session) = &self.session { command.push_str(" --session "); - command.push_str(session); + command.push_str(&shell_argument(session)); } for source in &self.source { command.push_str(" --source "); - command.push_str(source); + command.push_str(&shell_argument(source)); } if let Some(level) = &self.level { command.push_str(" --level "); - command.push_str(level); + command.push_str(&shell_argument(level)); }🤖 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/satelle-cli/src/logs.rs` around lines 124 - 148, Update LogReadPlan::follow_rerun_command so session, every source, and level are passed through shell_argument before being appended, matching the existing host quoting. Keep the command structure and option ordering unchanged, and quote each interpolated value independently.
865-878: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winThe target-required mapping depends on an internal detail key.
unresolved_log_targetclassifies the failure by readingfailure.error.details["candidate_count"] == 0from the resolver error. That is an untyped cross-module contract. Ifresolve_session_hostrenames the key, changes its type, or stops emitting it, this branch silently stops firing andlogsreturns the genericinvalid-usageerror instead of the typedlogs-target-requirederror that.factsL1594 requires. Nothing in the type system catches that.Expose a typed signal from the resolver, for example a dedicated
ErrorCodeor a small enum returned alongside the failure, and match on it here.🤖 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/satelle-cli/src/logs.rs` around lines 865 - 878, Replace the untyped candidate_count inspection in unresolved_log_target with a typed signal produced by resolve_session_host, such as a dedicated ErrorCode or resolver outcome enum for zero candidate hosts. Update the resolver to emit that signal and have unresolved_log_target match it alongside HostNotFound, preserving the logs-target-required mapping required by the existing behavior.
313-322: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the duplicated page-consumption block.
The
Ok(page)arm and the post-reconnect block repeat the same four steps: write entries, updatelast_delivered, advancequery_cursor, and sleep when the page is not truncated. The two copies must stay in sync or the resumed stream will drift from the normal stream. Extract one helper that consumes a page and returns the new cursor pair.Also applies to: 362-370
🤖 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/satelle-cli/src/logs.rs` around lines 313 - 322, Extract the repeated page-consumption logic from the Ok(page) arm and post-reconnect flow into a shared helper near the existing log-following code. Have the helper write entries, update last_delivered, advance query_cursor, and sleep for non-truncated pages, returning the updated cursor pair; replace both duplicated blocks with calls to this helper so normal and resumed streams remain consistent.
🤖 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 @.spec-gaps.md:
- Line 135: Update the reconnect classification around transient_follow_error so
connection-level RemoteExecution failures, including dropped streams and Host
Daemon restarts, are treated as transient alongside host/daemon reachability and
SSH bootstrap failures. Preserve terminal handling for non-connection-level
RemoteExecution errors and keep the existing reconnect budget, cursor, and
no-reconnect behavior unchanged.
In `@crates/satelle-cli/src/logs.rs`:
- Around line 216-246: Update follow_logs to check ProcessFollowRuntime’s
interrupted flag immediately after plan.emit_follow_initial and between
connection_factory and validate_follow_connection. If Ctrl-C was received,
terminate the follow command through the existing exit-130 path before
continuing blocking work, while preserving the normal polling behavior when the
flag is unset.
---
Outside diff comments:
In `@crates/satelle-host/src/storage.rs`:
- Around line 2979-3025: Extract the duplicated readiness logging logic into a
shared helper such as insert_readiness_summary_logs, preserving the conditional
NativeReadinessSummary and ProviderSmokeSummary behavior based on readiness_ref
and provider_result_id(). Replace both inline blocks in begin_session and
begin_follow_up with calls to the helper, passing the transaction, readiness
reference, session, and turn ID.
---
Nitpick comments:
In `@crates/satelle-cli/src/logs.rs`:
- Around line 124-148: Update LogReadPlan::follow_rerun_command so session,
every source, and level are passed through shell_argument before being appended,
matching the existing host quoting. Keep the command structure and option
ordering unchanged, and quote each interpolated value independently.
- Around line 865-878: Replace the untyped candidate_count inspection in
unresolved_log_target with a typed signal produced by resolve_session_host, such
as a dedicated ErrorCode or resolver outcome enum for zero candidate hosts.
Update the resolver to emit that signal and have unresolved_log_target match it
alongside HostNotFound, preserving the logs-target-required mapping required by
the existing behavior.
- Around line 313-322: Extract the repeated page-consumption logic from the
Ok(page) arm and post-reconnect flow into a shared helper near the existing
log-following code. Have the helper write entries, update last_delivered,
advance query_cursor, and sleep for non-truncated pages, returning the updated
cursor pair; replace both duplicated blocks with calls to this helper so normal
and resumed streams remain consistent.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1d00ab6f-0c30-4bb3-b373-d1bdf7932328
📒 Files selected for processing (38)
.facts.spec-gaps.mdcrates/satelle-cli/src/error-output.rscrates/satelle-cli/src/logs.rscrates/satelle-cli/src/main.rscrates/satelle-cli/src/mcp/arguments.rscrates/satelle-cli/src/ssh-bootstrap.rscrates/satelle-cli/src/tailscale-serve.rscrates/satelle-cli/src/tailscale.rscrates/satelle-cli/src/transport-tests.rscrates/satelle-cli/src/transport.rscrates/satelle-cli/tests/cli.rscrates/satelle-cli/tests/report-schema-contract.rscrates/satelle-cli/tests/session-host-routing.rscrates/satelle-core/src/daemon-service.rscrates/satelle-core/src/lib.rscrates/satelle-core/src/profiles.rscrates/satelle-host/src/daemon-reconnect-tests.rscrates/satelle-host/src/lib-tests.rscrates/satelle-host/src/lib.rscrates/satelle-host/src/log-page.rscrates/satelle-host/src/runtime-tests.rscrates/satelle-host/src/runtime.rscrates/satelle-host/src/storage.rscrates/satelle-host/src/storage/0016_normalized_log_events.sqlcrates/satelle-host/src/storage/codec.rscrates/satelle-host/src/storage/logs.rscrates/satelle-host/src/storage/open.rscrates/satelle-host/src/storage/operator-log.rscrates/satelle-host/src/storage/retention.rscrates/satelle-host/src/storage/sql.rscrates/satelle-host/src/storage/tests/lifecycle.rscrates/satelle-host/src/storage/tests/logs.rscrates/satelle-host/src/storage/tests/operational.rscrates/satelle-host/src/storage/tests/retention.rscrates/satelle-transport/src/server/host_error.rsdocs/reference/configuration.mdxdocs/reference/generated-cli.mdx
|
@codex review |
|
@greptileai review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8eac9d054a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@coderabbitai review |
|
@codex review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ab560546c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de3f958e41
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/satelle-host/src/runtime-codex-tests.rs (1)
336-342: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDo not make the regression assertion boolean-only.
perform_handshakereturnsfalsefor spawn, pipe, and initialize-write failures. This assertion can pass without exercising the escaped-child deadline path. Retain a separateControlPlaneAdmissionassertion that an incomplete handshake remains non-admitting, and add a fixture-start or handshake marker to prove that the intended path ran.As per path instructions, ambiguous timeout and transport outcomes must remain non-admitting and block conflicting admission.
🤖 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/satelle-host/src/runtime-codex-tests.rs` around lines 336 - 342, Update the regression test around perform_handshake so it proves the escaped-child deadline path actually started, using a fixture-start or handshake marker rather than relying only on handshake_completed being false. Keep a separate ControlPlaneAdmission assertion confirming incomplete handshakes remain non-admitting, and preserve non-admission for ambiguous timeout or transport failures.Source: Path instructions
🤖 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/satelle-host/src/runtime-codex-tests.rs`:
- Around line 323-334: Update the hang-with-escaped-descendant-exit test around
perform_handshake to retain a cleanup handle or signal for the spawned fixture
child and ensure it is terminated and awaited before the fixture is dropped.
Apply cleanup on successful completion, handshake error or timeout, and
unwinding paths, while preserving the existing handshake deadline.
---
Nitpick comments:
In `@crates/satelle-host/src/runtime-codex-tests.rs`:
- Around line 336-342: Update the regression test around perform_handshake so it
proves the escaped-child deadline path actually started, using a fixture-start
or handshake marker rather than relying only on handshake_completed being false.
Keep a separate ControlPlaneAdmission assertion confirming incomplete handshakes
remain non-admitting, and preserve non-admission for ambiguous timeout or
transport failures.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5131df7b-0a2f-4fee-a6a8-2a4de64e008c
📒 Files selected for processing (6)
crates/satelle-host/src/log-page.rscrates/satelle-host/src/runtime-codex-tests.rscrates/satelle-host/src/runtime-codex.rscrates/satelle-host/src/storage/logs.rscrates/satelle-host/src/storage/tests/logs.rscrates/satelle-transport/src/client.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/satelle-transport/src/client.rs
- crates/satelle-host/src/storage/logs.rs
|
@coderabbitai review |
|
@codex review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3ac3782000
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
satelle/crates/satelle-cli/src/logs.rs
Lines 784 to 788 in a703989
Paginate finite reads after a cursor
When more than 200 matching entries exist after the supplied cursor, this branch performs only one forward request and ignores page.truncated(), so satelle logs --after ... exits successfully after silently omitting the remaining records. The analogous read branch used by MCP has the same one-page behavior, while --since already demonstrates the required snapshot-bounded pagination; continue requesting from next_cursor until the captured high-water boundary is reached.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
fixed in |
|
@greptileai review |
|
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 823526a75b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .entries() | ||
| .iter() | ||
| .all(|entry| query.matches_entry(entry)); | ||
| let size_is_bound = page.entries().len() <= query.limit(); |
There was a problem hiding this comment.
When an authenticated daemon answers tail(200) with fewer than 200 entries (including zero) while setting truncated: true, this upper-bound-only check accepts the contradiction even though truncation claims older matching entries were omitted. Finite reads then return fewer records than requested, and follow mode advances to the Host-wide high-water cursor and permanently skips the omitted initial matches; require a truncated page to fill query.limit(). Fresh evidence after the forward-only continuation fix is that tail pages now bypass that structural check while size_is_bound still enforces only a maximum.
Useful? React with 👍 / 👎.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
what changed
satelle logstargeting, filtering, follow, reconnect, identity checks, and typed recovery outputwhy
this is source packet 23. it completes the log delivery contract on top of packet 22 retention without replaying live events or introducing a second log authority.
proof
facts lintpassescargo fmt --all -- --check, andgit diff --checkpass823526a75b132ea130e565d478a36ef92697934f65456f55ce7522aa18c2f1f379f0d907948cfa69b1ff9a7d9b6bb6865e494cc4Summary by CodeRabbit
--follow, cursor-based continuation, reconnect handling, and--no-reconnect.Greptile Summary
The PR completes normalized Host-owned log delivery, adding durable structured records and configurable retention together with CLI follow and reconnect behavior.
Confidence Score: 5/5
The PR appears safe to merge because no blocking failure remains within the eligible follow-up review scope.
No blocking failure remains.
Important Files Changed
Sequence Diagram
sequenceDiagram participant User participant CLI as satelle logs --follow participant Host as Host transport participant Store as Host SQLite logs User->>CLI: Start targeted log follow CLI->>Host: Validate Host identity and session CLI->>Host: GET /v1/logs Host->>Store: Query normalized entries Store-->>Host: Entries and high-water cursor Host-->>CLI: Log page CLI-->>User: Human lines or NDJSON loop Follow CLI->>Host: Query strictly after cursor Host->>Store: Read next page Store-->>Host: Entries and next cursor Host-->>CLI: Log page CLI-->>User: Append entries end opt Transient transport loss CLI->>Host: Recreate transport CLI->>Host: Revalidate identity and session CLI->>Host: Resume after durable cursor endReviews (13): Last reviewed commit: "fix(logs): paginate finite cursor reads" | Re-trigger Greptile
Context used (5)