Skip to content

feat(mcp): complete MCP settings and lifecycle follow-ups - #241

Merged
gitfromwildan merged 5 commits into
masterfrom
pullfrog/240-mcp-settings-lifecycle
Oct 9, 2026
Merged

gitfromwildan merged 5 commits into
masterfrom
pullfrog/240-mcp-settings-lifecycle

Conversation

@pullfrog

@pullfrog pullfrog Bot commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Propagate request-scoped cancellation through MCP discovery and tool calls.
  • Reuse Streamable HTTP sessions and terminate retired sessions when configuration changes.
  • Add Settings-backed MCP configuration with backend-held credentials and read-only server allow-lists.
  • Preserve DOCUBOOK_MCP_SERVERS override behavior, @mention retrieval, and applyDocumentOperations as the only document-edit tool.

Closes #240.

Verification

  • npm test
  • npm run lint
  • npx tsc -b
  • npm run build
  • cargo test --manifest-path core/Cargo.toml (154 passed)
  • cargo test --manifest-path server/Cargo.toml (50 passed)
  • node test/check-acl.mjs
  • node test/check-docker-paths.mjs
  • git diff --check

Desktop Tauri compilation was not verified because GTK/GLib development packages (glib-2.0) are unavailable in the runner.

Pullfrog  | View workflow run | via Pullfrog | Using gpt-6-luna | 𝕏

@pullfrog

pullfrog Bot commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor Author

Fixed the PR-introduced strict Clippy error by removing the unused app parameter from configure_mcp_servers_with_tokens and its only call site (src-tauri/lib/agent.rs:851-855). Committed as 3abc01e (fix(mcp): remove unused settings parameter, signed off) and pushed to PR #241. cargo test --manifest-path core/Cargo.toml passed and git diff --check passed. cargo clippy --manifest-path src-tauri/Cargo.toml -- -D warnings could not reach project linting because this runner lacks glib-2.0 and gio-2.0 system development packages; CI had reported the unused variable at lib/agent.rs:855.

Task list (3/3 completed)
  • Inspect PR failure and repo instructions
  • Reproduce and fix PR-introduced failure
  • Run required checks, commit, and push to PR

Pullfrog  | View workflow run | via Pullfrog | Using gpt-6-luna | 𝕏

Signed-off-by: pullfrog[bot] <226033991+pullfrog[bot]@users.noreply.github.com>
@gitfromwildan

Copy link
Copy Markdown
Member

CI failure @pullfrog

@pullfrog

pullfrog Bot commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor Author

Fixed the PR-introduced strict Clippy failure in core/rust-ai/context.rs by passing cancelled directly instead of needlessly calling as_deref_mut(). cargo clippy --manifest-path core/Cargo.toml -- -D warnings, cargo test --manifest-path core/Cargo.toml (154 passed), and cargo test --manifest-path server/Cargo.toml (50 passed) passed. Desktop Tauri test and Clippy commands could not compile because this Linux runner lacks the gobject-2.0/glib-2.0 system development packages. Committed as f030c6f (fix(mcp): satisfy strict core clippy) and pushed to the PR branch.

Task list (4/4 completed)
  • Inspect repository guidance and start downloading PR 241 checks
  • Identify the CI failure and confirm whether PR changes caused it
  • Reproduce and fix the root cause, then run required verification
  • Review, commit, push, and report the outcome

Pullfrog  | View workflow run | via Pullfrog | Using gpt-6-luna | 𝕏

Signed-off-by: pullfrog[bot] <226033991+pullfrog[bot]@users.noreply.github.com>
@gitfromwildan

gitfromwildan commented Oct 9, 2026 •

Copy link
Copy Markdown
Member

Code review findings (head f030c6f)

  1. HIGH — Desktop settings drop readOnlyTools. The UI sends camelCase readOnlyTools (frontend/components/SettingsModal.tsx:96), but Tauri deserializes Vec<ContextServer> whose field is read_only_tools without a camelCase rename/alias (core/rust-ai/context.rs:89-97, src-tauri/lib/agent.rs:754-762). Saving from desktop therefore persists an empty allow-list and exposes no MCP tools. Accept both field spellings or normalize through a settings DTO.

  2. HIGH — Web requests reset MCP discovery/session caches on every tool call. context_tools and call_context_tool both call configure_mcp (server/handlers.rs:218-247), which calls set_configured_servers; that unconditionally clears the discovery and session caches (core/rust-ai/context.rs:33-48). Consequently each tool invocation re-runs sequential tools/list discovery across all configured servers before tools/call. Only reconfigure when the effective server configuration changes.

  3. MEDIUM — Editing a server with the token field blank deletes its saved credential. The edit form initializes the optional replacement token to empty and sends token: "" on Save (frontend/components/SettingsModal.tsx:95-98). Both web (server/handlers.rs:869-873) and Tauri (src-tauri/lib/agent.rs:782-795) interpret that as a request to delete the existing key. Omit the token when unchanged and use a separate explicit action to remove it.

  4. MEDIUM — A rejected duplicate-ID save can still mutate persisted settings or credentials. The web handler writes keys before checking duplicate IDs (server/handlers.rs:869-889); Tauri writes the settings file and processes keychain updates before set_configured_servers rejects duplicate IDs (src-tauri/lib/agent.rs:774-815). Validate the full list, including uniqueness, before any side effects, and roll back on later failures.

  5. MEDIUM — Stop can miss cancellation of initial Tauri discovery. frontend/lib/ipc.ts:67-71 installs an abort listener but does not check whether the signal is already aborted; adding a listener to an already-aborted signal does not fire it. If Stop occurs while AI config is loading before discovery starts, the Tauri discovery continues until completion. Check the signal before invoking and make cancellation registration race-safe.

@pullfrog address this finding

@pullfrog

pullfrog Bot commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor Author

Implemented and pushed the five MCP review fixes in signed-off commit 58afa7b: full-list validation, credential-preserving updates, rollback on persistence/runtime failures, cache invalidation based on effective configuration, and race-safe Tauri cancellation. Core tests (156 passed), core strict Clippy, frontend lint, and diff checks pass; Tauri compilation/lint remains blocked by missing system GLib development packages (glib-2.0/gobject-2.0).

Task list (0/5 completed)
  • Checkout PR 241 and inspect review threads and repository guidance
  • Implement validated fixes for the five requested findings
  • Run focused and required frontend/Rust verification
  • Review diff, commit with sign-off, and push branch
  • Reply to and resolve addressed review threads, then report progress

Pullfrog  | View workflow run | via Pullfrog | Using gpt-6-luna | 𝕏

Signed-off-by: pullfrog[bot] <226033991+pullfrog[bot]@users.noreply.github.com>
@gitfromwildan

gitfromwildan commented Oct 9, 2026 •

Copy link
Copy Markdown
Member

P1 — MCP settings can erase existing servers and credentials after a load failure

SettingsModal initializes mcpServers to [], then silently swallows errors from the initial mcp_settings request (frontend/components/SettingsModal.tsx:144-145,169-175). The Add/Save controls remain enabled. If that request fails transiently and the user adds a server, save() submits the incomplete full list; the backend replaces the saved MCP list and deletes keys for omitted server IDs. This can permanently remove existing MCP configuration and bearer tokens.

Track loading/error state for the MCP settings fetch, show a retry/error state, and disable all mutations until the server list has loaded successfully.

@pullfrog address this finding

Signed-off-by: pullfrog[bot] <226033991+pullfrog[bot]@users.noreply.github.com>
@pullfrog

pullfrog Bot commented Oct 9, 2026

Copy link
Copy Markdown
Contributor Author

Fixed in 89c85e5 (fix(mcp): block edits until settings load). SettingsModal now tracks MCP settings loading and errors, offers retry, and keeps configuration controls and mutations disabled until the full server list loads successfully. Added regression coverage for a failed initial load and successful retry.

Verified: npx oxlint frontend/ test/__fixtures__/, npx tsc -b, npm test, focused SettingsModal test (13 passed), node test/check-acl.mjs, node test/check-docker-paths.mjs, npm run build, cargo build in server/, and git diff --check passed. npm run test:e2e had three unrelated ai-mention failures (22/25 passed); ai-debug and web-smoke passed.

The commit is pushed to PR #241. The comment arrived as an issue comment rather than a PR review thread, so there is no review thread to resolve.

Pullfrog  | View workflow run | via Pullfrog | Using gpt-6-luna | 𝕏

@pullfrog
pullfrog Bot deployed to build-approval October 9, 2026 15:22 Active
@pullfrog
pullfrog Bot deployed to build-approval October 9, 2026 15:24 Active
@pullfrog
pullfrog Bot deployed to build-approval October 9, 2026 15:24 Active
@gitfromwildan
gitfromwildan merged commit ca94ed1 into master Oct 9, 2026
18 checks passed
@gitfromwildan
gitfromwildan deleted the pullfrog/240-mcp-settings-lifecycle branch October 16, 2026 06:01

This branch was successfully deployed

1 active deployment
build-approval — 89c85e52 Deployed Oct 9, 2026 by pullfrog[bot] via build (aarch64-apple-darwin) #878
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.

Complete MCP follow-ups: discovery cancellation, session lifecycle, and Settings configuration

1 participant