feat(cli): reflex syncs to relayhistory-cloud in-process via ai-hist SDK - #1233
Conversation
`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>
|
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. |
|
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:
📝 WalkthroughWalkthroughAdds shared Reflex state helpers, an ChangesReflex capture lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 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 integrates background services for ai-hist (automatic local history sync and cloud push) into the reflex command, allowing scheduling on reflex on, removal on reflex off, and status reporting on reflex status. The review feedback recommends improving platform compatibility on non-Darwin systems by allowing the status check to return 'unknown' instead of a false negative, updating the status output accordingly, and gracefully handling missing ai-hist binaries during uninstallation.
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.
| ? 'Cloud push service: scheduled.' | ||
| : 'Cloud push service: not scheduled — run `agent-relay reflex on`.' | ||
| ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Handle the 'unknown' status gracefully on non-Darwin platforms. This avoids showing a misleading message to Linux users telling them to run agent-relay reflex on when the service might already be scheduled.
| ? 'Cloud push service: scheduled.' | |
| : 'Cloud push service: not scheduled — run `agent-relay reflex on`.' | |
| ); | |
| return; | |
| } | |
| const syncInstalled = deps.cloudSyncInstalled(); | |
| if (syncInstalled === 'unknown') { | |
| deps.log('Cloud push service: scheduled (status check not supported on this platform).'); | |
| } else { | |
| deps.log( | |
| syncInstalled | |
| ? 'Cloud push service: scheduled.' | |
| : 'Cloud push service: not scheduled — run \'agent-relay reflex on\'.' | |
| ); | |
| } |
| /** Whether the automatic cloud push service is currently scheduled. */ | ||
| cloudSyncInstalled: () => boolean; |
There was a problem hiding this comment.
To prevent false negatives on non-Darwin platforms (like Linux) where we cannot easily check the status of the cron job synchronously, let's allow cloudSyncInstalled to return 'unknown' instead of forcing a boolean false.
| /** Whether the automatic cloud push service is currently scheduled. */ | |
| cloudSyncInstalled: () => boolean; | |
| /** Whether the automatic cloud push service is currently scheduled. */ | |
| cloudSyncInstalled: () => boolean | 'unknown'; |
| async function defaultUninstallCloudSync(): Promise<ServiceResult> { | ||
| // Only remove cloud upload; leave local `sync` capture in place. | ||
| const args = ['push', '--uninstall-service']; | ||
| try { | ||
| await execFileAsync('ai-hist', args); | ||
| return { ok: true }; | ||
| } catch (err) { | ||
| return { ok: false, error: aiHistFailure(err, args) }; | ||
| } | ||
| } |
There was a problem hiding this comment.
If ai-hist is not found on the PATH (i.e., ENOENT), it means ai-hist is not installed, so the background service cannot be installed or running anyway. Treating this as a success avoids printing a confusing error message to the user when they run agent-relay reflex off.
async function defaultUninstallCloudSync(): Promise<ServiceResult> {
// Only remove cloud upload; leave local 'sync' capture in place.
const args = ['push', '--uninstall-service'];
try {
await execFileAsync('ai-hist', args);
return { ok: true };
} catch (err) {
if (err && typeof err === 'object' && (err as NodeJS.ErrnoException).code === 'ENOENT') {
return { ok: true };
}
return { ok: false, error: aiHistFailure(err, args) };
}
}| function defaultCloudSyncInstalled(fsImpl: typeof fs, homedir: () => string): boolean { | ||
| if (process.platform !== 'darwin') { | ||
| // On Linux the push job lives in crontab; we don't shell out just to report | ||
| // status, so report unknown (false) rather than guess. | ||
| return false; | ||
| } | ||
| return fsImpl.existsSync(path.join(homedir(), 'Library', 'LaunchAgents', 'com.ai-hist.push.plist')); | ||
| } | ||
|
|
||
| function withDefaults(overrides: Partial<ReflexDependencies> = {}): ReflexDependencies { |
There was a problem hiding this comment.
Update defaultCloudSyncInstalled to return 'unknown' on non-Darwin platforms instead of false. This allows the CLI to distinguish between a service that is definitely not installed and a service whose status cannot be verified on the current platform.
| function defaultCloudSyncInstalled(fsImpl: typeof fs, homedir: () => string): boolean { | |
| if (process.platform !== 'darwin') { | |
| // On Linux the push job lives in crontab; we don't shell out just to report | |
| // status, so report unknown (false) rather than guess. | |
| return false; | |
| } | |
| return fsImpl.existsSync(path.join(homedir(), 'Library', 'LaunchAgents', 'com.ai-hist.push.plist')); | |
| } | |
| function withDefaults(overrides: Partial<ReflexDependencies> = {}): ReflexDependencies { | |
| function defaultCloudSyncInstalled(fsImpl: typeof fs, homedir: () => string): boolean | 'unknown' { | |
| if (process.platform !== 'darwin') { | |
| // On Linux the push job lives in crontab; we don't shell out just to report | |
| // status, so report unknown rather than guess. | |
| return 'unknown'; | |
| } | |
| return fsImpl.existsSync( | |
| path.join(homedir(), 'Library', 'LaunchAgents', 'com.ai-hist.push.plist') | |
| ); | |
| } |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| deps.log('Scheduled automatic history sync + cloud push.'); | ||
| } else { | ||
| deps.log(`Reflex is enabled, but automatic sync could not be scheduled: ${cloudSync.error}`); | ||
| } | ||
|
|
||
| deps.log('Reflex is on.'); |
There was a problem hiding this comment.
🟡 Changelog not updated with the new cloud sync scheduling feature
The changelog is not updated to reflect the new cloud sync scheduling behavior added by this PR (CHANGELOG.md:57), so the release narrative omits that reflex on now schedules background sync services, reflex off removes them, and reflex status reports their state.
Impact: The release notes will not document the new cloud sync scheduling capability for users.
AGENTS.md rule and missing changelog detail
AGENTS.md states: "Curate [Unreleased] in CHANGELOG.md as you land PRs. The root changelog is the cross-package, user-facing release narrative for Relay."
The existing [Unreleased] entry at CHANGELOG.md:57 reads:
- \agent-relay reflex on|off|status` manages Reflex history sync with a consent prompt and persisted `~/.agentworkforce/reflex.json` state.`
This PR adds three significant user-visible behaviors:
reflex onnow schedulesai-hist syncandai-hist pushbackground services (packages/cli/src/cli/commands/reflex.ts:268-273)reflex offnow uninstalls the cloud push service (packages/cli/src/cli/commands/reflex.ts:284-287)reflex statusnow reports whether the cloud push service is scheduled (packages/cli/src/cli/commands/reflex.ts:306-310)
None of these are reflected in the changelog entry.
Prompt for agents
AGENTS.md requires updating the [Unreleased] section of CHANGELOG.md when landing PRs. The existing entry at CHANGELOG.md:57 for reflex on/off/status should be updated (or a new bullet added) to mention that reflex on now schedules ai-hist background sync and push services (launchd on macOS, cron on Linux), reflex off removes the cloud push service, and reflex status reports whether the cloud push service is currently scheduled. This is a user-visible behavior change that should be documented in the Added section of the changelog.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (process.platform !== 'darwin') { | ||
| // On Linux the push job lives in crontab; we don't shell out just to report | ||
| // status, so report unknown (false) rather than guess. | ||
| return false; | ||
| } | ||
| return fsImpl.existsSync(path.join(homedir(), 'Library', 'LaunchAgents', 'com.ai-hist.push.plist')); | ||
| } | ||
|
|
||
| function withDefaults(overrides: Partial<ReflexDependencies> = {}): ReflexDependencies { |
There was a problem hiding this comment.
🔍 Cloud push status always reports 'not scheduled' on Linux
The defaultCloudSyncInstalled function at packages/cli/src/cli/commands/reflex.ts:179-187 returns false on all non-macOS platforms because it only knows how to check for the launchd plist. On Linux, even after a successful reflex on that installs the cron-based push service, reflex status will always show 'Cloud push service: not scheduled — run agent-relay reflex on.' This is documented in the comment but creates a confusing UX for Linux users who just successfully ran reflex on and then immediately check reflex status.
Was this helpful? React with 👍 or 👎 to provide feedback.
| async function defaultUninstallCloudSync(): Promise<ServiceResult> { | ||
| // Only remove cloud upload; leave local `sync` capture in place. | ||
| const args = ['push', '--uninstall-service']; | ||
| try { | ||
| await execFileAsync('ai-hist', args); | ||
| return { ok: true }; | ||
| } catch (err) { | ||
| return { ok: false, error: aiHistFailure(err, args) }; | ||
| } | ||
| } |
There was a problem hiding this comment.
🔍 Install schedules both sync and push, but uninstall only removes push
The defaultInstallCloudSync at packages/cli/src/cli/commands/reflex.ts:156-165 installs both sync --install-service and push --install-service, but defaultUninstallCloudSync at packages/cli/src/cli/commands/reflex.ts:167-176 only removes push. The comment says this is intentional ('leave local sync capture in place'), but it means reflex off → reflex on → reflex off will accumulate a sync service that is never cleaned up. If the intent is that local capture persists even when Reflex is off, this is fine, but it's worth confirming that's the desired behavior since users might expect reflex off to fully undo reflex on.
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: 2cd9390e54
ℹ️ 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".
| // ai-hist background services (launchd on macOS, cron on Linux). | ||
| const CLOUD_SYNC_STAGES: readonly string[][] = [ | ||
| ['sync', '--install-service'], | ||
| ['push', '--install-service'], |
There was a problem hiding this comment.
This new stage assumes ai-hist push accepts --install-service, but the current public relayhistory CLI only defines service flags on sync; push is declared with --limit, --incognito, and --json (checked upstream crates/ai-hist/src/main.rs). With current installs, reflex on installs local sync and then push --install-service fails as an unexpected argument, so cloud push is never scheduled while Reflex still writes enabled state. Gate this on a supported ai-hist version or land/update the dependency before enabling it here.
Useful? React with 👍 / 👎.
| if (process.platform !== 'darwin') { | ||
| // On Linux the push job lives in crontab; we don't shell out just to report | ||
| // status, so report unknown (false) rather than guess. | ||
| return false; |
There was a problem hiding this comment.
On Linux, reflex on schedules the push job through cron, but this status probe short-circuits every non-macOS platform to false. After a successful enable on Linux, agent-relay reflex status will still print Cloud push service: not scheduled — run agent-relay reflex on, causing users to rerun setup even though the cron job may already exist. Check the managed crontab entry or report an explicit unknown state instead of treating Linux as unscheduled.
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 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="packages/cli/src/cli/commands/reflex.test.ts">
<violation number="1" location="packages/cli/src/cli/commands/reflex.test.ts:111">
P3: The `reflex off` test only covers the happy path where `uninstallCloudSync` succeeds. The implementation also handles failure (`!removal.ok`) by logging a distinct warning message, but that branch is untested. Adding a test with a failing mock (e.g., `{ ok: false, error: 'ai-hist was not found on your PATH.' }`) would close the gap and mirror the coverage pattern already used for `installCloudSync` in the "reflex on still enables when scheduling the sync service fails" test.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| expect(outputLines(deps)).toContain('Reflex is on.'); | ||
| }); | ||
|
|
||
| it('reflex off writes disabled state, removes the push service, and prints confirmation', async () => { |
There was a problem hiding this comment.
P3: The reflex off test only covers the happy path where uninstallCloudSync succeeds. The implementation also handles failure (!removal.ok) by logging a distinct warning message, but that branch is untested. Adding a test with a failing mock (e.g., { ok: false, error: 'ai-hist was not found on your PATH.' }) would close the gap and mirror the coverage pattern already used for installCloudSync in the "reflex on still enables when scheduling the sync service fails" test.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/cli/src/cli/commands/reflex.test.ts, line 111:
<comment>The `reflex off` test only covers the happy path where `uninstallCloudSync` succeeds. The implementation also handles failure (`!removal.ok`) by logging a distinct warning message, but that branch is untested. Adding a test with a failing mock (e.g., `{ ok: false, error: 'ai-hist was not found on your PATH.' }`) would close the gap and mirror the coverage pattern already used for `installCloudSync` in the "reflex on still enables when scheduling the sync service fails" test.</comment>
<file context>
@@ -78,21 +81,40 @@ describe('registerReflexCommands', () => {
+ expect(outputLines(deps)).toContain('Reflex is on.');
+ });
+
+ it('reflex off writes disabled state, removes the push service, and prints confirmation', async () => {
const { program, deps } = createHarness();
</file context>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/cli/src/cli/commands/reflex.ts (2)
138-165: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider adding a timeout to
ai-histsubprocess calls.
execFileAsync('ai-hist', args)has no timeout, so if the externalai-histbinary hangs,reflex on/reflex offwill block indefinitely with no feedback to the user.♻️ Proposed fix
- await execFileAsync('ai-hist', args); + await execFileAsync('ai-hist', args, { timeout: 30_000 });Also applies to: 167-176
🤖 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 `@packages/cli/src/cli/commands/reflex.ts` around lines 138 - 165, The `defaultInstallCloudSync` flow currently calls `execFileAsync('ai-hist', args)` without any timeout, so a hung `ai-hist` process can block `reflex on`/`reflex off` indefinitely. Update the `execFileAsync` invocation (and any other `ai-hist` subprocess calls in this area) to enforce a reasonable timeout and surface a clear failure through `aiHistFailure`, so the user gets a deterministic error instead of a stuck command.
187-204: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant placeholder default for
cloudSyncInstalled.The inline
cloudSyncInstalled: () => falseis always overwritten — either byoverrides.cloudSyncInstalledvia the spread, or by the subsequentif (!overrides.cloudSyncInstalled)block. It never survives as the final value, so it's dead code that adds a bit of reading friction.♻️ Optional simplification
const deps: ReflexDependencies = { fs, homedir: os.homedir, readRelayAuth: defaultReadRelayAuth, loginToCloud: defaultLoginToCloud, prompt: promptYesNo, log: (...args: unknown[]) => console.log(...args), installCloudSync: defaultInstallCloudSync, uninstallCloudSync: defaultUninstallCloudSync, - cloudSyncInstalled: () => false, + cloudSyncInstalled: overrides.cloudSyncInstalled ?? (() => false), ...overrides, }; if (!overrides.cloudSyncInstalled) { // Probe the launchd plist using the same homedir the rest of the deps use. deps.cloudSyncInstalled = () => defaultCloudSyncInstalled(deps.fs, deps.homedir); }🤖 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 `@packages/cli/src/cli/commands/reflex.ts` around lines 187 - 204, The withDefaults helper in reflex.ts has a redundant placeholder for cloudSyncInstalled that is always replaced and should be removed. Update the ReflexDependencies construction in withDefaults so it no longer sets the inline false stub, and keep the existing override spread plus the fallback assignment that uses defaultCloudSyncInstalled when overrides.cloudSyncInstalled is missing.
🤖 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 `@packages/cli/src/cli/commands/reflex.ts`:
- Around line 178-185: `defaultCloudSyncInstalled()` currently hardcodes `false`
for every non-darwin platform, which makes `reflex status` report the cloud push
service as unscheduled on Linux even when the cron job exists. Update the
platform check in `reflex.ts` so Linux also performs a real installation probe,
using the existing cron-based setup or an `ai-hist` status command rather than
returning false unconditionally. Keep the macOS LaunchAgents check as-is, but
branch Linux separately and make the result reflect the actual cloud sync state.
---
Nitpick comments:
In `@packages/cli/src/cli/commands/reflex.ts`:
- Around line 138-165: The `defaultInstallCloudSync` flow currently calls
`execFileAsync('ai-hist', args)` without any timeout, so a hung `ai-hist`
process can block `reflex on`/`reflex off` indefinitely. Update the
`execFileAsync` invocation (and any other `ai-hist` subprocess calls in this
area) to enforce a reasonable timeout and surface a clear failure through
`aiHistFailure`, so the user gets a deterministic error instead of a stuck
command.
- Around line 187-204: The withDefaults helper in reflex.ts has a redundant
placeholder for cloudSyncInstalled that is always replaced and should be
removed. Update the ReflexDependencies construction in withDefaults so it no
longer sets the inline false stub, and keep the existing override spread plus
the fallback assignment that uses defaultCloudSyncInstalled when
overrides.cloudSyncInstalled is missing.
🪄 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: 265733fa-47ad-4f52-a839-2a5f09615406
📒 Files selected for processing (2)
packages/cli/src/cli/commands/reflex.test.tspackages/cli/src/cli/commands/reflex.ts
| function defaultCloudSyncInstalled(fsImpl: typeof fs, homedir: () => string): boolean { | ||
| if (process.platform !== 'darwin') { | ||
| // On Linux the push job lives in crontab; we don't shell out just to report | ||
| // status, so report unknown (false) rather than guess. | ||
| return false; | ||
| } | ||
| return fsImpl.existsSync(path.join(homedir(), 'Library', 'LaunchAgents', 'com.ai-hist.push.plist')); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether ai-hist exposes a status/introspection command usable for Linux detection.
rg -n "ai-hist" --type=ts -C3
rg -n "crontab|cron" --type=ts -C2Repository: AgentWorkforce/relay
Length of output: 158
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and inspect the surrounding implementation.
ast-grep outline packages/cli/src/cli/commands/reflex.ts --view expanded || true
sed -n '1,260p' packages/cli/src/cli/commands/reflex.ts | cat -nRepository: AgentWorkforce/relay
Length of output: 13122
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the status command and related control flow.
sed -n '260,420p' packages/cli/src/cli/commands/reflex.ts | cat -nRepository: AgentWorkforce/relay
Length of output: 2283
cloudSyncInstalled() needs a Linux probe
packages/cli/src/cli/commands/reflex.ts:178-185 returns false on every non-macOS platform, so reflex status will always report “Cloud push service: not scheduled” on Linux even after reflex on installs the cron job. Check the cron entry or an ai-hist status command instead of hardcoding false.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFile } from 'node:child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 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 `@packages/cli/src/cli/commands/reflex.ts` around lines 178 - 185,
`defaultCloudSyncInstalled()` currently hardcodes `false` for every non-darwin
platform, which makes `reflex status` report the cloud push service as
unscheduled on Linux even when the cron job exists. Update the platform check in
`reflex.ts` so Linux also performs a real installation probe, using the existing
cron-based setup or an `ai-hist` status command rather than returning false
unconditionally. Keep the macOS LaunchAgents check as-is, but branch Linux
separately and make the result reflect the actual cloud sync state.
|
✅ pr-reviewer applied fixes — committed and pushed Both changed files are formatting-only edits. Let me review the overall logic quality to leave any advisory notes, then write the review. Review of PR #1233:
|
…hell-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>
|
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
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)
packages/cli/src/cli/commands/reflex.ts (1)
159-174: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCloud-sync message is unconditional even when login is skipped/failed.
Lines 171-173 always claim history "syncs to relayhistory-cloud automatically," even in the branches above where the user isn't logged in or cloud login failed. This contradicts the warnings just printed and misleads users about whether their data is actually being synced.
🐛 Proposed fix
const relayAuth = await deps.readRelayAuth(); + let cloudSyncActive = false; if (!relayAuth) { deps.log( 'Not logged in to Agent Relay. Run `agent-relay login` first to sync Reflex history to the cloud.' ); } else { const result = await deps.loginToCloud(relayAuth.accessToken); if (!result.ok) { deps.log(`Reflex is enabled locally, but cloud login did not complete: ${result.error}`); + } else { + cloudSyncActive = true; } } deps.log('Reflex is on.'); - deps.log('History syncs to relayhistory-cloud automatically while `agent-relay up` is running.'); + if (cloudSyncActive) { + deps.log('History syncs to relayhistory-cloud automatically while `agent-relay up` is running.'); + } deps.log('State file: ~/.agentworkforce/reflex.json');🤖 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 `@packages/cli/src/cli/commands/reflex.ts` around lines 159 - 174, The sync status message in the Reflex command is unconditional and can contradict the login warnings. Update the logic in the `reflex` command so the history-sync message is only shown when `deps.readRelayAuth()` succeeds and `deps.loginToCloud()` returns ok, and use a separate fallback message when cloud login is skipped or fails. Keep the main flow around `readRelayAuth`, `loginToCloud`, and the final `deps.log` calls consistent with the user’s actual sync state.
🤖 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 `@packages/cli/package.json`:
- Line 57: The dependency declaration for ai-hist in the CLI package is pinned
to a version that is not available on the default npm registry. Update the
ai-hist entry in package.json to a published version that exists on npm, or
otherwise ensure the referenced 0.4.0 release is published and resolvable before
merging; use the ai-hist dependency key in the CLI package manifest to locate
and adjust it.
In `@packages/cli/src/cli/lib/reflex-capture.ts`:
- Around line 75-108: The capture loop in startReflexCapture only checks
deps.isEnabled() once, so “reflex off” does not stop an already-running
interval. Update tick() to re-check deps.isEnabled() before calling deps.push(),
and skip/log nothing when disabled mid-run; keep the existing stop handling and
inFlight dedup logic intact. Use the startReflexCapture and tick symbols to
locate the change, and add a reflex-capture.test.ts case for disabling after
startup to verify no further pushes occur.
---
Outside diff comments:
In `@packages/cli/src/cli/commands/reflex.ts`:
- Around line 159-174: The sync status message in the Reflex command is
unconditional and can contradict the login warnings. Update the logic in the
`reflex` command so the history-sync message is only shown when
`deps.readRelayAuth()` succeeds and `deps.loginToCloud()` returns ok, and use a
separate fallback message when cloud login is skipped or fails. Keep the main
flow around `readRelayAuth`, `loginToCloud`, and the final `deps.log` calls
consistent with the user’s actual sync state.
🪄 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: c676e5fc-cfbd-4308-b2b8-81603ea7c15f
📒 Files selected for processing (9)
packages/cli/package.jsonpackages/cli/scripts/build-cjs.mjspackages/cli/src/cli/commands/reflex.test.tspackages/cli/src/cli/commands/reflex.tspackages/cli/src/cli/lib/broker-lifecycle.tspackages/cli/src/cli/lib/reflex-capture.test.tspackages/cli/src/cli/lib/reflex-capture.tspackages/config/src/index.tspackages/config/src/reflex-config.ts
There was a problem hiding this comment.
3 issues found across 9 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="packages/cli/src/cli/commands/reflex.test.ts">
<violation number="1" location="packages/cli/src/cli/commands/reflex.test.ts:111">
P3: The `reflex off` test only covers the happy path where `uninstallCloudSync` succeeds. The implementation also handles failure (`!removal.ok`) by logging a distinct warning message, but that branch is untested. Adding a test with a failing mock (e.g., `{ ok: false, error: 'ai-hist was not found on your PATH.' }`) would close the gap and mirror the coverage pattern already used for `installCloudSync` in the "reflex on still enables when scheduling the sync service fails" test.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
- 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>
|
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. |
|
Thanks all — addressed the feedback in CI root cause (all install/test jobs): Addressed on the current (in-process SDK) code:
Not applicable: several comments (execFileAsync timeout, |
|
I made no code edits — the PR is clean, well-tested, and all mechanical checks pass. Here is my review. Review: PR #1233 —
|
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>
|
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. |
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>
|
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.
Actionable comments posted: 1
🤖 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 `@packages/cli/src/cli/lib/reflex-capture.ts`:
- Around line 47-119: Add a timeout to the process launched by runAiHist so hung
ai-hist sync/push invocations can’t keep the promise pending forever and block
later capture ticks or stop(). Update the spawnFn call inside runAiHist to pass
a timeout option supported by the Node 20+ runtime, and make sure the existing
error/close handling still resolves or rejects correctly for ReflexPushOptions
and reflexSyncAndPush.
🪄 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: a7386155-f7f9-4cb6-ba5a-38d195a6880c
📒 Files selected for processing (4)
packages/cli/src/cli/lib/ai-hist-path.test.tspackages/cli/src/cli/lib/ai-hist-path.tspackages/cli/src/cli/lib/reflex-capture.test.tspackages/cli/src/cli/lib/reflex-capture.ts
There was a problem hiding this comment.
2 issues found across 4 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="packages/cli/src/cli/commands/reflex.test.ts">
<violation number="1" location="packages/cli/src/cli/commands/reflex.test.ts:111">
P3: The `reflex off` test only covers the happy path where `uninstallCloudSync` succeeds. The implementation also handles failure (`!removal.ok`) by logging a distinct warning message, but that branch is untested. Adding a test with a failing mock (e.g., `{ ok: false, error: 'ai-hist was not found on your PATH.' }`) would close the gap and mirror the coverage pattern already used for `installCloudSync` in the "reflex on still enables when scheduling the sync service fails" test.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
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>
|
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 4 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="packages/cli/src/cli/commands/reflex.test.ts">
<violation number="1" location="packages/cli/src/cli/commands/reflex.test.ts:111">
P3: The `reflex off` test only covers the happy path where `uninstallCloudSync` succeeds. The implementation also handles failure (`!removal.ok`) by logging a distinct warning message, but that branch is untested. Adding a test with a failing mock (e.g., `{ ok: false, error: 'ai-hist was not found on your PATH.' }`) would close the gap and mirror the coverage pattern already used for `installCloudSync` in the "reflex on still enables when scheduling the sync service fails" test.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
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>
|
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. |
…osync # Conflicts: # package-lock.json # packages/cli/src/cli/lib/broker-lifecycle.ts
|
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. |
…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>
|
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. |
|
Went through all the review threads. This PR evolved through several approaches (shell-out → ai-hist SDK → bundled binary spawn → in-process napi addon), so most inline comments target code from earlier commits that no longer exists. Addressed (current code):
Already handled in the current code (no change needed):
Moot — target deleted code (the shell-out / bundled-binary-spawn eras):
The final design is: |
There was a problem hiding this comment.
1 issue found across 1 file (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="packages/cli/src/cli/lib/reflex-capture.ts">
<violation number="1" location="packages/cli/src/cli/lib/reflex-capture.ts:62">
P2: The broad `MODULE_NOT_FOUND` / `ERR_MODULE_NOT_FOUND` catch returns `null` for *any* module resolution failure, not just when `ai-hist-native` itself is absent. This means a broken installation (e.g., missing native binding, missing transitive dependency, corrupted `node_modules`) would silently no-op, contradicting the adjacent comment that says ABI mismatches and addon init failures should be rethrown so the caller logs them.
Node.js throws these same error codes both for top-level package absence and for nested internal requires within an already-resolved package. Consider narrowing the check so it only swallows errors that are specifically about the `ai-hist-native` specifier itself — for example, by also inspecting the error message for `ai-hist-native` or `requireStack` — so that real broken-install problems still surface.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| // mismatch, missing system lib, addon init failure) is a real problem — | ||
| // rethrow so the caller logs it instead of silently doing nothing. | ||
| const code = (err as NodeJS.ErrnoException | undefined)?.code; | ||
| if (code === 'ERR_MODULE_NOT_FOUND' || code === 'MODULE_NOT_FOUND') { |
There was a problem hiding this comment.
P2: The broad MODULE_NOT_FOUND / ERR_MODULE_NOT_FOUND catch returns null for any module resolution failure, not just when ai-hist-native itself is absent. This means a broken installation (e.g., missing native binding, missing transitive dependency, corrupted node_modules) would silently no-op, contradicting the adjacent comment that says ABI mismatches and addon init failures should be rethrown so the caller logs them.
Node.js throws these same error codes both for top-level package absence and for nested internal requires within an already-resolved package. Consider narrowing the check so it only swallows errors that are specifically about the ai-hist-native specifier itself — for example, by also inspecting the error message for ai-hist-native or requireStack — so that real broken-install problems still surface.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/cli/src/cli/lib/reflex-capture.ts, line 62:
<comment>The broad `MODULE_NOT_FOUND` / `ERR_MODULE_NOT_FOUND` catch returns `null` for *any* module resolution failure, not just when `ai-hist-native` itself is absent. This means a broken installation (e.g., missing native binding, missing transitive dependency, corrupted `node_modules`) would silently no-op, contradicting the adjacent comment that says ABI mismatches and addon init failures should be rethrown so the caller logs them.
Node.js throws these same error codes both for top-level package absence and for nested internal requires within an already-resolved package. Consider narrowing the check so it only swallows errors that are specifically about the `ai-hist-native` specifier itself — for example, by also inspecting the error message for `ai-hist-native` or `requireStack` — so that real broken-install problems still surface.</comment>
<file context>
@@ -46,18 +46,26 @@ export interface NativeAiHist {
+ // mismatch, missing system lib, addon init failure) is a real problem —
+ // rethrow so the caller logs it instead of silently doing nothing.
+ const code = (err as NodeJS.ErrnoException | undefined)?.code;
+ if (code === 'ERR_MODULE_NOT_FOUND' || code === 'MODULE_NOT_FOUND') {
+ return null;
+ }
</file context>
1 participant
Why
agent-relay reflex onwrote a flag and did a one-time login but nothing ever pushed history — the flag was read by nothing at runtime.What
The long-running
agent-relay uphost now periodically pushes new local history to relayhistory-cloud in-process, gated on the reflex flag. It does this via theai-histSDK'spushToCloud, which drives the realai-hist pushRust binary (relayhistory#38) — no CLI shell-out to schedule launchd/cron, no TypeScript re-port of the push logic.@agent-relay/config→reflex-config.ts: single source of truth for~/.agentworkforce/reflex.json(isReflexEnabled, read/write).reflex.tsuses it.reflex onpersists therth_at_session where the binary reads it —$RELAYHISTORY_HOME/auth.json(default~/.agentworkforce/relayhistory/auth.json), Rust snake_case shape — so the spawnedai-hist pushauthenticates. Only prints "History syncs automatically…" when cloud login actually succeeded.reflex-capture.ts: unref'd periodic loop (mirrors the telemetry client) started after the fleet sidecar and stopped (final flush) inshutdownOnce. Re-checksisEnabled()each tick soreflex off/ontake effect immediately; interval starts only after the initial delay. Loadsai-hist/cloudvia a lazy dynamic import → silent no-op if the SDK/binary is unavailable or unauthed.Dependency / ordering
ai-histis intentionally not a hard dependency yet:ai-hist@0.4.0(relayhistory#38, which shipspushToCloud) isn't published, and pinning an unpublished version brokenpm ci(ETARGET) across all CI. The capture loop reaches it via the lazy dynamic import and no-ops when absent. Once 0.4.0 is published, a follow-up addsai-hist@^0.4.0as a dependency so real installs pick it up (needs theai-histbinary present too, via install.sh / PATH).Testing
vitestreflex + reflex-capture ✅ 14 passedtsc(cli + config) ✅,eslint✅,prettier --check✅🤖 Generated with Claude Code