Repository navigation
[3391 by Claude Opus 5.5] Zero-config MCP through ProPR Connect in hosted UI tunnel mode - #3392
Conversation
![propr-dev[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
… UI tunnel on (`PROPR_UI_TUNNEL_ENABLED=true` and `PROPR_INSTANCE_ID` set), a Plus user only needs to add `https://mcp.propr.dev/mcp` to their MCP client. The 15 new backend tests, 3 new UI tests and the related existing suites pass, and typecheck and zero-warning lint are clean. Nothing ran against the real Connect gateway; the registration tests use a stand-in for it. Each item from the issue (only when its variable is unset; explicit values still win): 1. Origin: `MCP_PUBLIC_ORIGIN` comes from the same `proprInstanceProxyUrl()` the tunnel-mode `API_PUBLIC_URL` uses, so it is `https://t-<id>.propr.dev`. 2. Instance id: `MCP_INSTANCE_ID` is `PROPR_INSTANCE_ID`. 3. Key: a random 32-byte key is created once at `<data dir>/mcp/encryption-key`, next to the web-push keys, with owner-only permissions. Its fingerprint is saved in the database on first use. If the file later goes missing, MCP reports `KEY_MISSING` instead of making a new key; a replaced file reports `KEY_CHANGED`. The key is never logged. 4. Trust: `MCP_CONNECT_TRUST` is implied. 5. On by default: the existing admin-only `connect_enabled` database setting is now the "Allow MCP clients through ProPR Connect" toggle in Settings. `MCP_ENABLED=false` still forces it off. 6. Automatic registration: a background job in `packages/api/mcp/connectRegistration.ts` runs the same flow as `mcp:connect:register`. It leaves a current registration alone. It registers again when the registration is missing, older than a day, for a different tunnel id, or rejected by Connect with a 409. Failures retry after 5 s, doubling up to 15 minutes, and API startup never waits for it. `/health`, `/api/status` and the Settings page report `connect_mcp`. The npm script still works for manual repair and now uses the same derived config. 7. Outside tunnel mode: behaviour is unchanged. The job only reports whether a manual registration exists and never registers by itself. Decisions you may want to revisit: - Env templates: `.env.example` and the CLI copy shipped `MCP_ENABLED=false` and `MCP_CONNECT_TRUST=false`. That would have forced the feature off for every new stack, so both are now blank with advanced-override comments. Existing `.env` files that still contain those lines keep MCP off. - Scope ceiling: default-on tunnel mode uses the Settings scope ceiling, which defaults to `read`, `plan`, `review`. An admin has to widen it before clients can execute or merge. - Tunnel token: the API no longer requires `PROPR_UI_TUNNEL_TOKEN` in tunnel mode, because the launcher only gives it to the cloudflared sidecar. The explicit setup outside tunnel mode still requires it. - Re-provisioned tunnel: when the instance id comes from `PROPR_INSTANCE_ID` and that changes, the instance creates a new identity and users reconnect. An explicit `MCP_INSTANCE_ID` still can't be overwritten. - Plus: there is no local Plus check. Connect enforces it, so a Community installation shows `failed:ACCESS_REVOKED`. The safety guarantees are unchanged: requests still need a valid Connect-signed delegation, the instance fails closed when online validation is unavailable, and the local `/api/mcp` checks still apply. Turning it on by default only makes the instance reachable through Connect. Docs and tests: - Docs: the Connect section in `docs/mcp.md` is now "MCP works automatically with the hosted UI tunnel", with the explicit settings kept under "Advanced". The contract doc says the gateway is live, and the feature guide is updated. - New tests: `packages/api/test/mcpTunnelMode.test.ts`, added to `test:mcp`, covers every case listed in the issue. `McpServerSection.test.tsx` covers the toggle. - Changed existing test: I added `connect_mcp`, `registered`, `pending` and `disabled` to the allowed-words list in `mcpObservableSurface.test.ts`, because it checks every code word in `docs/mcp.md`. I captured two Settings previews, toggle on (Connected) and off (Disabled), using the existing Playwright settings fixture with mocked data.
|
/review |
|
✅ AI Code Review Complete requested by @integry Posted 1 review: |
🔍 AI Code Review — codex:gpt-6-astraOverall EvaluationThe PR implements automatic tunnel-derived MCP configuration, persistent encryption keys, background registration, and Settings visibility. It needs changes before merge: the new scope ceiling is ineffective for Connect delegations, an explicit encryption-key change can silently replace an identity, and rejection recovery can abandon its retry. ✅ Persistent-key protection — Generated keys use private filesystem permissions and a database fingerprint; missing or replaced files fail closed. ✅ Background registration — Startup registration runs asynchronously, with bounded exponential backoff and shutdown timer cleanup. This review uses static tracing of the supplied code; no commands or tests were run. The issue specification is truncated, so conclusions rely on visible requirements and changed-code correctness. Current checks show no failures, with seven still pending. Merge blockersEvery finding below was introduced by this PR and must be resolved before merging. F1: 🔴 Enforce the ceiling on Connect delegations
F2: 🔴 Prevent silent identity replacement under an explicit key
F3: 🔴 Retain forced registration after a failed attempt
SuggestionsThese are optional follow-ups and are not sent to S1: 🟢 Exercise the paired gateway harnessThe new registration tests use a stand-in that accepts registration payloads. Running the existing paired gateway harness with tunnel-derived configuration would strengthen evidence for the zero-config flow, including entitlement refusal and registration compatibility. This is additional integration assurance, not a demonstrated gateway defect or an independent merge blocker. ScoreThe implementation has useful persistence safeguards and focused tests, but three correctness defects remain in authorization and registration recovery. Pending checks are not themselves blockers. Score: 5/10 🤖 Review Details
Copy and edit to choose records from this review: 🤖 Review by ProPR |
|
/fix |
|
✅ Applied the requested follow-up changes in commit 778cdff
Summary of ChangesI fixed all three findings, and each has a regression test that I confirmed fails on the old code and passes now. The full F1 – scope ceiling on Connect delegations
F2 – identity not replaced when the key can't decrypt it
F3 – forced registration survives a failed attempt
There are no visual changes, so I made no previews. Something I found but didn't change: with an explicit 🤖 Implementation Details
Undo Changes • View Task Execution 💡 ProPR Slash Commands
🤖 Generated by ProPR |
…that I confirmed fails on the old code and passes now. The full `npm run test:mcp` run had 439 of 446 tests passing. The 7 failures were all in `relevanceServiceCommit.test.ts`, caused by this container's read-only `/tmp/git-processor`. That file passes 7/7 when pointed at writable clone paths. Typecheck and zero-warning lint for `packages/api` are clean. I fixed all three findings, and each has a regression test that I confirmed fails on the old code and passes now. The full `npm run test:mcp` run had 439 of 446 tests passing. The 7 failures were all in `relevanceServiceCommit.test.ts`, caused by this container's read-only `/tmp/git-processor`. That file passes 7/7 when pointed at writable clone paths. Typecheck and zero-warning lint for `packages/api` are clean. **F1 – scope ceiling on Connect delegations** - `policy.ts`: a Connect delegation's scopes are now filtered by the current admin ceiling before `requireScope` sees them. This is the same filter `verifyAccessToken` already applies to direct OAuth tokens. - The ceiling is read live on every request through the existing resolver-backed callback, so narrowing it in Settings applies to the next request. `allowedScopes()` in `oauth.ts` is now public so the policy can reuse it. - Test (`mcpDelegation.test.ts`): a valid delegation asking for `read, review, execute, merge` under the default ceiling gets only `read, review`, and `execute` is refused. After the ceiling is narrowed it gets only `read`, and `review` is refused. **F2 – identity not replaced when the key can't decrypt it** - `McpStore.has()` reports whether a row exists even when it can't be decrypted. `get()` still treats such a row as missing. - `registerMcpInstance` now stops with `KEY_CHANGED` when an identity row exists but can't be decrypted, even with `replaceIdentity`. It does this before generating a key or contacting Connect. Automatic registration and `mcp:connect:register` both go through this function, so both are covered. - Automatic registration then reports `failed:KEY_CHANGED` and retries with backoff. - Test (`mcpTunnelMode.test.ts`): after registering, the explicit `MCP_ENCRYPTION_KEY` is changed. Both automatic and manual registration then fail, the stored identity row is unchanged, and nothing is sent to Connect. **F3 – forced registration survives a failed attempt** - `connectRegistration.ts`: the forced-registration flag is no longer cleared before the request. It is cleared only after a successful registration, and only if no new 409 arrived during that request (tracked with a rejection counter). A rejection that may postdate the request therefore still forces another registration. - Test: a fresh receipt, then a 409, then a registration that fails because Connect is unavailable. The state stays `registered` with a 5 s retry instead of the daily refresh. The timer-driven retry registers again and success clears the flag. A second part sends a 409 during an in-flight registration and checks that one more registration follows. There are no visual changes, so I made no previews. **Something I found but didn't change:** with an explicit `MCP_ENCRYPTION_KEY` in tunnel mode, `loadHostedTunnelMcpConfig` still never compares the key with the stored fingerprint. Records other than the Connect identity, like grants and credentials, will therefore just look missing after a key change instead of producing a `KEY_CHANGED` setup error. F2 asked only for the fail-closed check in the shared registration function, so I didn't add this. PR: #3392 Comment by: @integry (ID: 6511180777) Model: Claude Opus 5.5
|
/review |
|
Posted 0 review, 1 failed: |
🔍 AI Code Review — codex:gpt-6-astra❌ Review failed: Analysis failed: Unable to find image 'propr/runtime-agent:7662b103e3c08511' locally Run 'docker run --help' for more information |
|
/review |
|
✅ AI Code Review Complete requested by @integry Posted 1 review: |
🔍 AI Code Review — codex:gpt-6-astraOverall EvaluationThe PR implements automatic tunnel configuration, durable encryption keys, background Connect registration, and Settings controls. It needs a localized retry-scheduling correction before merge. ✅ Key preservation — Missing keys and undecryptable persisted identities fail closed instead of silently replacing existing identity material. ✅ Scope enforcement — Connect delegations are filtered through the current instance scope ceiling, with coverage for subsequent ceiling reductions. The supplied issue description is truncated; this review uses the visible objective and complete supplied diff. Verification was static only, as requested. Current checks report 26 passed, none failed, and one pending. Merge blockersEvery finding below was introduced by this PR and must be resolved before merging. F4: 🔴 Preserve retries scheduled during registration
SuggestionsThese are optional follow-ups and are not sent to S2: 🟢 Exercise the paired gateway flowThe new tests use a registration stand-in that accepts requests without exercising gateway verification. Running the documented cross-repository harness with tunnel-derived configuration would strengthen confidence in registration and first-request credential handoff. This is optional integration evidence, not a demonstrated additional blocker. ScoreThe implementation has useful failure protections and focused coverage, but its automatic recovery scheduler can postpone an outstanding rejection retry for a day. The otherwise passing current checks do not cover that timer-driven sequence. Score: 6/10 🤖 Review Details
Copy and edit to choose records from this review: 🤖 Review by ProPR |
|
/fix |
|
✅ Applied the requested follow-up changes in commit 2dc9009
Summary of ChangesI fixed F4. When a Connect rejection arrives while a registration request is still in flight, the successful registration no longer replaces that rejection's retry timer with the 24-hour refresh. Change ( Regression (
Verification:
Other paths I checked, unchanged:
🤖 Implementation Details
Undo Changes • View Task Execution 💡 ProPR Slash Commands
🤖 Generated by ProPR |
…ration request is still in flight, the successful registration no longer replaces that rejection's retry timer with the 24-hour refresh. I fixed F4. When a Connect rejection arrives while a registration request is still in flight, the successful registration no longer replaces that rejection's retry timer with the 24-hour refresh. **Change (`packages/api/mcp/connectRegistration.ts`):** after a successful registration, `runOnce` now calls a new `nextCheckMs()` instead of always scheduling the daily refresh. If a newer rejection has left `forceRegister` set, it schedules the time left on that rejection's 60-second cooldown (counted from `lastRegistrationAt`). Otherwise it schedules the daily refresh as before. It reads `forceRegister` when it schedules, so a rejection that lands between `attempt()` returning and `runOnce` resuming is covered too. **Regression (`packages/api/test/mcpTunnelMode.test.ts`):** I extended "a 409 keeps forcing registration through transient failures until one succeeds". After a rejection injected during a successful registration, the test now checks two things: - The next scheduled check is the 60-second cooldown, not the daily refresh. - Firing that timer by itself sends the next registration. The test no longer calls `manager.trigger()` by hand for this step. After that registration succeeds, the schedule goes back to the daily refresh. **Verification:** - `mcpTunnelMode.test.ts` passes 17/17. - With the old line put back temporarily, the new assertion fails (16 pass, 1 fail). - Zero-warning eslint on both files and `tsc --noEmit` for `packages/api` are clean. **Other paths I checked, unchanged:** - **Failed registration:** this path also replaces the rejection's timer, with the backoff retry (5 s, growing to a 15-minute cap). The registration obligation is still kept, because `forceRegister` stays set and a retry is always scheduled, so this is a slower retry rather than a lost one. I left it alone. - **Rejection after the cooldown has passed:** if the rejection arrives during a request that has run longer than 60 seconds, the listener calls `trigger()`. That sets `rerun`, so the loop runs again right away. This path is not affected. PR: #3392 Comment by: @integry (ID: 6511800858) Model: Claude Opus 5.5
|
/review |
|
✅ AI Code Review Complete requested by @integry Posted 1 review: |
🔍 AI Code Review — codex:gpt-6-astraOverall EvaluationThe PR implements automatic tunnel configuration, durable key storage, background registration, and Settings controls. It needs a platform compatibility correction before merge. ✅ Preserved identity — Registration refuses to overwrite an existing identity that the configured encryption key cannot decrypt. ✅ Bounded delegated scopes — Connect grants are intersected with the current instance scope ceiling. ✅ Persistent retry obligation — A registration rejection remains actionable through transient failures and rejections arriving during registration. The supplied current checks show 27 passed and no failures or pending checks. This review is static; no commands were run. The issue specification is explicitly truncated, so requirement assessment is limited to its visible content. Merge blockersEvery finding below was introduced by this PR and must be resolved before merging. F5: 🔴 Key creation fails on native Windows
SuggestionsThese are optional follow-ups and are not sent to S3: 🟢 Show explicit Connect opt-out accuratelyWith S4: 🟢 Exercise the actual gateway contractThe new tests use a registration stand-in that accepts requests and returns matching receipts. Running the existing paired gateway harness with automatic tunnel configuration would strengthen confidence in registration and entitlement refusal. This is additional integration evidence, not a demonstrated contract defect. ScoreThe implementation has useful coverage and passing current checks, but automatic key provisioning fails on native Windows. The required correction is localized. Score: 6/10 🤖 Review Details
Copy and edit to choose records from this review: 🤖 Review by ProPR |
|
/fix |
|
✅ Applied the requested follow-up changes in commit 2d1cccc
Summary of ChangesI fixed F5. On native Windows, the MCP encryption key is now created and reused using Windows file permissions (ACLs) instead of POSIX mode bits. On Linux and macOS the exact I could not run anything on Windows. The What changed
Tests: I added
The three Windows tests fail on the previous Separate finding, not changed: The change has no visual effect, so I made no previews. 🤖 Implementation Details
Undo Changes • View Task Execution 💡 ProPR Slash Commands
🤖 Generated by ProPR |
…w created and reused using Windows file permissions (ACLs) instead of POSIX mode bits. On Linux and macOS the exact `0700`/`0600` checks are unchanged. I fixed F5. On native Windows, the MCP encryption key is now created and reused using Windows file permissions (ACLs) instead of POSIX mode bits. On Linux and macOS the exact `0700`/`0600` checks are unchanged. **I could not run anything on Windows.** The `icacls` and PowerShell calls have not been run on a real Windows machine. The tests swap in a stand-in for the Windows permission checks. So creating and reusing a key under an ordinary Windows account, which the finding asks for, still needs checking on a Windows machine. **What changed** - **New file, `packages/api/mcp/privateKeyStorage.ts`**: it decides what "private" means on each platform. - **Linux and macOS**: the same mode checks as before. - **Windows**: a key file or directory counts as private only if it is owned by the current account, SYSTEM or Administrators, and only those can access it. Any other entry, such as Users, Everyone or Authenticated Users, makes it fail. The permissions are read with Windows PowerShell, called by its full System32 path. - **Locking the folder on Windows**: if the `mcp` folder is not private, the key creation code runs `icacls <dir> /inheritance:r /grant:r *<SID>:(OI)(CI)F`. This removes the permissions it inherited from its parent and gives full access to the current account only, and the new key file inherits that. The folder is checked again afterwards, and if it is still shared no key is written. - **Saving to disk on Windows**: the step that flushes the folder to disk is skipped, because Windows does not support it. The key file itself is still flushed before it is linked into place. - **`tunnelMode.ts`**: the key reader and key creation code use these platform checks instead of fixed mode bits. - Windows ignores the "don't follow symlinks" flag, so the reader now checks the path before opening the file and rejects symlinks. It then confirms the opened file is the same one it checked. - The existing error codes are unchanged: a non-private file or folder gives `KEY_STORAGE_INVALID`, and a failure running PowerShell or `icacls` gives `KEY_STORAGE_UNAVAILABLE`. - **`docs/mcp.md`**: the key-storage row now covers Windows. **Tests**: I added `packages/api/test/mcpTunnelKeyWindows.test.ts` and listed it in `test:mcp`. It uses real files on disk with the stand-in Windows checks, and covers: - First creation succeeds even though the folder looks like `0777`, and reuse succeeds even though the file looks like `0666`; the folder is locked down exactly once. - A key file other accounts can read, or one owned by another account, is rejected and left untouched rather than replaced. - A folder that is still shared after the lock-down attempt never receives a key. - The rules for which accounts count as private. - The Linux/macOS `0600` check still rejects a `0644` file. The three Windows tests fail on the previous `tunnelMode.ts` and pass now. The new file and `mcpTunnelMode.test.ts` together pass 22 of 22. Typecheck and zero-warning lint are clean for the changed `packages/api` files. **Separate finding, not changed:** `packages/api/services/instanceWebPushConfiguration.ts` has the same POSIX-only `0700`/`0600` checks, so automatic Web Push key creation would fail on native Windows the same way. That code predates this PR, which only extracted `instanceDataDirectory` from it, so I left it alone. The change has no visual effect, so I made no previews. PR: #3392 Comment by: @integry (ID: 6517634976) Model: Claude Opus 5.5
|
CI failed: Full Test Suite Shard 2/4 Please investigate and fix this CI failure.
Failure evidence
|
|
✅ Applied the requested follow-up changes in commit c2bcfec Summary of ChangesI fixed the Shard 2/4 failure with a one-line doc change. The failing test passes again locally. Cause: the shard failed in Fix: in Checked:
I didn't run the rest of Shard 2/4. 🤖 Implementation Details
Undo Changes • View Task Execution 💡 ProPR Slash Commands
🤖 Generated by ProPR |
…he failing test passes again locally. I fixed the Shard 2/4 failure with a one-line doc change. The failing test passes again locally. **Cause:** the shard failed in `packages/api/test/mcpObservableSurface.test.ts`. That test finds every backticked lowercase word in `docs/mcp.md` and requires each to be either a tool the admin can see or listed as an allowed non-tool word. The last commit (the F5 Windows ACL fix) added the line "on Windows the `` `mcp` `` directory's ACL…" to the `MCP_ENCRYPTION_KEY` row. A bare `` `mcp` `` is neither, so the test failed with: *"docs/mcp.md names mcp, but it is neither an admin-visible tool nor an explicit non-tool token"*. No other test in that shard failed. **Fix:** in `docs/mcp.md` (line 227), the phrase now reads "on Windows the `` `<data dir>/mcp` `` directory's ACL". This matches the path written earlier in the same sentence and is more precise. It also no longer reads as a tool name, so the allow-list in the test didn't need a new entry. **Checked:** - Running `mcpObservableSurface.test.ts` and `mcpDocs.test.ts` through `scripts/run-test-suite.mjs`: both pass. - `usageTipsCatalog` and `buildDocsManifest` tests pass. The usage-tip generator reads a different paragraph of `docs/mcp.md`, so its output doesn't change. I didn't run the rest of Shard 2/4. PR: #3392 Comment by: @propr-dev[bot] (ID: 6518132330) Model: Claude Opus 5.5
|
/review |
|
✅ AI Code Review Complete requested by @integry Posted 1 review: |
🔍 AI Code Review — codex:gpt-6-astraOverall EvaluationThe PR implements automatic tunnel configuration, persistent encryption keys, background registration, and Settings controls. It needs a localized correction to how Settings reports an explicit Connect opt-out. ✅ Identity preservation — Registration refuses to overwrite an existing identity that the configured encryption key cannot decrypt. ✅ Delegation scope enforcement — Connect delegations are filtered through the current instance scope ceiling, with coverage for narrowing that ceiling. ✅ Recovery coverage — Tests cover registration retries, tunnel replacement, missing keys, and rejections arriving during registration. The issue specification is truncated, so requirements beyond its visible portion could not be assessed. This review used static analysis only; no commands were run. Current checks show zero failures and three pending. Merge blockersEvery finding below was introduced by this PR and must be resolved before merging. F6: 🔴 Report explicit Connect opt-out correctly
SuggestionsThese are optional follow-ups and are not sent to S5: 🟢 Validate automatic registration against ConnectThe new registration tests use an accepting stand-in. Running the documented paired gateway harness with the automatically derived configuration would strengthen evidence that the zero-configuration flow works across both services. This is additional integration confidence, not a demonstrated code blocker. S6: 🟢 Clarify direct versus hosted instructions
ScoreThe backend implementation has substantial recovery and authorization coverage, but the explicit opt-out produces a misleading new Settings state that needs correction. No current checks have failed; three remain pending. Score: 6/10 🤖 Review Details
Copy and edit to choose records from this review: 🤖 Review by ProPR |
|
/fix |
|
✅ Applied the requested follow-up changes in commit 4201315
Summary of ChangesI fixed F6. With What changed
Tests
Completeness check: the other places that touch this state are the admin MCP route, the I captured a Settings screenshot of the new state with the project's Playwright setup and fake API data. It is attached as a preview. Visual previewMCP server with MCP_CONNECT_TRUST=false
Hosted-tunnel Settings: status reads 'Disabled by configuration', the Connect toggle is unchecked and disabled, and the description explains the toggle cannot override MCP_CONNECT_TRUST=false Connect sign-in required. Original retained until 2027-01-28T09:24:31.089Z. View the full-resolution original in ProPR Connect 🤖 Implementation Details
Undo Changes • View Task Execution 💡 ProPR Slash Commands
🤖 Generated by ProPR |
… mode, the MCP section in Settings now says **Disabled by configuration** instead of **Registering** forever. I fixed F6. With `MCP_CONNECT_TRUST=false` in hosted tunnel mode, the MCP section in Settings now says **Disabled by configuration** instead of **Registering** forever. **What changed** - **Backend:** in tunnel mode, the MCP status now includes a new `connectTrustDisabled: true` flag whenever `MCP_CONNECT_TRUST` is set to anything other than `true` (`packages/api/mcp/configResolver.ts`). An unset value still counts as on, because tunnel mode defaults it to `true`. The flag is also added to the UI's status type in `propr-ui/src/api/adminMcpApi.ts`. - **Status text** (`mcpConnectTunnelStatus.ts`): the label is **Disabled by configuration** with a neutral colour, checked just after the operator-off case. The description now reads: "MCP_CONNECT_TRUST=false turns off MCP through ProPR Connect, and this setting cannot override it. Remove MCP_CONNECT_TRUST to use Connect." It no longer says clients connect with no extra setup. - **Toggle** (`McpServerSection.tsx`): "Allow MCP clients through ProPR Connect" is now unchecked and greyed out in this state. The saved `connect_enabled` value is kept as it was. No Connect URL is shown, since none is available. **Tests** - The backend tunnel-mode test (`mcpTunnelMode.test.ts`) now checks that the opt-out reports `connectTrustDisabled`, Connect as unavailable and no Connect URL. It also checks that the default setup does not report the flag. All 17 tests in the file pass. With the backend change removed, the new check fails. - A new UI test in `McpServerSection.test.tsx` covers this exact case: label, unchecked and disabled toggle, the explanation, no "no extra setup" text and no Connect URL. All 4 tests pass. Against the previous UI code, the new test fails. - Typecheck and zero-warning lint pass for `propr-ui` and `packages/api`. **Completeness check:** the other places that touch this state are the admin MCP route, the `connect_mcp` field in the status route, and the registration manager. All of them already handle the opt-out correctly, so nothing else needed changing. I didn't find any other problems. I captured a Settings screenshot of the new state with the project's Playwright setup and fake API data. It is attached as a preview. PR: #3392 Comment by: @integry (ID: 6519315886) Model: Claude Opus 5.5
|
/review |
|
✅ AI Code Review Complete requested by @integry Posted 1 review: |
🔍 AI Code Review — codex:gpt-6-astraOverall EvaluationThe PR implements tunnel-derived MCP configuration, persistent encryption keys, automatic Connect registration, and Settings controls. No merge-blocking defect was verified in the supplied code. It is ready to merge within the visible scope, conditional on the remaining checks passing. ✅ Persistent identity protection — Missing or changed encryption keys fail closed, and registration refuses to overwrite an identity it cannot decrypt. ✅ Delegation scope enforcement — Connect grants are filtered against the current instance scope ceiling before becoming principal scopes. ✅ Registration recovery — Background registration preserves retry obligations across transient failures and rejections arriving during registration. This was a static review; no commands or tests were run. The supplied original issue body is explicitly truncated, so unseen acceptance criteria could not be assessed. Current checks report no failures and five pending checks. Merge blockersNo merge blockers. SuggestionsThese are optional follow-ups and are not sent to S7: 🟢 Exercise the actual gatewayThe new registration tests use a stand-in that accepts registration payloads. Running the existing paired Connect harness with tunnel-derived configuration would strengthen evidence that automatic registration works against the gateway’s actual validation and entitlement rules. This is additional integration confidence, not a demonstrated code blocker. S8: 🟢 Clarify connection instructions
ScoreThe implementation has substantial focused coverage and protects the important key, identity, and scope boundaries. Static review found no actionable regression; actual-gateway validation and the pending checks remain outstanding evidence. Score: 8/10 🤖 Review Details
Copy and edit to choose records from this review: 🤖 Review by ProPR |
AI Implementation Summary
Closes #3391
Branch:
3391/claude-opus-5-5-zero-config-mcp-through-p-20261029-2313-cntCommits: ✅ Changes committed (b56670d)
AI Processing Completed
Execution Details:
Summary:
Zero-config MCP through ProPR Connect is implemented. With the hosted UI tunnel on (
PROPR_UI_TUNNEL_ENABLED=trueandPROPR_INSTANCE_IDset), a Plus user only needs to addhttps://mcp.propr.dev/mcpto their MCP client. The 15 new backend tests, 3 new UI tests and the related existing suites pass, and typecheck and zero-warning lint are clean. Nothing ran against the real Connect gateway; the registration tests use a stand-in for it.Each item from the issue (only when its variable is unset; explicit values still win):
MCP_PUBLIC_ORIGINcomes from the sameproprInstanceProxyUrl()the tunnel-modeAPI_PUBLIC_URLuses, so it ishttps://t-<id>.propr.dev.MCP_INSTANCE_IDisPROPR_INSTANCE_ID.<data dir>/mcp/encryption-key, next to the web-push keys, with owner-only permissions. Its fingerprint is saved in the database on first use. If the file later goes missing, MCP reportsKEY_MISSINGinstead of making a new key; a replaced file reportsKEY_CHANGED. The key is never logged.MCP_CONNECT_TRUSTis implied.connect_enableddatabase setting is now the "Allow MCP clients through ProPR Connect" toggle in Settings.MCP_ENABLED=falsestill forces it off.packages/api/mcp/connectRegistration.tsruns the same flow asmcp:connect:register. It leaves a current registration alone. It registers again when the registration is missing, older than a day, for a different tunnel id, or rejected by Connect with a 409. Failures retry after 5 s, doubling up to 15 minutes, and API startup never waits for it./health,/api/statusand the Settings page reportconnect_mcp. The npm script still works for manual repair and now uses the same derived config.Decisions you may want to revisit:
.env.exampleand the CLI copy shippedMCP_ENABLED=falseandMCP_CONNECT_TRUST=false. That would have forced the feature off for every new stack, so both are now blank with advanced-override comments. Existing.envfiles that still contain those lines keep MCP off.read,plan,review. An admin has to widen it before clients can execute or merge.PROPR_UI_TUNNEL_TOKENin tunnel mode, because the launcher only gives it to the cloudflared sidecar. The explicit setup outside tunnel mode still requires it.PROPR_INSTANCE_IDand that changes, the instance creates a new identity and users reconnect. An explicitMCP_INSTANCE_IDstill can't be overwritten.failed:ACCESS_REVOKED.The safety guarantees are unchanged: requests still need a valid Connect-signed delegation, the instance fails closed when online validation is unavailable, and the local
/api/mcpchecks still apply. Turning it on by default only makes the instance reachable through Connect.Docs and tests:
docs/mcp.mdis now "MCP works automatically with the hosted UI tunnel", with the explicit settings kept under "Advanced". The contract doc says the gateway is live, and the feature guide is updated.packages/api/test/mcpTunnelMode.test.ts, added totest:mcp, covers every case listed in the issue.McpServerSection.test.tsxcovers the toggle.connect_mcp,registered,pendinganddisabledto the allowed-words list inmcpObservableSurface.test.ts, because it checks every code word indocs/mcp.md.I captured two Settings previews, toggle on (Connected) and off (Disabled), using the existing Playwright settings fixture with mocked data.
Detailed Logs:
0e67c8f5-75fe-4e3f-8340-621f41ee5138Log files stored at:
/usr/src/app/logs/executions/issue-3391-2026-10-29T23-37-23-251Z-conversation.json/usr/src/app/logs/executions/issue-3391-2026-10-29T23-37-23-251Z-output.txtLatest Conversation Messages
This PR was created automatically by ProPR after processing issue #3391.
💡 Need changes?
Comment on this PR to request refinements — the AI agent monitors comments and will update the implementation based on your feedback. Keep iterating until you're satisfied!
Visual preview
MCP through ProPR Connect turned off
The same section after an admin turns the toggle off: status Disabled, with no connection details.
Connect sign-in required. Original retained until 2027-01-27T23:37:28.124Z.
View the full-resolution original in ProPR Connect
MCP Server settings in hosted UI tunnel mode
Settings → Integrations & MCP → MCP Server with the hosted tunnel: the admin toggle 'Allow MCP clients through ProPR Connect' is on, the status reports Connected (connect_mcp: registered), and the connection URL is https://mcp\.propr\.dev/mcp\. Rendered with the Playwright settings fixture and mocked A
Connect sign-in required. Original retained until 2027-01-27T23:37:29.268Z.
View the full-resolution original in ProPR Connect