fix(mcp): MCP_harden inline review follow-ups - #187
Conversation
Bound stub setup waits, reclaim malformed smoke locks without busy-spin, use rmdir for empty config dirs, and harden idempotency tests against real spawn/fs side effects. Co-authored-by: Cursor <cursoragent@cursor.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @tonythethompson, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
Warning Review limit reached
Next review available in: 41 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe PR hardens MCP smoke execution, Studio lock publication and reclamation, configuration cleanup, Python resolution coverage, and Olive job setup and idempotency tests. ChangesSmoke and lock hardening
Olive job lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 8✅ Passed checks (8 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Warning Review ran into problems🔥 ProblemsLinked repositories: Public OSS repositories can only analyze public repositories installed in this organization. Analyzed 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 |
PR Summary by Qodofix(mcp): harden MCP smoke locks and stubbed Olive job setup
AI Description
Diagram
High-Level Assessment
Files changed (7)
|
Greptile SummaryThis follow-up hardens MCP smoke execution, lock ownership and cleanup, setup-stub timeouts, and related tests.
Confidence Score: 4/5The PR does not yet appear safe to merge because interrupted fallback publication can leave the smoke lock permanently unrecoverable. The earlier incomplete reclaim-mutex failure remains reachable when hard-link publication falls back to a direct exclusive write: interruption can leave an incomplete final-path body, and current recovery explicitly refuses to remove it, causing every later smoke acquisition to time out. Files Needing Attention: scripts/studioConfigSmokeLock.mjs
|
| Filename | Overview |
|---|---|
| scripts/studioConfigSmokeLock.mjs | Replaces direct lock creation with temp-file publication and serialized reclamation, but the fallback can still leave an unrecoverable incomplete lock after interruption. |
| scripts/mcp-agent-smoke.mjs | Adds isolated mcporter installation, direct Node CLI invocation, and explicit SIGHUP exit handling. |
| scripts/studioConfigSnapshot.mjs | Uses non-recursive directory removal when cleaning up a newly created Studio configuration directory. |
| src/server/services/olive/jobRunner.ts | Bounds setup-stub waiting with a configurable timeout and reports expiration as job failure. |
| src/lib/tests/studioConfigSmokeLock.test.ts | Substantially expands lock contention, publication fallback, reclamation, and cleanup coverage. |
| src/server/services/olive/jobRunner.idempotency.test.ts | Strengthens idempotency coverage with controlled process and filesystem behavior. |
| src/server/services/olive/jobRunner.stubSetup.test.ts | Covers setup-stub timeout and completion behavior. |
| vite.config.ts | Updates test configuration supporting the expanded smoke and job-runner tests. |
Reviews (18): Last reviewed commit: "fix(mcp): never age-reclaim incomplete s..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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 automated review suggestions for this pull request.
Reviewed commit: 5ee7ab58fe
ℹ️ 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
- 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 address that feedback".
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Qodo Fixer✅ Committed (1) · ☑ Fixed (1) Commits pushed directly to this PR — no separate fix PR opened. Process — 1 fixed
|
Create the Studio config smoke lock with O_EXCL write of the owner PID so waiters never see a live empty body from this publisher, and only reclaim aged malformed locks after a publish grace window. Co-authored-by: Cursor <cursoragent@cursor.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/studioConfigSmokeLock.mjs`:
- Around line 93-105: The lock-body classification around malformed and
reclaimable in studioConfigSmokeLock must treat partial numeric PID contents as
incomplete while the writer may still be publishing. Validate that the parsed
PID exactly matches the complete numeric body rather than accepting parseInt
prefixes, and apply the existing publish grace window to incomplete numeric
bodies before reclaiming; add a regression test covering a partial numeric PID
body.
In `@src/server/services/olive/jobRunner.idempotency.test.ts`:
- Line 3: Update the test setup and teardown around the detached setup task and
settled helper to track every created job ID, then cancel or finalize each stub
job through the supported lifecycle path in afterEach. Await each job’s
finishedAt before clearing jobRegistry or restoring mocks, enforce the teardown
deadline, and fail teardown if any job remains unfinished.
In `@src/server/services/olive/jobRunner.ts`:
- Around line 316-329: Add deterministic timer-controlled coverage for the
setup-stub timeout flow around the job runner’s timeout logic, including an
invalid or non-positive OLIVE_JOB_SETUP_STUB_TIMEOUT_MS value to verify the
120-second fallback. Assert the job becomes failed, the timeout log is emitted,
and finalizeJob sets finishedAt; run lint, typecheck, and the required server
smoke checks before completion.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b91dafca-5e8c-480a-b090-f1d135442e12
📒 Files selected for processing (7)
scripts/mcp-agent-smoke.mjsscripts/studioConfigSmokeLock.mjsscripts/studioConfigSnapshot.mjssrc/lib/__tests__/resolvePython.test.tssrc/lib/__tests__/studioConfigSmokeLock.test.tssrc/server/services/olive/jobRunner.idempotency.test.tssrc/server/services/olive/jobRunner.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
tonythethompson/QuickShell(manual)tonythethompson/numan(manual)tonythethompson/dependency-chain-substrate(manual)
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: Greptile Review
- GitHub Check: docker-build
- GitHub Check: python-tests
- GitHub Check: validate
- GitHub Check: olive-pass-availability
🧰 Additional context used
📓 Path-based instructions (7)
**/*
📄 CodeRabbit inference engine (AGENTS.md)
Do not implement the listed backburner AI providers unless explicitly requested; prefer Custom or OpenAI-compatible providers for OpenAI-shaped hosts.
Files:
scripts/mcp-agent-smoke.mjssrc/lib/__tests__/resolvePython.test.tssrc/server/services/olive/jobRunner.tssrc/lib/__tests__/studioConfigSmokeLock.test.tsscripts/studioConfigSnapshot.mjsscripts/studioConfigSmokeLock.mjssrc/server/services/olive/jobRunner.idempotency.test.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
src/**/*.{ts,tsx}: Match existing naming, file layout, and TypeScript patterns insrc/.
Put shared recipe logic insrc/lib/, especiallypipelineValidation.ts,oliveRecipeBuilder.ts, andrecipePipeline.ts.
src/**/*.{ts,tsx}: Keep validation logic in shared libraries rather than duplicating it in UI cell helpers or inspectors.
Split theInputEnvironmentPanel,IHVIntegrationPanel, andExecutionWorkspacemega-panels into feature folders with colocated hooks and tests.
Keep server and UI AI provider catalogs synchronized, preferably through a shared provider ID list or synchronization test; register new providers in both catalogs.
Add test coverage forrecipe-graph/,passCatalog,oliveRecipeHub,jobHistoryStore, andvramEstimate, and strengthen component tests for the large panels.
src/**/*.{ts,tsx}: All UI state mutations must go throughcommitUiStateUpdateinsrc/lib/pipelineValidation.tsso invariants are enforced; useusePipelineState()for state access andreplaceStatefor recipe imports or preset loads.
Avoidexport *barrel imports; import directly from the actual module file to preserve Vite tree-shaking and component-test isolation.Follow the React performance guidance in
docs/REACT_BEST_PRACTICES.md, especially eliminating waterfalls, avoiding barrel imports, and deferring non-critical third-party libraries.
Files:
src/lib/__tests__/resolvePython.test.tssrc/server/services/olive/jobRunner.tssrc/lib/__tests__/studioConfigSmokeLock.test.tssrc/server/services/olive/jobRunner.idempotency.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{ts,tsx,js,jsx}: Place imports at the top of modules; use inline imports only for a documented circular dependency.
Run linting and ensure typecheck-related CI checks pass before submitting changes.
For UI or server changes, manually smoke-test development startup, recipe loading/building, validation banners, and live execution when execution behavior is touched.Use the project's React 19, Vite, Express, and Tauri 2 stack conventions for frontend and server TypeScript/JavaScript code.
Files:
src/lib/__tests__/resolvePython.test.tssrc/server/services/olive/jobRunner.tssrc/lib/__tests__/studioConfigSmokeLock.test.tssrc/server/services/olive/jobRunner.idempotency.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
When working with React 19 or Vite 8 APIs, consult current Context7 documentation instead of assuming conventions from earlier major versions.
Files:
src/lib/__tests__/resolvePython.test.tssrc/server/services/olive/jobRunner.tssrc/lib/__tests__/studioConfigSmokeLock.test.tssrc/server/services/olive/jobRunner.idempotency.test.ts
src/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use the unit-test configuration for
src/lib/unit tests and keep unit tests compatible with Vitest.
Files:
src/lib/__tests__/resolvePython.test.tssrc/lib/__tests__/studioConfigSmokeLock.test.tssrc/server/services/olive/jobRunner.idempotency.test.ts
src/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not trigger real Olive optimization runs in tests or CI; use CPU-only recipe building, JSON export, and validation flows instead.
Files:
src/lib/__tests__/resolvePython.test.tssrc/lib/__tests__/studioConfigSmokeLock.test.tssrc/server/services/olive/jobRunner.idempotency.test.ts
src/server/services/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Keep server-side business logic in services, including AI providers, Olive job/virtual-environment handling, and related service modules.
Files:
src/server/services/olive/jobRunner.tssrc/server/services/olive/jobRunner.idempotency.test.ts
🔍 Remote MCP DeepWiki, GitHub Copilot
Additional review context
-
The current PR is
#187, open againstMCP_harden, with 7 changed files and +164/-31. Its listed targeted tests passed locally, but CI was still unchecked in the PR description. -
OLIVE_JOB_SETUP_STUBnow accepts only finite positive timeout values; invalid, zero, or negative values fall back to 120 seconds. The loop polls every 200 ms, marks the jobfailed, logs the timeout, cleans artifacts, and finalizes it. -
The stub timeout path has no dedicated assertion in the changed tests; the idempotency tests instead track and drain detached setup tasks.
-
Lock reclamation now:
- Reclaims finite dead-PID locks immediately.
- Gives malformed/empty locks a grace period of
max(1000, pollMs * 4)based onstatSync().mtimeMs. - Sleeps normally if unlink fails, avoiding a reclaim busy-spin.
- Uses non-recursive
rmdirSyncfor empty directory cleanup.
-
The new lock tests cover ownership-preserving release and
rmdirSync, but do not cover the newly added malformed-lock grace-period behavior. -
Earlier review tools identified the empty-lock race caused by
openSync("wx")creating the file before writing its PID. Those review threads are now marked outdated/resolved after the grace-period fix. -
At retrieval time,
validate,security,python-tests,docker-build,olive-pass-availability, and Greptile checks were still running; only the auto-merge check had completed successfully. -
DeepWiki could not provide repository context because neither the requested
Babel-Playerrepository norOlive-Studiowas indexed.
🔇 Additional comments (11)
src/lib/__tests__/resolvePython.test.ts (1)
60-79: LGTM!scripts/studioConfigSmokeLock.mjs (3)
11-12: LGTM!Also applies to: 39-39, 55-55
106-114: LGTM!
131-136: LGTM!src/lib/__tests__/studioConfigSmokeLock.test.ts (2)
56-61: LGTM!
111-116: LGTM!scripts/studioConfigSnapshot.mjs (1)
6-13: LGTM!Also applies to: 125-125
scripts/mcp-agent-smoke.mjs (2)
612-612: LGTM!
609-613: 📐 Maintainability & Code QualityVerify required CI checks before merge.
The PR reports that CI is unchecked. Run linting and all typecheck-related CI checks before merge.
As per coding guidelines: “Run linting and ensure typecheck-related CI checks pass before submitting changes.”
Sources: Coding guidelines, MCP tools
src/server/services/olive/jobRunner.idempotency.test.ts (2)
5-20: LGTM!Also applies to: 40-45
221-234: LGTM!
Write the owner PID to a temp file, then hard-link it to the lock path so waiters never see an empty final lock, with exclusive wx fallback when links are unsupported and grace reclaim for aged empty bodies. Co-authored-by: Cursor <cursoragent@cursor.com>
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
![codefactor-io[bot]](https://avatars.githubusercontent.com/in/25603?s=60&v=4){kind=small}
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
Require exact `<pid>\n` lock bodies before reclaim/release, cancel and await finishedAt for tracked jobs in idempotency teardown, and cover stub setup timeout fallback with fake timers. Co-authored-by: Cursor <cursoragent@cursor.com>
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/studioConfigSmokeLock.mjs (1)
137-151: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftPartial numeric PID bodies still bypass the grace window.
Line 138 uses
Number.parseInt, which accepts a numeric prefix. A torn body"12345\n"read as"12"parses to a finite12.malformedis then false, so the grace window at lines 144-151 never applies, and a dead PID12reclaims the lock immediately.
publishLockFileno longer produces torn bodies, so this now applies to legacy locks and external writers only. The classification is still wrong. Require a complete numeric body before you trust the PID.🛡️ Proposed fix for the classification
const current = read(lockPath, "utf8"); - const holder = Number.parseInt(String(current).trim().split(/\r?\n/)[0] ?? "", 10); + const firstLine = String(current).trim().split(/\r?\n/)[0] ?? ""; + // Require a complete numeric body. A torn write such as "12" from + // "12345\n" must not be trusted as a finished PID. + const complete = /^\d+$/.test(firstLine) && String(current).endsWith("\n"); + const holder = complete ? Number.parseInt(firstLine, 10) : Number.NaN; // Fresh empty/malformed bodies can mean a concurrent publisher has not // finished yet (or a crash left an empty legacy lock). Only reclaim // after publishGraceMs. Finite dead PIDs reclaim immediately. const malformed = !Number.isFinite(holder);Note:
release()at line 126 uses the same prefix parse. Keep both parsers in one shared helper so they cannot diverge.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/studioConfigSmokeLock.mjs` around lines 137 - 151, Replace the prefix-based PID parsing in the shared lock-PID parsing logic used by both the classification flow and release() with complete-body validation, so partial or otherwise malformed numeric bodies are treated as malformed and remain subject to publishGraceMs. Introduce or reuse one helper for both call sites, preserving immediate reclamation only for fully parsed finite dead PIDs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/__tests__/studioConfigSmokeLock.test.ts`:
- Around line 36-41: Add a test alongside the existing studio lock tests that
configures unlinkSync to throw an EPERM error for the lock path, injects a
mocked sleep function, and invokes acquireStudioConfigSmokeLock with the
existing filesystem and polling options until acquisition fails. Assert the
operation rejects and sleep is called, covering the failed-unlink polling path.
- Around line 132-151: Update the aged-lock test around
acquireStudioConfigSmokeLock so its mocked clock advances during sleep, matching
the grace-window test pattern. Replace the constant now value with mutable time
and have the sleep stub advance it, while preserving the initial timestamp and
reclaim scenario so a regression reaches the deadline and rejects instead of
hanging.
- Around line 156-186: Update the “falls back to exclusive write when hardlinks
are unsupported” test to create its filesystem mocks through makeFs, overriding
only linkSync to throw ENOTSUP. Preserve the existing lock assertions, and add
the same temporary-residue cleanup assertion used by the link-path test around
line 62.
---
Outside diff comments:
In `@scripts/studioConfigSmokeLock.mjs`:
- Around line 137-151: Replace the prefix-based PID parsing in the shared
lock-PID parsing logic used by both the classification flow and release() with
complete-body validation, so partial or otherwise malformed numeric bodies are
treated as malformed and remain subject to publishGraceMs. Introduce or reuse
one helper for both call sites, preserving immediate reclamation only for fully
parsed finite dead PIDs.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b8262d66-3abe-446c-a52f-a7d4ddffb83d
📒 Files selected for processing (2)
scripts/studioConfigSmokeLock.mjssrc/lib/__tests__/studioConfigSmokeLock.test.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
tonythethompson/QuickShell(manual)tonythethompson/numan(manual)tonythethompson/dependency-chain-substrate(manual)
📜 Review details
⏰ Context from checks skipped due to timeout. (6)
- GitHub Check: olive-pass-availability
- GitHub Check: validate
- GitHub Check: security
- GitHub Check: python-tests
- GitHub Check: docker-build
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (6)
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
src/**/*.{ts,tsx}: Match existing naming, file layout, and TypeScript patterns insrc/.
Put shared recipe logic insrc/lib/, especiallypipelineValidation.ts,oliveRecipeBuilder.ts, andrecipePipeline.ts.
src/**/*.{ts,tsx}: Keep validation logic in shared libraries rather than duplicating it in UI cell helpers or inspectors.
Split theInputEnvironmentPanel,IHVIntegrationPanel, andExecutionWorkspacemega-panels into feature folders with colocated hooks and tests.
Keep server and UI AI provider catalogs synchronized, preferably through a shared provider ID list or synchronization test; register new providers in both catalogs.
Add test coverage forrecipe-graph/,passCatalog,oliveRecipeHub,jobHistoryStore, andvramEstimate, and strengthen component tests for the large panels.
src/**/*.{ts,tsx}: All UI state mutations must go throughcommitUiStateUpdateinsrc/lib/pipelineValidation.tsso invariants are enforced; useusePipelineState()for state access andreplaceStatefor recipe imports or preset loads.
Avoidexport *barrel imports; import directly from the actual module file to preserve Vite tree-shaking and component-test isolation.Follow the React performance guidance in
docs/REACT_BEST_PRACTICES.md, especially eliminating waterfalls, avoiding barrel imports, and deferring non-critical third-party libraries.
Files:
src/lib/__tests__/studioConfigSmokeLock.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{ts,tsx,js,jsx}: Place imports at the top of modules; use inline imports only for a documented circular dependency.
Run linting and ensure typecheck-related CI checks pass before submitting changes.
For UI or server changes, manually smoke-test development startup, recipe loading/building, validation banners, and live execution when execution behavior is touched.Use the project's React 19, Vite, Express, and Tauri 2 stack conventions for frontend and server TypeScript/JavaScript code.
Files:
src/lib/__tests__/studioConfigSmokeLock.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
When working with React 19 or Vite 8 APIs, consult current Context7 documentation instead of assuming conventions from earlier major versions.
Files:
src/lib/__tests__/studioConfigSmokeLock.test.ts
src/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use the unit-test configuration for
src/lib/unit tests and keep unit tests compatible with Vitest.
Files:
src/lib/__tests__/studioConfigSmokeLock.test.ts
src/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not trigger real Olive optimization runs in tests or CI; use CPU-only recipe building, JSON export, and validation flows instead.
Files:
src/lib/__tests__/studioConfigSmokeLock.test.ts
**/*
📄 CodeRabbit inference engine (AGENTS.md)
Do not implement the listed backburner AI providers unless explicitly requested; prefer Custom or OpenAI-compatible providers for OpenAI-shaped hosts.
Files:
src/lib/__tests__/studioConfigSmokeLock.test.tsscripts/studioConfigSmokeLock.mjs
🔍 Remote MCP DeepWiki, GitHub Copilot
Additional review context
- Issue
#186explicitly targets hardening Studio config cleanup, MCP smoke locking, job policy, and idempotency behavior. - The lock implementation atomically publishes PID contents via hard-linking, with exclusive-write fallback; malformed locks use
statSync().mtimeMsand a grace period, while dead finite-PID locks are reclaimed immediately. resetJobRegistry()cancels active setup jobs, terminates child processes, cleans artifacts, detaches listeners, stops metrics timers, and clears idempotency indexes—relevant to the detached setup-test teardown.- Idempotency semantics explicitly allow a keyed submission to adopt an active fingerprint-only MCP job, while distinct keyed jobs remain separate; failed/cancelled jobs are retryable.
- Current PR checks are not complete: validate, security, Python tests, Docker build, Olive pass availability, and Greptile remain in progress; CodeFactor and auto-merge checks succeeded.
- DeepWiki could not provide repository context because
tonythethompson/Olive-Studiois not indexed.
🔇 Additional comments (8)
scripts/studioConfigSmokeLock.mjs (5)
6-8: LGTM!
32-67: LGTM!
110-132: LGTM!
152-166: LGTM!
82-88: 📐 Maintainability & Code QualityAll injected dependencies are declared in the
depsJSDoc.src/lib/__tests__/studioConfigSmokeLock.test.ts (3)
75-93: LGTM!
95-130: LGTM!
44-63: 📐 Maintainability & Code QualityTest already covers the release behavior described by the name.
The test calls
lock.release()for both a foreign PID and an owned PID, then asserts that only the owned lock is cleared there.
Assert acquisition keeps polling when a reclaimable lock cannot be unlinked, so failed reclaim cannot busy-spin without sleep. Co-authored-by: Cursor <cursoragent@cursor.com>
Drive the hardlink-unsupported path through the shared store mocks and assert temp residue is cleaned after exclusive write fallback. Co-authored-by: Cursor <cursoragent@cursor.com>
Clear lockPath.reclaim when its owner is dead or the body is aged incomplete so a crashed reclaimer cannot pin later smoke runs until manual cleanup. Co-authored-by: Cursor <cursoragent@cursor.com>
Sleep only until the stub timeout deadline on the final poll, cancel tracked jobs and advance one fake-timer poll before finalize fallback in stub tests, and assert 500ms timeouts fire after +1ms past 499ms. Co-authored-by: Cursor <cursoragent@cursor.com>
Only dead complete reclaim PIDs may clear the gate, and reclaimers must still own lockPath.reclaim immediately before unlinking the main lock or releasing the mutex. Co-authored-by: Cursor <cursoragent@cursor.com>
Empty or malformed lockPath.reclaim markers left by a crash mid-write are cleared once publishGraceMs elapses, while fresh incomplete markers and live complete PIDs stay protected and gate ownership is still re-verified. Co-authored-by: Cursor <cursoragent@cursor.com>
Make lockPath.reclaim appear with a complete PID body atomically so age-clear of incomplete orphans cannot strip a live reclaim publisher mid-gate. Co-authored-by: Cursor <cursoragent@cursor.com>
Avoid streaming wx writes to the final lock pathname so a delayed reclaim gate publish cannot expose an incomplete body that peers age-clear. Co-authored-by: Cursor <cursoragent@cursor.com>
Satisfy tsc --noEmit in CI lint: the vitest mock used string paths while acquireStudioConfigSmokeLock expects Node PathLike for copyFileSync. Co-authored-by: Cursor <cursoragent@cursor.com>
Avoid npx+cmd shell spawning so --args JSON keeps its quotes; cmd was stripping them and breaking submit_optimization_job on Windows. Co-authored-by: Cursor <cursoragent@cursor.com>
copyFileSync can expose a partial body at the final path; rename moves an already-complete temp so reclaimers never age-clear a live publish mid-copy. Co-authored-by: Cursor <cursoragent@cursor.com>
When hardlinks fail, keep Windows rename (exclusive) but use exclusive wx writes on POSIX so rename cannot replace another process's lock file. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/mcp-agent-smoke.mjs`:
- Around line 254-265: Extract the shared CLI resolution, config-augmented
argument construction, and banner logging currently duplicated in
runMcporterSync and callToolAsync into a reusable helper. Update both functions
to call that helper, preserving the existing arguments, config flag, and log
format while ensuring future changes apply consistently.
- Around line 68-110: Update ensureMcporterCli to avoid the predictable shared
tmpdir prefix: create a unique per-run installation directory with mkdtempSync,
or use a trusted repository-local cache with appropriate ownership validation.
Ensure concurrent runs cannot observe a partially installed CLI, and preserve
the existing install verification and mcporterCliPath caching behavior.
In `@scripts/studioConfigSmokeLock.mjs`:
- Around line 211-229: Update tryAcquireReclaimGate and the corresponding guard
at the later call site to reflect exception-based control flow: remove the
unconditional boolean return and eliminate both constant-result checks, allowing
publishLockFile exceptions to propagate while preserving the existing success
path.
- Around line 98-106: The fallback in scripts/studioConfigSmokeLock.mjs at lines
98-106 must use exclusive write semantics on every platform: remove the
Windows-specific rename path or guard renameSync with an existence check, while
preserving the write(lockPath, body, { flag: "wx" }) behavior. In
src/lib/__tests__/studioConfigSmokeLock.test.ts at lines 30-42, update the
filesystem mock so rename overwrites an existing destination and adjust the
Windows fallback test to verify a live peer lock is not stolen.
In `@src/lib/__tests__/studioConfigSmokeLock.test.ts`:
- Around line 237-264: Update the aged-incomplete reclaim test around
acquireStudioConfigSmokeLock so the mocked clock advances whenever sleep is
called, allowing timeout-based loops to terminate if acquisition regresses. Keep
sleep asynchronous while advancing the value returned by now beyond the
deadline, and preserve the existing recovery assertions.
In `@src/server/services/olive/jobRunner.stubSetup.test.ts`:
- Around line 152-159: Update the pending-settlement check in the test around
pending.then so both fulfillment and rejection callbacks set settled = true.
Attach a rejection handler to the derived promise as well, preventing an
unhandled rejection while preserving the assertion that the request remains
unsettled before the deadline.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f9279bf2-339c-4604-9df4-f9bd5a951d72
📒 Files selected for processing (6)
scripts/mcp-agent-smoke.mjsscripts/studioConfigSmokeLock.mjssrc/lib/__tests__/studioConfigSmokeLock.test.tssrc/server/services/olive/jobRunner.stubSetup.test.tssrc/server/services/olive/jobRunner.tsvite.config.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
tonythethompson/QuickShell(manual)tonythethompson/numan(manual)tonythethompson/dependency-chain-substrate(manual)
📜 Review details
⏰ Context from checks skipped due to timeout. (6)
- GitHub Check: Greptile Review
- GitHub Check: security
- GitHub Check: docker-build
- GitHub Check: olive-pass-availability
- GitHub Check: python-tests
- GitHub Check: validate
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{ts,tsx,js,jsx}: Place imports at the top of modules; use inline imports only for a documented circular dependency.
Run linting and ensure typecheck-related CI checks pass before submitting changes.
For UI or server changes, manually smoke-test development startup, recipe loading/building, validation banners, and live execution when execution behavior is touched.Use the project's React 19, Vite, Express, and Tauri 2 stack conventions for frontend and server TypeScript/JavaScript code.
Files:
vite.config.tssrc/server/services/olive/jobRunner.stubSetup.test.tssrc/server/services/olive/jobRunner.tssrc/lib/__tests__/studioConfigSmokeLock.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
When working with React 19 or Vite 8 APIs, consult current Context7 documentation instead of assuming conventions from earlier major versions.
Files:
vite.config.tssrc/server/services/olive/jobRunner.stubSetup.test.tssrc/server/services/olive/jobRunner.tssrc/lib/__tests__/studioConfigSmokeLock.test.ts
**/*
📄 CodeRabbit inference engine (AGENTS.md)
Do not implement the listed backburner AI providers unless explicitly requested; prefer Custom or OpenAI-compatible providers for OpenAI-shaped hosts.
Files:
vite.config.tssrc/server/services/olive/jobRunner.stubSetup.test.tssrc/server/services/olive/jobRunner.tsscripts/mcp-agent-smoke.mjssrc/lib/__tests__/studioConfigSmokeLock.test.tsscripts/studioConfigSmokeLock.mjs
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
src/**/*.{ts,tsx}: Match existing naming, file layout, and TypeScript patterns insrc/.
Put shared recipe logic insrc/lib/, especiallypipelineValidation.ts,oliveRecipeBuilder.ts, andrecipePipeline.ts.
src/**/*.{ts,tsx}: Keep validation logic in shared libraries rather than duplicating it in UI cell helpers or inspectors.
Split theInputEnvironmentPanel,IHVIntegrationPanel, andExecutionWorkspacemega-panels into feature folders with colocated hooks and tests.
Keep server and UI AI provider catalogs synchronized, preferably through a shared provider ID list or synchronization test; register new providers in both catalogs.
Add test coverage forrecipe-graph/,passCatalog,oliveRecipeHub,jobHistoryStore, andvramEstimate, and strengthen component tests for the large panels.
src/**/*.{ts,tsx}: All UI state mutations must go throughcommitUiStateUpdateinsrc/lib/pipelineValidation.tsso invariants are enforced; useusePipelineState()for state access andreplaceStatefor recipe imports or preset loads.
Avoidexport *barrel imports; import directly from the actual module file to preserve Vite tree-shaking and component-test isolation.Follow the React performance guidance in
docs/REACT_BEST_PRACTICES.md, especially eliminating waterfalls, avoiding barrel imports, and deferring non-critical third-party libraries.
Files:
src/server/services/olive/jobRunner.stubSetup.test.tssrc/server/services/olive/jobRunner.tssrc/lib/__tests__/studioConfigSmokeLock.test.ts
src/server/services/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Keep server-side business logic in services, including AI providers, Olive job/virtual-environment handling, and related service modules.
Files:
src/server/services/olive/jobRunner.stubSetup.test.tssrc/server/services/olive/jobRunner.ts
src/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use the unit-test configuration for
src/lib/unit tests and keep unit tests compatible with Vitest.
Files:
src/server/services/olive/jobRunner.stubSetup.test.tssrc/lib/__tests__/studioConfigSmokeLock.test.ts
src/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not trigger real Olive optimization runs in tests or CI; use CPU-only recipe building, JSON export, and validation flows instead.
Files:
src/server/services/olive/jobRunner.stubSetup.test.tssrc/lib/__tests__/studioConfigSmokeLock.test.ts
🪛 ast-grep (0.45.0)
src/lib/__tests__/studioConfigSmokeLock.test.ts
[warning] 281-281: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFileSync(p, body, opts as never)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
🔍 Remote MCP DeepWiki, GitHub Copilot
Additional review context
- Issue
#186explicitly targets MCP smoke hardening and Olive job policy/idempotency cleanup. - Related PR
#28established the surrounding lifecycle behavior: setup cancellation, registry cleanup, SSE terminal handling, and artifact reclamation. - Related PR
#171introduced the MCP agent smoke path and job-control flow; this PR substantially extends that smoke with policy denial, concurrent idempotent submissions, cancellation, and cleanup checks. - The current implementation’s documented contract is:
- MCP jobs are the only jobs indexed for idempotency.
- Same keys reuse jobs; fingerprint-only jobs can later be adopted by keyed submissions.
- Distinct keys create distinct jobs.
- Failed/cancelled jobs are retryable.
- UI jobs must not be exposed through agent routes or absorbed by MCP fingerprint reuse.
- Agent capability defaults are conservative: inspection is enabled, submission/cancellation are disabled unless policy or environment overrides enable them. Agent routes are loopback-gated, and artifact paths are redacted unless explicitly opted into.
- The smoke test deliberately uses
OLIVE_JOB_SETUP_STUB=1, so it exercises policy, status, cancellation, and idempotency without spawning a real Olive optimization. - DeepWiki could not provide repository architecture context because
tonythethompson/Olive-Studiois not indexed.
🔇 Additional comments (14)
vite.config.ts (1)
8-8: LGTM!scripts/studioConfigSmokeLock.mjs (4)
56-64: LGTM!
146-169: LGTM!
247-265: LGTM!
289-304: LGTM!Also applies to: 369-388
src/lib/__tests__/studioConfigSmokeLock.test.ts (4)
145-201: LGTM!
203-235: LGTM!
266-311: LGTM!
516-567: LGTM!scripts/mcp-agent-smoke.mjs (2)
116-134: LGTM!
74-74: 🩺 Stability & AvailabilityNo change needed for the
mcporterCLI path. The pinnedmcporter@0.13.0package declaresbin.mcporterasdist/cli.js, so this hard-coded path matches the package entry map.> Likely an incorrect or invalid review comment.src/server/services/olive/jobRunner.ts (2)
316-346: Complete the required validation before merge.The PR context reports CI as unchecked. Run lint, typecheck-related checks, and the required server smoke tests before submission.
As per coding guidelines, “Run linting and ensure typecheck-related CI checks pass before submitting changes.” As per coding guidelines, “For UI or server changes, manually smoke-test development startup, recipe loading/building, validation banners, and live execution when execution behavior is touched.”
Source: Coding guidelines
316-346: LGTM!src/server/services/olive/jobRunner.stubSetup.test.ts (1)
55-66: LGTM!Also applies to: 97-97, 117-123, 125-151, 160-172
Use per-run mkdtemp for mcporter install, DRY invocation helpers, exclusive wx on all hardlink-fail platforms, void reclaim-gate helpers, and tighten related unit tests (clock advance, rejection-safe settled check). Co-authored-by: Cursor <cursoragent@cursor.com>
A paused wx publisher can leave a partial final-path body past any grace window; age-clearing it lets a peer steal exclusivity. Only reclaim complete dead PIDs; smokers wait out timeoutMs for incomplete leftovers. Co-authored-by: Cursor <cursoragent@cursor.com>
Summary
OLIVE_JOB_SETUP_STUBwait loops with a finite timeout (default 120s, overridable viaOLIVE_JOB_SETUP_STUB_TIMEOUT_MS) and fail the job on expirationrmdirSyncfor empty.olive-studiocleanupresolvePythonpreference testsSkipped (still not valid)
state.test.tsimport reorder: vitest requiresvi.mockbefore importing modules that load the mocked dependency; current order aftervi.hoisted/vi.mockis correctTest plan
pnpm exec vitest run src/lib/__tests__/resolvePython.test.ts src/lib/__tests__/studioConfigSmokeLock.test.ts src/lib/__tests__/studioConfigSnapshot.test.tspnpm exec vitest run src/server/services/olive/jobRunner.idempotency.test.ts --config vitest.server.config.tsMade with Cursor