fix(mcp): MCP_harden review follow-ups (Codex + CodeRabbit) - #184
Conversation
|
Deployment failed for project olive-studio with the following error: Learn More: https://vercel.com/trackdub?upgradeToPro=build-rate-limit |
📝 WalkthroughWalkthroughThe PR adds deterministic semantic-worker cleanup, explicit budget fallback tests, serialized MCP submission locks, Olive job-registry reset support, Python and Studio configuration utilities, and expanded MCP smoke and route integration validation. ChangesSemantic retrieval budget handling
Olive job state and submission control
MCP Studio smoke validation
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
Suggested reviewers: 🚥 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 |
![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
PR Summary by QodoHarden MCP job controls, tool allowlisting, and test isolation
AI Description
Diagram
High-Level Assessment
Files changed (13)
|
|
Pushed follow-up |
![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: 25ced6a26e
ℹ️ 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".
Greptile SummaryThe PR expands the MCP smoke to exercise policy-gated job submission, idempotent reuse, status, and cancellation while adding exact configuration restoration, inter-process locking, and bounded submission-lock state.
Confidence Score: 0/5The PR is not yet safe to merge because configuration restoration can still report success while leaving permissive policy behind, and concurrent configuration updates can still be lost. The current catch path ignores restoration failure when selecting the default non-strict exit code, while the snapshot-to-write and write-to-reread windows still permit unrelated configuration bytes to be overwritten or treated as smoke-owned. Files Needing Attention: scripts/mcp-agent-smoke.mjs, scripts/studioConfigSnapshot.mjs
|
| Filename | Overview |
|---|---|
| scripts/mcp-agent-smoke.mjs | Adds the end-to-end job-control smoke and cleanup protocol, but previously reported configuration restoration and concurrent-write failures remain. |
| scripts/studioConfigSnapshot.mjs | Adds exact snapshot restoration and a conditional final-state check, though that check cannot protect writes occurring before the smoke's first mutation. |
| scripts/studioConfigSmokeLock.mjs | Adds exclusive locking that correctly serializes cooperating smoke processes. |
| src/server/services/olive/jobRunner.ts | Adds ordered MCP submission locks with ownership-checked tail eviction; the previously reported retention issue is resolved. |
| src/server/services/olive/state.ts | Adds registry reset behavior used to isolate integration tests. |
Reviews (12): Last reviewed commit: "fix(mcp): stamp smoke config ownership a..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
2e2db52 to
6b99b3c
Compare
|
Rebuilt this stack on the rebased |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Qodo Fixer✅ Committed (2) · ☑ Fixed (2) Commits pushed directly to this PR — no separate fix PR opened. Process — 2 fixed
|
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 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 306-310: Update scripts/mcp-agent-smoke.mjs at lines 306-310 to
snapshot the raw config bytes and whether the file existed, rather than coercing
agentAccess booleans; update cleanup() at lines 431-440 to restore those exact
bytes or delete the file when it was originally absent. In readStudioDiskConfig
at lines 101-119, distinguish an absent file from an unreadable or unparsable
file and fail the smoke run on parse errors so patchAgentAccessDisk never
overwrites an invalid existing config.
- Around line 246-255: Update startStudio so its spawned child environment
explicitly removes OLIVE_MCP_ALLOW_JOBS after spreading process.env, ensuring
the variable is unset regardless of the parent environment. Keep the existing
comment aligned with this enforced deny-path behavior.
- Around line 416-423: Update the post-cancellation status check in the smoke
test to poll olive.get_optimization_job until the job reports status "cancelled"
and terminal true, using a bounded deadline and retry delay. Preserve the
existing timeout and error context, and throw only when the deadline expires
without observing the settled state.
- Around line 139-142: Update run() so the expectOk failure path throws an error
containing the mcporter status and signal instead of calling process.exit.
Preserve the existing failure condition, allowing the surrounding try/catch to
invoke cleanup() for temporary resources, child processes, and patched policy
state.
- Around line 147-155: Update parseJsonPayload to locate the true start of the
trailing JSON value instead of using lastIndexOf on opening braces or brackets;
scan forward from the first structural character of the trailing value or parse
the last non-empty line emitted by mcporter --output json. Preserve the existing
empty-output, missing-JSON, and JSON.parse error behavior for valid nested
object and array payloads.
- Around line 62-76: Update resolvePython so bare command candidates are
validated through PATH before returning them, rather than immediately returning
the first bare name and making python unreachable. Preserve the ordered
candidate chain, selecting the first available interpreter—including python when
python3 is unavailable—and retain the final fallback behavior if none can be
found.
- Around line 162-183: Update callToolAsync to preserve the parent environment
when opts.env is omitted by falling back to process.env in the spawn
environment. Also align the timeout error message with the actual timer delay by
reporting timeoutMs plus the additional 15-second grace period.
- Around line 351-368: Prevent the smoke test’s concurrent
olive.submit_optimization_job calls from launching real Olive processes.
Configure a no-op or stub executor for the submit path by default, while
allowing real execution only through an explicit opt-in flag; preserve the
existing idempotency and concurrency assertions.
In `@src/server/__tests__/setup.integration.ts`:
- Line 20: Remove the duplicate childProcessLaunchLog declaration in the
integration test setup module, retaining a single exported const with the
existing type and initialization.
In `@src/server/services/olive/jobRunner.ts`:
- Around line 137-150: The lock admission flow around startMcpOliveJobLocked
must reuse a job registered by an overlapping fingerprint-only submission when a
keyed request arrives afterward, while preserving sequential distinct-key
behavior; retain or propagate sufficient admission state for fingerprint
fallback without weakening explicit-key conflict checks. In
src/server/services/olive/jobRunner.ts lines 137-150, update the
locking/submission path accordingly. In
src/server/services/olive/jobRunner.idempotency.test.ts lines 54-65, add a
regression test that starts the fingerprint-only request before the keyed
request and asserts exactly one job registry entry.
- Around line 65-99: Update withMcpSubmitLocks to retain each assigned tail
promise and, after that tail resolves, delete its key from mcpSubmitLockTails
only when the map still references the same promise. Preserve newer queued tails
by guarding cleanup with exact promise identity, and keep lock release behavior
unchanged.
In `@src/server/services/olive/state.ts`:
- Around line 67-85: Update src/server/services/olive/state.ts:67-85 in
resetJobRegistry to use the normal venv-listener detach function, clear and null
metricsTimer, and reset sampling state for each job before cleanup and
finalization. Update src/server/services/olive/state.test.ts:145-175 to add
listener and timer fixtures and assert that reset detaches the listener and
clears both resources.
🪄 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: 09eec32c-31ba-4189-97dd-7787f453b432
📒 Files selected for processing (13)
olive-mcp-server/tests/conftest.pyolive-mcp-server/tests/test_capabilities.pyolive-mcp-server/tests/test_docs_search_semantic.pyscripts/mcp-agent-smoke.mjssrc/server/__tests__/routes.integration.test.tssrc/server/__tests__/setup.integration.tssrc/server/routes/mcp.test.tssrc/server/routes/olive.stream.test.tssrc/server/services/olive/jobPreflight.test.tssrc/server/services/olive/jobRunner.idempotency.test.tssrc/server/services/olive/jobRunner.tssrc/server/services/olive/state.test.tssrc/server/services/olive/state.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)
💤 Files with no reviewable changes (1)
- olive-mcp-server/tests/test_docs_search_semantic.py
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Greptile Review
⚠️ CI failures not shown inline (1)
Commit Status: Vercel: Vercel
Conclusion: failure
Deployment rate limited — retry in 24 hours.
🧰 Additional context used
📓 Path-based instructions (12)
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/routes/mcp.test.tssrc/server/services/olive/jobPreflight.test.tssrc/server/services/olive/jobRunner.tssrc/server/services/olive/state.tssrc/server/services/olive/state.test.tssrc/server/__tests__/routes.integration.test.tssrc/server/routes/olive.stream.test.tssrc/server/services/olive/jobRunner.idempotency.test.tssrc/server/__tests__/setup.integration.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/server/routes/mcp.test.tssrc/server/services/olive/jobPreflight.test.tssrc/server/services/olive/jobRunner.tssrc/server/services/olive/state.tssrc/server/services/olive/state.test.tssrc/server/__tests__/routes.integration.test.tssrc/server/routes/olive.stream.test.tssrc/server/services/olive/jobRunner.idempotency.test.tssrc/server/__tests__/setup.integration.ts
src/server/routes/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Each API route module must export a
mountXxxRoutes(router)function and be wired intoserver.ts.
Files:
src/server/routes/mcp.test.tssrc/server/routes/olive.stream.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/server/routes/mcp.test.tssrc/server/services/olive/jobPreflight.test.tssrc/server/services/olive/jobRunner.tssrc/server/services/olive/state.tssrc/server/services/olive/state.test.tssrc/server/__tests__/routes.integration.test.tssrc/server/routes/olive.stream.test.tssrc/server/services/olive/jobRunner.idempotency.test.tssrc/server/__tests__/setup.integration.ts
src/server/routes/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Organize Express server endpoints as modular route modules under
src/server/routes/.
Files:
src/server/routes/mcp.test.tssrc/server/routes/olive.stream.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/server/routes/mcp.test.tssrc/server/services/olive/jobPreflight.test.tssrc/server/services/olive/state.test.tssrc/server/__tests__/routes.integration.test.tssrc/server/routes/olive.stream.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/server/routes/mcp.test.tssrc/server/services/olive/jobPreflight.test.tssrc/server/services/olive/state.test.tssrc/server/__tests__/routes.integration.test.tssrc/server/routes/olive.stream.test.tssrc/server/services/olive/jobRunner.idempotency.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/server/routes/mcp.test.tssrc/server/services/olive/jobPreflight.test.tssrc/server/services/olive/jobRunner.tssrc/server/services/olive/state.tsolive-mcp-server/tests/conftest.pysrc/server/services/olive/state.test.tssrc/server/__tests__/routes.integration.test.tssrc/server/routes/olive.stream.test.tssrc/server/services/olive/jobRunner.idempotency.test.tsolive-mcp-server/tests/test_capabilities.pyscripts/mcp-agent-smoke.mjssrc/server/__tests__/setup.integration.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/jobPreflight.test.tssrc/server/services/olive/jobRunner.tssrc/server/services/olive/state.tssrc/server/services/olive/state.test.tssrc/server/services/olive/jobRunner.idempotency.test.ts
olive-mcp-server/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Pin the Python
mcpdependency to a version below 2 because version 2.x removesmcp.server.fastmcpand breaks imports.Implement the Olive MCP server as a Python FastMCP stdio server compatible with Python >=3.10.
Files:
olive-mcp-server/tests/conftest.pyolive-mcp-server/tests/test_capabilities.py
olive-mcp-server/tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Run and maintain pytest coverage for the Olive MCP tools; use
python -m pytest tests -qfromolive-mcp-server.
Files:
olive-mcp-server/tests/conftest.pyolive-mcp-server/tests/test_capabilities.py
src/server/**/__tests__/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Server tests must use the server Vitest configuration; integration tests mock child_process, AI providers, and fetch and run against a real Express server on a random port.
Files:
src/server/__tests__/routes.integration.test.tssrc/server/__tests__/setup.integration.ts
🪛 ast-grep (0.45.0)
src/server/services/olive/state.test.ts
[warning] 148-148: 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(tmp, "{}", "utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
🔍 Remote MCP GitHub Copilot
Review-relevant context
-
MCP lock-tail leak:
mcpSubmitLockTailsstores a promise for every fingerprint/key, but cleanup only resolves promises; entries are never deleted. Long-lived Studio processes can accumulate these keys indefinitely. -
Smoke test may execute real Olive: The allowed path starts a real
server.ts, whose job setup eventually callsspawn(...). Repository instructions explicitly prohibit real Olive optimization runs in CI/VM checks; a stubbed executor is needed. -
Smoke configuration restoration is lossy: The script snapshots only boolean policy values and writes them back during cleanup. Missing fields—and potentially a previously nonexistent config file—are not restored exactly. It also inherits
OLIVE_MCP_ALLOW_JOBSinto the Studio child process, which can override the denied disk policy. -
Python fallback bug:
resolvePython()immediately returnspython3without checking PATH availability, making the documentedpythonfallback unreachable. -
Listener cleanup:
resetJobRegistry()clearsjob.venvListenerwithout callingdetachVenvListener(), while the normal cancellation path explicitly detaches it. -
Validation status: PR
#184’s reported targeted validation passed, and its current CI checks—including CodeQL, Python tests, security, validation, Docker build, and Olive-pass availability—are successful. However, the reported smoke validation only runsnode --check; it does not demonstrate execution of the new smoke workflow.
🔇 Additional comments (13)
olive-mcp-server/tests/conftest.py (1)
1-13: LGTM!Also applies to: 16-39, 42-45, 48-53
olive-mcp-server/tests/test_capabilities.py (1)
123-145: LGTM!Also applies to: 163-201
src/server/services/olive/jobRunner.idempotency.test.ts (1)
19-25: LGTM!Also applies to: 39-52, 68-77
src/server/services/olive/state.ts (1)
4-8: LGTM!src/server/__tests__/routes.integration.test.ts (1)
13-18: LGTM!Also applies to: 58-59, 626-651
src/server/services/olive/jobPreflight.test.ts (1)
1-13: LGTM!Also applies to: 124-140
src/server/routes/mcp.test.ts (1)
233-240: LGTM!src/server/routes/olive.stream.test.ts (1)
11-16: LGTM!scripts/mcp-agent-smoke.mjs (5)
14-31: LGTM!
33-56: LGTM!
217-243: LGTM!
261-298: LGTM!
427-458: LGTM!Also applies to: 460-491
Rebuild the #184 stack on current MCP_harden and fix the Codex findings: - Stub Olive setup under OLIVE_JOB_SETUP_STUB so agent smoke never downloads models or runs olive - Snapshot/restore exact Studio config on SIGINT/SIGTERM and strip inherited OLIVE_MCP_ALLOW_JOBS from the Studio child - Serialize MCP submits with lock tails that evict when released - resetJobRegistry detaches venv listeners; restore the review follow-up tests Co-authored-by: Anthony Thompson <github@trackdub.com>
77496b2 to
e27b48d
Compare
|
Addressed Codex review on this PR (rebuilt on current
Also kept Greptile/Qodo overlap: |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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)
src/server/services/olive/state.ts (1)
68-90: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMark active jobs as cancelled before registry cleanup.
resetJobRegistry()removes a stubbed job without changing itssetting_upstatus.continueOliveJobSetup()then keeps its polling timer active because no process exists for this reset path to terminate. Set active jobs tocancelledbefore cleanup so setup code exits.Proposed fix
export function resetJobRegistry(): void { for (const job of [...jobRegistry.values()]) { + if (job.status === "setting_up" || job.status === "running") { + job.status = "cancelled"; + } if (job.venvListener) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/services/olive/state.ts` around lines 68 - 90, Update resetJobRegistry() to mark each active job as cancelled before cleanupJobArtifacts() and finalizeJob() run. Ensure the setting_up state is transitioned to cancelled so continueOliveJobSetup() stops polling even when no process exists, while preserving existing listener detachment and process termination behavior.
🤖 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 509-516: Update the restoration error handling around
restoreStudioConfigFile to retain the caught error, complete all existing
process and temporary-file cleanup, then rethrow the restoration error so the
smoke run fails instead of only warning. Preserve the current cleanup behavior
and error-message handling for successful restoration.
In `@src/lib/__tests__/studioConfigSnapshot.test.ts`:
- Around line 21-24: Replace the JSDoc-only declarations for the fixture
variables root and configPath with native TypeScript string type annotations,
preserving their existing let declarations and usage.
In `@src/server/services/olive/jobRunner.idempotency.test.ts`:
- Around line 28-31: Move the static imports for preflightOliveRecipe,
mcpSubmitLockTailCount, startOliveJob, clearIdempotencyIndex, and jobRegistry to
the module’s top import section before all vi.mock declarations. Preserve the
existing mocked-module behavior and run the Vitest file to verify the hoisted
mocks still work.
---
Outside diff comments:
In `@src/server/services/olive/state.ts`:
- Around line 68-90: Update resetJobRegistry() to mark each active job as
cancelled before cleanupJobArtifacts() and finalizeJob() run. Ensure the
setting_up state is transitioned to cancelled so continueOliveJobSetup() stops
polling even when no process exists, while preserving existing listener
detachment and process termination behavior.
🪄 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: a748a5d8-6092-44b9-b37d-b8d0a2f4b56c
📒 Files selected for processing (9)
scripts/mcp-agent-smoke.mjsscripts/resolvePython.mjsscripts/studioConfigSnapshot.mjssrc/lib/__tests__/resolvePython.test.tssrc/lib/__tests__/studioConfigSnapshot.test.tssrc/server/__tests__/routes.integration.test.tssrc/server/services/olive/jobRunner.idempotency.test.tssrc/server/services/olive/jobRunner.tssrc/server/services/olive/state.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: security
- GitHub Check: olive-pass-availability
- GitHub Check: docker-build
- GitHub Check: python-tests
⚠️ CI failures not shown inline (1)
Commit Status: Vercel: Vercel
Conclusion: failure
Deployment was blocked
🧰 Additional context used
📓 Path-based instructions (8)
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__/studioConfigSnapshot.test.tssrc/server/__tests__/routes.integration.test.tssrc/lib/__tests__/resolvePython.test.tssrc/server/services/olive/jobRunner.idempotency.test.tssrc/server/services/olive/state.tssrc/server/services/olive/jobRunner.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__/studioConfigSnapshot.test.tssrc/server/__tests__/routes.integration.test.tssrc/lib/__tests__/resolvePython.test.tssrc/server/services/olive/jobRunner.idempotency.test.tssrc/server/services/olive/state.tssrc/server/services/olive/jobRunner.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__/studioConfigSnapshot.test.tssrc/server/__tests__/routes.integration.test.tssrc/lib/__tests__/resolvePython.test.tssrc/server/services/olive/jobRunner.idempotency.test.tssrc/server/services/olive/state.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/lib/__tests__/studioConfigSnapshot.test.tssrc/server/__tests__/routes.integration.test.tssrc/lib/__tests__/resolvePython.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__/studioConfigSnapshot.test.tssrc/server/__tests__/routes.integration.test.tssrc/lib/__tests__/resolvePython.test.tssrc/server/services/olive/jobRunner.idempotency.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__/studioConfigSnapshot.test.tssrc/server/__tests__/routes.integration.test.tssrc/lib/__tests__/resolvePython.test.tsscripts/resolvePython.mjssrc/server/services/olive/jobRunner.idempotency.test.tssrc/server/services/olive/state.tsscripts/studioConfigSnapshot.mjsscripts/mcp-agent-smoke.mjssrc/server/services/olive/jobRunner.ts
src/server/**/__tests__/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Server tests must use the server Vitest configuration; integration tests mock child_process, AI providers, and fetch and run against a real Express server on a random port.
Files:
src/server/__tests__/routes.integration.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.idempotency.test.tssrc/server/services/olive/state.tssrc/server/services/olive/jobRunner.ts
🪛 GitHub Check: validate
src/lib/__tests__/studioConfigSnapshot.test.ts
[failure] 55-55:
Variable 'configPath' implicitly has an 'any' type.
[failure] 54-54:
Variable 'configPath' implicitly has an 'any' type.
[failure] 42-42:
Variable 'configPath' implicitly has an 'any' type.
[failure] 40-40:
Variable 'configPath' implicitly has an 'any' type.
[failure] 38-38:
Variable 'configPath' implicitly has an 'any' type.
[failure] 36-36:
Variable 'configPath' implicitly has an 'any' type.
[failure] 32-32:
Variable 'root' implicitly has an 'any' type.
[failure] 24-24:
Variable 'configPath' implicitly has type 'any' in some locations where its type cannot be determined.
[failure] 22-22:
Variable 'root' implicitly has type 'any' in some locations where its type cannot be determined.
src/lib/__tests__/resolvePython.test.ts
[failure] 16-16:
Type 'Mock<(p: string) => boolean>' is not assignable to type '((path: PathLike) => boolean) | undefined'.
🔍 Remote MCP DeepWiki, GitHub Copilot
Review-relevant context
- Security issue remains:
cleanup()catches and suppressesrestoreStudioConfigFile()failures, so the smoke can exit successfully while leavingallowJobSubmission: trueon disk. This was flagged by Greptile and is still present in the PR diff. - CI status: PR
#184is currently blocked; thevalidatecheck has failed, while several other checks remain in progress. - Other previously reported findings were addressed: async Studio shutdown escalation, signal cleanup, Python fallback probing, venv-listener detachment, lock-tail eviction, and exact config snapshot/restore.
- Architecture lookup limitation: DeepWiki could not index
tonythethompson/Olive-Studio, so no repository-architecture context was available from that source.
🔇 Additional comments (13)
src/server/services/olive/jobRunner.idempotency.test.ts (2)
65-70: Cover the fingerprint-only-first submission order.Line 68 starts the keyed request before Line 69 starts the fingerprint-only request. This does not cover the prior failure mode where the fingerprint-only request registers first. Start the fingerprint-only request first, then start the keyed request, and assert one job registry entry.
58-63: LGTM!Also applies to: 77-142
src/server/services/olive/state.ts (1)
78-90: Stop the metrics timer during registry reset.
resetJobRegistry()still leavesjob.metricsTimeractive. Clearing the registry does not clear its interval. This remains the existing resource-cleanup finding.src/server/services/olive/jobRunner.ts (2)
68-71: 📐 Maintainability & Code QualityResolve the failed validation before merge.
The reported checks do not show successful lint and typecheck results. The
validatecheck is currently failing. Run the required checks and resolve the failure before merge.As per coding guidelines, “Run linting and ensure typecheck-related CI checks pass before submitting changes.”
Sources: Coding guidelines, MCP tools
77-112: LGTM!src/server/__tests__/routes.integration.test.ts (2)
58-58: 📐 Maintainability & Code QualityRun this suite with the server Vitest configuration.
Confirm that the server integration suite completed with its required configuration. Resolve the currently failing validation check before merge.
As per coding guidelines, “Server tests must use the server Vitest configuration; integration tests mock child_process, AI providers, and fetch and run against a real Express server on a random port.”
Sources: Coding guidelines, MCP tools
627-627: LGTM!scripts/mcp-agent-smoke.mjs (1)
10-11: LGTM!Also applies to: 30-34, 67-87, 275-325, 350-503, 505-508, 517-539
scripts/resolvePython.mjs (1)
1-58: LGTM!scripts/studioConfigSnapshot.mjs (1)
1-50: LGTM!src/lib/__tests__/resolvePython.test.ts (2)
1-12: LGTM!Also applies to: 23-79
13-17: 🎯 Functional CorrectnessNo change needed for
existsSyncparameter typing.
existsSync(c)is only called with string paths, and the untyped mock in theresolvePython()dependency option is acceptable becauseresolvePython()is JavaScript with JSDoc.> Likely an incorrect or invalid review comment.src/lib/__tests__/studioConfigSnapshot.test.ts (1)
1-20: LGTM!Also applies to: 26-74
Replay the PR #184 tip tree onto MCP_harden after main was merged (vercel.json retained). Intermediate commit rebase conflicted on smoke/jobRunner/state history merges. Co-authored-by: Cursor <cursoragent@cursor.com>
02fa7d2 to
38bd8e7
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
scripts/mcp-agent-smoke.mjs (2)
189-201: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUse
exitCode/signalCodeto decide the escalation kill.
child.killedonly meansSIGTERMwas sent successfully, not thatmcporterexited. A process that ignoresSIGTERMwill skipSIGKILLat line 191 and can leave the smoke flow waiting after rejection.Proposed fix
killEscalation = setTimeout(() => { try { - if (!child.killed) child.kill("SIGKILL"); + if (child.exitCode === null && child.signalCode === null) { + child.kill("SIGKILL"); + } } catch { /* ignore */ }🤖 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/mcp-agent-smoke.mjs` around lines 189 - 201, Update the escalation callback in the child-process timeout flow to use exitCode and signalCode to determine whether mcporter has exited, rather than relying on child.killed. Send SIGKILL only while both exitCode and signalCode indicate the process is still running, preserving the existing error rejection and timer behavior.
516-533: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRestore the Studio configuration and remove the temp config in
finally.If
stopStudio(studioChild)rejects,cleanup()exits before restoringSTUDIO_CONFIG_PATHor removingsmokeConfigDir, leaving the smoke run with a modified job-submission policy on disk. Keep the shutdown order, but add the restore andrmSync(smokeConfigDir, ...)cleanup to afinallyblock surrounding the await, then add a regression test for this path.🤖 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/mcp-agent-smoke.mjs` around lines 516 - 533, Update cleanup() so the existing stopStudio(studioChild) await remains first, while restoring STUDIO_CONFIG_PATH and removing smokeConfigDir execute in a finally block even when shutdown rejects; preserve configRestoreError handling and add a regression test covering stopStudio failure.src/server/services/olive/jobRunner.ts (1)
309-330: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winExit the setup stub loop on finalization.
The stub loop only checks
job.status, butfinalizeJob()can setfinishedAtand drain subscribers without changing the status. A terminal condition like cancelled cleanup can leave this polling promise alive and keep the process running. Add afinishedAtguard or await a job completion signal, and ensure the returned status is terminal before cleanup completes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/services/olive/jobRunner.ts` around lines 309 - 330, The stub setup polling loop in the job runner must also exit when the job is finalized, not only when status changes from setting_up. Update the stubSetup branch around the loop to guard on job.finishedAt or an equivalent completion signal, then perform cleanup and finalization only after the wait ends and return the resulting terminal job.status.
🤖 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/server/services/olive/jobRunner.idempotency.test.ts`:
- Around line 80-95: Extend the test around startOliveJob to submit a third
request using the adopted "after-fp-key" idempotency key. Assert that this
replay succeeds, returns the same jobId as fpOnly, and has reused set to true,
while preserving the existing registry and lock assertions.
In `@src/server/services/olive/state.test.ts`:
- Around line 188-204: Extend the test around resetJobRegistry to spy on
globalThis.clearInterval or use Vitest timer assertions, and verify that the
specific handle created for job.metricsTimer was cleared. Keep the existing
assertions for listener detachment and metricsTimer being set to null.
---
Outside diff comments:
In `@scripts/mcp-agent-smoke.mjs`:
- Around line 189-201: Update the escalation callback in the child-process
timeout flow to use exitCode and signalCode to determine whether mcporter has
exited, rather than relying on child.killed. Send SIGKILL only while both
exitCode and signalCode indicate the process is still running, preserving the
existing error rejection and timer behavior.
- Around line 516-533: Update cleanup() so the existing stopStudio(studioChild)
await remains first, while restoring STUDIO_CONFIG_PATH and removing
smokeConfigDir execute in a finally block even when shutdown rejects; preserve
configRestoreError handling and add a regression test covering stopStudio
failure.
In `@src/server/services/olive/jobRunner.ts`:
- Around line 309-330: The stub setup polling loop in the job runner must also
exit when the job is finalized, not only when status changes from setting_up.
Update the stubSetup branch around the loop to guard on job.finishedAt or an
equivalent completion signal, then perform cleanup and finalization only after
the wait ends and return the resulting terminal job.status.
🪄 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: 1c07bb49-5a14-461d-961e-8a1d3be4707e
📒 Files selected for processing (7)
scripts/mcp-agent-smoke.mjssrc/server/services/olive/jobIdempotency.test.tssrc/server/services/olive/jobIdempotency.tssrc/server/services/olive/jobRunner.idempotency.test.tssrc/server/services/olive/jobRunner.tssrc/server/services/olive/state.test.tssrc/server/services/olive/state.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
⚠️ CI failures not shown inline (2)
GitHub Actions: CI / validate: fix(mcp): MCP_harden review follow-ups (Codex + CodeRabbit)
Conclusion: failure
##[group]Run pnpm lint
�[36;1mpnpm lint�[0m
shell: /usr/bin/bash -e {0}
env:
PNPM_HOME: /home/runner/setup-pnpm/node_modules/.bin
##[endgroup]
$ tsc --noEmit && eslint
##[error]src/lib/__tests__/resolvePython.test.ts(16,9): error TS2322: Type 'Mock<(p: string) => boolean>' is not assignable to type '((path: PathLike) => boolean) | undefined'.
GitHub Actions: CI / 0_validate.txt: fix(mcp): MCP_harden review follow-ups (Codex + CodeRabbit)
Conclusion: failure
##[group]Run pnpm lint
�[36;1mpnpm lint�[0m
shell: /usr/bin/bash -e {0}
env:
PNPM_HOME: /home/runner/setup-pnpm/node_modules/.bin
##[endgroup]
$ tsc --noEmit && eslint
##[error]src/lib/__tests__/resolvePython.test.ts(16,9): error TS2322: Type 'Mock<(p: string) => boolean>' is not assignable to type '((path: PathLike) => boolean) | undefined'.
🧰 Additional context used
📓 Path-based instructions (7)
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/jobIdempotency.test.tssrc/server/services/olive/jobRunner.idempotency.test.tssrc/server/services/olive/jobIdempotency.tssrc/server/services/olive/state.test.tssrc/server/services/olive/jobRunner.tssrc/server/services/olive/state.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/server/services/olive/jobIdempotency.test.tssrc/server/services/olive/jobRunner.idempotency.test.tssrc/server/services/olive/jobIdempotency.tssrc/server/services/olive/state.test.tssrc/server/services/olive/jobRunner.tssrc/server/services/olive/state.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/server/services/olive/jobIdempotency.test.tssrc/server/services/olive/jobRunner.idempotency.test.tssrc/server/services/olive/jobIdempotency.tssrc/server/services/olive/state.test.tssrc/server/services/olive/jobRunner.tssrc/server/services/olive/state.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/jobIdempotency.test.tssrc/server/services/olive/jobRunner.idempotency.test.tssrc/server/services/olive/jobIdempotency.tssrc/server/services/olive/state.test.tssrc/server/services/olive/jobRunner.tssrc/server/services/olive/state.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/jobIdempotency.test.tssrc/server/services/olive/jobRunner.idempotency.test.tssrc/server/services/olive/state.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/jobIdempotency.test.tssrc/server/services/olive/jobRunner.idempotency.test.tssrc/server/services/olive/state.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/server/services/olive/jobIdempotency.test.tssrc/server/services/olive/jobRunner.idempotency.test.tssrc/server/services/olive/jobIdempotency.tssrc/server/services/olive/state.test.tssrc/server/services/olive/jobRunner.tsscripts/mcp-agent-smoke.mjssrc/server/services/olive/state.ts
🔍 Remote MCP GitHub Copilot
Additional review context
- Validation is failing. PR
#184is blocked; thevalidatecheck failed, while CodeQL, Docker, Python tests, security, and Olive-pass checks passed. - Actionable compile issue remains:
src/lib/__tests__/studioConfigSnapshot.test.tsdeclaresrootandconfigPathusing JSDoc-only types in a TypeScript file. The review reports both as implicitany, matching the failed validation output. Replace them withlet root: string;andlet configPath: string;. - Non-blocking maintainability issue:
jobRunner.idempotency.test.tsplaces static imports aftervi.mockdeclarations. Moving imports to the top would align with the repository’s import conventions, but the finding is not inherently functional. - Previously reported smoke-test, lock-tail, idempotency, cleanup, signal-handling, and configuration-restoration findings are marked addressed or resolved in the current review threads.
🔇 Additional comments (9)
scripts/mcp-agent-smoke.mjs (1)
357-358: LGTM!Also applies to: 543-547, 557-577
src/server/services/olive/jobRunner.idempotency.test.ts (2)
28-31: Move the static imports above the mock declarations.Lines 28-31 remain below
vi.mock(...)calls. This repeats the existing finding.As per coding guidelines,
**/*.{ts,tsx,js,jsx}files must place imports at the top of modules; inline imports are allowed only for a documented circular dependency.Source: Coding guidelines
19-24: LGTM!Also applies to: 33-45, 58-78, 97-159
src/server/services/olive/jobIdempotency.ts (1)
14-15: LGTM!Also applies to: 129-138
src/server/services/olive/jobRunner.ts (1)
65-112: LGTM!Also applies to: 150-163, 197-202
src/server/services/olive/jobIdempotency.test.ts (1)
91-91: LGTM!Also applies to: 105-117
src/server/services/olive/state.ts (1)
10-10: LGTM!Also applies to: 79-80
src/server/services/olive/state.test.ts (2)
2-13: LGTM!Also applies to: 46-52, 156-186
205-207: LGTM!
Restack PR #184 onto the linearized MCP_harden base from stack #185. Co-authored-by: Cursor <cursoragent@cursor.com>
38bd8e7 to
2ba92cf
Compare
Annotate Studio config snapshot locals and PathLike-compatible existsSync mocks so pnpm lint / tsc --noEmit passes on CI validate. Co-authored-by: Cursor <cursoragent@cursor.com>
Mark non-terminal jobs cancelled during resetJobRegistry so stub setting_up loops exit, and move jobRunner idempotency test imports above vi.mock. Co-authored-by: Cursor <cursoragent@cursor.com>
Track the smoke's last written config bytes and skip snapshot restore (failing the run) when another process changed the file first. Co-authored-by: Cursor <cursoragent@cursor.com>
Serialize overlapping mcp-agent-smoke runs with a pid lock so one process cannot snapshot or restore another run's temporary allowJobSubmission state. Co-authored-by: Cursor <cursoragent@cursor.com>
Exit stub setup when the job is finalized, SIGKILL only while mcporter is still alive, restore Studio config in a finally after stop, and cover adopted-key replay plus clearInterval on registry reset. Co-authored-by: Cursor <cursoragent@cursor.com>
Mark temporary Studio policy patches with a live-pid owner stamp and make cleanup reject when snapshot restore fails so Greptile confidence gating no longer sees silent success. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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 609-623: Update failExitCode to return the conventional nonzero
SIGHUP exit code, 129, before the STRICT fallback. Keep the existing SIGINT and
SIGTERM mappings and ensure handleSignal continues routing SIGHUP through
failExitCode.
In `@scripts/studioConfigSmokeLock.mjs`:
- Around line 84-102: Update the EEXIST handling around the lock acquisition
loop: treat a non-finite parsed holder, including an empty or malformed lock
body, as stale and attempt reclamation; only continue immediately after unlink
succeeds, and otherwise fall through to the existing sleep(pollMs) path so
failed reclaim attempts cannot busy-spin.
- Around line 115-122: Use directory-specific removal APIs for empty Studio
config cleanup: in scripts/studioConfigSmokeLock.mjs#115-122, change
tryRemoveEmptyStudioConfigDir to inject and call rmdirSync/rmdir; in
scripts/studioConfigSnapshot.mjs#111-123, import and call rmdirSync instead of
rmSync with recursive options. Update
src/lib/__tests__/studioConfigSmokeLock.test.ts#104-109 to stub rmdirSync.
In `@src/lib/__tests__/resolvePython.test.ts`:
- Around line 47-58: Add a test case for resolvePython where spawnSync reports
both "python3" and "python" as available, and assert that it returns "python3".
Keep the existing Linux configuration and mock setup, ensuring the test locks
the candidate ordering preference.
In `@src/lib/__tests__/studioConfigSmokeLock.test.ts`:
- Around line 12-58: Extend the test "acquires with wx and releases only own
pid" by replacing the stored lock body with a different PID after acquisition
and before calling lock.release(). Assert that release leaves the lock present,
then restore or separately verify the owning-PID case so the test covers both
ownership behaviors.
In `@src/server/services/olive/jobRunner.idempotency.test.ts`:
- Around line 163-169: The test around the concurrent startOliveJob calls must
assert the expected registry and reuse behavior, not just successful results and
an empty lock map. Verify that hold-1 and hold-2 register as distinct jobs, that
the fingerprint-only request reuses hold-2, and that the final registry reflects
exactly those expected jobs; use the existing registry/reuse assertion helpers
or symbols in the test file.
- Around line 22-45: Update the test setup around startOliveJob and
continueOliveJobSetup to mock the child_process spawn boundary and filesystem
operations used for run artifacts, preventing real echo processes and
.olive-runs writes. Track or await detached setup promises so afterEach drains
all pending work before clearing jobRegistry, then retain the existing
idempotency and mock reset cleanup; keep tests limited to CPU-only recipe
building, JSON export, and validation flows.
In `@src/server/services/olive/jobRunner.ts`:
- Around line 309-330: Bound the wait loop in the stubSetup branch of the job
runner with a finite timeout, and ensure expiration exits the loop and finalizes
the job as a deterministic failure rather than hanging indefinitely. Preserve
the existing cancellation and external-finalization exits, and report the stub’s
non-termination through the established job status/logging mechanisms.
In `@src/server/services/olive/state.test.ts`:
- Around line 7-21: Move the static imports from state.ts and jobIdempotency.ts
above the vi.hoisted and vi.mock setup in the test module. Preserve the existing
mock behavior for ../venv/index.ts and ensure lint and typecheck checks continue
to pass.
🪄 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: 4f8e8048-d0f1-435d-8cb6-9b2446ed95a1
📒 Files selected for processing (12)
scripts/mcp-agent-smoke.mjsscripts/stopStudioThenAlways.mjsscripts/studioConfigSmokeLock.mjsscripts/studioConfigSnapshot.mjssrc/lib/__tests__/resolvePython.test.tssrc/lib/__tests__/stopStudioThenAlways.test.tssrc/lib/__tests__/studioConfigSmokeLock.test.tssrc/lib/__tests__/studioConfigSnapshot.test.tssrc/server/services/olive/jobRunner.idempotency.test.tssrc/server/services/olive/jobRunner.tssrc/server/services/olive/state.test.tssrc/server/services/olive/state.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: olive-pass-availability
- GitHub Check: security
- GitHub Check: validate
- GitHub Check: docker-build
- GitHub Check: python-tests
🧰 Additional context used
📓 Path-based instructions (7)
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__/stopStudioThenAlways.test.tssrc/lib/__tests__/studioConfigSmokeLock.test.tssrc/server/services/olive/state.tssrc/server/services/olive/jobRunner.idempotency.test.tssrc/server/services/olive/jobRunner.tssrc/lib/__tests__/studioConfigSnapshot.test.tssrc/server/services/olive/state.test.tssrc/lib/__tests__/resolvePython.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__/stopStudioThenAlways.test.tssrc/lib/__tests__/studioConfigSmokeLock.test.tssrc/server/services/olive/state.tssrc/server/services/olive/jobRunner.idempotency.test.tssrc/server/services/olive/jobRunner.tssrc/lib/__tests__/studioConfigSnapshot.test.tssrc/server/services/olive/state.test.tssrc/lib/__tests__/resolvePython.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__/stopStudioThenAlways.test.tssrc/lib/__tests__/studioConfigSmokeLock.test.tssrc/server/services/olive/state.tssrc/server/services/olive/jobRunner.idempotency.test.tssrc/server/services/olive/jobRunner.tssrc/lib/__tests__/studioConfigSnapshot.test.tssrc/server/services/olive/state.test.tssrc/lib/__tests__/resolvePython.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__/stopStudioThenAlways.test.tssrc/lib/__tests__/studioConfigSmokeLock.test.tssrc/server/services/olive/jobRunner.idempotency.test.tssrc/lib/__tests__/studioConfigSnapshot.test.tssrc/server/services/olive/state.test.tssrc/lib/__tests__/resolvePython.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__/stopStudioThenAlways.test.tssrc/lib/__tests__/studioConfigSmokeLock.test.tssrc/server/services/olive/jobRunner.idempotency.test.tssrc/lib/__tests__/studioConfigSnapshot.test.tssrc/server/services/olive/state.test.tssrc/lib/__tests__/resolvePython.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__/stopStudioThenAlways.test.tsscripts/stopStudioThenAlways.mjssrc/lib/__tests__/studioConfigSmokeLock.test.tssrc/server/services/olive/state.tssrc/server/services/olive/jobRunner.idempotency.test.tsscripts/studioConfigSmokeLock.mjssrc/server/services/olive/jobRunner.tssrc/lib/__tests__/studioConfigSnapshot.test.tssrc/server/services/olive/state.test.tsscripts/studioConfigSnapshot.mjssrc/lib/__tests__/resolvePython.test.tsscripts/mcp-agent-smoke.mjs
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/state.tssrc/server/services/olive/jobRunner.idempotency.test.tssrc/server/services/olive/jobRunner.tssrc/server/services/olive/state.test.ts
🪛 ast-grep (0.45.0)
src/server/services/olive/jobRunner.ts
[warning] 424-424: 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(configPath, JSON.stringify(enrichedRecipe, null, 2), "utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 3-3: Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn } from "child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
src/server/services/olive/state.test.ts
[warning] 159-159: 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(tmp, "{}", "utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
🔍 Remote MCP GitHub Copilot
Additional review context
- PR
#184’s latest commit isda1866b; all current checks are still in progress, includingvalidate, Python tests, Docker, security, Olive-pass availability, and Greptile review. No current pass/fail result is available yet. - The previously reported
studioConfigSnapshot.test.tsimplicit-anyissue appears addressed in the current diff:rootandconfigPathare now explicitly typed asstring. - The static-import ordering issue in
jobRunner.idempotency.test.tsalso appears addressed: imports now precedevi.mockdeclarations. - Review threads report fixes for:
- Python interpreter probing and
pythonfallback. - Nested JSON payload parsing.
- Studio config exact-byte restoration and concurrent-write protection.
- Signal cleanup and restoration-failure exit handling.
- Stubbed Olive setup during smoke tests.
- MCP lock-tail eviction.
- Fingerprint-only/keyed idempotency adoption.
- Registry resource cleanup and timer/listener teardown.
- Python interpreter probing and
- One remaining review-relevant test-quality suggestion was to assert that the adopted idempotency key works on a later replay; the current diff includes that third replay assertion.
DeepWiki and Context7 were not used because this PR concerns Olive-Studio’s TypeScript/Python MCP tooling and tests, not the specified Babel-Player architectural boundaries or a dependency/API upgrade.
🔇 Additional comments (21)
src/server/services/olive/state.ts (1)
4-10: LGTM!Also applies to: 65-106, 131-138
src/server/services/olive/state.test.ts (1)
45-52: LGTM!Also applies to: 69-86, 156-225
src/server/services/olive/jobRunner.ts (3)
65-112: LGTM!
148-223: LGTM!
225-293: LGTM!Also applies to: 332-485
src/server/services/olive/jobRunner.idempotency.test.ts (2)
47-117: LGTM!
119-146: LGTM!scripts/stopStudioThenAlways.mjs (1)
10-16: LGTM!scripts/studioConfigSmokeLock.mjs (1)
21-29: LGTM!scripts/studioConfigSnapshot.mjs (2)
22-30: LGTM!Also applies to: 37-40, 46-62
70-88: LGTM!src/lib/__tests__/stopStudioThenAlways.test.ts (1)
8-22: LGTM!src/lib/__tests__/studioConfigSnapshot.test.ts (1)
23-24: LGTM!Also applies to: 35-57, 59-73, 75-95, 97-112
scripts/mcp-agent-smoke.mjs (8)
103-134: LGTM!
161-174: LGTM!
181-260: LGTM!
262-300: LGTM!
302-316: LGTM!
318-354: LGTM!
550-607: LGTM!
526-536: 🩺 Stability & AvailabilityNo change needed.
The stub setup loop does not transition to
completed; it exits only when the job status changes tocancelledor whenfinishedAtis set.
1 participant
Summary
Rebuilds the squashed MCP_harden review follow-ups on current
MCP_hardenand addresses Codex review findings on this PR.Codex findings addressed
OLIVE_JOB_SETUP_STUB=1; setup parks insetting_upuntil cancelcleanup()OLIVE_MCP_ALLOW_JOBSbroke deny-path assertsstudioEnvwithMcpSubmitLocksevicts installed tails when still ownedAlso included (former #175–#183)
resetJobRegistry()(detaches venv listeners) in integrationbeforeEachOLIVE_MCP_ACCESSafter mcpAccess suitewriteStudioConfigimport in olive.stream testsexecutable+python)Validation
Base
Stacked on latest
MCP_harden(2962d4f).