feat: cloud push SDK (drives the ai-hist binary) + installer test fix - #38
Conversation
…obber Two related fixes so local and cloud history sync are both automatic and never fight the developer's real machine state: - Add `ai-hist push --install-service/--uninstall-service/--interval`. The launchd/cron plumbing is generalized behind a `ServiceSpec` shared by the existing `sync` service and the new `push` service (com.ai-hist.push, default 300s). `push --install-service` doesn't require auth so it can be scheduled before first login; the job itself authenticates with the stored rth_at_ token. This is what `agent-relay reflex on` will call to make cloud upload automatic. - Fix test isolation in test_install.py: both installer runs inherited the real $HOME and omitted AI_HIST_NO_AUTOSYNC, so install.sh's auto-sync step overwrote ~/Library/LaunchAgents/com.ai-hist.sync.plist with a pointer to a pytest tmp binary — silently breaking the developer's real sync service (EX_CONFIG, launchd penalty box). Sandbox HOME + set AI_HIST_NO_AUTOSYNC=1 for the install subprocesses, and add a regression test asserting the opt-out writes no plist. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Warning Review limit reached
Next review available in: 52 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a shared managed-service path for sync and push, documents continuous cloud push, and introduces a TypeScript wrapper that resolves and runs ChangesManaged push/sync service
TypeScript cloud push wrapper
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![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 continuous cloud push background service (push) alongside the existing local sync service, refactoring the background service management into a generic ServiceSpec struct to share launchd and cron installation logic. It also updates the documentation and sandboxes the installation tests to prevent overwriting real user configurations. Feedback on the changes suggests improving the Linux cron service installation by supporting step-value intervals (e.g., every 5 minutes), properly quoting the binary path to handle spaces, and dynamically displaying the scheduled interval in the output message.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| fn install_cron_service(spec: &ServiceSpec, bin: &str, interval: u64) -> Result<()> { | ||
| if interval != 60 { | ||
| eprintln!( | ||
| "Note: cron runs at 1-minute granularity; ignoring --interval={interval} and \ | ||
| scheduling every minute." | ||
| ); | ||
| } | ||
| let line = format!("* * * * * {bin} sync >> /tmp/ai-hist-sync.log 2>&1 {CRON_MARKER}"); | ||
| let marker = cron_marker(spec); | ||
| let line = format!( | ||
| "* * * * * {bin} {} >> /tmp/{}.log 2>&1 {marker}", | ||
| spec.subcommand, spec.log_stem | ||
| ); | ||
| // Drop any previously managed line, then append the current one. | ||
| let mut lines: Vec<String> = read_crontab() | ||
| .lines() | ||
| .filter(|l| !l.contains(CRON_MARKER)) | ||
| .filter(|l| !l.contains(&marker)) | ||
| .map(str::to_string) | ||
| .collect(); | ||
| lines.push(line); | ||
| write_crontab(&format!("{}\n", lines.join("\n")))?; | ||
|
|
||
| println!("Installed cron sync job; syncing every minute."); | ||
| println!("Installed cron {} job; running every minute.", spec.human); | ||
| println!(" view: crontab -l"); | ||
| println!(" remove: ai-hist sync --uninstall-service"); | ||
| println!( | ||
| " remove: ai-hist {} --uninstall-service", | ||
| spec.subcommand | ||
| ); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
There are three opportunities to improve the cron service installation on Linux:
- Interval Granularity: Currently, any interval other than 60 seconds (including the default 300 seconds for the push service) is ignored and scheduled to run every minute (
* * * * *). Since cron supports step values (e.g.,*/5 * * * *), we can support intervals that are multiples of 60 seconds up to 59 minutes. - Shell Quoting: The binary path (
bin) is inserted directly into the crontab line without quoting. If the installation path contains spaces or other shell-sensitive characters, the cron job will fail to parse. We can use the existingsh_single_quotehelper to safely escape the binary path. - Dynamic Output Message: The confirmation message is hardcoded to say "running every minute." If a custom interval (like 5 minutes) is successfully scheduled, the output should dynamically reflect the actual interval.
fn install_cron_service(spec: &ServiceSpec, bin: &str, interval: u64) -> Result<()> {
let minutes = (interval + 29) / 60;
let minutes = minutes.max(1);
if interval % 60 != 0 {
eprintln!(
"Note: cron runs at 1-minute granularity; rounding --interval={interval}s to {minutes} minute(s)."
);
}
let schedule = if minutes > 1 && minutes < 60 {
format!("*/{minutes} * * * *")
} else {
"* * * * *".to_string()
};
let marker = cron_marker(spec);
let safe_bin = sh_single_quote(bin);
let line = format!(
"{schedule} {safe_bin} {} >> /tmp/{}.log 2>&1 {marker}",
spec.subcommand, spec.log_stem
);
// Drop any previously managed line, then append the current one.
let mut lines: Vec<String> = read_crontab()
.lines()
.filter(|l| !l.contains(&marker))
.map(str::to_string)
.collect();
lines.push(line);
write_crontab(&format!("{}\n", lines.join("\n")))?;
if minutes > 1 {
println!("Installed cron {} job; running every {} minutes.", spec.human, minutes);
} else {
println!("Installed cron {} job; running every minute.", spec.human);
}
println!(" view: crontab -l");
println!(
" remove: ai-hist {} --uninstall-service",
spec.subcommand
);
Ok(())
}![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| anyhow::bail!( | ||
| "Automatic sync service install is only supported on macOS and Linux. \ | ||
| Run `ai-hist watch` to keep syncing in the foreground instead." | ||
| "Automatic {} service install is only supported on macOS and Linux. \ | ||
| Run `ai-hist watch` to keep syncing in the foreground instead.", | ||
| spec.human | ||
| ) |
There was a problem hiding this comment.
🟡 Error message on unsupported platforms incorrectly suggests 'watch' as a workaround for cloud push
The fallback error message for unsupported platforms always suggests running ai-hist watch (install_managed_service at crates/ai-hist/src/main.rs:1692), but watch only performs local sync — it does not push to the cloud, so the suggestion is wrong when installing the push service.
Impact: Users on unsupported platforms who try ai-hist push --install-service receive an incorrect workaround that won't actually push history to the cloud.
Mechanism: the generic error message doesn't account for the push service
The install_managed_service function at crates/ai-hist/src/main.rs:1682-1696 is shared by both SYNC_SERVICE and PUSH_SERVICE. The error message on line 1692 unconditionally says "Run ai-hist watch to keep syncing in the foreground instead." However, watch_loop at crates/ai-hist/src/main.rs:1621 only calls sync_basic, which does local history sync — it never calls cloud::push. When spec.human is "cloud push", the message reads: "Automatic cloud push service install is only supported on macOS and Linux. Run ai-hist watch to keep syncing in the foreground instead." — suggesting a command that doesn't perform the desired action.
| anyhow::bail!( | |
| "Automatic sync service install is only supported on macOS and Linux. \ | |
| Run `ai-hist watch` to keep syncing in the foreground instead." | |
| "Automatic {} service install is only supported on macOS and Linux. \ | |
| Run `ai-hist watch` to keep syncing in the foreground instead.", | |
| spec.human | |
| ) | |
| anyhow::bail!( | |
| "Automatic {} service install is only supported on macOS and Linux.", | |
| spec.human | |
| ) |
Was this helpful? React with 👍 or 👎 to provide feedback.
| fn install_cron_service(spec: &ServiceSpec, bin: &str, interval: u64) -> Result<()> { | ||
| if interval != 60 { | ||
| eprintln!( | ||
| "Note: cron runs at 1-minute granularity; ignoring --interval={interval} and \ | ||
| scheduling every minute." | ||
| ); |
There was a problem hiding this comment.
🔍 Push service on Linux will run every minute instead of every 5 minutes
The push service's default interval is 300 seconds (crates/ai-hist/src/main.rs:284), but install_cron_service at crates/ai-hist/src/main.rs:1787-1792 ignores the interval and always schedules * * * * * (every minute) because cron only supports 1-minute granularity. This means on Linux, ai-hist push --install-service will push every 60 seconds rather than every 300 seconds. The function does print a warning, but this is a significant behavioral difference from macOS. Consider whether the push command should implement its own interval guard (e.g. checking a timestamp file to skip runs within the interval window).
Was this helpful? React with 👍 or 👎 to provide feedback.
![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: 183ba7bf67
ℹ️ 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".
| if install_service { | ||
| return install_managed_service(&PUSH_SERVICE, interval); |
There was a problem hiding this comment.
With inputs such as ai-hist push --install-service --incognito <session>, clap accepts the privacy filter but this early return discards it and installs a service that later runs plain ai-hist push. That can upload the very session the user tried to exclude; either make service installation conflict with one-off push options like --incognito/--limit or persist those arguments into the scheduled command.
Useful? React with 👍 / 👎.
| let line = format!( | ||
| "* * * * * {bin} {} >> /tmp/{}.log 2>&1 {marker}", | ||
| spec.subcommand, spec.log_stem |
There was a problem hiding this comment.
On Linux, ai-hist push --install-service uses the new default interval of 300 seconds, but this shared cron installer always writes * * * * *, so the push job runs every minute after ignoring --interval=300. That contradicts the documented/default five-minute cadence and can increase cloud traffic significantly; render an appropriate cron schedule for push's default or make the Linux/default behavior explicitly 60 seconds.
Useful? React with 👍 / 👎.
| # plist pointing at this throwaway tmp binary. Sandbox HOME too as | ||
| # defense in depth. | ||
| "AI_HIST_NO_AUTOSYNC": "1", | ||
| "HOME": str(tmp_path / "install-home"), |
There was a problem hiding this comment.
In environments where cargo is the rustup shim and RUSTUP_HOME/CARGO_HOME are not exported, this source-mode installer test now runs cargo build with HOME pointed at a fresh temp directory, so rustup cannot find the configured toolchain and the test fails before it reaches the autosync regression. Keep the launchd sandboxing without hiding the Rust toolchain, for example by preserving the original cargo/rustup homes or only sandboxing HOME for the service-install step.
Useful? React with 👍 / 👎.
Reflex is documented in relayhistory-cloud, not here; keep the ai-hist docs scoped to the `push`/`sync` services themselves. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
![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
🧹 Nitpick comments (1)
test_install.py (1)
210-259: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate fake-binary/env setup with the adjacent test.
The
fake_binaryscript (lines 224-231) and thebinary-mode env dict (lines 233-245) are near-identical copies of lines 141-148 and 151-164 intest_install_script_binary_mode_does_not_require_cargo. Extracting a small helper (e.g._write_fake_ai_hist_binary(path)and a_binary_install_env(bin_dir, install_dir, home, extra=None)builder) would avoid drift between the two tests as install.sh's env contract evolves.♻️ Sketch of shared helper
def _write_fake_ai_hist_binary(path: Path) -> None: path.write_text( "#!/bin/sh\n" "if [ \"$1\" = \"--version\" ]; then echo 'ai-hist 9.9.9'; exit 0; fi\n" "if [ \"$1\" = \"recent\" ]; then echo '[]'; exit 0; fi\n" "echo \"fake ai-hist: $*\"\n" ) path.chmod(0o755)Also note
fake_tools(line 221-222) is created but unused in this test (nofake_cargostub is added here), which the extraction would naturally clean up.🤖 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 `@test_install.py` around lines 210 - 259, This test duplicates the fake ai-hist binary setup and binary-mode environment construction from the adjacent install test, and the unused fake_tools setup adds extra noise. Extract shared helpers for creating the fake binary and building the binary-install env, then update this test and test_install_script_binary_mode_does_not_require_cargo to call them so the install.sh env contract stays consistent and the dead fake_tools setup can be removed.
🤖 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/ai-hist/src/main.rs`:
- Around line 1787-1798: The warning in install_cron_service is misleading
because it treats the push command’s default interval as if the user explicitly
passed it. Update the message logic around install_cron_service and the interval
check so it only implies an override when the caller actually supplied a
non-default value, or otherwise phrase it as cron’s fixed 1-minute granularity
without referencing --interval={interval}. Keep the cron scheduling behavior
unchanged and use the existing symbols install_cron_service, ServiceSpec, and
interval to locate the message.
In `@README.md`:
- Around line 249-265: The push service quick-reference currently advertises the
default cadence without the Linux cron limitation, which can mislead users.
Update the README’s push section to match the sync section by mentioning the
`--interval` option and adding the same cron granularity caveat used by
`install_cron_service` in `main.rs` so Linux users know cron still runs at
1-minute intervals.
---
Nitpick comments:
In `@test_install.py`:
- Around line 210-259: This test duplicates the fake ai-hist binary setup and
binary-mode environment construction from the adjacent install test, and the
unused fake_tools setup adds extra noise. Extract shared helpers for creating
the fake binary and building the binary-install env, then update this test and
test_install_script_binary_mode_does_not_require_cargo to call them so the
install.sh env contract stays consistent and the dead fake_tools setup can be
removed.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a52fc8a9-b45b-4db6-8c76-a0a009fbfb8d
📒 Files selected for processing (3)
README.mdcrates/ai-hist/src/main.rstest_install.py
|
ℹ️ pr-reviewer: review only — no file changes were applied to the PR (nothing to commit after review). The notes below are advisory and were not pushed. The The review is complete. Let me summarize findings. Review SummaryPR #38 ( Verification performed (as CI runs it)
One initial Findings
No semantic, safety, or fail-closed defaults were altered. No lifecycle/reaper/dispatch code touched. No mechanical (lint/format/typo) issues found requiring auto-edit. The working tree is unchanged from the PR head. Addressed comments
Advisory Notes
The PR is coherent, well-scoped, and all CI-equivalent build/test steps pass locally. The remaining decision (merge, and the cross-repo reflex coordination) requires human judgment. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
6 issues found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/ai-hist/src/main.rs">
<violation number="1" location="crates/ai-hist/src/main.rs:741">
P1: Installing the push service currently ignores one-off push filters like `--incognito` and `--limit` because this early return bypasses the normal push execution path. It would be safer to reject those combinations (or persist them) so users don’t think privacy/filter flags are applied to the scheduled job.</violation>
<violation number="2" location="crates/ai-hist/src/main.rs:1692">
P3: For `push --install-service` on unsupported platforms, this fallback suggests `ai-hist watch`, but `watch` is a local sync loop and does not perform cloud upload. A subcommand-specific fallback message would avoid sending users to a workflow that doesn’t match cloud push.</violation>
<violation number="3" location="crates/ai-hist/src/main.rs:1796">
P2: Linux scheduled push/sync can fail to execute when the ai-hist binary path contains spaces or shell-significant characters. The crontab command interpolates `{bin}` unquoted, so quoting the executable path before writing the cron line would make the managed job robust.</violation>
<violation number="4" location="crates/ai-hist/src/main.rs:1796">
P2: This cron entry runs every minute regardless of `--interval`, so `ai-hist push --install-service` on Linux runs at 60s even with the 300s default. That changes upload cadence and can increase cloud traffic; consider mapping whole-minute intervals to cron step schedules or aligning Linux defaults/docs to one minute.</violation>
</file>
<file name="README.md">
<violation number="1" location="README.md:256">
P3: This quick-reference line advertises a 300s default without noting that Linux cron currently runs every minute. Adding the same cron-granularity caveat used in the sync section would prevent a platform-specific cadence mismatch in the docs.</violation>
</file>
<file name="test_install.py">
<violation number="1" location="test_install.py:26">
P2: Setting `HOME` to a fresh temp directory here can hide default `CARGO_HOME`/`RUSTUP_HOME` locations in source-mode installs, so `cargo build` may fail before the autosync regression is exercised. Preserving or explicitly passing cargo/rustup home paths would keep the launchd sandboxing without breaking toolchain discovery.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Review SummaryPR #38 ( What I verifiedRust refactor (
Required fix (CI-failing) — left for human, not auto-edited
I reproduced this deterministically ( I did not edit the test. The rules forbid me from modifying tests to make CI pass — how to reconcile the launchd-sandboxing intent with rustup resolution is a design decision. Suggested fix for the author (choose one):
The binary-mode tests and the new Addressed comments
Advisory Notes
I made no file edits; the working tree is unchanged (aside from the pre-existing, unrelated |
Adds `pushToCloud` (exported from `ai-hist/cloud`) plus the primitives it needs — `buildOutboxBatch`, `promptHash`, `batchId`, `machineId`, cursor load/save, `normalizeHomePath`. This is a faithful TypeScript port of the Rust `ai-hist push` (crates/ai-hist/src/cloud.rs + ai-hist-core outbox/ convergence): identical envelope shapes, eventId formats, prompt/batch hashing, and cursor semantics, so the server's (orgId, machineId, batchId) batch dedup and per-event upsert keys line up regardless of which client pushes. Auth reuses the SDK's existing ~/.config/ai-hist/auth.json; cursor and machine-id persist alongside it. Lets the Agent Relay runtime sync Reflex history in-process instead of shelling out to the CLI. Tests cover the hashing vectors, path scrub, cursor advance (incl. incognito skip), and history/trajectory/compacted mapping against a real sql.js DB. Bumps sdk-ts to 0.4.0 (new public API). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="README.md">
<violation number="1" location="README.md:256">
P3: This quick-reference line advertises a 300s default without noting that Linux cron currently runs every minute. Adding the same cron-granularity caveat used in the sync section would prevent a platform-specific cadence mismatch in the docs.</violation>
</file>
<file name="crates/ai-hist/src/main.rs">
<violation number="1" location="crates/ai-hist/src/main.rs:741">
P1: Installing the push service currently ignores one-off push filters like `--incognito` and `--limit` because this early return bypasses the normal push execution path. It would be safer to reject those combinations (or persist them) so users don’t think privacy/filter flags are applied to the scheduled job.</violation>
<violation number="2" location="crates/ai-hist/src/main.rs:1692">
P3: For `push --install-service` on unsupported platforms, this fallback suggests `ai-hist watch`, but `watch` is a local sync loop and does not perform cloud upload. A subcommand-specific fallback message would avoid sending users to a workflow that doesn’t match cloud push.</violation>
<violation number="3" location="crates/ai-hist/src/main.rs:1796">
P2: Linux scheduled push/sync can fail to execute when the ai-hist binary path contains spaces or shell-significant characters. The crontab command interpolates `{bin}` unquoted, so quoting the executable path before writing the cron line would make the managed job robust.</violation>
<violation number="4" location="crates/ai-hist/src/main.rs:1796">
P2: This cron entry runs every minute regardless of `--interval`, so `ai-hist push --install-service` on Linux runs at 60s even with the 300s default. That changes upload cadence and can increase cloud traffic; consider mapping whole-minute intervals to cron step schedules or aligning Linux defaults/docs to one minute.</violation>
</file>
<file name="sdk-ts/src/cloud-client.ts">
<violation number="1" location="sdk-ts/src/cloud-client.ts:22">
P2: Circular dependency introduced between `cloud-client.ts` and `cloud-push.ts`. The re-export block (lines 6–22) makes `cloud-client` depend on `cloud-push`, while `cloud-push` already imports `loadStoredRelayhistoryAuth` and `RelayhistoryAuth` from `cloud-client`. This works today because function declarations are hoisted in ESM, but the cycle makes the module graph fragile: adding a top-level constant, expression, or side effect in either module could silently produce `undefined` bindings at module-evaluation time. Consider breaking the cycle by extracting the shared auth types/function into a separate module (e.g. `cloud-auth.ts`) or removing the re-exports and having consumers import from `cloud-push` directly.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| type PushReport, | ||
| type PushOptions, | ||
| type OutboxBatch, | ||
| } from './cloud-push.js'; |
There was a problem hiding this comment.
P2: Circular dependency introduced between cloud-client.ts and cloud-push.ts. The re-export block (lines 6–22) makes cloud-client depend on cloud-push, while cloud-push already imports loadStoredRelayhistoryAuth and RelayhistoryAuth from cloud-client. This works today because function declarations are hoisted in ESM, but the cycle makes the module graph fragile: adding a top-level constant, expression, or side effect in either module could silently produce undefined bindings at module-evaluation time. Consider breaking the cycle by extracting the shared auth types/function into a separate module (e.g. cloud-auth.ts) or removing the re-exports and having consumers import from cloud-push directly.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At sdk-ts/src/cloud-client.ts, line 22:
<comment>Circular dependency introduced between `cloud-client.ts` and `cloud-push.ts`. The re-export block (lines 6–22) makes `cloud-client` depend on `cloud-push`, while `cloud-push` already imports `loadStoredRelayhistoryAuth` and `RelayhistoryAuth` from `cloud-client`. This works today because function declarations are hoisted in ESM, but the cycle makes the module graph fragile: adding a top-level constant, expression, or side effect in either module could silently produce `undefined` bindings at module-evaluation time. Consider breaking the cycle by extracting the shared auth types/function into a separate module (e.g. `cloud-auth.ts`) or removing the re-exports and having consumers import from `cloud-push` directly.</comment>
<file context>
@@ -2,6 +2,25 @@ import { readFile, writeFile, mkdir } from 'node:fs/promises';
+ type PushReport,
+ type PushOptions,
+ type OutboxBatch,
+} from './cloud-push.js';
+
export interface RelayhistoryAuth {
</file context>
khaliqgant
changed the title
… to TS
Replaces the ~500-line TypeScript port of the Rust push pipeline with a thin
wrapper: `pushToCloud` now resolves the `ai-hist` binary
($AI_HIST_RUST_BIN → install.sh location → PATH) and spawns
`ai-hist push --json`, parsing `{sent, accepted, batchId, cursor}`. The Rust
binary stays the single source of truth for batching/cursor/dedup — no more
wire-compatibility duplication to keep in sync. Resolves null (no-op) when the
binary is missing or the user isn't authenticated; rejects on other failures.
Also exports `resolveAiHistBinary`. Drops the sql.js-based DB reading and the
outbox/convergence/hash re-implementations.
Also fixes the from-source install test that broke CI `verify`: it sandboxed
$HOME, which blinds rustup (it resolves the toolchain via ~/.rustup / ~/.cargo)
during `cargo build`. AI_HIST_NO_AUTOSYNC=1 alone is the real launchd-pollution
fix; the HOME override is removed from that path (kept on the binary-mode tests,
which don't run cargo).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
khaliqgant
changed the title
|
Let me verify one last edge case in I have completed the review. No code changes were needed — the PR is mechanically clean, typechecks, builds, and all 65 tests pass against the current checkout. Review: PR #38 — Agent-scoped channel creation, retry rate-limiting, single-serve lockSummaryThis PR adds three coordinated capabilities to the
Verification (ran the canonical commands CI uses, from
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
sdk-ts/src/cloud-push.test.ts (1)
35-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for malformed JSON output.
Tests cover valid JSON, empty stdout, ENOENT, auth failure, and other non-zero exits, but not the
JSON.parsefailure path (Lines 123-127 incloud-push.ts) — e.g. truncated/corrupted stdout on exit 0. Worth a test asserting the rejection message.🤖 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 `@sdk-ts/src/cloud-push.test.ts` around lines 35 - 68, Add a test in pushToCloud that covers malformed JSON from the spawn result, since cloud-push.test.ts currently misses the JSON.parse failure path in pushToCloud. Use fakeSpawn with exit 0 and truncated/corrupted stdout, then assert pushToCloud rejects with the parse error message or a clear wrapped failure from the pushToCloud implementation. Keep the new case alongside the existing pushToCloud tests so it validates the handling around the JSON parsing branch.
🤖 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 `@sdk-ts/src/cloud-push.ts`:
- Around line 54-63: The explicit binPath override in resolveAiHistBinary is
still treated as a discovery candidate instead of a true override. Update
resolveAiHistBinary so that when explicit is provided it is returned directly
(or fails immediately if invalid) without falling through to AI_HIST_RUST_BIN,
the install path, or PATH-based ai-hist resolution; keep fallback discovery only
for the non-explicit case.
---
Nitpick comments:
In `@sdk-ts/src/cloud-push.test.ts`:
- Around line 35-68: Add a test in pushToCloud that covers malformed JSON from
the spawn result, since cloud-push.test.ts currently misses the JSON.parse
failure path in pushToCloud. Use fakeSpawn with exit 0 and truncated/corrupted
stdout, then assert pushToCloud rejects with the parse error message or a clear
wrapped failure from the pushToCloud implementation. Keep the new case alongside
the existing pushToCloud tests so it validates the handling around the JSON
parsing branch.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6024641a-f0b4-411b-b304-0dc3555defc7
📒 Files selected for processing (8)
README.mdcrates/ai-hist/src/main.rssdk-ts/CHANGELOG.mdsdk-ts/package.jsonsdk-ts/src/cloud-client.tssdk-ts/src/cloud-push.test.tssdk-ts/src/cloud-push.tstest_install.py
✅ Files skipped from review due to trivial changes (4)
- sdk-ts/package.json
- sdk-ts/CHANGELOG.md
- sdk-ts/src/cloud-client.ts
- README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- test_install.py
- crates/ai-hist/src/main.rs
There was a problem hiding this comment.
2 issues found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="sdk-ts/src/cloud-client.ts">
<violation number="1" location="sdk-ts/src/cloud-client.ts:22">
P2: Circular dependency introduced between `cloud-client.ts` and `cloud-push.ts`. The re-export block (lines 6–22) makes `cloud-client` depend on `cloud-push`, while `cloud-push` already imports `loadStoredRelayhistoryAuth` and `RelayhistoryAuth` from `cloud-client`. This works today because function declarations are hoisted in ESM, but the cycle makes the module graph fragile: adding a top-level constant, expression, or side effect in either module could silently produce `undefined` bindings at module-evaluation time. Consider breaking the cycle by extracting the shared auth types/function into a separate module (e.g. `cloud-auth.ts`) or removing the re-exports and having consumers import from `cloud-push` directly.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
sdk-ts: - resolveAiHistBinary: an explicit binPath is now authoritative (returned verbatim), never falling through to $AI_HIST_RUST_BIN / install path / PATH, so callers can't get a different binary than they asked for. Fix the JSDoc (always returns a string). Add a test for the malformed-JSON reject path. ai-hist CLI (push --install-service): - Reject `--incognito` when installing the service: it's a per-run privacy filter that would be silently dropped on the scheduled job. - Linux cron now honors --interval via step schedules (300s -> `*/5 * * * *`) instead of always running every minute; the confirmation prints the real cadence. Add cron_schedule unit tests. - Quote the binary path in the cron line so paths with spaces/metacharacters work (shell_single_quote + test). - Unsupported-platform message no longer points to `ai-hist watch` (a local sync loop); it's now subcommand-specific. - README: note the Linux cron granularity caveat for push. test_install.py: drop the unused fake-tools/PATH setup from the opt-out regression test (binary mode runs no cargo). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Addressed the review feedback (latest commit). Disposition: Fixed
Skipped (with reason)
Note: much of the earlier round targeted the pre-rework TS port of the push pipeline — that code was replaced by the thin binary wrapper, so those are moot. |
There was a problem hiding this comment.
2 issues found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="sdk-ts/src/cloud-client.ts">
<violation number="1" location="sdk-ts/src/cloud-client.ts:22">
P2: Circular dependency introduced between `cloud-client.ts` and `cloud-push.ts`. The re-export block (lines 6–22) makes `cloud-client` depend on `cloud-push`, while `cloud-push` already imports `loadStoredRelayhistoryAuth` and `RelayhistoryAuth` from `cloud-client`. This works today because function declarations are hoisted in ESM, but the cycle makes the module graph fragile: adding a top-level constant, expression, or side effect in either module could silently produce `undefined` bindings at module-evaluation time. Consider breaking the cycle by extracting the shared auth types/function into a separate module (e.g. `cloud-auth.ts`) or removing the re-exports and having consumers import from `cloud-push` directly.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
ℹ️ pr-reviewer: review only — no file changes were applied to the PR (nothing to commit after review). The notes below are advisory and were not pushed. Review: PR #38 —
|
Two real bugs the re-review caught in the prior fix: - cron_schedule over-ran for large intervals: a daily (86400s) or 90-minute interval fell through to the hourly fallback (up to 24x too frequent). It now rounds toward a *coarser* cadence (90 min -> every 2h) and caps at a daily run, never firing more often than requested. It also returns the effective period so install_cron_service only prints the granularity note on an actual mismatch (not on the plain 300s default). Tests cover 90 min / 1 day / 25h. - pushToCloud only treated ENOENT as "binary unavailable", so an explicit binPath that exists but isn't executable (EACCES/EPERM) or is a directory (ENOTDIR/EISDIR) rejected instead of resolving null, breaking the background no-op contract. Now all of these no-op. Added an EACCES test and aligned the JSDoc. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="sdk-ts/src/cloud-client.ts">
<violation number="1" location="sdk-ts/src/cloud-client.ts:22">
P2: Circular dependency introduced between `cloud-client.ts` and `cloud-push.ts`. The re-export block (lines 6–22) makes `cloud-client` depend on `cloud-push`, while `cloud-push` already imports `loadStoredRelayhistoryAuth` and `RelayhistoryAuth` from `cloud-client`. This works today because function declarations are hoisted in ESM, but the cycle makes the module graph fragile: adding a top-level constant, expression, or side effect in either module could silently produce `undefined` bindings at module-evaluation time. Consider breaking the cycle by extracting the shared auth types/function into a separate module (e.g. `cloud-auth.ts`) or removing the re-exports and having consumers import from `cloud-push` directly.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
`*/N` in a cron field is only uniform when N divides the field's range — a non-divisor step fires a short interval at the rollover (`*/45` → :00, :45, :00 = a 15-minute gap, i.e. faster than intended). cron_schedule now rounds the minute step up to the next divisor of 60 (7 min → */10, 45 min → hourly) and the hour step up to the next divisor of 24 (7h → */8), keeping every schedule uniform and never faster than requested. Tests cover the non-divisor cases. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
…SDK (#1233) * feat(cli): reflex on/off schedules automatic cloud sync `agent-relay reflex on` previously only flipped a flag and did a one-time cloud login — nothing ever pushed history to relayhistory-cloud, so "Reflex is on" never actually synced anything. Now `reflex on` also schedules the ai-hist background services (local `sync` + cloud `push`, via `ai-hist <cmd> --install-service`), `reflex off` removes the push service (leaving local capture in place), and `reflex status` reports whether cloud push is scheduled. Scheduling is best-effort: if ai-hist isn't on PATH the flag still flips and a clear hint is printed, matching the existing cloud-login resilience. Wiring goes through injectable ReflexDependencies (installCloudSync / uninstallCloudSync / cloudSyncInstalled) so it stays unit-testable without shelling out. Requires ai-hist with `push --install-service` (AgentWorkforce/relayhistory#38). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * style: auto-format with Prettier * feat(cli): reflex syncs to cloud in-process via ai-hist SDK (no CLI shell-out) Replaces the earlier shell-out approach. `agent-relay reflex on` no longer spawns `ai-hist ... --install-service`; instead the long-running `agent-relay up` host pushes new local session history to relayhistory-cloud in-process, gated on the reflex flag. - `@agent-relay/config`: `reflex-config.ts` is the single source of truth for the `~/.agentworkforce/reflex.json` shape/location — `isReflexEnabled()`, `readReflexState()`, `writeReflexState()`. `reflex.ts` now uses it and drops its private copy + the CLI shell-out deps; `reflex on` = flip flag + cloud login only. - `reflex-capture.ts`: an unref'd periodic push loop (mirrors the telemetry client) started after the fleet sidecar in `broker-lifecycle.ts` and stopped (with a final flush) in `shutdownOnce`. It calls `ai-hist/cloud`'s `pushToCloud` via a lazy, non-analyzable dynamic import so the CLI does not statically depend on it — a silent no-op if ai-hist is unavailable or the user isn't authed. - Adds `ai-hist@^0.4.0` (the SDK gains `pushToCloud` in that release) and marks `ai-hist`/`sql.js` external in the esbuild library bundle. Auth is self-consistent: reflex login and the SDK push both use ~/.config/ai-hist/auth.json. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(cli): address reflex PR review + unbreak CI - Drop the `ai-hist@^0.4.0` dependency: that version isn't published yet (it ships in relayhistory#38), so `npm ci` failed with ETARGET across every CI job. The capture loop already loads `ai-hist/cloud` via a lazy dynamic import and no-ops gracefully when absent, so it's a runtime-optional peer; declare it as a real dependency once 0.4.0 is published. Reverts the now-moot esbuild external entry too. - reflex-capture: re-check `isEnabled()` on every tick so `reflex off` (or `on`) takes effect immediately in a running `agent-relay up`, and start the interval only after the initial delay so the first push can't fire before initialDelayMs when intervalMs is smaller. Adds tests for both. - reflex on: only print "History syncs automatically…" when cloud login actually succeeded, so it no longer contradicts the not-logged-in / login- failed warnings. Adds negative assertions. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore: apply pr-reviewer fixes for #1233 * fix(cli): reflex writes Rust-binary auth; capture drives the binary Follows the SDK pivot to a binary-backed push (relayhistory sdk-ts). Since the in-process push now spawns `ai-hist push`, reflex login must persist the rth_at_ session where that binary reads it: $RELAYHISTORY_HOME/auth.json (default ~/.agentworkforce/relayhistory/auth.json), in the Rust snake_case shape (base_url/access_token/refresh_token) — not the old camelCase ~/.config/ai-hist path. The capture loop no longer pre-checks auth via the TS SDK; it just calls pushToCloud, which drives the binary and returns null when the binary is missing or the user isn't logged in. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(cli): reflex capture syncs+pushes via the bundled ai-hist binary Toward `agent-relay reflex on` "just working" with no extra user commands: - Add `ai-hist-path.ts`: resolves the ai-hist binary like the broker resolves its own — `$AI_HIST_RUST_BIN` -> the per-platform optional-dep package (`ai-hist-bin-<platform>-<arch>`) -> the install.sh location -> `ai-hist` on PATH. Once the binary ships as an optional dependency, a plain agent-relay install has it with zero setup. - Rework the capture loop to drive that binary directly: it now runs `ai-hist sync` (populate the local DB from the user's agent history) then `ai-hist push --json` each tick — previously it only pushed, so a fresh machine had nothing to upload. Dropped the lazy `ai-hist/cloud` npm import, so relay has no dependency to publish/resolve; unavailable binary or missing auth is a silent no-op. Tests cover the resolver (override + package-name mapping) and sync→push (happy path, binary-unavailable skip, not-authenticated, hard failure). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * style: auto-format with Prettier * feat(cli): reflex capture runs in-process via napi (no subprocess) Replaces the spawn-based capture with an in-process call to the `ai-hist-native` napi addon's `syncAndPush()` — no `ai-hist` subprocess at all, per the requirement not to shell out to the CLI. The addon is lazy-loaded via a non-analyzable dynamic import (so it stays out of the esbuild bundle and resolves from its per-platform optional-dependency package), and is a silent no-op when unavailable or the user isn't authenticated. Removes ai-hist-path.ts (the binary resolver) and the spawn plumbing. Tests now inject the native addon instead of a fake child process. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(cli): depend on ai-hist-native for in-process reflex capture Now that ai-hist-native@0.4.1 is published, declare it as an optional dependency so a plain `agent-relay` install pulls the addon (and npm auto-selects the matching per-platform binary via os/cpu). The reflex capture loop loads it and calls syncAndPush() in-process; it stays a graceful no-op if the addon isn't available for a platform. Verified: npm install resolves ai-hist-native + ai-hist-native-darwin-arm64 and `require('ai-hist-native')` exposes syncAndPush. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(cli): surface broken ai-hist-native addon instead of silently no-op'ing reflex-capture's loadNative() caught every dynamic-import error and returned null, so an installed-but-broken addon (ABI mismatch, missing system lib, init failure) looked identical to "not installed" and the capture loop silently did nothing. Now only ERR_MODULE_NOT_FOUND / MODULE_NOT_FOUND is treated as a no-op; any other error is rethrown so the loop logs it (`[reflex] cloud sync failed: …`). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Proactive Runtime Bot <agent@agent-relay.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: agent-relay-code[bot] <agent-relay-code[bot]@users.noreply.github.com>
1 participant
Makes relayhistory cloud sync automatic and available as an SDK, without re-implementing the push logic.
1.
sdk-ts: cloud push that drives the Rust binaryai-hist/cloudexportspushToCloud, a thin wrapper over the realai-hist push --jsonbinary — it resolves the binary ($AI_HIST_RUST_BIN→ install.sh location →ai-histonPATH), spawns it, and parses{sent, accepted, batchId, cursor}.This replaces an earlier ~500-line TypeScript port of the Rust push pipeline. Per review discussion, a port was the wrong call: it duplicated batching/cursor/dedup logic that must stay byte-for-byte wire-compatible with the Rust client forever. Driving the binary keeps one source of truth (the Rust
ai-hist push) and drops thesql.jsDB reading entirely.pushToCloudresolvesnull(no-op) when the binary is missing or the user isn't authenticated; rejects on other failures. Net −900 lines.2. Installer test fix (was breaking CI
verify)The from-source install test sandboxed
$HOME, which blinds rustup (it resolves the toolchain via~/.rustup/~/.cargo) duringcargo build→rustup could not choose a version of cargo.AI_HIST_NO_AUTOSYNC=1alone is the real launchd-pollution fix; theHOMEoverride is removed from the cargo path (kept on binary-mode tests, which don't compile).3.
ai-hist push --install-service(CLI parity)Standalone launchd/cron push service (
com.ai-hist.push), independent of the SDK path above.Testing
cargo build/cargo test --no-run✅npm test✅ (wrapper tests: arg forwarding, JSON parse, ENOENT→null, not-authed→null, error→reject)pytest test_install.py/test_cli_dispatch.py✅🤖 Generated with Claude Code