Skip to content

feat: add datasource apify connect command#179

Merged
CarmenDou merged 8 commits into
mainfrom
feat/apify-integration
Jun 26, 2026
Merged

feat: add datasource apify connect command#179
CarmenDou merged 8 commits into
mainfrom
feat/apify-integration

Conversation

@CarmenDou

@CarmenDou CarmenDou commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Summary by cubic

Add a new datasource CLI 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 --json and --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 in apify and install apify/agent-skills (non-blocking; suggests login on failure).
    • insforge datasource apify login: headless setup using the InsForge-managed token. Installs apify-cli if missing, runs apify login --token, and installs apify/agent-skills. Use this to fix Apify 401s.
  • Bug Fixes

    • Hardened auth bridge: Windows-safe spawn (shell: true), 5m per-step timeout, token charset validation to block shell metacharacters, and login verification via ~/.apify/auth.json. Sets APIFY_TOKEN for child processes.
    • Clear 404 handling for the token endpoint: distinguishes “not connected” (prompts 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.
    • Connect now warns when apify/agent-skills fails to install instead of implying success.

Written for commit 086b722. Summary will update on new commits.

Review in cubic

Note

Add datasource apify connect CLI command for OAuth-based Apify account linking

  • Adds a new datasource top-level CLI group with an apify subgroup, exposing connect and login subcommands in src/commands/datasource/.
  • datasource apify connect starts an OAuth flow via Apify's CLI API, optionally opens a browser, and polls up to 15 minutes for connection confirmation; supports --skip-browser and --json flags.
  • datasource apify login performs headless Apify CLI authentication using an InsForge-managed token and installs Apify skills without opening a browser.
  • Adds runApifyAuthBridge which installs apify-cli if missing, logs in with the managed token, verifies persistence via ~/.apify/auth.json, and sets APIFY_TOKEN in the process environment.
  • Extends ossFetch to return a clear error for /api/datasources routes on self-hosted backends, where Apify is cloud-only.

Changes since #179 opened

  • Added HTTP 401 unauthorized response handling to Apify connection workflows [4f04500]
  • Modified token validation in isApifyLoggedIn to use exact JSON field matching [4f04500]
  • Added token character set validation to runApifyAuthBridge [4f04500]
  • Added test coverage for Apify API connection utilities [4f04500]
  • Added test coverage for Apify bridge token validation [4f04500]
  • Updated comments in registerApifyConnectCommand catch block [4f04500]
  • Added skill installation status feedback to runConnect command handler in the datasource apify connect command [086b722]

Macroscope summarized 3ae6dfc.

Summary by CodeRabbit

  • New Features
    • Added a new datasource command group with Apify connect and login.
    • Introduced an OAuth-based Apify connection flow with status output, optional browser skipping, and backend verification/polling.
    • Automatically attempts to set up the required Apify skills during login/connect.
  • Bug Fixes
    • Improved error handling for forbidden/not-connected and unexpected response shapes.
    • Ensures clean shutdown of analytics and reliable termination on all outcomes (including aborts).
  • Documentation
    • Updated README with a Data Sources section and Apify command instructions.
  • Tests
    • Added coverage for token retrieval and “not connected” error behavior.

@coderabbitai

coderabbitai Bot commented Jun 24, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

Adds Apify datasource helpers, login/connect commands, CLI wiring, and README guidance for the new datasource apify flows.

Changes

Apify datasource flow

Layer / File(s) Summary
Apify API helpers
src/lib/api/apify.ts
fetchApifyConnection, pollApifyConnection, startApifyCliFlow, and sleep add timeout-bound Apify requests, tagged connection results, polling retries, and CLI-start response handling.
Token fetch and auth bridge
src/lib/api/apify-token.ts, src/lib/apify-bridge.ts, src/lib/skills.ts, src/lib/skills.test.ts, src/lib/api/apify-token.test.ts
Apify access-token retrieval, CLI login verification, provider skill-pack installation, the PROVIDER_SKILLS.apify mapping, and the related tests are added together.
Connect command workflow
src/commands/datasource/apify/connect.ts
The new connect subcommand validates project linkage and tokens, tracks analytics, resolves existing or newly created Apify connections, and renders JSON or interactive output.
Login command workflow
src/commands/datasource/apify/login.ts
The new login subcommand runs the Apify auth bridge in JSON or human-readable mode and routes failures through shared error handling.
Datasource registration and docs
src/commands/datasource/index.ts, src/index.ts, README.md
The new datasource command group is registered at the CLI root, the Apify subcommands are wired in, and the README documents the new Apify datasource commands.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • InsForge/CLI#119: Extends the same provider-based skill-installation pattern used here in src/lib/skills.ts.

Suggested reviewers

  • jwfing

Poem

A rabbit hopped through CLI trees,
With Apify breeze and JSON ease. 🐰
It polled till connections chimed bright,
Then bounded away in spinner-light.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 65.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly matches the main change by adding the new Apify datasource connect command, though it omits the broader datasource and login additions.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/apify-integration

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@greptile-apps

greptile-apps Bot commented Jun 24, 2026

Copy link
Copy Markdown

Greptile Summary

This PR adds a datasource CLI group with Apify as the first provider, exposing connect (OAuth flow + polling) and login (headless token auth) subcommands, plus the shared runApifyAuthBridge that installs the Apify CLI and agent skills.

  • datasource apify connect starts the OAuth flow via /integrations/apify/v1/cli-start, opens a browser, then polls /integrations/apify/v1/connection every 2 s (15 min timeout, 5-transient-retry budget) until connected; 401/403 short-circuit immediately. Auth bridge runs automatically but failures are non-fatal with a clear warning.
  • datasource apify login is a headless path using the InsForge-managed token — installs apify-cli if absent, runs apify login --token, verifies success via ~/.apify/auth.json (not exit code), and installs apify/agent-skills.
  • installProviderSkillPack is extracted from installSkills and now returns a boolean, letting callers distinguish a failed skills install from a successful auth and warn the user accurately.

Confidence Score: 5/5

Safe 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 run.

Important Files Changed

Filename Overview
src/commands/datasource/apify/connect.ts New connect command: OAuth flow with browser open, 15-minute polling, gracefully degraded auth bridge, and well-structured error handling for all HTTP status variants. Previously reported issues are all addressed.
src/commands/datasource/apify/login.ts New headless login command: delegates to runApifyAuthBridge and correctly branches on skillsInstalled vs thrown error for user messaging.
src/lib/api/apify.ts Core API layer: fetchApifyConnection, pollApifyConnection, and startApifyCliFlow with correct >= threshold for retry limit and explicit short-circuit on 401/403. Minor: abort-during-fetch is absorbed as a transient error rather than propagating immediately.
src/lib/apify-bridge.ts Auth bridge: charset-validates the token before shell use, verifies login via ~/.apify/auth.json rather than exit codes, and returns skillsInstalled boolean. The run helper's rejection messages include full args, so the token would appear in any unhandled future re-throw.
src/lib/api/apify-token.ts Token fetch layer: correctly distinguishes resource-level 404 (not_connected) from route-level 404 (backend unsupported) and remaps only the former.
src/lib/skills.ts Adds installProviderSkillPack returning a boolean and wires apify into PROVIDER_SKILLS. installSkills delegates to it, preserving existing behaviour.
src/lib/api/oss.ts Adds route-level 404 rewrite for /api/datasources paths to surface a clear cloud-only message instead of a generic 404.
src/commands/datasource/index.ts Registers datasource command group with apify subgroup; mirrors existing payments pattern cleanly.

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
Loading
%%{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
Loading

Reviews (7): Last reviewed commit: "fix: warn on skills-install failure duri..." | Re-trigger Greptile

Comment thread src/lib/api/apify.ts Outdated
Comment thread src/commands/datasource/apify/connect.ts
Comment thread src/lib/api/apify.ts

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 13a7935 and 1071b0d.

📒 Files selected for processing (4)
  • src/commands/datasource/apify/connect.ts
  • src/commands/datasource/index.ts
  • src/index.ts
  • src/lib/api/apify.ts

Comment thread src/commands/datasource/apify/connect.ts
Comment thread src/lib/api/apify.ts

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread src/lib/api/apify.ts Outdated
Comment thread src/lib/api/apify.ts Outdated
}

try {
const connection = await pollApifyConnection(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

Comment thread src/lib/api/apify.ts
Comment thread src/commands/datasource/apify/connect.ts
Comment thread src/commands/datasource/apify/connect.ts
Comment thread src/lib/api/apify.ts

// Wraps fetch with a per-request 30s timeout. If `callerSignal` aborts, the
// fetch aborts too. Always clears the timeout on completion.
async function fetchWithTimeout(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (1)
src/lib/api/apify-token.test.ts (1)

23-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert the structured 404 contract, not just the message.

handleError() uses CLIError.code in 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 assert code === 'APIFY_NOT_CONNECTED' (and ideally statusCode === 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

📥 Commits

Reviewing files that changed from the base of the PR and between d0a085e and 1074fe9.

📒 Files selected for processing (9)
  • README.md
  • src/commands/datasource/apify/connect.ts
  • src/commands/datasource/apify/login.ts
  • src/commands/datasource/index.ts
  • src/lib/api/apify-token.test.ts
  • src/lib/api/apify-token.ts
  • src/lib/apify-bridge.ts
  • src/lib/skills.test.ts
  • src/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

Comment thread src/lib/api/apify-token.ts
Comment thread src/lib/apify-bridge.ts
Comment thread src/lib/skills.ts Outdated

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread src/lib/apify-bridge.ts
// 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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

Comment thread src/lib/skills.ts Outdated
Comment thread src/lib/apify-bridge.ts Outdated
let res: Response;
try {
res = await ossFetch('/api/datasources/apify/token');
} catch (err) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

Comment thread src/lib/api/apify-token.ts Outdated
Comment thread src/lib/apify-bridge.ts Outdated

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread src/lib/apify-bridge.ts
Comment thread src/lib/api/apify-token.ts Outdated

@jwfing jwfing left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-180pollApifyConnection's state machine is exactly the kind of logic tests pay off on: transient-error counter increments on error, resets on not-connected, forbidden short-circuits, timeout/abort paths, and revokednot-connected remapping in fetchApifyConnection (src/lib/api/apify.ts:138-152). A regression here is silent today.
  • src/lib/apify-bridge.tsrunApifyAuthBridge's token charset guard and the ~/.apify/auth.json login 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, the run helper). 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-46shutdownAnalytics() is called in both catch and finally. It isn't actually invoked twice (the catch path ends in handleErrorprocess.exit, which skips finally), and the comment explains the intent, but the duplication reads as suspicious; a one-line note that handleError exits would save the next reader a double-take. Separately, login.ts does no trackGroupCommand/shutdownAnalytics — likely intentional, just inconsistent with connect.ts.
  • src/lib/apify-bridge.ts isApifyLoggedIn — verifies login via raw.includes(token) (substring match on the raw auth.json text). Fine for long Apify tokens, but a JSON.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> under shell: true on Windows) is charset-validated to ^[A-Za-z0-9_-]+$ before exec and passed as a discrete spawn arg, 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.ts refactor extracting installProviderSkillPack is justified by the new caller and preserves the prior installSkills behavior — 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.

Comment thread src/commands/datasource/apify/connect.ts

@jwfing jwfing left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - approved.

@CarmenDou
CarmenDou merged commit 28fc178 into main Jun 26, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants