trunk-merge/pr-110638/5f4de5de-d4c7-40d5-a69d-f54c0fb69415-bisection

[trunk-io]

This pull request was created and is being managed by Trunk Merge.

This pull request is based on the master branch at SHA 1830cfd7be9848bd447c33b37e218156da32d806.

See more details here.

When CI completes, this pull request will be closed automatically.

Pull Requests Being Tested

This pull request is testing the changes from pull request 110638.

Dependencies

This pull request depends on the changes from pull request 110199.

Batch Bisection

This pull request is in a batch bisection. Pull requests successfully tested by this PR will re-enter the main queue.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

🧰 Additional context used

📚 Code guidelines (18)

.cursor/rules/react-typescript.mdc — auto-discovered
.agents/skills/using-kea-disposables/SKILL.md — configured
.agents/skills/writing-ui-components/SKILL.md — configured
.agents/skills/authoring-ci-workflows/SKILL.md — configured
.agents/skills/gating-production-deploys/SKILL.md — configured
.agents/security.md — configured
services/mcp/AGENTS.md — auto-discovered
docs/published/handbook/engineering/type-system.md — configured
.agents/skills/implementing-mcp-tools/SKILL.md — configured
.agents/skills/writing-tests/SKILL.md — configured
.agents/skills/adopting-generated-api-types/SKILL.md — configured
.agents/skills/placing-product-frontend-code/SKILL.md — configured
.agents/skills/writing-kea-logics/SKILL.md — configured
.agents/skills/writing-user-facing-copy/SKILL.md — configured
.agents/skills/setting-feature-flags-in-storybook/SKILL.md — configured
.claude/commands/conventions.md — configured
.agents/skills/implementing-mcp-ui-apps/SKILL.md — configured
… and 1 more

📝 Walkthrough

Walkthrough

The data modeling lineage graph now supports optional node dragging, saved positions, reset controls, external links, and focus fitting. The data modeling tab connects these interactions to feature-flagged state and move analytics. MCP changes add local resource archive configuration and integration coverage, plus a dedicated live-test configuration and scheduled GitHub Actions workflow.

Priority: ➖ Normal

Merge Risk: 🔵 Low · up to 3422b

Restore the archive URL after harness use and add the ready-for-review trigger before merging. The remaining test assertion would strengthen protection for local archive loading.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 3422b

The main risk is configuration isolation: an alternate archive can populate resource storage consumed by other MCP instances if they share a Redis database. Ordinary requests do not control the archive URL. Lineage movement remains client-side, and Slack credentials are used only for scheduled failure notifications.

Retained concerns

• Medium · security · inferred: The newly effective archive override is not isolated from the shared MCP resource cache. If instances using different archive sources share a Redis database, an override-enabled writer can publish a manifest and overwrite URI-addressed bodies consumed by instances configured for the default source. Changing or removing the override does not itself invalidate those entries. The cache mechanism predates this PR, but environment-based activation makes mixed-source operation reachable through the normal Hono configuration path. Production sharing and use of the override are not established.

Security review details

Security Blast Radius

• inferred — The archive override's potential propagation scope is all MCP consumers using the same Redis database and ContextMill keys, not merely the configuring process. Reaching this outcome requires authority over process configuration or control of the selected archive producer. No inspected request or tenant input selects the archive URL, and no specific production environment or tenant exposure is established.

Security Findings and Attack Paths

• inferred — A configured alternate archive can supply text for an existing resource URI and publish it under the same body key used by default-source instances. Other instances can then return that text to downstream MCP clients. This is a conditional content-integrity path, not evidence of unauthenticated URL control, automatic agent execution, or a deployed compromise.

Trust Boundaries and Controls

• observed — The archive path requires manifest.json, passes its contents through the manifest loader, and filters entries whose referenced files are missing. Resource reads require membership in the loaded URI map. These controls constrain archive shape and resource lookup but do not bind cache entries to their archive source; the loader directly fetches the configured URL.
• observed — The canary grants contents-read permission and uses pinned checkout and Slack actions. The Slack token is referenced only in a notification step gated to scheduled live-test failure, rather than in the pull-request test command.

Resilience and Maintainability Implications

• observed — The existing cache coordinates writers with expiring, token-owned locks and serves stale manifests during refresh. Resource bodies are overwritten before manifest publication; partial failure can therefore leave updated bodies with an older manifest. The timeout fallback also calls the body-writing load path. These pre-existing behaviors coordinate availability but do not enforce source isolation or atomic body-and-manifest publication.

Hardening Proposals

• proposed — Bind manifest, lock, and body namespaces to the archive source, or restrict alternate archives to explicitly isolated development/test storage. Make source changes and rollback discard or segregate prior-source content rather than relying solely on eventual refresh and expiry.

🚥 Pre-merge checks | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description only documents Trunk Merge batch-bisection metadata. It does not explain the MCP and data-modeling changes, user impact, testing, release status, or agent context required by the repos…	Replace or supplement the Trunk Merge text with a complete description using the required sections: Problem, Changes, How did you test this code?, Test rationale, Release status, Automatic notifications, Docs update, and Agent context when …

Full details: Description check

Explanation

The description only documents Trunk Merge batch-bisection metadata. It does not explain the MCP and data-modeling changes, user impact, testing, release status, or agent context required by the repository template.

Resolution

Replace or supplement the Trunk Merge text with a complete description using the required sections: Problem, Changes, How did you test this code?, Test rationale, Release status, Automatic notifications, Docs update, and Agent context when applicable. Include frontend screenshots and testing evidence where required.

• Fix all pre-merge checks with AI

✨ Finishing Touches

📝 Generate docstrings

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.

Usage-based review receipt

• Mode: Continue automatically
• Reviewed files: 19
• Waived: $4.75 (charged $0.00)
• View usage details

Note

This review exceeded your plan’s limits and used usage-based reviews—free during trial. After your trial, your Enterprise plan’s existing billing terms apply. Manage usage-based reviews.

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Note

Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.

🟡 Other comments (2)

.github/workflows/ci-mcp-live-canary.yml-15-16 (1)

15-16: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Include ready_for_review in the PR trigger.

The default pull_request event types omit ready_for_review. If a draft skips CI, marking it ready will not dispatch this canary until another listed event occurs. Declare opened, synchronize, reopened, and ready_for_review while keeping the path filter.

As per coding guidelines, “Still add ready_for_review to the pull_request types.”

Source: Coding guidelines

services/mcp/tests/integration/harness/hono.ts-67-67 (1)

67-67: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Restore POSTHOG_MCP_LOCAL_SKILLS_URL during cleanup.

stop() closes contextMillArchive but leaves the environment variable set to its stopped URL. A later McpDispatcher can construct a ResourceCatalog with that URL and fail to load ContextMill resources. Save the previous value and restore it in stop(), including when startup fails after the assignment.

Suggested fix

 export async function startHonoHarness(env: IntegrationEnv): Promise<IntegrationHarness> {
     process.env.POSTHOG_API_BASE_URL = env.apiBaseUrl
+    const previousLocalSkillsUrl = process.env.POSTHOG_MCP_LOCAL_SKILLS_URL

     // ResourceCatalog snapshots this URL at construction.
     ...
     const stop = async (): Promise<void> => {
         await new Promise<void>((resolve) => server.close(() => resolve()))
         await skillArchive?.stop().catch(() => undefined)
         await contextMillArchive?.stop().catch(() => undefined)
+        if (previousLocalSkillsUrl === undefined) {
+            delete process.env.POSTHOG_MCP_LOCAL_SKILLS_URL
+        } else {
+            process.env.POSTHOG_MCP_LOCAL_SKILLS_URL = previousLocalSkillsUrl
+        }
         await redis?.quit().catch(() => undefined)
     }

🧹 Nitpick comments (1)

services/mcp/tests/unit/context-mill-archive.test.ts (1)

48-50: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Assert that extraction fetches ARCHIVE_URL.

The fetch stub returns the same archive for every URL. This test can pass if the loader fetches CONTEXT_MILL_URL instead of ARCHIVE_URL. Assert the requested URL after the call.

Suggested test assertion

         const entries = await fetchAndExtractEntries(ARCHIVE_URL)

+        expect(vi.mocked(fetch).mock.calls[0]?.[0]).toBe(ARCHIVE_URL)
         expect(entries.map((entry) => entry.id)).toEqual(['bundled', 'inline'])

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository: PostHog/posthog/.coderabbit.yaml

Review profile: QUIET

Plan: Enterprise

Run ID: 73d8a81f-2a82-4cda-a298-20a1b977b063

📥 Commits

Reviewing files that changed from the base of the PR and between 1830cfd and 3422ba8.

📒 Files selected for processing (20)

• .github/workflows/ci-mcp-live-canary.yml
• frontend/snapshots.yml
• frontend/src/lib/constants.tsx
• products/data_modeling/frontend/lineage/LineageGraph.stories.tsx
• products/data_modeling/frontend/lineage/LineageGraph.tsx
• products/data_modeling/frontend/lineage/LineageNode.tsx
• products/data_modeling/frontend/lineage/ModelsLineageTab.tsx
• products/data_modeling/frontend/lineage/modelsLineageLogic.test.ts
• products/data_modeling/frontend/lineage/modelsLineageLogic.ts
• services/mcp/package.json
• services/mcp/src/hono/constants.ts
• services/mcp/src/hono/resource-catalog.ts
• services/mcp/src/tools/types.ts
• services/mcp/tests/hono/integration-harness.test.ts
• services/mcp/tests/integration/harness/hono.ts
• services/mcp/tests/integration/harness/skill-archive.ts
• services/mcp/tests/live/context-mill-manifest.live.test.ts
• services/mcp/tests/unit/context-mill-archive.test.ts
• services/mcp/vitest.config.mts
• services/mcp/vitest.live.config.mts

Limit details: You’ve used all 12 included reviews currently available.

[trunk-io]

   

View Full Report ↗︎ ⋅ Docs