-
-
Notifications
You must be signed in to change notification settings - Fork 238
test: mock connector for e2e/browser/local dev #258
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
# 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 Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
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 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)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this 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: SplitcreateMockConnectorAppinto 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 replacingwaitForTimeoutwith a proper assertion or waitFor condition.
page.waitForTimeout(500)is a fixed delay that can cause flaky tests. Playwright recommends usingexpect(...).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
waitForTimeoutcalls 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.
PendingOperationis re-exported fromtest/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
createMockUseConnectorfunction replicates much of the interface defined intest/test-utils/mock-connector-composable.ts. Consider whether reusingcreateMockConnectorComposablefrom 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 aligningcompletedOperationsandactiveOperationslogic with the unit test mock.The logic here includes
op.result?.requiresOtpchecks for categorising operations, but the inline mock inHeaderConnectorModal.spec.tsuses 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 aspattern is a code smell. Consider whether the interface could be adjusted to avoid the double cast.
test/e2e/helpers/mock-connector.ts
Outdated
| const query = getQuery(event) | ||
| const id = query.id as string | ||
| if (!id) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
cat -n test/e2e/helpers/mock-connector.ts | head -250 | tail -100Repository: npmx-dev/npmx.dev
Length of output: 3569
🏁 Script executed:
rg "getQuery" test/e2e/helpers/mock-connector.ts -B 2 -A 2Repository: npmx-dev/npmx.dev
Length of output: 532
🏁 Script executed:
rg "const getQuery" test/e2e/helpers/ -A 5Repository: npmx-dev/npmx.dev
Length of output: 43
🏁 Script executed:
head -30 test/e2e/helpers/mock-connector.tsRepository: npmx-dev/npmx.dev
Length of output: 776
🏁 Script executed:
rg "import.*getQuery" test/e2e/helpers/mock-connector.ts -A 2 -B 2Repository: 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
There was a problem hiding this 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
| 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' }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| 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' }) |
There was a problem hiding this 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
goToPackageConnectedhelper 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 splittingcreateMockConnectorAppinto 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:applyOperationEffectis 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 andtype: stringcan diverge from the contract; prefer shared types for roles, permissions, and operation bodies.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".🛠️ 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 }> {
| "exports": { | ||
| ".": { | ||
| "import": "./dist/index.mjs", | ||
| "types": "./dist/index.d.mts" | ||
| } | ||
| ".": "./dist/index.mjs" | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
cat -n cli/package.json | head -50Repository: npmx-dev/npmx.dev
Length of output: 1657
🏁 Script executed:
rg "from\s+['\"]npmx-connector['\"]|import.*npmx-connector" --type ts --type jsRepository: npmx-dev/npmx.dev
Length of output: 43
🏁 Script executed:
fd tsconfig -E node_modulesRepository: 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 -10Repository: 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.jsonRepository: 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.jsonRepository: 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.jsonhas"types"/"typings", tsdown enables dts automatically. (tsdown.dev) - Or explicitly:
- CLI:
tsdown --dts(tsdown.dev) - Config:
(tsdown.dev)
// tsdown.config.ts import { defineConfig } from 'tsdown' export default defineConfig({ dts: true, })
- CLI:
- If your
Common dts-related options/notes:
- Declaration maps: set
compilerOptions.declarationMap: trueintsconfig.json, ordts: { sourcemap: true }in tsdown config. (tsdown.dev) - tsdown generates/bundles
.d.tsviarolldown-plugin-dts. (tsdown.dev) - Performance: if
tsconfig.jsonenablesisolatedDeclarations, tsdown uses a faster path; otherwise it falls back to the TypeScript compiler. (tsdown.dev)
Citations:
- 1: https://tsdown.dev/options/dts?utm_source=openai
- 2: https://tsdown.dev/options/dts?utm_source=openai
- 3: https://tsdown.dev/reference/cli?utm_source=openai
- 4: https://tsdown.dev/options/dts?utm_source=openai
- 5: https://tsdown.dev/options/dts?utm_source=openai
- 6: https://tsdown.dev/options/dts?utm_source=openai
- 7: https://tsdown.dev/options/dts?utm_source=openai
🏁 Script executed:
git log --all --oneline cli/package.json 2>/dev/null | head -20Repository: 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_modulesRepository: 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.tsRepository: 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.
| // 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']> | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| // 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']> | |
| }) |
| 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 }, | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 },
}
}| ``` | ||
| 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) | ||
| ``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this 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:applyOperationEffectis 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)".
| /** 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 } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| /** 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 } | |
| } |
resolves #157