ci: make MCP registry publish optional; fix video background; pin AXe 1.1.1 #114
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds simulator video capture support. Introduces a new tool ( Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Tool as record_sim_video Tool
participant AXe as AXe CLI
participant VC as Video Capture Utils
participant FS as File System
Note over Tool: Preflight
User->>Tool: start=true, simulatorUuid, fps?
Tool->>Tool: areAxeToolsAvailable + isAxeAtLeastVersion(>=1.1.0)
alt unavailable or too old
Tool-->>User: Error (AXe not available/version)
else start
Tool->>VC: startSimulatorVideoCapture(simUuid, fps)
VC->>AXe: axe record-video --sim <uuid> --fps <n>
AXe-->>VC: streaming stdout/stderr
VC-->>Tool: session info
Tool-->>User: Started (next steps with stop=true)
end
User->>Tool: stop=true, simulatorUuid, outputFile?
Tool->>VC: stopSimulatorVideoCapture(simUuid)
VC->>AXe: send SIGINT/terminate
AXe-->>VC: final stdout (includes MP4 path)
VC-->>Tool: stop result + parsedPath?
alt outputFile provided
Tool->>FS: mkdir -p dirname(outputFile)
Tool->>FS: copy parsedPath -> outputFile
Tool->>FS: cleanup source (best-effort)
Tool-->>User: Saved to outputFile (meta.outputFile)
else parsedPath only
Tool-->>User: Saved to parsedPath
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Cursor review |
|
Claude finished @cameroncooke's task —— View job PR Code Review CompleteReview Checklist
Overall Assessment: ✅ EXCELLENTThis PR introduces video recording capabilities and improves release workflow reliability. The implementation is well-architected with excellent test coverage and follows the project's dependency injection patterns. 🎯 Key Strengths1. Architecture & Design
2. Process Management (Major Fix)
3. Version Management Excellence
4. Test Coverage
🔍 Technical ReviewVideo Recording Implementation (
|
commit: |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codex Review: Here are some suggestions.
Reply with @codex fix comments to fix any unresolved comments.
About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".
src/utils/video_capture.ts
Outdated
| // Wait for process to close | ||
| await new Promise<void>((resolve) => { | ||
| if (!child) return resolve(); | ||
| try { | ||
| child.once('close', () => resolve()); | ||
| child.once('exit', () => resolve()); | ||
| } catch { | ||
| resolve(); |
There was a problem hiding this comment.
[P1] Avoid hanging when AXe process already exited
If the AXe record-video process terminates before stopSimulatorVideoCapture is called (e.g., the simulator closes or AXe crashes), the session still exists but the child has already emitted its exit/close events. The current stop logic installs listeners after that point and never resolves the promise, so the tool call hangs indefinitely and never returns a response to the user. Consider resolving immediately when child.exitCode is set or when child.killed is already true before registering the listeners.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (11)
src/utils/axe-helpers.ts (1)
59-74: Semver compare is minimal; consider edge casesCurrent numeric split ignores pre-release/build metadata and variable segment lengths. If AXe ever emits versions like 1.2 or 1.2.0-beta, results may be surprising. Not urgent given regex below, but worth hardening.
Apply if you want more robust handling without new deps:
-function compareSemver(a: string, b: string): number { - const pa = a.split('.').map((n) => parseInt(n, 10)); - const pb = b.split('.').map((n) => parseInt(n, 10)); +function compareSemver(a: string, b: string): number { + // strip leading 'v' and any pre-release/build metadata + const norm = (v: string) => v.replace(/^v/i, '').split('-')[0]; + const pa = norm(a).split('.').map((n) => parseInt(n, 10)); + const pb = norm(b).split('.').map((n) => parseInt(n, 10)); const len = Math.max(pa.length, pb.length); for (let i = 0; i < len; i++) { const da = Number.isFinite(pa[i]) ? pa[i] : 0; const db = Number.isFinite(pb[i]) ? pb[i] : 0; if (da > db) return 1; if (da < db) return -1; } return 0; }README.md (2)
89-90: Nice: feature bullet clearly states version gateExplicit “AXe v1.1.0+” note is helpful. Consider linking to the new tool in TOOLS.md for discoverability.
121-124: Heading formatting broke (“Configure your MCP client” not rendered as a heading)It lost the markdown heading marker and will not anchor correctly from the TOC.
Apply:
-> Video capture requires the bundled AXe binary (v1.1.0+). Run `npm run bundle:axe` once locally before using `record_sim_video`. This is not required for unit tests. - -Configure your MCP client +> Video capture requires the bundled AXe binary (v1.1.0+). Run `npm run bundle:axe` once locally before using `record_sim_video`. This is not required for unit tests. + +## Configure your MCP clientscripts/bundle-axe.sh (5)
6-13: Safer shell options recommendedUse nounset and pipefail to catch more classes of script errors.
-set -e +set -euo pipefail
16-30: Version resolution is solid; minor robustness nits
- Avoid Useless Use of cat; trim via redirection.
- Quote variables; tolerate unset AXE_VERSION under nounset.
-elif [ -f "$VERSION_FILE" ]; then - PINNED_AXE_VERSION="$(cat "$VERSION_FILE" | tr -d ' \n\r')" +elif [ -f "$VERSION_FILE" ]; then + PINNED_AXE_VERSION="$(tr -d ' \n\r' < "$VERSION_FILE")"
40-44: AXE_FORCE_REMOTE check should handle unset under nounsetWhen using
set -u, referencing an unset AXE_FORCE_REMOTE errors. Use${VAR:-}.-if [ -z "${AXE_FORCE_REMOTE}" ] && [ -d "$AXE_LOCAL_DIR" ] && [ -f "$AXE_LOCAL_DIR/Package.swift" ]; then +if [ -z "${AXE_FORCE_REMOTE:-}" ] && [ -d "$AXE_LOCAL_DIR" ] && [ -f "$AXE_LOCAL_DIR/Package.swift" ]; then
93-96: Harden download: fail on HTTP errors and add retriesCurl without -f may save an HTML error and proceed. Add fail-fast and retry.
-echo "📥 Downloading AXe release archive ($AXE_RELEASE_URL)..." -curl -L -o "axe-release.tar.gz" "$AXE_RELEASE_URL" +echo "📥 Downloading AXe release archive ($AXE_RELEASE_URL)..." +curl -fSL --retry 3 --retry-connrefused -o "axe-release.tar.gz" "$AXE_RELEASE_URL"
157-159: Avoid shadowing AXE_VERSION env varReusing AXE_VERSION here can confuse logs vs input. Rename for clarity.
-AXE_VERSION=$(DYLD_FRAMEWORK_PATH="$BUNDLED_DIR/Frameworks" "$BUNDLED_DIR/axe" --version 2>/dev/null || echo "unknown") -echo "📋 AXe version: $AXE_VERSION" +AXE_VERSION_STR=$(DYLD_FRAMEWORK_PATH="$BUNDLED_DIR/Frameworks" "$BUNDLED_DIR/axe" --version 2>/dev/null || echo "unknown") +echo "📋 AXe version: $AXE_VERSION_STR"docs/TOOLS.md (1)
33-34: Clarify version gate in tool descriptionAdd explicit “Requires AXe v1.1.0+” here to mirror README.
-- `record_sim_video` - Starts or stops video capture for an iOS simulator using AXe. Provide exactly one of start=true or stop=true. On stop, outputFile is required. fps defaults to 30. +- `record_sim_video` - Starts or stops video capture for an iOS simulator using AXe (requires AXe v1.1.0+). Provide exactly one of start=true or stop=true. On stop, outputFile is required. fps defaults to 30.src/utils/video_capture.ts (2)
32-32: Add return type annotation for thestopAllfunction.The static analysis tool flagged a missing return type annotation for the
stopAllfunction on Line 32.Apply this diff to add the return type:
- const stopAll = () => { + const stopAll = (): void => {
162-162: Verify necessity of thevoid executorstatement.Found at src/utils/video_capture.ts:162 with the comment "Mark executor as used to satisfy lint rule" and the single statement
void executor;— either remove the unusedexecutorparameter, rename it to_executor(or_) to indicate intentionally unused, or use the executor for process management (e.g., terminate/cleanup simulator processes) so the linter justification isn't needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.axe-version(1 hunks).gitignore(1 hunks)README.md(2 hunks)docs/TOOLS.md(5 hunks)scripts/bundle-axe.sh(5 hunks)src/mcp/tools/simulator/__tests__/record_sim_video.test.ts(1 hunks)src/mcp/tools/simulator/record_sim_video.ts(1 hunks)src/utils/axe-helpers.ts(2 hunks)src/utils/axe/index.ts(1 hunks)src/utils/video-capture/index.ts(1 hunks)src/utils/video_capture.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/*.ts: Use .ts extensions for all internal relative imports (e.g., import { tool } from './tool.ts')
Use .ts extensions in re-exports for internal files (e.g., export { default } from '../shared/tool.ts')
Use .js extension only for external package entry points (e.g., import ... from '@scope/pkg/file.js')
Never import internal project files using .js extensions
Files:
src/utils/axe/index.tssrc/mcp/tools/simulator/__tests__/record_sim_video.test.tssrc/utils/video_capture.tssrc/mcp/tools/simulator/record_sim_video.tssrc/utils/axe-helpers.tssrc/utils/video-capture/index.ts
**/*.{test,spec}.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{test,spec}.ts: Do not use Vitest mocking APIs in tests (no vi.mock, vi.fn, vi.spyOn, etc.)
In tests, import the core ...Logic function from the tool and use createMockExecutor/createMockFileSystemExecutor to simulate outcomes
Files:
src/mcp/tools/simulator/__tests__/record_sim_video.test.ts
🧠 Learnings (1)
📚 Learning: 2025-09-21T19:27:22.782Z
Learnt from: CR
PR: cameroncooke/XcodeBuildMCP#0
File: AGENTS.md:0-0
Timestamp: 2025-09-21T19:27:22.782Z
Learning: Applies to **/*.{test,spec}.ts : In tests, import the core ...Logic function from the tool and use createMockExecutor/createMockFileSystemExecutor to simulate outcomes
Applied to files:
src/mcp/tools/simulator/__tests__/record_sim_video.test.tssrc/utils/axe-helpers.ts
🧬 Code graph analysis (3)
src/mcp/tools/simulator/__tests__/record_sim_video.test.ts (2)
src/test-utils/mock-executors.ts (1)
createMockFileSystemExecutor(165-181)src/mcp/tools/simulator/record_sim_video.ts (1)
record_sim_videoLogic(57-219)
src/utils/video_capture.ts (1)
src/utils/video-capture/index.ts (3)
AxeHelpers(4-4)startSimulatorVideoCapture(2-2)stopSimulatorVideoCapture(3-3)
src/mcp/tools/simulator/record_sim_video.ts (2)
src/utils/video_capture.ts (2)
startSimulatorVideoCapture(69-146)stopSimulatorVideoCapture(151-214)src/utils/typed-tool-factory.ts (1)
createTypedTool(29-60)
🪛 GitHub Check: build-and-test (24.x)
src/utils/video_capture.ts
[warning] 32-32:
Missing return type on function
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Cursor Bugbot
- GitHub Check: Cursor Bugbot
- GitHub Check: claude-review
🔇 Additional comments (22)
.gitignore (1)
110-110: LGTM: ignore .mcpliGood addition; low risk and aligns with keeping local state out of VCS.
src/utils/axe-helpers.ts (2)
13-15: Import extension compliance looks correctInternal relative imports use .ts as per guidelines. No issues.
81-102: Incorrect assumption — third exec param is useShell, not detached; no detachment issueCommandExecutor signature is (command, logPrefix?, useShell?, opts?, detached?). The call exec([axePath, '--version'], 'AXe Version', true) sets useShell=true (not detached). To pass environment variables, provide the opts object as the fourth argument, e.g. exec([axePath, '--version'], 'AXe Version', true, { env: getBundledAxeEnvironment() }).
Likely an incorrect or invalid review comment.
.axe-version (1)
1-1: Pinned AXe to 1.1.1Matches the PR intent and bundling script precedence. No action needed.
src/utils/axe/index.ts (1)
6-7: Re-export added correctly with .ts extensionPublic API surface now exposes isAxeAtLeastVersion; import style complies with guidelines.
src/utils/video-capture/index.ts (1)
1-5: Barrel exports look good and conform to .ts extension ruleType-only re-export via
typeis correct.docs/TOOLS.md (3)
3-3: Doc count updatedTool/workflow counts look consistent with the summary below.
107-113: Summary stats/date updatedLooks consistent; no issues.
72-73: Param rename confirmed — simulatorUdid applied and tests updated.Schema, handler hints, and unit tests updated in src/mcp/tools/simulator-management/erase_sims.ts and src/mcp/tools/simulator-management/tests/erase_sims.test.ts.
src/mcp/tools/simulator/__tests__/record_sim_video.test.ts (4)
15-38: LGTM on validation tests!These tests properly verify the mutually exclusive start/stop validation and the required outputFile for stop operations. The test implementation correctly uses the tool handler and validates error messages, following the expected patterns.
40-87: LGTM on start behavior test with proper dependency injection!This test correctly follows the coding guidelines by importing the core logic function and using dependency injection to mock the AXe helpers and video capture utilities. The test validates the default FPS behavior and warning when outputFile is ignored during start.
89-149: LGTM on end-to-end stop test with file operations!This test demonstrates proper end-to-end functionality by testing both start and stop operations with file renaming. The test correctly injects mock dependencies and validates the file move operation results and metadata.
151-189: LGTM on AXe version gating test!This test properly validates the version requirement enforcement by injecting a mock that returns
falseforisAxeAtLeastVersion, ensuring the tool correctly rejects operations when AXe version is insufficient.src/utils/video_capture.ts (3)
54-60: LGTM on MP4 path parsing logic!The
parseLastAbsoluteMp4Pathfunction correctly uses a regex to extract absolute MP4 file paths from the AXe output buffer and returns the last match, which is the expected behavior for video capture completion.
188-196: Efficient process termination with proper cleanup.The process termination logic properly attempts graceful shutdown with SIGINT before falling back to default termination, and includes proper cleanup of session state regardless of termination success.
102-102: Approve: detached executor usage is valid.CommandExecutor in src/utils/CommandExecutor.ts is typed as (command, logPrefix?, useShell?, opts?: CommandExecOptions, detached?: boolean), so executor(command, 'Start Simulator Video Capture', true, { env }, true) is correct.
src/mcp/tools/simulator/record_sim_video.ts (6)
37-53: LGTM on comprehensive input validation schema!The schema properly enforces mutually exclusive start/stop operations and requires outputFile when stopping. The validation messages are clear and user-friendly.
57-77: Well-designed dependency injection for testability!The function signature with default parameter objects for AXe helpers, video capture functions, and file system executor provides excellent testability while maintaining clean production usage.
78-88: Appropriate AXe version gating for simulator video capture.The preflight checks ensure AXe tools are available and meet the minimum version requirement (1.1.0), providing clear error messages when requirements aren't met.
92-141: LGTM on start operation implementation!The start logic properly handles FPS defaults, provides helpful warnings about ignored outputFile parameter, and includes clear next steps instructions for users.
156-191: Robust file operation handling with proper error management.The file move/rename logic properly creates destination directories, handles cleanup failures gracefully, and provides comprehensive error reporting when operations fail.
221-227: LGTM on tool configuration and export!The tool object properly exposes the schema for MCP consumption and uses the typed tool factory with appropriate handlers and executor configuration.
| # Find the extracted directory (might be named differently) | ||
| EXTRACTED_DIR=$(find . -type d -name "*AXe*" -o -name "*axe*" | head -1) | ||
| if [ -z "$EXTRACTED_DIR" ]; then | ||
| # If no AXe directory found, assume files are in current directory | ||
| EXTRACTED_DIR="." | ||
| fi | ||
|
|
||
| cd "$EXTRACTED_DIR" |
There was a problem hiding this comment.
find expression may select non-directories; group predicates
Without grouping, -name "*axe*" can match files of any type. Group and constrain both to directories.
-EXTRACTED_DIR=$(find . -type d -name "*AXe*" -o -name "*axe*" | head -1)
+EXTRACTED_DIR=$(find . \( -type d -name "*AXe*" -o -type d -name "*axe*" \) | head -1)📝 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.
| # Find the extracted directory (might be named differently) | |
| EXTRACTED_DIR=$(find . -type d -name "*AXe*" -o -name "*axe*" | head -1) | |
| if [ -z "$EXTRACTED_DIR" ]; then | |
| # If no AXe directory found, assume files are in current directory | |
| EXTRACTED_DIR="." | |
| fi | |
| cd "$EXTRACTED_DIR" | |
| # Find the extracted directory (might be named differently) | |
| EXTRACTED_DIR=$(find . \( -type d -name "*AXe*" -o -type d -name "*axe*" \) | head -1) | |
| if [ -z "$EXTRACTED_DIR" ]; then | |
| # If no AXe directory found, assume files are in current directory | |
| EXTRACTED_DIR="." | |
| fi | |
| cd "$EXTRACTED_DIR" |
🤖 Prompt for AI Agents
In scripts/bundle-axe.sh around lines 99 to 106, the find expression can match
non-directories because the name tests are not grouped and -type d may not apply
as intended; update the EXTRACTED_DIR assignment to use a find that constrains
results to directories and groups the name predicates (use escaped parentheses
and -o) and prefer a case-insensitive name test (iname); keep the fallback to
"." and the subsequent cd unchanged.
| @@ -0,0 +1,189 @@ | |||
| import { describe, it, expect, vi, afterEach } from 'vitest'; | |||
There was a problem hiding this comment.
Violation of Vitest mocking constraint in coding guidelines.
The coding guidelines explicitly state that files matching **/*.{test,spec}.ts should not use Vitest mocking APIs (vi.mock, vi.fn, vi.spyOn, etc.), but this test file imports vi from Vitest.
Apply this diff to remove the Vitest mocking import:
-import { describe, it, expect, vi, afterEach } from 'vitest';
+import { describe, it, expect, afterEach } from 'vitest';📝 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.
| import { describe, it, expect, vi, afterEach } from 'vitest'; | |
| import { describe, it, expect, afterEach } from 'vitest'; |
🤖 Prompt for AI Agents
In src/mcp/tools/simulator/__tests__/record_sim_video.test.ts around line 1, the
test imports vi from Vitest which violates the guideline banning Vitest mocking
APIs in files matching **/*.{test,spec}.ts; remove vi from the import list
(import { describe, it, expect, afterEach } from 'vitest') and then remove or
refactor any uses of vi in this file — replace vi.mock/vi.fn/vi.spyOn calls with
non-Vitest alternatives or manual stubs/mocks appropriate for the test, ensuring
no Vitest mocking APIs remain.
| afterEach(() => { | ||
| vi.restoreAllMocks(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove mocking-related cleanup since no mocking is used.
With the removal of vi import and adherence to the coding guidelines that prohibit Vitest mocking APIs, the afterEach cleanup that calls vi.restoreAllMocks() is unnecessary.
Apply this diff to remove the unnecessary cleanup:
-afterEach(() => {
- vi.restoreAllMocks();
-});📝 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.
| afterEach(() => { | |
| vi.restoreAllMocks(); | |
| }); |
🤖 Prompt for AI Agents
In src/mcp/tools/simulator/__tests__/record_sim_video.test.ts around lines 11 to
13, remove the unnecessary afterEach cleanup that calls vi.restoreAllMocks()
because the test no longer imports or uses Vitest mocking APIs; delete the
entire afterEach(() => { vi.restoreAllMocks(); }); block so there is no leftover
mocking-related cleanup.
| import { | ||
| startSimulatorVideoCapture, | ||
| stopSimulatorVideoCapture, | ||
| } from '../../../utils/video-capture/index.ts'; |
There was a problem hiding this comment.
Use correct import path for video capture module.
The import path uses video-capture but based on the relevant code snippets, the actual module is at video_capture (with underscore). The current import may cause module resolution issues.
Apply this diff to fix the import path:
-} from '../../../utils/video-capture/index.ts';
+} from '../../../utils/video_capture/index.ts';📝 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.
| } from '../../../utils/video-capture/index.ts'; | |
| } from '../../../utils/video_capture/index.ts'; |
🤖 Prompt for AI Agents
In src/mcp/tools/simulator/record_sim_video.ts around line 17 the import path
references '../../../utils/video-capture/index.ts' but the actual module
directory is named video_capture; update the import to use
'../../../utils/video_capture/index.ts' (or '../../../utils/video_capture' if
extensions/resolver allow) so the module resolves correctly, and run a quick
build or lint to verify no other imports use the hyphenated name.
…eady exited\n\n- Spawn AXe with detached exec and pass env correctly\n- Track session.ended on child exit/close\n- Fast-path resolve in stop when process already ended\n- Add 5s safety timeout to prevent indefinite waits
|
Claude finished @cameroncooke's task —— View job PR Code Review CompleteReview Checklist
Overall Assessment: ✅ EXCELLENTThis PR introduces video recording capabilities and improves release workflow reliability. The implementation is well-architected with excellent test coverage and follows the project's dependency injection patterns. 🎯 Key Strengths1. Architecture & Design
2. Process Management (Major Fix)
3. Version Management Excellence
4. Test Coverage
🔍 Technical ReviewVideo Recording Implementation (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
src/utils/video_capture.ts (6)
202-228: Fix Biome errors: don’t return a value from a void callback.The early-returns use
return resolve()/return finish(), which violatesnoVoidTypeReturn. Call, thenreturn;.- await new Promise<void>((resolve): void => { - if (!child) return resolve(); + await new Promise<void>((resolve): void => { + if (!child) { resolve(); return; } @@ - if (alreadyEnded || hasExitCode || hasSignal) { - return resolve(); - } + if (alreadyEnded || hasExitCode || hasSignal) { + resolve(); + return; + } @@ - } catch { - return finish(); - } + } catch { + finish(); + return; + }
55-61: Make MP4 path parsing robust (quotes, spaces, Windows paths).Current regex misses quoted/space-containing and Windows paths. Use broader patterns and return the last match found.
-function parseLastAbsoluteMp4Path(buffer: string | undefined): string | null { - if (!buffer) return null; - const matches = [...buffer.matchAll(/(\s|^)(\/[^\s'"]+\.mp4)\b/gi)]; - if (matches.length === 0) return null; - const last = matches[matches.length - 1]; - return last?.[2] ?? null; -} +function parseLastAbsoluteMp4Path(buffer?: string): string | null { + if (!buffer) return null; + const patterns = [ + /(["'])(\/[^"']+?\.mp4)\1/gi, // Unix, quoted (allow spaces) + /(\s|^)(\/[^\s'"]+\.mp4)\b/gi, // Unix, unquoted + /(["'])(([a-zA-Z]:\\|\\\\)[^"']+?\.mp4)\1/gi, // Windows, quoted + /(\s|^)(([a-zA-Z]:\\|\\\\)[^\s'"]+\.mp4)\b/gi, // Windows, unquoted + ]; + let last: string | null = null; + for (const re of patterns) { + const matches = [...buffer.matchAll(re)]; + if (matches.length) last = matches[matches.length - 1][2] ?? last; + } + return last; +}
80-86: Auto-clean stale sessions so a crashed/ended child doesn’t block restart.If AXe exits before
stop, the stale entry preventsstarteven though nothing is running.- if (sessions.has(simulatorUuid)) { - return { - started: false, - error: 'A video recording session is already active for this simulator. Stop it first.', - }; - } + { + const existing = sessions.get(simulatorUuid); + if (existing) { + const ch = existing.process as ChildProcess | undefined; + const stale = existing.ended || ch?.exitCode != null || (ch as unknown as { signalCode?: string | null }).signalCode != null; + if (!stale) { + return { + started: false, + error: 'A video recording session is already active for this simulator. Stop it first.', + }; + } + sessions.delete(simulatorUuid); + } + }
33-41: Signal the process group when detached to avoid orphaned children.With
detached: true, AXe may be the group leader. Prefer sending the signal to the group on POSIX to ensure clean teardown.- const child = sess.process as ChildProcess | undefined; - child?.kill?.('SIGINT'); + const child = sess.process as ChildProcess | undefined; + if (child?.pid && process.platform !== 'win32') { + try { process.kill(-child.pid, 'SIGINT'); } catch { child?.kill?.('SIGINT'); } + } else { + child?.kill?.('SIGINT'); + }- try { - child?.kill?.('SIGINT'); - } catch { + try { + if (child?.pid && process.platform !== 'win32') { + try { process.kill(-child.pid, 'SIGINT'); } catch { child?.kill?.('SIGINT'); } + } else { + child?.kill?.('SIGINT'); + } + } catch { try { child?.kill?.(); } catch { // ignore } }Also applies to: 191-199
13-19: Tighten typing: store ChildProcess in Session to drop casts.Using
unknownforces repeated casts. PreferChildProcess | undefinedin the Session shape.-type Session = { - process: unknown; +type Session = { + process?: ChildProcess; sessionId: string; startedAt: number; buffer: string; ended: boolean; };- const child = sess.process as ChildProcess | undefined; + const child = sess.process;- const child = session.process as ChildProcess | undefined; + const child = session.process;Also applies to: 36-37, 188-189
101-105: Add sessionId to logs for easier correlation.Including the sessionId in logs helps trace start/stop pairs.
- log('info', `Starting AXe video recording for simulator ${simulatorUuid} at ${fps} fps`); + log('info', `Starting AXe video recording for simulator ${simulatorUuid} at ${fps} fps`); @@ sessions.set(simulatorUuid, session); ensureSignalHandlersAttached(); + log('info', `AXe video recording session started: simulator=${simulatorUuid}, sessionId=${session.sessionId}`);Also applies to: 152-159
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/utils/video_capture.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/*.ts: Use .ts extensions for all internal relative imports (e.g., import { tool } from './tool.ts')
Use .ts extensions in re-exports for internal files (e.g., export { default } from '../shared/tool.ts')
Use .js extension only for external package entry points (e.g., import ... from '@scope/pkg/file.js')
Never import internal project files using .js extensions
Files:
src/utils/video_capture.ts
🪛 Biome (2.1.2)
src/utils/video_capture.ts
[error] 203-203: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
[error] 210-210: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
[error] 224-224: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Cursor Bugbot
- GitHub Check: claude-review
🔇 Additional comments (3)
src/utils/video_capture.ts (3)
201-228: Nice: addresses prior hang-on-stop scenario.Immediate resolve on ended/exitCode/signal plus a safety timeout prevents indefinite hangs noted in the previous review.
9-12: Imports comply with project path/extension rules.Internal imports use .ts and external/built-ins omit extensions. All good.
103-110: CommandExecutor signature verified — call is correctCommandExecutor is defined as (command, logPrefix?, useShell?, opts?, detached?) in src/utils/CommandExecutor.ts and defaultExecutor implements detached handling; the call in src/utils/video_capture.ts matches the signature — no change required.
1 participant
Summary
Background/Details
npm view, which isn’t guaranteed instant due to CDN/cache.Solution
mcp_registryjob that runs only onpush, afterrelease(needs: release). It skips when the secret is missing and logs warnings on retry exhaustion (exit 0) so it never fails the workflow.gh run view --json jobs) and treat success if thereleasejob concluded success. Removed npm-view fallback.{ env }anddetached=trueto background AXe and return immediately..axe-version(1.1.1). bundle-axe.sh readsAXE_VERSIONenv, then.axe-version, else defaults to 1.1.1; constructs the URL accordingly. Also addedAXE_FORCE_REMOTE=1to force downloading in CI/local tests.Testing
record_sim_videostart/stop on a booted simulator; saved MP4 tosim_videos/.npm run bundle:axewithAXE_FORCE_REMOTE=1and confirmed AXe v1.1.1 bundled.Notes
Summary by CodeRabbit