feat: add datasource apify connect command#179
Conversation
WalkthroughAdds Apify datasource helpers, login/connect commands, CLI wiring, and README guidance for the new ChangesApify datasource flow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Greptile SummaryThis PR adds a
Confidence Score: 5/5Safe to merge — the OAuth flow, polling, auth bridge, and skills install are all well-guarded and previously reported blocking bugs have been addressed. The command surface is new and additive. The poll loop correctly uses >= for the retry threshold, 401/403 short-circuit rather than burning retries, the token charset is validated before any shell exec, and login verification reads auth.json instead of trusting unreliable exit codes. The two remaining notes are narrow edge cases that do not affect the common path. src/lib/api/apify.ts and src/lib/apify-bridge.ts: the former has a subtle abort-during-fetch / transient-retry edge case; the latter builds rejection messages that include the full argument list (token included), which is safe today but fragile for future callers of Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant User
participant CLI as insforge CLI
participant Cloud as Cloud Backend
participant Apify as apify.com
User->>CLI: datasource apify connect
CLI->>Cloud: GET /integrations/apify/v1/cli-start
alt Already connected
Cloud-->>CLI: type connected
CLI->>Cloud: GET /integrations/apify/v1/connection (verify)
Cloud-->>CLI: status active
else Needs OAuth
Cloud-->>CLI: type authorize + authorizeUrl
CLI->>User: Open browser / print URL
User->>Apify: Authorize OAuth
loop Poll every 2s up to 15min
CLI->>Cloud: GET /integrations/apify/v1/connection
Cloud-->>CLI: 404 not-connected or 200 active
end
end
CLI->>Cloud: GET /api/datasources/apify/token
Cloud-->>CLI: accessToken
CLI->>CLI: Validate token charset
CLI->>CLI: apify login --token
CLI->>CLI: Verify ~/.apify/auth.json
CLI->>CLI: npx skills add apify/agent-skills
CLI->>User: Connected
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant User
participant CLI as insforge CLI
participant Cloud as Cloud Backend
participant Apify as apify.com
User->>CLI: datasource apify connect
CLI->>Cloud: GET /integrations/apify/v1/cli-start
alt Already connected
Cloud-->>CLI: type connected
CLI->>Cloud: GET /integrations/apify/v1/connection (verify)
Cloud-->>CLI: status active
else Needs OAuth
Cloud-->>CLI: type authorize + authorizeUrl
CLI->>User: Open browser / print URL
User->>Apify: Authorize OAuth
loop Poll every 2s up to 15min
CLI->>Cloud: GET /integrations/apify/v1/connection
Cloud-->>CLI: 404 not-connected or 200 active
end
end
CLI->>Cloud: GET /api/datasources/apify/token
Cloud-->>CLI: accessToken
CLI->>CLI: Validate token charset
CLI->>CLI: apify login --token
CLI->>CLI: Verify ~/.apify/auth.json
CLI->>CLI: npx skills add apify/agent-skills
CLI->>User: Connected
Reviews (7): Last reviewed commit: "fix: warn on skills-install failure duri..." | Re-trigger Greptile |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/commands/datasource/apify/connect.ts`:
- Around line 44-58: The failed connect path is exiting before analytics can
flush, so the queued events are lost. In `connect.ts`, update the `runCommand`
flow around `runConnect`, `handleError`, and `shutdownAnalytics` so
`shutdownAnalytics()` is awaited inside the `catch` block before calling
`handleError(err, json)`, ensuring it runs even when `handleError` terminates
the process. Keep the existing `finally` cleanup for the success path, but make
the failure path flush analytics explicitly first.
In `@src/lib/api/apify.ts`:
- Around line 162-164: The timeout message in the Apify connection flow points
users to the wrong command path. Update the CLIError text in the Apify
connection logic to reference the actual registered command under datasource →
apify, so the hint matches the real invocation used by the apify connect flow.
Keep the message wording otherwise the same and make sure it correctly guides
users to rerun the proper connect command after authorizing.
🪄 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
Run ID: 864c44dd-e931-44f5-839e-91013512e449
📒 Files selected for processing (4)
src/commands/datasource/apify/connect.tssrc/commands/datasource/index.tssrc/index.tssrc/lib/api/apify.ts
There was a problem hiding this comment.
7 issues found across 4 files
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="src/commands/datasource/apify/connect.ts">
<violation number="1" location="src/commands/datasource/apify/connect.ts:195">
P2: Timeout guidance for the new Apify flow points to an outdated command (`insforge apify connect`), which can mislead users after a failed connect attempt.</violation>
</file>
<file name="src/lib/api/apify.ts">
<violation number="1" location="src/lib/api/apify.ts:8">
P3: This adds duplicated timeout/abort fetch logic that already exists in another API module, increasing maintenance drift risk.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| } | ||
|
|
||
| try { | ||
| const connection = await pollApifyConnection( |
There was a problem hiding this comment.
P2: Timeout guidance for the new Apify flow points to an outdated command (insforge apify connect), which can mislead users after a failed connect attempt.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/commands/datasource/apify/connect.ts, line 195:
<comment>Timeout guidance for the new Apify flow points to an outdated command (`insforge apify connect`), which can mislead users after a failed connect attempt.</comment>
<file context>
@@ -0,0 +1,229 @@
+ }
+
+ try {
+ const connection = await pollApifyConnection(
+ projectId,
+ token,
</file context>
|
|
||
| // Wraps fetch with a per-request 30s timeout. If `callerSignal` aborts, the | ||
| // fetch aborts too. Always clears the timeout on completion. | ||
| async function fetchWithTimeout( |
There was a problem hiding this comment.
P3: This adds duplicated timeout/abort fetch logic that already exists in another API module, increasing maintenance drift risk.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/lib/api/apify.ts, line 8:
<comment>This adds duplicated timeout/abort fetch logic that already exists in another API module, increasing maintenance drift risk.</comment>
<file context>
@@ -0,0 +1,272 @@
+
+// Wraps fetch with a per-request 30s timeout. If `callerSignal` aborts, the
+// fetch aborts too. Always clears the timeout on completion.
+async function fetchWithTimeout(
+ url: string,
+ init: RequestInit,
</file context>
…ath, retry threshold, forbidden/error handling, abort guard, flush analytics on error)
…and datasource CLI docs
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/lib/api/apify-token.test.ts (1)
23-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the structured 404 contract, not just the message.
handleError()usesCLIError.codein JSON mode. This test would still pass if the remap regressed back to a generic 404 as long as the message matched, so please also assertcode === 'APIFY_NOT_CONNECTED'(and ideallystatusCode === 404).🤖 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 `@src/lib/api/apify-token.test.ts` around lines 23 - 29, The apify-token test only checks the thrown message, so it can miss regressions in the structured 404 mapping. Update the `fetchApifyAccessToken` test in `apify-token.test.ts` to assert the rejected `CLIError` also has `code === 'APIFY_NOT_CONNECTED'` and `statusCode === 404`, alongside the existing message check, so the `handleError()` JSON contract is covered.
🤖 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 `@src/lib/api/apify-token.ts`:
- Around line 17-28: The apify-token fetch error handling is remapping every
CLIError 404 from ossFetch() to “Apify is not connected,” which incorrectly
hides route-level 404s. Update the catch block in getApifyToken so it only
translates the backend error that explicitly indicates a missing Apify
connection, and preserve the original CLIError 404 for unsupported or missing
routes. Use the existing CLIError handling in apify-token.ts and the
ossFetch('/api/datasources/apify/token') call to distinguish these cases before
throwing the fallback connect-first message.
In `@src/lib/apify-bridge.ts`:
- Around line 18-25: The run() helper in apify-bridge currently spawns child
processes without any timeout or abort support, so stalled Apify CLI commands
can hang indefinitely. Update run() to mirror the 60s lifecycle handling used in
skills.ts by wiring in a timeout/abort mechanism for spawn, and ensure both the
child-process error/close paths and timeout cleanup are handled in run() so
datasource apify login and the post-connect bridge can terminate cleanly.
In `@src/lib/skills.ts`:
- Around line 163-177: installProviderSkillPack currently catches install
failures and only logs them, which hides the failure from callers like
runApifyAuthBridge and connect. Update installProviderSkillPack in skills.ts to
either throw the caught error or return an explicit success/failure status after
the execAsync call so callers can detect the failed provider skill install. Then
adjust the Apify login flow and any other callers to handle that result and
avoid printing success messages when the skill pack did not install.
---
Nitpick comments:
In `@src/lib/api/apify-token.test.ts`:
- Around line 23-29: The apify-token test only checks the thrown message, so it
can miss regressions in the structured 404 mapping. Update the
`fetchApifyAccessToken` test in `apify-token.test.ts` to assert the rejected
`CLIError` also has `code === 'APIFY_NOT_CONNECTED'` and `statusCode === 404`,
alongside the existing message check, so the `handleError()` JSON contract is
covered.
🪄 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
Run ID: 7467c5a6-367c-4fb0-80f3-ad7098a1c70d
📒 Files selected for processing (9)
README.mdsrc/commands/datasource/apify/connect.tssrc/commands/datasource/apify/login.tssrc/commands/datasource/index.tssrc/lib/api/apify-token.test.tssrc/lib/api/apify-token.tssrc/lib/apify-bridge.tssrc/lib/skills.test.tssrc/lib/skills.ts
✅ Files skipped from review due to trivial changes (1)
- src/lib/skills.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/commands/datasource/index.ts
- src/commands/datasource/apify/connect.ts
There was a problem hiding this comment.
6 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="src/commands/datasource/apify/connect.ts">
<violation number="1" location="src/commands/datasource/apify/connect.ts:195">
P2: Timeout guidance for the new Apify flow points to an outdated command (`insforge apify connect`), which can mislead users after a failed connect attempt.</violation>
</file>
<file name="src/lib/api/apify.ts">
<violation number="1" location="src/lib/api/apify.ts:8">
P3: This adds duplicated timeout/abort fetch logic that already exists in another API module, increasing maintenance drift risk.</violation>
</file>
<file name="src/lib/api/apify-token.ts">
<violation number="1" location="src/lib/api/apify-token.ts:19">
P2: `installProviderSkillPack` swallows install errors silently (logs a warning only in non-JSON mode but never throws or returns a failure indicator). As a result, `runApifyAuthBridge()` resolves successfully and `login.ts` prints "Apify CLI authenticated and agent skills installed." even when the skill pack failed to install. Return a boolean or throw so callers can adjust their messaging.</violation>
</file>
<file name="src/lib/apify-bridge.ts">
<violation number="1" location="src/lib/apify-bridge.ts:79">
P2: Apify access token exposed in process list via `--token` CLI arg</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| // HARD REQ: always --token; never plain `apify login` (browser OAuth). | ||
| // Do not trust the exit code (see above) — verify with `apify info`. | ||
| try { | ||
| await run('apify', ['login', '--token', token], json); |
There was a problem hiding this comment.
P2: Apify access token exposed in process list via --token CLI arg
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/lib/apify-bridge.ts, line 79:
<comment>Apify access token exposed in process list via `--token` CLI arg</comment>
<file context>
@@ -0,0 +1,90 @@
+ // HARD REQ: always --token; never plain `apify login` (browser OAuth).
+ // Do not trust the exit code (see above) — verify with `apify info`.
+ try {
+ await run('apify', ['login', '--token', token], json);
+ } catch {
+ // fall through to verification
</file context>
| let res: Response; | ||
| try { | ||
| res = await ossFetch('/api/datasources/apify/token'); | ||
| } catch (err) { |
There was a problem hiding this comment.
P2: installProviderSkillPack swallows install errors silently (logs a warning only in non-JSON mode but never throws or returns a failure indicator). As a result, runApifyAuthBridge() resolves successfully and login.ts prints "Apify CLI authenticated and agent skills installed." even when the skill pack failed to install. Return a boolean or throw so callers can adjust their messaging.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/lib/api/apify-token.ts, line 19:
<comment>`installProviderSkillPack` swallows install errors silently (logs a warning only in non-JSON mode but never throws or returns a failure indicator). As a result, `runApifyAuthBridge()` resolves successfully and `login.ts` prints "Apify CLI authenticated and agent skills installed." even when the skill pack failed to install. Return a boolean or throw so callers can adjust their messaging.</comment>
<file context>
@@ -0,0 +1,40 @@
+ let res: Response;
+ try {
+ res = await ossFetch('/api/datasources/apify/token');
+ } catch (err) {
+ if (err instanceof CLIError && err.statusCode === 404) {
+ throw new CLIError(
</file context>
…ll result, 404 message)
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="src/commands/datasource/apify/connect.ts">
<violation number="1" location="src/commands/datasource/apify/connect.ts:195">
P2: Timeout guidance for the new Apify flow points to an outdated command (`insforge apify connect`), which can mislead users after a failed connect attempt.</violation>
</file>
<file name="src/lib/api/apify.ts">
<violation number="1" location="src/lib/api/apify.ts:8">
P3: This adds duplicated timeout/abort fetch logic that already exists in another API module, increasing maintenance drift risk.</violation>
</file>
<file name="src/lib/api/apify-token.ts">
<violation number="1" location="src/lib/api/apify-token.ts:19">
P2: `installProviderSkillPack` swallows install errors silently (logs a warning only in non-JSON mode but never throws or returns a failure indicator). As a result, `runApifyAuthBridge()` resolves successfully and `login.ts` prints "Apify CLI authenticated and agent skills installed." even when the skill pack failed to install. Return a boolean or throw so callers can adjust their messaging.</violation>
</file>
<file name="src/lib/apify-bridge.ts">
<violation number="1" location="src/lib/apify-bridge.ts:79">
P2: Apify access token exposed in process list via `--token` CLI arg</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
… misleading connect prompt on unsupported backends)
…aracter injection on Windows
jwfing
left a comment
There was a problem hiding this comment.
Code Review — feat: add datasource apify connect command
Summary: A well-structured, defensively-coded addition of a datasource apify command group (OAuth connect + headless login) with a shared auth bridge; the core connection/polling and shell-bridge logic ships largely untested, which is the main thing worth tightening before merge — but nothing blocking.
Requirements context
No /docs/superpowers/ directory exists in this repo; specs live under docs/specs/, and none of the three present (diagnose, db-migrations) match this PR's scope. No matching spec/plan found — assessed against the PR description, the payments/posthog command conventions, and the repo's insforge-development (TDD / scope discipline) skill.
Critical
(none)
Suggestion
Software engineering — core logic is untested (src/lib/api/apify.ts, src/lib/apify-bridge.ts)
The repo's dev convention is TDD ("every new behavior has at least one test"). apify-token.ts got a thorough 5-case suite, but the two more complex new modules have none:
src/lib/api/apify.ts:120-180—pollApifyConnection's state machine is exactly the kind of logic tests pay off on: transient-error counter increments onerror, resets onnot-connected,forbiddenshort-circuits, timeout/abort paths, andrevoked→not-connectedremapping infetchApifyConnection(src/lib/api/apify.ts:138-152). A regression here is silent today.src/lib/apify-bridge.ts—runApifyAuthBridge's token charset guard and the~/.apify/auth.jsonlogin verification (isApifyLoggedIn) are unverified. The charset guard is a security control (see below); a future loosening of the regex would go uncaught. These are pure-ish functions that mock cleanly (fetchApifyAccessToken,installProviderSkillPack, therunhelper). Recommend adding coverage for at least the poll state machine and the token-validation reject path.
Functionality — 401 during polling treated as transient (src/lib/api/apify.ts:167-176)
fetchApifyConnection maps any non-404/403 non-ok response to kind: 'error', so a 401 mid-poll is counted against maxTransientRetries and surfaces after 5 tries as a generic "Connection check failed after N retries" rather than an auth-specific "re-run insforge login" message. The 403 case is already special-cased; a 401 is similarly non-transient and could short-circuit with a clearer remediation.
Information
src/commands/datasource/apify/connect.ts:36-46—shutdownAnalytics()is called in bothcatchandfinally. It isn't actually invoked twice (thecatchpath ends inhandleError→process.exit, which skipsfinally), and the comment explains the intent, but the duplication reads as suspicious; a one-line note thathandleErrorexits would save the next reader a double-take. Separately,login.tsdoes notrackGroupCommand/shutdownAnalytics— likely intentional, just inconsistent withconnect.ts.src/lib/apify-bridge.tsisApifyLoggedIn— verifies login viaraw.includes(token)(substring match on the rawauth.jsontext). Fine for long Apify tokens, but aJSON.parse+ explicit field compare would be marginally more robust against false positives/format drift.- Security (positive): new user-influenced value reaching a shell (
apify login --token <token>undershell: trueon Windows) is charset-validated to^[A-Za-z0-9_-]+$before exec and passed as a discretespawnarg, not string-concatenated; the token is never logged or echoed into errors. This is the right pattern — good defensive work. - Performance (no concerns): polling is fully bounded (2s interval, 15m deadline, 30s per-request timeout, 5 consecutive-error cap); no N+1, unbounded loop, or blocking I/O on a hot path. No DB queries added.
- Scope: the
skills.tsrefactor extractinginstallProviderSkillPackis justified by the new caller and preserves the priorinstallSkillsbehavior — not scope creep.
Verdict
approved (informational; explicit GitHub approval remains a separate human action). No Critical findings — the implementation solves what the PR claims, with sound security and error handling. The main follow-up is test coverage for apify.ts and apify-bridge.ts to match the repo's TDD bar; treat that as a strong suggestion rather than a blocker.
…ll; robust login check
Summary by cubic
Add a new
datasourceCLI group with Apify integration. Connect your project via OAuth and auto-prepare the local agent with an InsForge‑managed token; warns if skills install fails.New Features
insforge datasource apify connect: guided OAuth with--jsonand--skip-browser. Skips if already connected (verifies via/connection). Polls every 2s up to 15m with transient retries and 30s per-request timeouts; short-circuits on 401/403. Runs an auth bridge to log inapifyand installapify/agent-skills(non-blocking; suggestsloginon failure).insforge datasource apify login: headless setup using the InsForge-managed token. Installsapify-cliif missing, runsapify login --token, and installsapify/agent-skills. Use this to fix Apify 401s.Bug Fixes
shell: true), 5m per-step timeout, token charset validation to block shell metacharacters, and login verification via~/.apify/auth.json. SetsAPIFY_TOKENfor child processes.connect) from route-level 404s (unsupported backend). Added polling abort guard, cancellation messaging, and avoid retrying 401s; maps forbidden/unauthorized cleanly. Analytics flushes on all error paths.apify/agent-skillsfails to install instead of implying success.Written for commit 086b722. Summary will update on new commits.
Note
Add
datasource apify connectCLI command for OAuth-based Apify account linkingdatasourcetop-level CLI group with anapifysubgroup, exposingconnectandloginsubcommands in src/commands/datasource/.datasource apify connectstarts an OAuth flow via Apify's CLI API, optionally opens a browser, and polls up to 15 minutes for connection confirmation; supports--skip-browserand--jsonflags.datasource apify loginperforms headless Apify CLI authentication using an InsForge-managed token and installs Apify skills without opening a browser.runApifyAuthBridgewhich installsapify-cliif missing, logs in with the managed token, verifies persistence via~/.apify/auth.json, and setsAPIFY_TOKENin the process environment.ossFetchto return a clear error for/api/datasourcesroutes on self-hosted backends, where Apify is cloud-only.Changes since #179 opened
isApifyLoggedInto use exact JSON field matching [4f04500]runApifyAuthBridge[4f04500]registerApifyConnectCommandcatch block [4f04500]runConnectcommand handler in thedatasource apify connectcommand [086b722]Macroscope summarized 3ae6dfc.
Summary by CodeRabbit
datasourcecommand group with Apifyconnectandlogin.