Skip to content

Conversation

@cameroncooke
Copy link
Collaborator

@cameroncooke cameroncooke commented Oct 5, 2025

This PR migrates the simulator build tool to the new session-aware defaults layer and enforces concise tool descriptions.

Changes:

  • simulator/build_sim: switch to createSessionAwareTool, introduce minimal public schema, add preflight requirements, and set concise description ('Builds an app for an iOS simulator.').
  • tests: update build_sim tests to expect session-aware preflight messages and public schema; clear session store in beforeEach; update session-management index description test accordingly.
  • docs: add Tool Description Policy (concise, no session-defaults mention) and update build_sim example in session_management_plan.md.

Validation:

  • All tests passing locally: 1140 passed, 3 skipped.
  • Typecheck, lint, and build successful.

Note

Introduce session-aware defaults (store + factory), add session-management tools, and migrate build_sim to minimal public schema with preflight checks and concise description.

  • Session-aware defaults:
    • Utils: Add SessionStore (src/utils/session-store.ts) and createSessionAwareTool with preflight requirements and exclusivePairs (src/utils/typed-tool-factory.ts).
  • New tools (session-management):
    • session-set-defaults, session-clear-defaults, session-show-defaults with schemas/handlers and workflow metadata.
  • Simulator tool:
    • Migrate simulator/build_sim to createSessionAwareTool; expose minimal public schema; add requirements; set concise description.
  • Tests:
    • Add tests for session store, session-aware factory, and session-management tools; update build_sim tests for new public schema and preflight messaging.
  • Docs:
    • Add docs/session_management_plan.md detailing design, middleware, migration plan.
  • Misc:
    • Update .gitignore to exclude .factory.

Written by Cursor Bugbot for commit de72b23. This will update automatically on new commits. Configure here.

Summary by CodeRabbit

  • New Features

    • Added session-management tools to set, clear, and show persistent session defaults and a session-aware mechanism that merges saved defaults with user inputs.
  • Refactor

    • Simulator build tool now uses session-aware behavior with a simplified public interface and explicit requirement handling.
  • Tests

    • Added unit tests for the session store, session-aware tool factory, session-management tools, and updated simulator tests.
  • Documentation

    • Added a session management plan.
  • Chores

    • Updated ignore rules to exclude a local config file.

…ion-aware middleware; add tests; update plan doc

- Add in-memory SessionStore (set/get/clear/show)
- New workflow: session-management (session-set-defaults, session-clear-defaults, session-show-defaults)
- Add createSessionAwareTool with requirements preflight and args-over-defaults merge
- Comprehensive tests for tools, store, and factory
- Update session_management_plan.md with DI checklist, commit protocol, and pre-commit hook note

Existing tools unchanged; no remote push.
…ise description

docs(session-management): add Tool Description Policy and update plan example

test: update build_sim and session-management tests for session-aware preflight and description
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 5, 2025

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Walkthrough

Adds an in-memory SessionStore and a session-aware tool factory, three session-management tools (set/clear/show) with tests, migrates the simulator build tool to use session-aware validation and a public schema, adds docs/plan, and updates .gitignore to ignore .factory.

Changes

Cohort / File(s) Summary of changes
Repo config
.gitignore
Added ignore entry for .factory.
Docs / plan
docs/session_management_plan.md
New design and migration plan describing session-aware defaults, tool factory, migration steps, testing and CI checklist.
Session store (impl)
src/utils/session-store.ts
New SessionDefaults type, SessionStore class, and exported singleton sessionStore with setDefaults, get, getAll, and clear.
Session-aware tool factory
src/utils/typed-tool-factory.ts
Added SessionRequirement type and createSessionAwareTool; merges session defaults with args, enforces allOf/oneOf requirements, prunes exclusive pairs, validates via schema, and updated createTypedTool behavior.
Session utilities tests
src/utils/__tests__/*
src/utils/__tests__/session-store.test.ts, src/utils/__tests__/session-aware-tool-factory.test.ts
Tests for SessionStore and createSessionAwareTool covering merge behavior, precedence, requirement errors, and validation messaging.
Session-management tools (impl)
src/mcp/tools/session-management/*
.../index.ts, .../session_set_defaults.ts, .../session_clear_defaults.ts, .../session_show_defaults.ts
New workflow metadata and three tools (set/clear/show) with Zod schemas, logic functions, handlers, and default exports.
Session-management tests
src/mcp/tools/session-management/__tests__/*
Unit tests validating workflow metadata and behavior/validation of the three session-management tools.
Simulator tool
src/mcp/tools/simulator/build_sim.ts, src/mcp/tools/simulator/__tests__/build_sim.test.ts
build_sim switched to createSessionAwareTool with a public schema (omitting session-managed fields), explicit requirements (allOf/oneOf), exclusivePairs, simplified description; tests updated to reflect new validation and messages.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Caller
  participant Handler as Session-aware Handler
  participant Store as SessionStore
  participant Validator as Zod
  participant Logic as LogicFunction
  participant Exec as Executor

  Caller->>Handler: invoke tool with args
  Handler->>Store: getAll()
  Handler->>Handler: merge args + session defaults
  Handler->>Handler: prune exclusivePairs
  Handler->>Handler: evaluate requirements (allOf / oneOf)
  alt requirements missing
    Handler-->>Caller: Error (missing session defaults + guidance)
  else requirements satisfied
    Handler->>Validator: validate merged params
    alt validation error
      Handler-->>Caller: Error (parameter validation + hint to set defaults)
    else valid
      Handler->>Exec: getExecutor()
      Handler->>Logic: call logic(params, executor)
      Logic-->>Handler: ToolResponse
      Handler-->>Caller: ToolResponse
    end
  end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Suggested labels

enhancement

Poem

I hopped in with defaults stored tight,
Set and cleared them by soft moonlight,
Showing values neat and bright,
Tools now borrow what’s in sight,
🥕 — the session-setting rabbit

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Title Check ⚠️ Warning The title only mentions migrating build_sim and includes a generic “concise description” phrase, failing to capture the broader addition of the session defaults system, new session-management tools, and factory updates. Update the title to summarize the primary addition of the session defaults system and factory along with the build_sim migration—for example, “feat(session-management): add session defaults system and migrate build_sim to use session-aware defaults.”
Docstring Coverage ⚠️ Warning Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat/session-management

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.

❤️ Share

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

@claude
Copy link
Contributor

claude bot commented Oct 5, 2025

Claude encountered an error —— View job


I'll analyze this and get back to you.

@pkg-pr-new
Copy link

pkg-pr-new bot commented Oct 5, 2025

Open in StackBlitz

npm i https://pkg.pr.new/cameroncooke/XcodeBuildMCP/xcodebuildmcp@120

commit: de72b23

cursor[bot]

This comment was marked as outdated.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ 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

Comment on lines 83 to 86
return async (rawArgs: Record<string, unknown>): Promise<ToolResponse> => {
try {
const merged: Record<string, unknown> = { ...sessionStore.getAll(), ...rawArgs };

Choose a reason for hiding this comment

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

P1 Badge Allow overriding XOR defaults without clearing session

createSessionAwareTool merges saved defaults and call arguments with const merged = { ...sessionStore.getAll(), ...rawArgs };. When session defaults hold one side of a mutually exclusive pair (e.g. workspacePath) and the caller passes the other (projectPath) to override it, both keys survive in merged, so the internal schema rejects the call as having conflicting values. This forces callers to clear or reset session defaults before every override and violates the documented rule that explicit args take precedence. Consider dropping conflicting defaults when an alternate key is provided or otherwise resolving XOR pairs before validation.

Useful? React with 👍 / 👎.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e9d9b7d and a613b9f.

📒 Files selected for processing (16)
  • .gitignore (1 hunks)
  • docs/session_management_plan.md (1 hunks)
  • src/mcp/tools/session-management/__tests__/index.test.ts (1 hunks)
  • src/mcp/tools/session-management/__tests__/session_clear_defaults.test.ts (1 hunks)
  • src/mcp/tools/session-management/__tests__/session_set_defaults.test.ts (1 hunks)
  • src/mcp/tools/session-management/__tests__/session_show_defaults.test.ts (1 hunks)
  • src/mcp/tools/session-management/index.ts (1 hunks)
  • src/mcp/tools/session-management/session_clear_defaults.ts (1 hunks)
  • src/mcp/tools/session-management/session_set_defaults.ts (1 hunks)
  • src/mcp/tools/session-management/session_show_defaults.ts (1 hunks)
  • src/mcp/tools/simulator/__tests__/build_sim.test.ts (3 hunks)
  • src/mcp/tools/simulator/build_sim.ts (2 hunks)
  • src/utils/__tests__/session-aware-tool-factory.test.ts (1 hunks)
  • src/utils/__tests__/session-store.test.ts (1 hunks)
  • src/utils/session-store.ts (1 hunks)
  • src/utils/typed-tool-factory.ts (2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.ts

📄 CodeRabbit inference engine (AGENTS.md)

**/*.ts: Use .ts extensions for all internal relative imports (e.g., import { tool } from './tool.ts')
Use .ts extensions in re-exports for internal files (e.g., export { default } from '../shared/tool.ts')
Use .js extension only for external package entry points (e.g., import ... from '@scope/pkg/file.js')
Never import internal project files using .js extensions

Files:

  • src/mcp/tools/session-management/index.ts
  • src/utils/__tests__/session-store.test.ts
  • src/utils/__tests__/session-aware-tool-factory.test.ts
  • src/mcp/tools/session-management/__tests__/session_clear_defaults.test.ts
  • src/mcp/tools/simulator/__tests__/build_sim.test.ts
  • src/mcp/tools/session-management/session_set_defaults.ts
  • src/mcp/tools/session-management/session_clear_defaults.ts
  • src/mcp/tools/session-management/__tests__/session_show_defaults.test.ts
  • src/mcp/tools/session-management/session_show_defaults.ts
  • src/utils/typed-tool-factory.ts
  • src/utils/session-store.ts
  • src/mcp/tools/session-management/__tests__/index.test.ts
  • src/mcp/tools/session-management/__tests__/session_set_defaults.test.ts
  • src/mcp/tools/simulator/build_sim.ts
**/*.{test,spec}.ts

📄 CodeRabbit inference engine (AGENTS.md)

**/*.{test,spec}.ts: Do not use Vitest mocking APIs in tests (no vi.mock, vi.fn, vi.spyOn, etc.)
In tests, import the core ...Logic function from the tool and use createMockExecutor/createMockFileSystemExecutor to simulate outcomes

Files:

  • src/utils/__tests__/session-store.test.ts
  • src/utils/__tests__/session-aware-tool-factory.test.ts
  • src/mcp/tools/session-management/__tests__/session_clear_defaults.test.ts
  • src/mcp/tools/simulator/__tests__/build_sim.test.ts
  • src/mcp/tools/session-management/__tests__/session_show_defaults.test.ts
  • src/mcp/tools/session-management/__tests__/index.test.ts
  • src/mcp/tools/session-management/__tests__/session_set_defaults.test.ts
🧠 Learnings (1)
📚 Learning: 2025-09-21T19:27:22.795Z
Learnt from: CR
PR: cameroncooke/XcodeBuildMCP#0
File: AGENTS.md:0-0
Timestamp: 2025-09-21T19:27:22.795Z
Learning: Applies to **/*.{test,spec}.ts : In tests, import the core ...Logic function from the tool and use createMockExecutor/createMockFileSystemExecutor to simulate outcomes

Applied to files:

  • src/utils/__tests__/session-aware-tool-factory.test.ts
  • src/mcp/tools/simulator/__tests__/build_sim.test.ts
🧬 Code graph analysis (12)
src/utils/__tests__/session-store.test.ts (1)
src/utils/session-store.ts (1)
  • sessionStore (42-42)
src/utils/__tests__/session-aware-tool-factory.test.ts (3)
src/utils/session-store.ts (1)
  • sessionStore (42-42)
src/utils/typed-tool-factory.ts (1)
  • createSessionAwareTool (74-128)
src/test-utils/mock-executors.ts (1)
  • createMockExecutor (27-75)
src/mcp/tools/session-management/__tests__/session_clear_defaults.test.ts (2)
src/utils/session-store.ts (1)
  • sessionStore (42-42)
src/mcp/tools/session-management/session_clear_defaults.ts (1)
  • sessionClearDefaultsLogic (26-30)
src/mcp/tools/simulator/__tests__/build_sim.test.ts (1)
src/utils/session-store.ts (1)
  • sessionStore (42-42)
src/mcp/tools/session-management/session_set_defaults.ts (2)
src/utils/session-store.ts (2)
  • sessionStore (42-42)
  • SessionDefaults (3-13)
src/utils/typed-tool-factory.ts (1)
  • createTypedTool (30-61)
src/mcp/tools/session-management/session_clear_defaults.ts (2)
src/utils/session-store.ts (1)
  • sessionStore (42-42)
src/utils/typed-tool-factory.ts (1)
  • createTypedTool (30-61)
src/mcp/tools/session-management/__tests__/session_show_defaults.test.ts (1)
src/utils/session-store.ts (1)
  • sessionStore (42-42)
src/mcp/tools/session-management/session_show_defaults.ts (1)
src/utils/session-store.ts (1)
  • sessionStore (42-42)
src/utils/typed-tool-factory.ts (2)
src/utils/session-store.ts (2)
  • SessionDefaults (3-13)
  • sessionStore (42-42)
src/utils/errors.ts (1)
  • createErrorResponse (111-122)
src/mcp/tools/session-management/__tests__/index.test.ts (1)
src/mcp/tools/session-management/index.ts (1)
  • workflow (1-8)
src/mcp/tools/session-management/__tests__/session_set_defaults.test.ts (2)
src/utils/session-store.ts (1)
  • sessionStore (42-42)
src/mcp/tools/session-management/session_set_defaults.ts (1)
  • sessionSetDefaultsLogic (21-28)
src/mcp/tools/simulator/build_sim.ts (2)
src/utils/typed-tool-factory.ts (1)
  • createSessionAwareTool (74-128)
src/utils/execution/index.ts (1)
  • getDefaultCommandExecutor (5-5)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: Cursor Bugbot
  • GitHub Check: code-review
🔇 Additional comments (8)
.gitignore (1)

111-111: LGTM!

The .factory ignore rule is appropriate for preventing unintended tracking of generated or auxiliary artifacts.

src/mcp/tools/session-management/__tests__/index.test.ts (1)

1-58: LGTM!

The test suite comprehensively validates the workflow metadata structure and values. Import paths correctly use .ts extensions per coding guidelines.

src/utils/__tests__/session-store.test.ts (1)

1-38: LGTM!

The test suite effectively covers SessionStore functionality including set/get, merging, selective clearing, and full clearing. Import paths correctly use .ts extensions per coding guidelines.

src/mcp/tools/session-management/session_show_defaults.ts (1)

1-12: LGTM!

The implementation is straightforward and correct. Import paths properly use .ts extensions per coding guidelines.

src/mcp/tools/session-management/__tests__/session_show_defaults.test.ts (1)

1-45: LGTM!

The test suite thoroughly validates the tool's export structure and handler behavior for both empty and populated defaults. Import paths correctly use .ts extensions per coding guidelines.

src/mcp/tools/session-management/__tests__/session_clear_defaults.test.ts (1)

1-75: LGTM!

The test suite comprehensively covers the tool's functionality including selective clearing, full clearing, and validation behavior. Tests correctly import the ...Logic function per coding guidelines, and import paths use .ts extensions.

src/utils/__tests__/session-aware-tool-factory.test.ts (1)

1-112: LGTM!

The test suite comprehensively validates the session-aware tool factory including default merging, argument precedence, and requirement validation. Tests correctly use createMockExecutor per coding guidelines and import paths use .ts extensions. Based on learnings.

src/mcp/tools/session-management/index.ts (1)

1-8: LGTM!

The workflow metadata object is well-structured and serves as a clear public descriptor for the session-management toolset.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Found 2 potential issues that should be addressed.

});

it('should return current defaults when set', async () => {
sessionStore.setDefaults({ scheme: 'MyScheme', simulatorId: 'SIM-123' });
Copy link

Choose a reason for hiding this comment

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

Setting defaults in this test without clearing them leaves the singleton sessionStore populated after the suite ends. Because sessionStore is shared across all test files, running this spec before others will leak scheme/simulatorId into later tests that assume a clean store. Please add something like afterEach(() => sessionStore.clear()); (or clear at the end of this test) so the store is reset after every spec.

describe('session-clear-defaults tool', () => {
beforeEach(() => {
sessionStore.clear();
sessionStore.setDefaults({
Copy link

Choose a reason for hiding this comment

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

beforeEach seeds the singleton sessionStore, but when validation fails (e.g. the "invalid" keys test) the store never gets cleared and stays populated after the suite finishes. That leaked state can break later tests that expect an empty store. Please add an afterEach(() => sessionStore.clear()); (or clear inside the failing test) so each spec leaves the store in a clean state.

…le; improve oneOf error hints; add exclusivePairs pruning; clear([]) no-op; typo fix in description
…ld) and mandate quick manual CLI test before review
@claude
Copy link
Contributor

claude bot commented Oct 5, 2025

Claude encountered an error —— View job


I'll analyze this and get back to you.

@claude
Copy link
Contributor

claude bot commented Oct 5, 2025

Claude encountered an error —— View job


I'll analyze this and get back to you.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a613b9f and 99c77be.

📒 Files selected for processing (9)
  • docs/session_management_plan.md (1 hunks)
  • src/mcp/tools/session-management/__tests__/session_clear_defaults.test.ts (1 hunks)
  • src/mcp/tools/session-management/__tests__/session_set_defaults.test.ts (1 hunks)
  • src/mcp/tools/session-management/__tests__/session_show_defaults.test.ts (1 hunks)
  • src/mcp/tools/session-management/session_set_defaults.ts (1 hunks)
  • src/mcp/tools/simulator/build_sim.ts (2 hunks)
  • src/utils/__tests__/session-store.test.ts (1 hunks)
  • src/utils/session-store.ts (1 hunks)
  • src/utils/typed-tool-factory.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/mcp/tools/session-management/tests/session_clear_defaults.test.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{test,spec}.ts

📄 CodeRabbit inference engine (AGENTS.md)

**/*.{test,spec}.ts: Do not use Vitest mocking APIs in tests (no vi.mock, vi.fn, vi.spyOn, etc.)
In tests, import the core ...Logic function from the tool and use createMockExecutor/createMockFileSystemExecutor to simulate outcomes

Files:

  • src/mcp/tools/session-management/__tests__/session_set_defaults.test.ts
  • src/utils/__tests__/session-store.test.ts
  • src/mcp/tools/session-management/__tests__/session_show_defaults.test.ts
**/*.ts

📄 CodeRabbit inference engine (AGENTS.md)

**/*.ts: Use .ts extensions for all internal relative imports (e.g., import { tool } from './tool.ts')
Use .ts extensions in re-exports for internal files (e.g., export { default } from '../shared/tool.ts')
Use .js extension only for external package entry points (e.g., import ... from '@scope/pkg/file.js')
Never import internal project files using .js extensions

Files:

  • src/mcp/tools/session-management/__tests__/session_set_defaults.test.ts
  • src/mcp/tools/simulator/build_sim.ts
  • src/mcp/tools/session-management/session_set_defaults.ts
  • src/utils/__tests__/session-store.test.ts
  • src/utils/typed-tool-factory.ts
  • src/mcp/tools/session-management/__tests__/session_show_defaults.test.ts
  • src/utils/session-store.ts
🧬 Code graph analysis (6)
src/mcp/tools/session-management/__tests__/session_set_defaults.test.ts (2)
src/utils/session-store.ts (1)
  • sessionStore (47-47)
src/mcp/tools/session-management/session_set_defaults.ts (1)
  • sessionSetDefaultsLogic (21-28)
src/mcp/tools/simulator/build_sim.ts (1)
src/utils/typed-tool-factory.ts (1)
  • createSessionAwareTool (74-152)
src/mcp/tools/session-management/session_set_defaults.ts (2)
src/utils/session-store.ts (2)
  • sessionStore (47-47)
  • SessionDefaults (3-13)
src/utils/typed-tool-factory.ts (1)
  • createTypedTool (30-61)
src/utils/__tests__/session-store.test.ts (1)
src/utils/session-store.ts (1)
  • sessionStore (47-47)
src/utils/typed-tool-factory.ts (1)
src/utils/session-store.ts (2)
  • SessionDefaults (3-13)
  • sessionStore (47-47)
src/mcp/tools/session-management/__tests__/session_show_defaults.test.ts (1)
src/utils/session-store.ts (1)
  • sessionStore (47-47)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: Cursor Bugbot
  • GitHub Check: code-review

…une merged payload to align with exclusivePairs pruning
@claude
Copy link
Contributor

claude bot commented Oct 5, 2025

Claude encountered an error —— View job


I'll analyze this and get back to you.

cursor[bot]

This comment was marked as outdated.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Found 1 potential issue that should be addressed.

type Params = z.infer<typeof schemaObj>;

export async function sessionSetDefaultsLogic(params: Params): Promise<ToolResponse> {
sessionStore.setDefaults(params as Partial<SessionDefaults>);
Copy link

Choose a reason for hiding this comment

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

session-set-defaults currently just merges the supplied values into the existing session defaults. If a user previously set workspacePath and later switches to projectPath (or similarly simulatorName to simulatorId), both values stay in the store, so the next call to build_sim hits the internal schema’s XOR constraint and fails with a validation error. That makes the new session defaults workflow break unless the user manually clears the old key first. Please clear the mutually exclusive counterpart before merging—for example, when projectPath is provided remove any stored workspacePath, and likewise for the other exclusive pairs—so that updating defaults doesn’t leave the store in an invalid state.

…ly provides null/undefined; add tests for null/undefined activation
@claude
Copy link
Contributor

claude bot commented Oct 5, 2025

Claude encountered an error —— View job


I'll analyze this and get back to you.

…ve counterparts before merge to prevent invalid store state; add tests
@claude
Copy link
Contributor

claude bot commented Oct 5, 2025

Claude encountered an error —— View job


I'll analyze this and get back to you.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Found 1 potential issue that should be addressed.

sessionStore.clear(Array.from(toClear));
}

sessionStore.setDefaults(params as Partial<SessionDefaults>);
Copy link

Choose a reason for hiding this comment

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

sessionSetDefaultsLogic clears previously stored counterparts, but if a caller provides both projectPath and workspacePath (or both simulator fields) in the same request, we still write both keys into the session store because params is passed through unchanged. That leaves the defaults in an impossible state, and the next tool invocation will fail the XOR refine in build_sim ("projectPath and workspacePath are mutually exclusive"). Please either reject the request (e.g. add a Zod refine) or drop the conflicting value before calling setDefaults so the store can never contain mutually exclusive entries.

cursor[bot]

This comment was marked as outdated.

…n-set-defaults via schema refine; fix exclusivePairs pruning to ignore null/undefined and not override session defaults; update tests
@claude
Copy link
Contributor

claude bot commented Oct 5, 2025

Claude encountered an error —— View job


I'll analyze this and get back to you.

cursor[bot]

This comment was marked as outdated.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
src/mcp/tools/session-management/session_set_defaults.ts (1)

34-37: Consider checking for concrete values before clearing counterparts.

The hasOwnProperty checks return true even when a value is explicitly undefined. If a user passes { projectPath: undefined }, line 34 clears workspacePath from the session even though no concrete value was provided. This might surprise users who expect undefined to mean "don't change this key."

Consider checking that the value is not null or undefined before clearing the counterpart:

-  if (Object.prototype.hasOwnProperty.call(params, 'projectPath')) toClear.add('workspacePath');
-  if (Object.prototype.hasOwnProperty.call(params, 'workspacePath')) toClear.add('projectPath');
-  if (Object.prototype.hasOwnProperty.call(params, 'simulatorId')) toClear.add('simulatorName');
-  if (Object.prototype.hasOwnProperty.call(params, 'simulatorName')) toClear.add('simulatorId');
+  if (Object.prototype.hasOwnProperty.call(params, 'projectPath') && params.projectPath != null) toClear.add('workspacePath');
+  if (Object.prototype.hasOwnProperty.call(params, 'workspacePath') && params.workspacePath != null) toClear.add('projectPath');
+  if (Object.prototype.hasOwnProperty.call(params, 'simulatorId') && params.simulatorId != null) toClear.add('simulatorName');
+  if (Object.prototype.hasOwnProperty.call(params, 'simulatorName') && params.simulatorName != null) toClear.add('simulatorId');

This aligns with the sanitization logic in createSessionAwareTool (lines 93-96 of typed-tool-factory.ts), which treats null/undefined as "not provided."

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0307df8 and cb6156a.

📒 Files selected for processing (4)
  • src/mcp/tools/session-management/__tests__/session_set_defaults.test.ts (1 hunks)
  • src/mcp/tools/session-management/session_set_defaults.ts (1 hunks)
  • src/utils/__tests__/session-aware-tool-factory.test.ts (1 hunks)
  • src/utils/typed-tool-factory.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/mcp/tools/session-management/tests/session_set_defaults.test.ts
  • src/utils/tests/session-aware-tool-factory.test.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.ts

📄 CodeRabbit inference engine (AGENTS.md)

**/*.ts: Use .ts extensions for all internal relative imports (e.g., import { tool } from './tool.ts')
Use .ts extensions in re-exports for internal files (e.g., export { default } from '../shared/tool.ts')
Use .js extension only for external package entry points (e.g., import ... from '@scope/pkg/file.js')
Never import internal project files using .js extensions

Files:

  • src/mcp/tools/session-management/session_set_defaults.ts
  • src/utils/typed-tool-factory.ts
🧬 Code graph analysis (2)
src/mcp/tools/session-management/session_set_defaults.ts (2)
src/utils/session-store.ts (2)
  • SessionDefaults (3-13)
  • sessionStore (47-47)
src/utils/typed-tool-factory.ts (1)
  • createTypedTool (30-61)
src/utils/typed-tool-factory.ts (2)
src/utils/session-store.ts (2)
  • SessionDefaults (3-13)
  • sessionStore (47-47)
src/utils/errors.ts (1)
  • createErrorResponse (111-122)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: Cursor Bugbot
  • GitHub Check: code-review
🔇 Additional comments (2)
src/mcp/tools/session-management/session_set_defaults.ts (1)

51-57: LGTM! Tool definition follows session-aware design.

The tool correctly:

  • Exposes baseSchema.shape as the public schema (without refinements) for user-facing API.
  • Uses schemaObj (with mutual exclusivity refinements) for internal validation in the handler.
  • Provides a clear description appropriate for a session-management tool.

This design prevents users from providing mutually exclusive keys in a single request while keeping the public API simple.

src/utils/typed-tool-factory.ts (1)

74-160: LGTM! Session-aware tool creation is well-designed.

The implementation correctly handles:

  1. Sanitization (lines 93-96): Filters null/undefined from args so they don't override session defaults. This ensures that explicit arguments take precedence while preserving session values when no concrete value is provided.

  2. Exclusive pair pruning (lines 101-114): Only activates when the caller provides a concrete value for any key in a pair, then drops conflicting session defaults. This allows users to override XOR pairs without manually clearing the session first.

  3. Requirement validation (lines 116-141): Checks allOf/oneOf requirements against the post-pruning merged object (not the original sessionStore), ensuring consistency between requirement enforcement and downstream logic.

  4. Error messages: Provide actionable guidance, including hints to use session-set-defaults when validation fails.

This design addresses all concerns from past review comments and enables a clean session-aware workflow.

…t factory level; add test; update build_sim tests to expect factory message
@claude
Copy link
Contributor

claude bot commented Oct 5, 2025

Claude encountered an error —— View job


I'll analyze this and get back to you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant