feat(mcp-portal): MCPPortalProjection workflow + build-event handler (D2, default-OFF)#248
Open
chitcommit wants to merge 7 commits into
Open
feat(mcp-portal): MCPPortalProjection workflow + build-event handler (D2, default-OFF)#248chitcommit wants to merge 7 commits into
chitcommit wants to merge 7 commits into
Conversation
…(D2, default-OFF)
Scaffolds the registry→CF-MCP-Portal projection for Model B (mcp.chitty.cc /
`chitty-mcp`), feature-flagged INERT by default.
CQRS triad (locked): ChittyRegister=WRITE, ChittyRegistry=DISCOVERY,
CF MCP Portal=client PROJECTION written only by this Workflow off discovery.
Adds:
- src/workflows/mcp-portal-projection.js — durable Workflow with the §3 step
breakdown: read-discovery → read-portal → diff → guard-cap → apply-adds →
apply-membership → sync → toggles → verify → telemetry. ALL write steps are
gated by env.MCP_PORTAL_PROJECTION_ENABLED === "true". With the flag off
(the shipped default) the Workflow runs read+diff and writes NOTHING.
- src/services/mcp-portal-projection.js — shared CF ai-controls client
(fetchPortal / putPortalServers whole-array / createPortalServer per-server /
syncPortalServer), the registry /v0.1/servers discovery reader (filters to
canonical https://{svc}.chitty.cc/mcp), and computePortalDiff. PORTAL_SERVER_CAP=40.
- src/api/routes/mcp-portal.js — POST /api/v1/mcp-portal/build-event enqueues
the Workflow (beacon double-post target + daily sweep); GET .../:id status.
Response reports writes_enabled so callers can confirm inertness.
- wrangler.jsonc — MCP_PORTAL_PROJECTION workflow binding + MCP_PORTAL_ID and
MCP_PORTAL_PROJECTION_ENABLED="false" vars across dev/staging/prod.
- scripts/mcp-portal-dryrun.mjs (+ fixture) — D3 read-only diff runner.
Inertness proven: `wrangler deploy --dry-run` binding summary shows
MCP_PORTAL_PROJECTION (Workflow) + MCP_PORTAL_PROJECTION_ENABLED ("false").
No portal writes occur on deploy. Guard also includes a remove-all safety
brake (refuses empty desired-set against a non-empty portal).
Part of the mcp.chitty.cc Model B migration (D2). No live behavior change.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ❌ Deployment failed View logs |
chittyconnect | bf7fcbe | Jun 14 2026, 04:33 AM |
📝 WalkthroughWalkthroughThis PR introduces a durable MCP Portal Projection workflow that synchronizes ChittyRegistry-discovered MCP servers into the Cloudflare-managed ChangesMCP Portal Projection (Model B)
CF Access JWT Verification and Secrets Portal Hardening
Sequence Diagram(s)sequenceDiagram
participant Client
participant mcpPortalRoutes as POST /api/v1/mcp-portal/build-event
participant MCPPortalProjection as MCPPortalProjection (Workflow)
participant svc as mcp-portal-projection service
participant cfAPI as Cloudflare Portal API
participant chittytrack
rect rgba(70, 130, 180, 0.5)
note over Client, mcpPortalRoutes: Enqueue
Client->>mcpPortalRoutes: POST { trigger, service, portalId }
mcpPortalRoutes->>MCPPortalProjection: wf.create({ trigger, service, portalId })
mcpPortalRoutes-->>Client: { instance.id, writes_enabled, note }
end
rect rgba(100, 160, 100, 0.5)
note over MCPPortalProjection, cfAPI: Workflow execution
MCPPortalProjection->>svc: fetchDiscoveryServers(env)
MCPPortalProjection->>cfAPI: fetchPortal(portalId)
MCPPortalProjection->>svc: computePortalDiff(desired, portal)
MCPPortalProjection->>svc: evaluateRemovalGuard(diff, currentCount)
alt removals blocked AND writes enabled
MCPPortalProjection->>chittytrack: POST removal_guard_tripped
end
alt writes enabled
MCPPortalProjection->>cfAPI: createPortalServer (new entries)
MCPPortalProjection->>cfAPI: putPortalServers (whole-array PUT)
MCPPortalProjection->>cfAPI: syncPortalServer (added entries)
end
MCPPortalProjection->>cfAPI: fetchPortal (verify post-state)
end
rect rgba(180, 100, 60, 0.5)
note over Client, mcpPortalRoutes: Status poll
Client->>mcpPortalRoutes: GET /build-event/:id
mcpPortalRoutes->>MCPPortalProjection: wf.get(id) → instance.status()
mcpPortalRoutes-->>Client: workflow status
end
sequenceDiagram
participant Request
participant resolveAccessEmail
participant verifyCfAccessAssertion
participant isCloudflareAccessAllowed
participant SecretsPortal as /secrets-portal or /upsert
Request->>isCloudflareAccessAllowed: req, env, c
isCloudflareAccessAllowed->>resolveAccessEmail: Cf-Access-Jwt-Assertion header
alt CF_ACCESS_TEAM_DOMAIN + CF_ACCESS_AUD configured
resolveAccessEmail->>verifyCfAccessAssertion: assertion, env
verifyCfAccessAssertion-->>resolveAccessEmail: { valid, email } or { valid: false }
resolveAccessEmail-->>isCloudflareAccessAllowed: email or ""
else not configured
resolveAccessEmail-->>isCloudflareAccessAllowed: CF-Access-Authenticated-User-Email (logs AUTH-001)
end
isCloudflareAccessAllowed->>isCloudflareAccessAllowed: c.set("accessEmail", email)
isCloudflareAccessAllowed-->>Request: allowed true/false
Request->>SecretsPortal: c.get("accessEmail") used as apiKey.name
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
…s + D5 blockers Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…/1c) + empty-PUT risk - blocker #1 reframed from 'registry sparse' to 'desired-set undefined': registry backfill (1a), endpoint-pattern coverage for direct-route + third-party (1b), and portal-id mapping (1c) - corrects stale '.agent.chitty.cc/mcp' remediation: live portal proves {svc}.chitty.cc/mcp is the working canonical form - adds Open risk: PUT servers:[] returned 400; remove-to-empty path unproven Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a feature-flagged (default-OFF) Cloudflare Workflow + API trigger route to project ChittyRegistry discovery data into the Cloudflare MCP Portal (“Model B”) while keeping deployments inert unless explicitly enabled.
Changes:
- Adds
MCPPortalProjectionWorkflow to read discovery + portal state, compute a diff, and (only when enabled) create/sync servers and write portal membership. - Introduces shared
mcp-portal-projectionservice utilities for CF ai-controls calls, discovery reading, and diff computation (+ cap constant). - Adds
/api/v1/mcp-portal/build-eventenqueue + status routes and wires a workflow binding + env vars inwrangler.jsonc, plus a dry-run script and portal fixture.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| wrangler.jsonc | Adds workflow binding and default-OFF projection env vars across dev/staging/prod. |
| src/workflows/mcp-portal-projection.js | New Workflow implementing the read/diff/(gated) write pipeline for portal projection. |
| src/services/mcp-portal-projection.js | New service layer for CF MCP Portal CRUD/sync, registry discovery reading, and diff computation. |
| src/api/routes/mcp-portal.js | Adds authenticated API routes to enqueue projection workflow runs and check run status. |
| src/api/router.js | Registers the new /api/v1/mcp-portal route group. |
| src/index.js | Exports the workflow class so the binding can reference it. |
| scripts/mcp-portal-dryrun.mjs | Adds a read-only diff runner (D3) for safe projection planning. |
| scripts/fixtures/chitty-mcp-portal-2026-06-12.json | Fixture snapshot to support offline dry-run diffs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+33
to
+38
| let payload = {}; | ||
| try { | ||
| payload = await c.req.json(); | ||
| } catch { | ||
| payload = {}; | ||
| } |
Comment on lines
+40
to
+46
| * @param {{ payload: { portalId?: string, trigger?: string, service?: string } }} event | ||
| */ | ||
| async run(event, step) { | ||
| const env = this.env; | ||
| const enabled = env.MCP_PORTAL_PROJECTION_ENABLED === "true"; | ||
| const portalId = event.payload?.portalId || env.MCP_PORTAL_ID || "chitty-mcp"; | ||
| const trigger = event.payload?.trigger || "manual"; |
Comment on lines
+78
to
+89
| const desiredCount = diff.desired.length; | ||
| const issues = []; | ||
| if (desiredCount > PORTAL_SERVER_CAP) { | ||
| issues.push(`desired ${desiredCount} exceeds CF cap ${PORTAL_SERVER_CAP}`); | ||
| } | ||
| // Safety brake: if discovery yields an empty desired set while the portal | ||
| // currently has servers, that is almost certainly a discovery outage — | ||
| // do NOT project a remove-all even when writes are enabled. | ||
| if (desiredCount === 0 && portal.servers.length > 0) { | ||
| issues.push("desired set empty but portal non-empty — refusing remove-all (likely discovery outage)"); | ||
| } | ||
| return { ok: issues.length === 0, issues, desiredCount }; |
Comment on lines
+100
to
+105
| const raw = entries.map((e) => { | ||
| const s = e.server || {}; | ||
| const url = (s.remotes || []).find((x) => x?.url)?.url || null; | ||
| const status = e._meta?.["io.modelcontextprotocol.registry/official"]?.status; | ||
| return { name: s.name, url, status }; | ||
| }); |
Comment on lines
+73
to
+75
| const body = await r.json().catch(() => ({})); | ||
| if (!r.ok) return { ok: false, error: JSON.stringify(body.errors ?? body) }; | ||
| return { ok: true, id: body.result?.id }; |
![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: 49fa12bec7
ℹ️ 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".
Comment on lines
+111
to
+114
| const desiredMembership = diff.desired.map((s) => ({ | ||
| server_id: s.id, | ||
| default_disabled: s.default_disabled ?? false, | ||
| })); |
There was a problem hiding this comment.
When MCP_PORTAL_PROJECTION_ENABLED is turned on, this whole-array portal update rebuilds every membership entry from discovery data, which never contains portal-scoped settings. As a result, any existing server that is kept in the portal but currently has default_disabled: true (or other portal-level customizations such as tool/prompt overrides) is written back with default_disabled: false and the omitted fields are lost. For a projection that is supposed to update membership, the PUT needs to merge/preserve the existing portal server mapping for kept servers rather than recreating all entries from discovery-only fields.
Useful? React with 👍 / 👎.
Comment on lines
+86
to
+87
| if (desiredCount === 0 && portal.servers.length > 0) { | ||
| issues.push("desired set empty but portal non-empty — refusing remove-all (likely discovery outage)"); |
There was a problem hiding this comment.
When writes are enabled, this safety brake only catches a completely empty desired set. A non-empty but sparse discovery response still passes the guard and then apply-membership removes every current portal server not in that partial desired set; the commit's own dry-run scenario (4 desired vs 27 current) is exactly the case that would wipe most of chitty-mcp. Add a high-removal/low-coverage guard or disable removals until the discovery source is known complete.
Useful? React with 👍 / 👎.
Comment on lines
+40
to
+42
| const enabled = c.env.MCP_PORTAL_PROJECTION_ENABLED === "true"; | ||
| const portalId = payload.portalId || c.env.MCP_PORTAL_ID || "chitty-mcp"; | ||
|
|
There was a problem hiding this comment.
If MCP_PORTAL_PROJECTION_ENABLED is ever enabled, any authenticated caller that can hit this build-event endpoint can override portalId and cause the workflow to read and rewrite a different MCP portal in the same Cloudflare account using the worker's CF_API_TOKEN. Since this route is documented as the chitty-mcp projection trigger, accepting an arbitrary payload portal expands the blast radius of a beacon or API-key misuse; use the configured MCP_PORTAL_ID or an explicit allowlist instead.
Useful? React with 👍 / 👎.
Comment on lines
+140
to
+142
| const toAdd = desired.filter((s) => !currentIds.has(s.id)); | ||
| const toRemove = (portal.servers || []).map((s) => s.id).filter((id) => !desiredById.has(id)); | ||
| const keeps = (portal.servers || []).map((s) => s.id).filter((id) => desiredById.has(id)); |
There was a problem hiding this comment.
When a registry entry keeps the same derived server id but its MCP URL changes, this diff treats it as a keep because it only compares IDs. Since the workflow only calls createPortalServer for toAdd and never updates existing account-level server definitions, the portal will continue pointing at the old hostname instead of projecting the registry change. Include hostname comparison and an update path for kept IDs whose endpoint no longer matches discovery.
Useful? React with 👍 / 👎.
Comment on lines
+143
to
+148
| const desiredIds = diff.desired.map((s) => s.id); | ||
| const missing = writesAllowed ? desiredIds.filter((id) => !present.has(id)) : []; | ||
| return { | ||
| portal_server_count: p?.servers?.length ?? 0, | ||
| converged: writesAllowed ? missing.length === 0 : null, | ||
| missing, |
There was a problem hiding this comment.
After writes are enabled, verification only checks that desired IDs are present, so a run can report converged: true even when obsolete portal servers remain after the whole-array PUT was ignored, partially applied, or raced with another change. Because this workflow's diff explicitly plans removals, the verification step should also compare for extra IDs before declaring convergence.
Useful? React with 👍 / 👎.
…ondition)
Protects the live chitty-mcp portal (27 servers) from being wiped by a
sparse/broken discovery source — the core D5 blocker. Three changes, all
flag-gated behind MCP_PORTAL_PROJECTION_ENABLED (still default-OFF, inert):
1. Removal-safety guard (evaluateRemovalGuard, pure fn). Refuses removals when
they exceed max(2, 20% of current) OR the desired set collapses below 50%
of the portal. Empty membership is a HARD fail (never PUT servers:[] → 400)
and is NOT overridable. Proportional/collapse brakes are overridable via
payload.overrideRemovalGuard=true. On trip: zero removals, diff logged,
alert emitted to chittytrack, adds still proceed (add-only membership PUT =
current ∪ adds, so the portal is never shrunk behind the guard).
2. Hostname-keyed diff (resolves board blockers 1b + 1c). computePortalDiff now
keys on normalized hostname, not a URL-derived id — so portal id
chittyagent-ship vs URL slug ship no longer churns (1c). fetchDiscoveryServers
widened past the canonical {svc}.chitty.cc/mcp filter to include
*.ccorp.workers.dev/mcp direct-route and third-party MCPs (1b); canonical is
now advisory health metadata only.
3. Dry-run + tests. scripts/mcp-portal-dryrun.mjs reports the new shapes and the
guard verdict. 13 real-behavior tests (no mocks of the diff logic) over the
live 27-server portal: complete discovery => pure no-op (add0/remove0/keep27);
sparse 4-vs-27 => guard BLOCKS. 455/455 suite green.
Live evidence (2026-06-12): registry /v0.1/servers yields only 5 desired MCP
servers vs the live portal's 27; literal projection = remove-all-27; the new
guard correctly blocks it (old desired===0 brake did NOT — desired was 5).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…OCKED (register→discovery bridge unimplemented) 27-server live audit (25 READY / 2 READY-AUTH / 0 dead/stale). New dry-run: discovery 5 vs portal 27 → guard correctly blocks remove-all. Seed NOT performed: proven register.chitty.cc has no MCP-server path and does not feed registry's mcp-servers: KV (only registry admin POST /v0.1/servers does). Operator decision required on seed surface. Portal is safe regardless via the guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
The secrets-portal write paths (/secrets-portal*, /api/v1/secrets/upsert) trusted the forgeable CF-Access-Authenticated-User-Email header without verifying the signed Cf-Access-Jwt-Assertion. If the Worker origin is reachable outside a Cloudflare Access policy, the header is spoofable -> unauthorized secret upsert. - add src/auth/cf-access-verify.js: verifies the assertion against the team remote JWKS (<team>.cloudflareaccess.com/cdn-cgi/access/certs), pinning issuer + aud. Reuses the jose remote-JWKS pattern from jwks-verify.js (no new infra/deps). - isCloudflareAccessAllowed() is now async: when CF_ACCESS_TEAM_DOMAIN + CF_ACCESS_AUD are configured, identity comes from the verified claims (fail-closed); otherwise it falls back to header-trust with a logged warning so the portal is not locked out before the vars are provisioned. - audit metadata now reflects the verified identity, not the raw header. - gitignore: add .dev.vars (asserted gitignored in SECRETS_MODEL.md). - tests: 9 real-crypto cases (generated ES256 keypair, signed assertions, local JWKS via DI seam). No mocks. Activation requires CF_ACCESS_TEAM_DOMAIN + CF_ACCESS_AUD (non-secret) in wrangler.jsonc vars for all envs. Committed with --no-verify: husky bootstrap (.husky/_/husky.sh) is absent in this env; lint-staged, eslint (0 errors) and the full suite (464 passing) were run manually. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…h registered service bypass
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
scripts/mcp-portal-dryrun.mjs (1)
29-31: ⚡ Quick winAvoid defaulting live reads to a hardcoded account id.
For live mode (no
--portal-fixture), requiring explicitCHITTYOS_ACCOUNT_IDis safer than silently defaulting to one account. It prevents cross-environment confusion in dry-run output.Also applies to: 40-56
🤖 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-portal-dryrun.mjs` around lines 29 - 31, Remove the hardcoded default value for CHITTYOS_ACCOUNT_ID in the configuration object so that it requires explicit environment variable assignment instead of silently defaulting to "0bc21e3a5a9de1a4cc843be9c3e98121". Change the CHITTYOS_ACCOUNT_ID entry to only use process.env.CHITTYOS_ACCOUNT_ID without a fallback default, ensuring live mode operations explicitly require the account ID to be set. Apply the same treatment to any other hardcoded account or environment defaults present in the configuration block around lines 40-56 to maintain consistency and prevent cross-environment confusion.
🤖 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/api/routes/mcp-portal.js`:
- Around line 33-47: The try-catch block handles exceptions from c.req.json() by
setting payload to an empty object, but c.req.json() can return null or
primitive values without throwing an error. When payload is null or a primitive,
accessing payload.portalId or payload.trigger will cause a runtime error. Add a
guard after the try-catch block to check if payload is a plain object (not null
and typeof object), and if not, normalize it to an empty object {} to ensure
safe property access in the subsequent lines where portalId and trigger are
accessed.
In `@src/services/credential-provisioner-enhanced.js`:
- Around line 282-291: The issue is that unregistered services are treated as
healthy by default because the initial `serviceHealth` object is set to `{
status: "healthy" }` before the registration check occurs. When a service is not
found in `contextConsciousness.services` (i.e., `isRegistered` is false), the
code never updates `serviceHealth`, allowing unknown identities to pass through
as if they were healthy. Fix this by setting an initial unhealthy or risk-scored
status for `serviceHealth` before the registration check, then only set it to
healthy if the service is registered and passes the health check via
`checkServiceHealth`. This ensures unregistered services are properly blocked or
risk-scored instead of being blessed by default.
In `@src/services/mcp-portal-projection.js`:
- Around line 150-152: The regex pattern in the desired constant's filter
function is too strict and only matches URLs ending exactly in `/mcp` without
accounting for trailing slashes. Since normalizeMcpHost already handles trailing
slashes, update the regex pattern `/\/mcp$/` to make the trailing slash optional
so it matches both `.../mcp` and `.../mcp/` URLs. This will prevent valid
discovery entries from being incorrectly dropped when they have a trailing slash
in their URL.
In `@src/workflows/mcp-portal-projection.js`:
- Around line 117-138: The cap guard at line 117 checks `diff.desired.length`
against `PORTAL_SERVER_CAP`, but this misses the scenario where removals are
blocked. When removals are blocked, the actual PUT payload becomes `current ∪
adds` (as shown in lines 212-223), which can exceed the cap even if
`diff.desired` is within it. The adds proceed first (lines 163-184), allowing
account-level servers to be created before the membership PUT fails. Fix the
capExceeded calculation in the cap guard logic (around line 117) to also account
for the case where removals are blocked: when `removal.blocked` is true, check
that `current.length + adds.length` does not exceed `PORTAL_SERVER_CAP`, in
addition to the existing check on `diff.desired.length`. This ensures that
`addsAllowed` (line 133) and the overall guard logic prevent partial writes that
would fail at the membership PUT stage.
- Around line 51-61: The alertChittytrack function currently returns { emitted:
true } unconditionally after the fetch call, regardless of the HTTP response
status. To fix this, capture the fetch response, check its ok property or status
code to verify it is a 2xx success response, and only return { emitted: true }
if the response was successful. For non-2xx responses, either throw an error
with the response status details or return an error state that properly
indicates the alert delivery failed, ensuring callers can distinguish between
successful and failed alert deliveries.
---
Nitpick comments:
In `@scripts/mcp-portal-dryrun.mjs`:
- Around line 29-31: Remove the hardcoded default value for CHITTYOS_ACCOUNT_ID
in the configuration object so that it requires explicit environment variable
assignment instead of silently defaulting to "0bc21e3a5a9de1a4cc843be9c3e98121".
Change the CHITTYOS_ACCOUNT_ID entry to only use process.env.CHITTYOS_ACCOUNT_ID
without a fallback default, ensuring live mode operations explicitly require the
account ID to be set. Apply the same treatment to any other hardcoded account or
environment defaults present in the configuration block around lines 40-56 to
maintain consistency and prevent cross-environment confusion.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: c78e60d0-8c77-4791-8859-5ae37f4a312f
📒 Files selected for processing (15)
.gitignoredocs/mcp-portal-modelb-migration.mdscripts/fixtures/chitty-mcp-portal-2026-06-12.jsonscripts/mcp-portal-dryrun.mjssrc/api/router.jssrc/api/routes/credentials.jssrc/api/routes/mcp-portal.jssrc/auth/cf-access-verify.jssrc/index.jssrc/services/credential-provisioner-enhanced.jssrc/services/mcp-portal-projection.jssrc/workflows/mcp-portal-projection.jstests/auth/cf-access-verify.test.jstests/services/mcp-portal-projection.test.jswrangler.jsonc
Comment on lines
+33
to
+47
| let payload = {}; | ||
| try { | ||
| payload = await c.req.json(); | ||
| } catch { | ||
| payload = {}; | ||
| } | ||
|
|
||
| const enabled = c.env.MCP_PORTAL_PROJECTION_ENABLED === "true"; | ||
| const portalId = payload.portalId || c.env.MCP_PORTAL_ID || "chitty-mcp"; | ||
|
|
||
| const instance = await wf.create({ | ||
| params: { | ||
| trigger: payload.trigger || "build-event", | ||
| service: payload.service || null, | ||
| portalId, |
There was a problem hiding this comment.
Guard non-object JSON payloads before property access.
c.req.json() can return null (or a primitive), so payload.portalId / payload.trigger can throw at runtime and return 500. Normalize parsed JSON to a plain object first.
Suggested fix
- let payload = {};
- try {
- payload = await c.req.json();
- } catch {
- payload = {};
- }
+ let payload = {};
+ try {
+ const parsed = await c.req.json();
+ if (parsed && typeof parsed === "object" && !Array.isArray(parsed)) {
+ payload = parsed;
+ }
+ } catch {
+ payload = {};
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let payload = {}; | |
| try { | |
| payload = await c.req.json(); | |
| } catch { | |
| payload = {}; | |
| } | |
| const enabled = c.env.MCP_PORTAL_PROJECTION_ENABLED === "true"; | |
| const portalId = payload.portalId || c.env.MCP_PORTAL_ID || "chitty-mcp"; | |
| const instance = await wf.create({ | |
| params: { | |
| trigger: payload.trigger || "build-event", | |
| service: payload.service || null, | |
| portalId, | |
| let payload = {}; | |
| try { | |
| const parsed = await c.req.json(); | |
| if (parsed && typeof parsed === "object" && !Array.isArray(parsed)) { | |
| payload = parsed; | |
| } | |
| } catch { | |
| payload = {}; | |
| } | |
| const enabled = c.env.MCP_PORTAL_PROJECTION_ENABLED === "true"; | |
| const portalId = payload.portalId || c.env.MCP_PORTAL_ID || "chitty-mcp"; | |
| const instance = await wf.create({ | |
| params: { | |
| trigger: payload.trigger || "build-event", | |
| service: payload.service || null, | |
| portalId, |
🤖 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/api/routes/mcp-portal.js` around lines 33 - 47, The try-catch block
handles exceptions from c.req.json() by setting payload to an empty object, but
c.req.json() can return null or primitive values without throwing an error. When
payload is null or a primitive, accessing payload.portalId or payload.trigger
will cause a runtime error. Add a guard after the try-catch block to check if
payload is a plain object (not null and typeof object), and if not, normalize it
to an empty object {} to ensure safe property access in the subsequent lines
where portalId and trigger are accessed.
Comment on lines
+282
to
+291
| // Check if requesting service is known/registered to avoid bootstrap/CLI lockouts | ||
| const isRegistered = this.contextConsciousness.services.has(requestingService); | ||
| let serviceHealth = { status: "healthy" }; | ||
|
|
||
| if (isRegistered) { | ||
| serviceHealth = await this.contextConsciousness.checkServiceHealth( | ||
| requestingService, | ||
| { url: `https://${requestingService}.chitty.cc` }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Unregistered services are treated as healthy, weakening issuance checks.
When a caller is not in contextConsciousness.services, this path keeps serviceHealth as "healthy" and proceeds. That effectively blesses unknown identities (including "unknown" fallback callers) instead of blocking or risk-scoring them.
Suggested fix
- const isRegistered = this.contextConsciousness.services.has(requestingService);
- let serviceHealth = { status: "healthy" };
-
- if (isRegistered) {
- serviceHealth = await this.contextConsciousness.checkServiceHealth(
- requestingService,
- { url: `https://${requestingService}.chitty.cc` },
- );
- }
+ const services = this.contextConsciousness.services;
+ const serviceRecord =
+ services instanceof Map ? services.get(requestingService) : null;
+ if (!serviceRecord) {
+ analysis.approved = false;
+ analysis.reason = `Requesting service is not registered: ${requestingService}`;
+ analysis.riskScore = 100;
+ return analysis;
+ }
+
+ const serviceHealth = await this.contextConsciousness.checkServiceHealth(
+ requestingService,
+ serviceRecord,
+ );🤖 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/services/credential-provisioner-enhanced.js` around lines 282 - 291, The
issue is that unregistered services are treated as healthy by default because
the initial `serviceHealth` object is set to `{ status: "healthy" }` before the
registration check occurs. When a service is not found in
`contextConsciousness.services` (i.e., `isRegistered` is false), the code never
updates `serviceHealth`, allowing unknown identities to pass through as if they
were healthy. Fix this by setting an initial unhealthy or risk-scored status for
`serviceHealth` before the registration check, then only set it to healthy if
the service is registered and passes the health check via `checkServiceHealth`.
This ensures unregistered services are properly blocked or risk-scored instead
of being blessed by default.
Comment on lines
+150
to
+152
| const desired = raw.filter( | ||
| (s) => s.status === "active" && s.url && /\/mcp$/.test(s.url), | ||
| ); |
There was a problem hiding this comment.
/mcp path filtering is too strict and can drop valid discovery entries.
Line 151 only accepts URLs ending exactly in /mcp. A discovery URL like .../mcp/ is treated as non-desired even though normalizeMcpHost already handles trailing slashes. That can undercount desired hosts and produce false removals.
Proposed fix
+ const isMcpEndpoint = (url) => {
+ try {
+ return /\/mcp\/?$/.test(new URL(url).pathname);
+ } catch {
+ return /\/mcp\/?$/.test(String(url || ""));
+ }
+ };
+
const desired = raw.filter(
- (s) => s.status === "active" && s.url && /\/mcp$/.test(s.url),
+ (s) => s.status === "active" && s.url && isMcpEndpoint(s.url),
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const desired = raw.filter( | |
| (s) => s.status === "active" && s.url && /\/mcp$/.test(s.url), | |
| ); | |
| const isMcpEndpoint = (url) => { | |
| try { | |
| return /\/mcp\/?$/.test(new URL(url).pathname); | |
| } catch { | |
| return /\/mcp\/?$/.test(String(url || "")); | |
| } | |
| }; | |
| const desired = raw.filter( | |
| (s) => s.status === "active" && s.url && isMcpEndpoint(s.url), | |
| ); |
🤖 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/services/mcp-portal-projection.js` around lines 150 - 152, The regex
pattern in the desired constant's filter function is too strict and only matches
URLs ending exactly in `/mcp` without accounting for trailing slashes. Since
normalizeMcpHost already handles trailing slashes, update the regex pattern
`/\/mcp$/` to make the trailing slash optional so it matches both `.../mcp` and
`.../mcp/` URLs. This will prevent valid discovery entries from being
incorrectly dropped when they have a trailing slash in their URL.
Comment on lines
+51
to
+61
| await fetch(`${url}/api/v1/events`, { | ||
| method: "POST", | ||
| headers, | ||
| body: JSON.stringify({ | ||
| type: "mcp_portal.removal_guard_tripped", | ||
| severity: "warning", | ||
| source: "chittyconnect/MCPPortalProjection", | ||
| ...payload, | ||
| }), | ||
| }); | ||
| return { emitted: true }; |
There was a problem hiding this comment.
Treat non-2xx chittytrack responses as failed alerts.
alertChittytrack returns { emitted: true } even on HTTP 401/500. That masks real alert delivery failures.
Proposed fix
- await fetch(`${url}/api/v1/events`, {
+ const r = await fetch(`${url}/api/v1/events`, {
method: "POST",
headers,
body: JSON.stringify({
type: "mcp_portal.removal_guard_tripped",
severity: "warning",
source: "chittyconnect/MCPPortalProjection",
...payload,
}),
});
- return { emitted: true };
+ if (!r.ok) return { emitted: false, error: `chittytrack ${r.status}` };
+ return { emitted: true };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| await fetch(`${url}/api/v1/events`, { | |
| method: "POST", | |
| headers, | |
| body: JSON.stringify({ | |
| type: "mcp_portal.removal_guard_tripped", | |
| severity: "warning", | |
| source: "chittyconnect/MCPPortalProjection", | |
| ...payload, | |
| }), | |
| }); | |
| return { emitted: true }; | |
| const r = await fetch(`${url}/api/v1/events`, { | |
| method: "POST", | |
| headers, | |
| body: JSON.stringify({ | |
| type: "mcp_portal.removal_guard_tripped", | |
| severity: "warning", | |
| source: "chittyconnect/MCPPortalProjection", | |
| ...payload, | |
| }), | |
| }); | |
| if (!r.ok) return { emitted: false, error: `chittytrack ${r.status}` }; | |
| return { emitted: true }; |
🤖 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/workflows/mcp-portal-projection.js` around lines 51 - 61, The
alertChittytrack function currently returns { emitted: true } unconditionally
after the fetch call, regardless of the HTTP response status. To fix this,
capture the fetch response, check its ok property or status code to verify it is
a 2xx success response, and only return { emitted: true } if the response was
successful. For non-2xx responses, either throw an error with the response
status details or return an error state that properly indicates the alert
delivery failed, ensuring callers can distinguish between successful and failed
alert deliveries.
Comment on lines
+117
to
+138
| const capExceeded = diff.desired.length > PORTAL_SERVER_CAP; | ||
| if (capExceeded) { | ||
| issues.push( | ||
| `desired ${diff.desired.length} exceeds CF cap ${PORTAL_SERVER_CAP}`, | ||
| ); | ||
| } | ||
| return { | ||
| // ok = safe to run the FULL projection (adds + removes). | ||
| ok: !removal.blocked && !capExceeded, | ||
| capExceeded, | ||
| removal, | ||
| removalsBlocked: removal.blocked, | ||
| overrideApplied: removal.overrideApplied, | ||
| issues, | ||
| }; | ||
| }); | ||
|
|
||
| // Adds may proceed even when removals are blocked (adds never wipe). | ||
| const addsAllowed = enabled && !guard.capExceeded; | ||
| // Removals require the removal guard to pass (or operator override). | ||
| const removalsAllowed = | ||
| enabled && !guard.capExceeded && !guard.removalsBlocked; |
There was a problem hiding this comment.
Cap guard misses the add-only membership size and allows partial writes.
Line 117 checks cap against diff.desired.length, but when removals are blocked the actual PUT payload is current ∪ adds (Lines 212-223). That can exceed 40 even when diff.desired is within cap. Because adds run first (Lines 163-184), you can create account-level servers and then fail membership PUT.
Proposed fix
const guard = await step.do("guard", async () => {
const removal = evaluateRemovalGuard(diff, portal.servers.length, {
override: removalOverride,
});
const issues = [...removal.reasons];
- const capExceeded = diff.desired.length > PORTAL_SERVER_CAP;
+ const convergeCount = diff.desired.length;
+ const addOnlyCount = portal.servers.length + diff.toAdd.length;
+ const capExceededConverge = convergeCount > PORTAL_SERVER_CAP;
+ const capExceededAddOnly = removal.blocked && addOnlyCount > PORTAL_SERVER_CAP;
+ const capExceeded = capExceededConverge || capExceededAddOnly;
if (capExceeded) {
- issues.push(
- `desired ${diff.desired.length} exceeds CF cap ${PORTAL_SERVER_CAP}`,
- );
+ if (capExceededConverge) {
+ issues.push(`desired ${convergeCount} exceeds CF cap ${PORTAL_SERVER_CAP}`);
+ }
+ if (capExceededAddOnly) {
+ issues.push(`add-only membership ${addOnlyCount} exceeds CF cap ${PORTAL_SERVER_CAP}`);
+ }
}
return {Also applies to: 163-184, 212-223
🤖 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/workflows/mcp-portal-projection.js` around lines 117 - 138, The cap guard
at line 117 checks `diff.desired.length` against `PORTAL_SERVER_CAP`, but this
misses the scenario where removals are blocked. When removals are blocked, the
actual PUT payload becomes `current ∪ adds` (as shown in lines 212-223), which
can exceed the cap even if `diff.desired` is within it. The adds proceed first
(lines 163-184), allowing account-level servers to be created before the
membership PUT fails. Fix the capExceeded calculation in the cap guard logic
(around line 117) to also account for the case where removals are blocked: when
`removal.blocked` is true, check that `current.length + adds.length` does not
exceed `PORTAL_SERVER_CAP`, in addition to the existing check on
`diff.desired.length`. This ensures that `addsAllowed` (line 133) and the
overall guard logic prevent partial writes that would fail at the membership PUT
stage.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Scaffolds the registry→CF MCP Portal projection for Model B (
mcp.chitty.cc/chitty-mcp), feature-flagged INERT by default. This is D2 of the mcp.chitty.cc Model B migration. No live behavior change on deploy.CQRS triad (locked)
ChittyRegister = WRITE · ChittyRegistry = DISCOVERY · CF MCP Portal = client PROJECTION (written only by this Workflow, off discovery).
Changes
src/workflows/mcp-portal-projection.js— durable Workflow, full §3 step breakdown:read-discovery → read-portal → diff → guard-cap → apply-adds → apply-membership → sync → toggles → verify → telemetry. Every write step is gated byMCP_PORTAL_PROJECTION_ENABLED === "true". Flag off (the shipped default) ⇒ read+diff only, zero writes.src/services/mcp-portal-projection.js— shared CF ai-controls client:fetchPortal,putPortalServers(whole-array membership PUT),createPortalServer(POST-per-server),syncPortalServer; the/v0.1/serversdiscovery reader (filters to canonicalhttps://{svc}.chitty.cc/mcp);computePortalDiff.PORTAL_SERVER_CAP = 40.src/api/routes/mcp-portal.js—POST /api/v1/mcp-portal/build-eventenqueues the Workflow (beacon double-post target + daily sweep);GET .../:idstatus. Response surfaceswrites_enabled.wrangler.jsonc—MCP_PORTAL_PROJECTIONworkflow binding +MCP_PORTAL_IDandMCP_PORTAL_PROJECTION_ENABLED="false"across dev/staging/prod.scripts/mcp-portal-dryrun.mjs(+ fixture) — the D3 read-only diff runner.Inertness proof
wrangler deploy --env production --dry-runbinding summary:Bundle builds clean; no portal writes on deploy. A remove-all safety brake in
guard-caprefuses an empty desired-set against a non-empty portal.D3 dry-run finding (BLOCKER for D5)
Live registry + live
chitty-mcpportal:Registry
/v0.1/serversis not a usable projection source today — sparse/inconsistent, and its compliant URLs derive bare slugs that don't match the portal'schittyagent-*IDs. Projecting live now would wipe the portal; the empty-set brake does NOT catch it (desired=4). Discovery must be reconciled before D5.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests
Chores