Skip to content

Conversation

@danielroe
Copy link
Member

@danielroe danielroe commented Jan 28, 2026

resolves #157

@vercel
Copy link

vercel bot commented Jan 28, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
npmx.dev Ready Ready Preview, Comment Feb 11, 2026 9:10am
2 Skipped Deployments
Project Deployment Actions Updated (UTC)
docs.npmx.dev Ignored Ignored Preview Feb 11, 2026 9:10am
npmx-lunaria Ignored Ignored Feb 11, 2026 9:10am

Request Review

# Conflicts:
#	CONTRIBUTING.md
#	package.json
#	playwright.config.ts
#	pnpm-lock.yaml
#	test/e2e/connector.spec.ts
#	test/e2e/global-setup.ts
#	test/e2e/global-teardown.ts
#	test/e2e/helpers/fixtures.ts
#	test/e2e/helpers/mock-connector-state.ts
#	test/e2e/helpers/mock-connector.ts
@codecov
Copy link

codecov bot commented Feb 10, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

📢 Thoughts on this report? Let us know!

@danielroe danielroe requested a review from serhalp February 10, 2026 13:25
@danielroe danielroe marked this pull request as ready for review February 10, 2026 13:25
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 10, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds a complete mock connector stack and test integration: a H3-based mock connector server with in-memory state and CLI entrypoint, a mock state manager, Playwright global setup and fixtures, E2E tests exercising connector-authenticated flows, Vitest component tests for the connector modal, test fixtures, and related TypeScript path and type mappings. Also updates build/test scripts, knip project exclusions, and Playwright configuration to wire the mock server into the test harness. No public API surface from existing runtime modules is removed; several new types and classes are exported for the mock/testing infrastructure.

Possibly related PRs

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description check ✅ Passed The PR description references a linked issue (#157) and is directly related to the changeset which implements a mock connector for E2E testing.
Linked Issues check ✅ Passed The PR successfully implements comprehensive mock connector infrastructure and E2E tests covering logged-in connector features, addressing issue #157's objective to add integration test coverage.
Out of Scope Changes check ✅ Passed All changes are scoped to mock connector infrastructure, E2E testing, configuration updates, and documentation—directly supporting the objective of testing logged-in connector features.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch test/mock-connector

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

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 6

🧹 Nitpick comments (7)
package.json (1)

128-129: Update h3-next to the latest v2 RC to avoid using an outdated pre-release.

The dual h3 versions (v1 and v2 RC) are intentionally isolated—v1 for the main codebase and v2 RC for the CLI (cli/src/server.ts) and E2E test helpers. This separation is sensible for gradual migration. However, h3@2.0.1-rc.11 (Jan 20, 2026) is outdated; newer RCs (rc.12, rc.13, rc.14) have been released since Feb 5, 2026. Consider updating to a more recent RC or documenting why rc.11 specifically is pinned (if there is a reason).

test/e2e/helpers/mock-connector.ts (1)

41-498: Split createMockConnectorApp into smaller route-registration helpers.

The function is far beyond the size guideline and is getting hard to navigate and extend safely.

♻️ Suggested extraction pattern
 function createMockConnectorApp(stateManager: MockConnectorStateManager) {
   const app = createApp()
   const router = createRouter()
+  registerAuthRoutes(router, stateManager)
+  registerOperationRoutes(router, stateManager)
+  registerOrgRoutes(router, stateManager)
+  registerUserRoutes(router, stateManager)
+  registerTestRoutes(router, stateManager)
-  // ...all routes inline...
   app.use(router.handler)
   return app
 }

As per coding guidelines "Keep functions focused and manageable (generally under 50 lines)".

test/e2e/connector.spec.ts (1)

177-177: Consider replacing waitForTimeout with a proper assertion or waitFor condition.

page.waitForTimeout(500) is a fixed delay that can cause flaky tests. Playwright recommends using expect(...).toBeVisible() or similar assertions that auto-retry.

♻️ Suggested approach
-    await page.waitForTimeout(500)
+    // Wait for operation to be registered by checking operations count
+    await expect(async () => {
+      const operations = await mockConnector.getOperations()
+      expect(operations).toHaveLength(1)
+    }).toPass({ timeout: 5000 })

Apply similar changes to other waitForTimeout calls at lines 194, 215, 301, 325, 394, and 403.

test/nuxt/components/HeaderConnectorModal.spec.ts (2)

12-12: Consider using the re-exported type from test-utils.

PendingOperation is re-exported from test/test-utils/mock-connector-types.ts, which provides a cleaner import path and ensures consistency with other test files.

♻️ Suggested change
-import type { PendingOperation } from '../../../cli/src/types'
+import type { PendingOperation } from '../../test-utils/mock-connector-types'

15-81: Inline mock duplicates shared mock-connector-composable logic.

The createMockUseConnector function replicates much of the interface defined in test/test-utils/mock-connector-composable.ts. Consider whether reusing createMockConnectorComposable from test-utils would reduce duplication and ensure consistency.

That said, for unit tests, a simpler inline mock may be preferable for isolation and clarity. This is acceptable as-is.

test/test-utils/mock-connector-composable.ts (2)

163-176: Consider aligning completedOperations and activeOperations logic with the unit test mock.

The logic here includes op.result?.requiresOtp checks for categorising operations, but the inline mock in HeaderConnectorModal.spec.ts uses simpler status-only checks. This inconsistency might cause subtle differences in test behaviour.


178-180: Type cast explanation would improve clarity.

The comment on line 179 explains the cast, but the as unknown as pattern is a code smell. Consider whether the interface could be adjusted to avoid the double cast.

Comment on lines 174 to 176
const query = getQuery(event)
const id = query.id as string
if (!id) {
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

cat -n test/e2e/helpers/mock-connector.ts | head -250 | tail -100

Repository: npmx-dev/npmx.dev

Length of output: 3569


🏁 Script executed:

rg "getQuery" test/e2e/helpers/mock-connector.ts -B 2 -A 2

Repository: npmx-dev/npmx.dev

Length of output: 532


🏁 Script executed:

rg "const getQuery" test/e2e/helpers/ -A 5

Repository: npmx-dev/npmx.dev

Length of output: 43


🏁 Script executed:

head -30 test/e2e/helpers/mock-connector.ts

Repository: npmx-dev/npmx.dev

Length of output: 776


🏁 Script executed:

rg "import.*getQuery" test/e2e/helpers/mock-connector.ts -A 2 -B 2

Repository: npmx-dev/npmx.dev

Length of output: 43


🌐 Web query:

h3-next getQuery return type documentation

💡 Result:

In h3-next / h3, getQuery(event) is typed to return ufo.QueryObject (i.e. the querystring parsed into an object). [1]

In practice, that object contains string values, and if a query key appears multiple times it becomes a string[] (e.g. { key: "value", key2: ["value1", "value2"] }). [2] This matches ufo’s query parsing behavior (first occurrence is a string; repeated keys accumulate into an array). [3]

So the effective shape is:

ufo.QueryObject  // roughly: Record<string, string | string[]>

Sources: [1] [2] [3]


Guard query parameters against array values to ensure type safety.

getQuery() returns ufo.QueryObject (effectively Record<string, string | string[]>), so query.id can be either string or string[]. The current cast as string bypasses type-checking and creates unsafe code. Extract a small helper to safely access single query parameters and reuse it for all id lookups.

Suggested fix
+  const getSingleQueryParam = (value: string | string[] | undefined) =>
+    Array.isArray(value) ? value[0] : value
   // ...
-      const id = query.id as string
+      const id = getSingleQueryParam(query.id)

This aligns with the coding guideline to ensure strictly type-safe code by checking when accessing array values.

Also applies to: lines 210–212, 246–248

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Comment on lines 210 to 215
const navigateConnected = async (path: string) => {
// Remove leading slash if present for clean URL construction
const cleanPath = path.startsWith('/') ? path : `/${path}`
const separator = cleanPath.includes('?') ? '&' : '?'
const urlWithParams = `${cleanPath}${separator}token=${testToken}&port=${connectorPort}`
await goto(urlWithParams, { waitUntil: 'networkidle' })
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

URL‑encode query parameters in gotoConnected.
If the token includes reserved characters, raw interpolation can break the query string. Using URLSearchParams avoids subtle failures.

🔧 Proposed fix
-      const urlWithParams = `${cleanPath}${separator}token=${testToken}&port=${connectorPort}`
+      const params = new URLSearchParams({
+        token: testToken,
+        port: String(connectorPort),
+      })
+      const urlWithParams = `${cleanPath}${separator}${params.toString()}`
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const navigateConnected = async (path: string) => {
// Remove leading slash if present for clean URL construction
const cleanPath = path.startsWith('/') ? path : `/${path}`
const separator = cleanPath.includes('?') ? '&' : '?'
const urlWithParams = `${cleanPath}${separator}token=${testToken}&port=${connectorPort}`
await goto(urlWithParams, { waitUntil: 'networkidle' })
const navigateConnected = async (path: string) => {
// Remove leading slash if present for clean URL construction
const cleanPath = path.startsWith('/') ? path : `/${path}`
const separator = cleanPath.includes('?') ? '&' : '?'
const params = new URLSearchParams({
token: testToken,
port: String(connectorPort),
})
const urlWithParams = `${cleanPath}${separator}${params.toString()}`
await goto(urlWithParams, { waitUntil: 'networkidle' })

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 4

🧹 Nitpick comments (4)
test/e2e/connector.spec.ts (1)

257-262: Consider extracting navigation pattern to reduce duplication.

The goToPackageConnected helper is specific to this describe block. If similar patterns emerge in other test files, consider moving it to the fixtures file.

cli/src/mock-app.ts (1)

45-416: Consider splitting createMockConnectorApp into smaller route modules.
The function is very large, which makes it harder to navigate and maintain; extracting route groups or handler helpers would improve clarity. As per coding guidelines, "Keep functions focused and manageable (generally under 50 lines)".

cli/src/mock-state.ts (1)

311-483: applyOperationEffect is too large; consider handler extraction.
A handler map or per-operation helper functions would keep this easier to reason about as operations grow. As per coding guidelines, "Keep functions focused and manageable (generally under 50 lines)".

test/e2e/helpers/fixtures.ts (1)

60-117: Use shared connector contract types in fixtures to avoid drift.
Inline unions and type: string can diverge from the contract; prefer shared types for roles, permissions, and operation bodies.

🛠️ Suggested refactor
 import { test as base } from '../test-utils'
 import { DEFAULT_MOCK_CONFIG } from './mock-connector'
+import type { AccessPermission, CreateOperationBody, OrgRole } from '../../../cli/src/types.ts'

@@
   async setOrgData(
     org: string,
     data: {
-      users?: Record<string, 'developer' | 'admin' | 'owner'>
+      users?: Record<string, OrgRole>
       teams?: string[]
       teamMembers?: Record<string, string[]>
     },
   ): Promise<void> {

@@
-  async setUserPackages(packages: Record<string, 'read-only' | 'read-write'>): Promise<void> {
+  async setUserPackages(packages: Record<string, AccessPermission>): Promise<void> {
     await this.testEndpoint('/__test__/user-packages', { packages })
   }

@@
   async setPackageData(
     pkg: string,
-    data: { collaborators?: Record<string, 'read-only' | 'read-write'> },
+    data: { collaborators?: Record<string, AccessPermission> },
   ): Promise<void> {

@@
-  async addOperation(operation: {
-    type: string
-    params: Record<string, string>
-    description: string
-    command: string
-    dependsOn?: string
-  }): Promise<{ id: string; status: string }> {
+  async addOperation(operation: CreateOperationBody): Promise<{ id: string; status: string }> {
As per coding guidelines, "Ensure you write strictly type-safe code, for example by ensuring you always check when accessing an array value by index".

Comment on lines 19 to 21
"exports": {
".": {
"import": "./dist/index.mjs",
"types": "./dist/index.d.mts"
}
".": "./dist/index.mjs"
},
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

cat -n cli/package.json | head -50

Repository: npmx-dev/npmx.dev

Length of output: 1657


🏁 Script executed:

rg "from\s+['\"]npmx-connector['\"]|import.*npmx-connector" --type ts --type js

Repository: npmx-dev/npmx.dev

Length of output: 43


🏁 Script executed:

fd tsconfig -E node_modules

Repository: npmx-dev/npmx.dev

Length of output: 92


🏁 Script executed:

ls -la cli/dist/ 2>/dev/null || echo "dist directory not found or empty"

Repository: npmx-dev/npmx.dev

Length of output: 94


🏁 Script executed:

git log --oneline -n 20 cli/package.json 2>/dev/null | head -10

Repository: npmx-dev/npmx.dev

Length of output: 136


🏁 Script executed:

git show HEAD:cli/package.json 2>/dev/null | sed -n '19,25p'

Repository: npmx-dev/npmx.dev

Length of output: 152


🏁 Script executed:

cat -n cli/tsconfig.json

Repository: npmx-dev/npmx.dev

Length of output: 532


🏁 Script executed:

git diff HEAD~1 cli/package.json 2>/dev/null || echo "Unable to get diff"

Repository: npmx-dev/npmx.dev

Length of output: 79


🏁 Script executed:

git status cli/package.json

Repository: npmx-dev/npmx.dev

Length of output: 126


🌐 Web query:

tsdown TypeScript declarations .d.ts output

💡 Result:

To make tsdown emit TypeScript declaration files (.d.ts):

  • Install TypeScript (required for d.ts generation). (tsdown.dev)
  • Enable dts output in one of these ways:
    • If your package.json has "types" / "typings", tsdown enables dts automatically. (tsdown.dev)
    • Or explicitly:
      • CLI: tsdown --dts (tsdown.dev)
      • Config:
        // tsdown.config.ts
        import { defineConfig } from 'tsdown'
        
        export default defineConfig({
          dts: true,
        })
        (tsdown.dev)

Common dts-related options/notes:

  • Declaration maps: set compilerOptions.declarationMap: true in tsconfig.json, or dts: { sourcemap: true } in tsdown config. (tsdown.dev)
  • tsdown generates/bundles .d.ts via rolldown-plugin-dts. (tsdown.dev)
  • Performance: if tsconfig.json enables isolatedDeclarations, tsdown uses a faster path; otherwise it falls back to the TypeScript compiler. (tsdown.dev)

Citations:


🏁 Script executed:

git log --all --oneline cli/package.json 2>/dev/null | head -20

Repository: npmx-dev/npmx.dev

Length of output: 1108


🏁 Script executed:

git show HEAD~5:cli/package.json 2>/dev/null | grep -A 5 "exports" || echo "Cannot access earlier commit"

Repository: npmx-dev/npmx.dev

Length of output: 89


🏁 Script executed:

fd tsdown.config -E node_modules

Repository: npmx-dev/npmx.dev

Length of output: 81


🏁 Script executed:

git show 588e1a1:cli/package.json 2>/dev/null | grep -A 5 "exports" || echo "Cannot access that commit"

Repository: npmx-dev/npmx.dev

Length of output: 169


🏁 Script executed:

git show 1f03429:cli/package.json 2>/dev/null | grep -A 5 "exports" || echo "Cannot access that commit"

Repository: npmx-dev/npmx.dev

Length of output: 169


🏁 Script executed:

cat -n cli/tsdown.config.ts

Repository: npmx-dev/npmx.dev

Length of output: 299


Restore types entry to exports object for TypeScript consumers.

The exports field was simplified from { "import": "./dist/index.mjs", "types": "./dist/index.d.mts" } to a single path. Since tsdown is configured to generate .d.ts files (dts: true), but the types entry is missing from exports, TypeScript consumers cannot resolve type definitions. Modern TypeScript respects the package.json exports field and requires explicit type entries. This breaks type resolution for external consumers of the published package. Restore the object format with separate import and types entries.

Comment on lines +133 to +147
// POST /operations/batch
app.post('/operations/batch', async (event: H3Event) => {
requireAuth(event)

const body = await event.req.json()
if (!Array.isArray(body)) {
throw new HTTPError({ statusCode: 400, message: 'Expected array of operations' })
}

const operations = stateManager.addOperations(body)
return {
success: true,
data: operations,
} satisfies ApiResponse<ConnectorEndpoints['POST /operations/batch']['data']>
})
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Validate batch operation payloads before enqueueing.
/operations/batch only checks for an array, so malformed entries can create incomplete operations and odd downstream behaviour. Mirror the single-operation validation for each entry.

🛠️ Suggested fix
     const body = await event.req.json()
     if (!Array.isArray(body)) {
       throw new HTTPError({ statusCode: 400, message: 'Expected array of operations' })
     }
+    if (body.some(op => !op?.type || !op?.description || !op?.command)) {
+      throw new HTTPError({ statusCode: 400, message: 'Missing required fields' })
+    }

     const operations = stateManager.addOperations(body)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// POST /operations/batch
app.post('/operations/batch', async (event: H3Event) => {
requireAuth(event)
const body = await event.req.json()
if (!Array.isArray(body)) {
throw new HTTPError({ statusCode: 400, message: 'Expected array of operations' })
}
const operations = stateManager.addOperations(body)
return {
success: true,
data: operations,
} satisfies ApiResponse<ConnectorEndpoints['POST /operations/batch']['data']>
})
// POST /operations/batch
app.post('/operations/batch', async (event: H3Event) => {
requireAuth(event)
const body = await event.req.json()
if (!Array.isArray(body)) {
throw new HTTPError({ statusCode: 400, message: 'Expected array of operations' })
}
if (body.some(op => !op?.type || !op?.description || !op?.command)) {
throw new HTTPError({ statusCode: 400, message: 'Missing required fields' })
}
const operations = stateManager.addOperations(body)
return {
success: true,
data: operations,
} satisfies ApiResponse<ConnectorEndpoints['POST /operations/batch']['data']>
})

Comment on lines +128 to +135
setOrgData(org: string, data: Partial<MockOrgData>): void {
const existing = this.state.orgs[org] ?? { users: {}, teams: [], teamMembers: {} }
this.state.orgs[org] = {
users: { ...existing.users, ...data.users },
teams: data.teams ?? existing.teams,
teamMembers: { ...existing.teamMembers, ...data.teamMembers },
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Normalise org keys on write to match read paths.
getOrgUsers/getOrgTeams normalise org names with @, but setOrgData stores raw keys, so unscoped inputs won’t be readable later.

🛠️ Suggested fix
   setOrgData(org: string, data: Partial<MockOrgData>): void {
-    const existing = this.state.orgs[org] ?? { users: {}, teams: [], teamMembers: {} }
-    this.state.orgs[org] = {
+    const normalizedOrg = org.startsWith('@') ? org : `@${org}`
+    const existing = this.state.orgs[normalizedOrg] ?? { users: {}, teams: [], teamMembers: {} }
+    this.state.orgs[normalizedOrg] = {
       users: { ...existing.users, ...data.users },
       teams: data.teams ?? existing.teams,
       teamMembers: { ...existing.teamMembers, ...data.teamMembers },
     }
   }

Comment on lines +798 to +807
```
cli/src/
├── types.ts # ConnectorEndpoints contract (shared by real + mock)
├── mock-state.ts # MockConnectorStateManager (canonical source)
├── mock-app.ts # H3 mock app + MockConnectorServer class
└── mock-server.ts # CLI entry point (pnpm mock-connector)

test/test-utils/ # Re-exports from cli/src/ for test convenience
test/e2e/helpers/ # E2E-specific wrappers (fixtures, global setup)
```
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Add language identifier to the fenced code block.

The code block showing the directory structure is missing a language identifier, which violates the markdownlint rule MD040. Add a language identifier to maintain consistency with the project's documentation standards.

📝 Proposed fix
-```
+```text
 cli/src/
 ├── types.ts           # ConnectorEndpoints contract (shared by real + mock)
 ├── mock-state.ts      # MockConnectorStateManager (canonical source)
🧰 Tools
🪛 markdownlint-cli2 (0.20.0)

[warning] 798-798: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

@danielroe danielroe changed the title test: mock connector for e2e (and browser) tests test: mock connector for e2e/browser/local dev Feb 11, 2026
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
cli/src/mock-state.ts (1)

311-482: applyOperationEffect is too large; split into per-operation handlers.

Consider extracting per-operation handlers (or a handler map) to keep this method under 50 lines and easier to scan. As per coding guidelines: "Keep functions focused and manageable (generally under 50 lines)".

Comment on lines +255 to +309
/** Execute all approved operations (mock: instant success unless configured otherwise). */
executeOperations(options?: ExecuteOptions): ExecuteResult {
const results: Array<{ id: string; result: OperationResult }> = []
const approved = this.state.operations.filter(op => op.status === 'approved')

// Sort by dependencies
const sorted = this.sortByDependencies(approved)

for (const op of sorted) {
// Check if dependent operation completed successfully
if (op.dependsOn) {
const dep = this.state.operations.find(d => d.id === op.dependsOn)
if (!dep || dep.status !== 'completed') {
// Skip - dependency not met
continue
}
}

op.status = 'running'

// Check for configured result
const configuredResult = options?.results?.[op.id]
if (configuredResult) {
const result: OperationResult = {
stdout: configuredResult.stdout ?? '',
stderr: configuredResult.stderr ?? '',
exitCode: configuredResult.exitCode ?? 1,
requiresOtp: configuredResult.requiresOtp,
authFailure: configuredResult.authFailure,
}
op.result = result
op.status = result.exitCode === 0 ? 'completed' : 'failed'
results.push({ id: op.id, result })

if (result.requiresOtp && !options?.otp) {
return { results, otpRequired: true }
}
} else {
// Default: success
const result: OperationResult = {
stdout: `Mock: ${op.command}`,
stderr: '',
exitCode: 0,
}
op.result = result
op.status = 'completed'
results.push({ id: op.id, result })

// Apply the operation's effects to mock state
this.applyOperationEffect(op)
}
}

return { results }
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

OTP-required operations get marked complete/failed too early.

Line 255-309 returns otpRequired after mutating status/result, so a later call with OTP won’t re-run the operation (it’s no longer approved). Short-circuit before mutating the op to preserve the retry flow.

🛠️ Suggested fix
-      op.status = 'running'
-
       // Check for configured result
       const configuredResult = options?.results?.[op.id]
-      if (configuredResult) {
+      if (configuredResult?.requiresOtp && !options?.otp) {
+        return { results, otpRequired: true }
+      }
+
+      op.status = 'running'
+      if (configuredResult) {
         const result: OperationResult = {
           stdout: configuredResult.stdout ?? '',
           stderr: configuredResult.stderr ?? '',
           exitCode: configuredResult.exitCode ?? 1,
           requiresOtp: configuredResult.requiresOtp,
           authFailure: configuredResult.authFailure,
         }
         op.result = result
         op.status = result.exitCode === 0 ? 'completed' : 'failed'
         results.push({ id: op.id, result })
-
-        if (result.requiresOtp && !options?.otp) {
-          return { results, otpRequired: true }
-        }
       } else {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
/** Execute all approved operations (mock: instant success unless configured otherwise). */
executeOperations(options?: ExecuteOptions): ExecuteResult {
const results: Array<{ id: string; result: OperationResult }> = []
const approved = this.state.operations.filter(op => op.status === 'approved')
// Sort by dependencies
const sorted = this.sortByDependencies(approved)
for (const op of sorted) {
// Check if dependent operation completed successfully
if (op.dependsOn) {
const dep = this.state.operations.find(d => d.id === op.dependsOn)
if (!dep || dep.status !== 'completed') {
// Skip - dependency not met
continue
}
}
op.status = 'running'
// Check for configured result
const configuredResult = options?.results?.[op.id]
if (configuredResult) {
const result: OperationResult = {
stdout: configuredResult.stdout ?? '',
stderr: configuredResult.stderr ?? '',
exitCode: configuredResult.exitCode ?? 1,
requiresOtp: configuredResult.requiresOtp,
authFailure: configuredResult.authFailure,
}
op.result = result
op.status = result.exitCode === 0 ? 'completed' : 'failed'
results.push({ id: op.id, result })
if (result.requiresOtp && !options?.otp) {
return { results, otpRequired: true }
}
} else {
// Default: success
const result: OperationResult = {
stdout: `Mock: ${op.command}`,
stderr: '',
exitCode: 0,
}
op.result = result
op.status = 'completed'
results.push({ id: op.id, result })
// Apply the operation's effects to mock state
this.applyOperationEffect(op)
}
}
return { results }
}
/** Execute all approved operations (mock: instant success unless configured otherwise). */
executeOperations(options?: ExecuteOptions): ExecuteResult {
const results: Array<{ id: string; result: OperationResult }> = []
const approved = this.state.operations.filter(op => op.status === 'approved')
// Sort by dependencies
const sorted = this.sortByDependencies(approved)
for (const op of sorted) {
// Check if dependent operation completed successfully
if (op.dependsOn) {
const dep = this.state.operations.find(d => d.id === op.dependsOn)
if (!dep || dep.status !== 'completed') {
// Skip - dependency not met
continue
}
}
// Check for configured result
const configuredResult = options?.results?.[op.id]
if (configuredResult?.requiresOtp && !options?.otp) {
return { results, otpRequired: true }
}
op.status = 'running'
if (configuredResult) {
const result: OperationResult = {
stdout: configuredResult.stdout ?? '',
stderr: configuredResult.stderr ?? '',
exitCode: configuredResult.exitCode ?? 1,
requiresOtp: configuredResult.requiresOtp,
authFailure: configuredResult.authFailure,
}
op.result = result
op.status = result.exitCode === 0 ? 'completed' : 'failed'
results.push({ id: op.id, result })
} else {
// Default: success
const result: OperationResult = {
stdout: `Mock: ${op.command}`,
stderr: '',
exitCode: 0,
}
op.result = result
op.status = 'completed'
results.push({ id: op.id, result })
// Apply the operation's effects to mock state
this.applyOperationEffect(op)
}
}
return { results }
}

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.

no test coverage for app features requiring logged-in connector

1 participant