Repository navigation
feat(mcp): derive scopes and annotations, add tools.yaml hooks - #111811
trunk-io[bot] merged 6 commits into
Conversation
Hand-typed scopes drift from what the API enforces. A token can see a tool and then get a 403 on the call. The spec already carries the required scopes for every operation, so use them when the YAML has no `scopes`. Explicit YAML still wins, which keeps intentional gating and the endpoints whose scopes are computed per request (no spec scopes, so codegen fails and asks for them by hand). Annotations follow from the HTTP method for GET, DELETE and PATCH, so those get defaults. POST and PUT vary too much (search vs create vs upsert) and still need explicit annotations. A YAML scope list that misses a scope the spec requires now warns, with a GitHub annotation on CI. Warn only for now, since a handful of tools trip it today and fixing them belongs to the owning teams. Existing YAML and generated output are unchanged. The feature flags eval test read annotations from YAML, so it reads the generated definitions instead.
Customizing a generated tool meant writing a hand-written tool with the same name, which silently wins over the generated one. Two such shadows existed and a third is in flight. Each one copies the generated tool's shape and has to be kept in sync by hand. A per-tool `hooks:` key now points at a module that exports beforeRequest, afterResponse and onError. Codegen wraps the generated handler with them, so name, schema and metadata stay generated. Both shadows (update-feature-flag, session-recording-get) moved to hook modules with the same logic and tests. The shadow list in the name validation test is empty now. hooks cannot be combined with confirmed_action yet, since the prepare and execute handlers are built on a separate path. mergeToolFactories stays because hand-written tools can still collide by design.
|
FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review 🦔 PostHog Review is reviewing this pull requestStep 4/6 · Merging overlapping findings · 4/4 Specialist review skills read the changed code in parallel each from their own perspective, a blind-spot sweep catches what they missed, and only validated findings are published back to this pull request. This comment updates as the review progresses. |
🤖 CI report
|
| First copy | Second copy | Lines | Tokens |
|---|---|---|---|
products/warehouse_sources/backend/temporal/data_imports/sources/devin_ai/devin_ai.py:2 |
products/warehouse_sources/backend/temporal/data_imports/sources/humanitix/humanitix.py:1 |
19 | 125 |
products/warehouse_sources/backend/temporal/data_imports/sources/dbt/dbt.py:5 |
products/warehouse_sources/backend/temporal/data_imports/sources/instana/instana.py:6 |
13 | 112 |
products/warehouse_sources/backend/temporal/data_imports/sources/harvey/harvey.py:4 |
products/warehouse_sources/backend/temporal/data_imports/sources/hyperspell/hyperspell.py:2 |
13 | 105 |
products/warehouse_sources/backend/temporal/data_imports/sources/aha/aha.py:227 |
products/warehouse_sources/backend/temporal/data_imports/sources/humanitix/humanitix.py:144 |
15 | 104 |
products/error_tracking/backend/presentation/views/assignment_rules.py:3 |
products/error_tracking/backend/presentation/views/suppression_rules.py:1 |
21 | 103 |
products/error_tracking/backend/presentation/views/assignment_rules.py:3 |
products/error_tracking/backend/presentation/views/bypass_rules.py:1 |
18 | 102 |
products/error_tracking/backend/presentation/views/grouping_rules.py:4 |
products/error_tracking/backend/presentation/views/severity_rules.py:1 |
16 | 96 |
products/error_tracking/backend/presentation/views/assignment_rules.py:6 |
products/error_tracking/backend/presentation/views/grouping_rules.py:7 |
19 | 94 |
products/warehouse_sources/backend/temporal/data_imports/sources/humanitix/humanitix.py:121 |
products/warehouse_sources/backend/temporal/data_imports/sources/thinkific_courses/thinkific_courses.py:98 |
11 | 84 |
products/approvals/backend/actions/feature_flags.py:193 |
products/approvals/backend/actions/feature_flags.py:434 |
14 | 75 |
products/warehouse_sources/backend/temporal/data_imports/sources/aha/aha.py:146 |
products/warehouse_sources/backend/temporal/data_imports/sources/humanitix/humanitix.py:121 |
15 | 74 |
✅ Duplication (TypeScript) — clean
New TypeScript code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.
🚨 Comment density — 9% of added code lines are comments (47 of 497)
This section warns when comments are more than 3% of the code lines a PR adds, and alerts above 6%. Before agent-assisted PRs, the typical share was about 2%. Only full-line comments count. Docstrings, generated files, snapshots, migrations, and workflow files are left out.
Comments that restate the code, record how the change came about, or narrate the next line add noise for the next reader. Keep the comments that explain a reason the code cannot show, and remove the rest. See .agents/skills/writing-code-comments/SKILL.md for the house rules.
Files with the most added comment lines:
| File | Comment lines | Added lines |
|---|---|---|
services/mcp/src/tools/tool-hooks.ts |
16 | 40 |
services/mcp/scripts/generate-tools.ts |
12 | 129 |
services/mcp/scripts/yaml-config-schema.ts |
7 | 16 |
services/mcp/src/tools/featureFlags/updateFeatureFlagHooks.ts |
7 | 30 |
products/posthog_ai/eval_harness/test/test_feature_flags_scorers.py |
2 | 8 |
services/mcp/tests/unit/tool-name-validation.test.ts |
2 | 4 |
services/mcp/src/tools/replay/sessionRecordingGetHooks.ts |
1 | 29 |
This check does not block merging. It updates on every push and clears when the share drops.
✅ Bundle size — no change
Uncompressed size of every built .js bundle, compared against the base branch.
Total: 69.49 MiB · no change
No file changed by more than 1000 B.
Posted automatically by build-bundle-size-report · uncompressed bytes from dist-report
✅ Eager graph — within budget
How much code each root ships on the eager path — downloaded and parsed before the surface is interactive. Measured from the esbuild output chunks (post-tree-shake, static imports only); lazy import() / React.lazy chunks are not counted.
| Root | Eager (shipped) | Δ vs base | Budget |
|---|---|---|---|
entry (logged-out pages, app bootstrap)src/index.tsx |
1.63 MiB · 23 files | no change | █████████░ 88.6% of 1.84 MiB |
logged-out boot: index + App + bootApp (preloaded by every page, including /login)src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts |
3.72 MiB · 663 files | no change | █████████░ 92.2% of 4.03 MiB |
authenticated shell (every logged-in page)src/scenes/AuthenticatedShell.tsx |
7.61 MiB · 2,415 files | no change | █████████░ 91.3% of 8.34 MiB |
dashboard scenesrc/scenes/dashboard/Dashboard.tsx |
9.68 MiB · 3,403 files | no change | ███████░░░ 71.8% of 13.48 MiB |
today home pathsrc/scenes/AuthenticatedShell.tsx + src/scenes/project-homepage/ProjectHomepage.tsx + src/scenes/project-homepage/today/TodayHome.tsx |
7.62 MiB · 2,423 files | no change | █████████░ 88.8% of 8.58 MiB |
events scenesrc/scenes/activity/explore/EventsScene.tsx |
9.29 MiB · 3,255 files | no change | ███████░░░ 73.6% of 12.64 MiB |
replay detail scenesrc/scenes/session-recordings/detail/SessionRecordingDetail.tsx |
12.14 MiB · 4,145 files | 🔺 +376 B (+0.0%) | ████████░░ 77.2% of 15.72 MiB |
🟢 node_modules/monaco-editor/ stays out of src/index.tsx
🟢 src/lib/components/ActivityLog/describers stays out of src/index.tsx
🟢 [object Object] stays out of src/index.tsx
🟢 [object Object] stays out of src/index.tsx
🟢 node_modules/monaco-editor/ stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/layout/navigation-3000/navigationLogic.tsx stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/scenes/dashboard/dashboardLogic.tsx stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/lib/lemon-ui/LemonMarkdown/ stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/lib/components/RichContentEditor/ stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/lib/components/CodeSnippet/ stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/taxonomy/core-filter-definitions-by-group.json stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 node_modules/monaco-editor/ stays out of src/scenes/AuthenticatedShell.tsx
🟢 src/lib/components/ActivityLog/describers stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
🟢 src/scenes/session-recordings/player/sessionRecordingPlayerLogic.ts stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
🟢 src/scenes/project-homepage/ai-first/AiFirstHomepage.tsx stays out of src/scenes/AuthenticatedShell.tsx + src/scenes/project-homepage/ProjectHomepage.tsx + src/scenes/project-homepage/today/TodayHome.tsx
🟢 src/scenes/project-homepage/today/TodayReportPage.tsx stays out of src/scenes/AuthenticatedShell.tsx + src/scenes/project-homepage/ProjectHomepage.tsx + src/scenes/project-homepage/today/TodayHome.tsx
Largest files eagerly shipped from src/index.tsx
| Size | File |
|---|---|
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 24.6 KiB | ../node_modules/.pnpm/buffer@6.0.3/node_modules/buffer/index.js |
| 6.3 KiB | ../node_modules/.pnpm/react@18.3.1/node_modules/react/cjs/react.production.min.js |
| 4.5 KiB | ../node_modules/.pnpm/@jspm+core@2.1.0/node_modules/@jspm/core/nodelibs/browser/process.js |
| 3.9 KiB | ../node_modules/.pnpm/scheduler@0.23.2/node_modules/scheduler/cjs/scheduler.production.min.js |
| 1.4 KiB | ../node_modules/.pnpm/base64-js@1.5.1/node_modules/base64-js/index.js |
| 1.3 KiB | src/index.tsx |
| 1.3 KiB | src/RootErrorBoundary.tsx |
| 912 B | ../node_modules/.pnpm/ieee754@1.2.1/node_modules/ieee754/index.js |
| 839 B | src/scenes/ChunkLoadErrorBoundary.tsx |
Largest files eagerly shipped from src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
| Size | File |
|---|---|
| 306.2 KiB | ../node_modules/.pnpm/posthog-js@1.435.5_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/module.mjs |
| 220.3 KiB | ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 99.7 KiB | src/lib/api.ts |
| 90.2 KiB | src/products.tsx |
| 69.4 KiB | src/lib/lemon-ui/icons/icons.tsx |
| 40.4 KiB | src/lib/utils/eventUsageLogic.ts |
| 38.7 KiB | ../node_modules/.pnpm/@dnd-kit+core@6.0.8_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@dnd-kit/core/dist/core.esm.js |
| 33.9 KiB | ../node_modules/.pnpm/kea@4.0.0-pre.6_patch_hash=139b8d1f1304f9d9da452a9a1244c94ea679dbcb85687d8999563146879fb6f5_react@18.3.1/node_modules/kea/lib/index.cjs.js |
| 29.0 KiB | ../node_modules/.pnpm/zod@4.3.6/node_modules/zod/v4/core/schemas.js |
Largest files eagerly shipped from src/scenes/AuthenticatedShell.tsx
| Size | File |
|---|---|
| 306.2 KiB | ../node_modules/.pnpm/posthog-js@1.435.5_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/module.mjs |
| 280.3 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 220.3 KiB | ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 153.7 KiB | ../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js |
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 112.1 KiB | ../packages/quill/packages/quill/dist/index.js |
| 99.7 KiB | src/lib/api.ts |
| 93.3 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
| 90.6 KiB | ../node_modules/.pnpm/@tiptap+core@3.20.6_@tiptap+pm@3.20.6/node_modules/@tiptap/core/dist/index.js |
| 90.2 KiB | src/products.tsx |
Largest files eagerly shipped from src/scenes/dashboard/Dashboard.tsx
| Size | File |
|---|---|
| 306.2 KiB | ../node_modules/.pnpm/posthog-js@1.435.5_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/module.mjs |
| 280.3 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 220.3 KiB | ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 181.8 KiB | src/queries/validators.js |
| 153.7 KiB | ../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js |
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 112.1 KiB | ../packages/quill/packages/quill/dist/index.js |
| 99.7 KiB | src/lib/api.ts |
| 93.3 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
| 90.6 KiB | ../node_modules/.pnpm/@tiptap+core@3.20.6_@tiptap+pm@3.20.6/node_modules/@tiptap/core/dist/index.js |
Largest files eagerly shipped from src/scenes/AuthenticatedShell.tsx + src/scenes/project-homepage/ProjectHomepage.tsx + src/scenes/project-homepage/today/TodayHome.tsx
| Size | File |
|---|---|
| 306.2 KiB | ../node_modules/.pnpm/posthog-js@1.435.5_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/module.mjs |
| 280.3 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 220.3 KiB | ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 153.7 KiB | ../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js |
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 112.1 KiB | ../packages/quill/packages/quill/dist/index.js |
| 99.7 KiB | src/lib/api.ts |
| 93.3 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
| 90.6 KiB | ../node_modules/.pnpm/@tiptap+core@3.20.6_@tiptap+pm@3.20.6/node_modules/@tiptap/core/dist/index.js |
| 90.2 KiB | src/products.tsx |
Largest files eagerly shipped from src/scenes/activity/explore/EventsScene.tsx
| Size | File |
|---|---|
| 306.2 KiB | ../node_modules/.pnpm/posthog-js@1.435.5_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/module.mjs |
| 280.3 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 220.3 KiB | ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 181.8 KiB | src/queries/validators.js |
| 153.7 KiB | ../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js |
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 112.1 KiB | ../packages/quill/packages/quill/dist/index.js |
| 99.7 KiB | src/lib/api.ts |
| 93.3 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
| 90.6 KiB | ../node_modules/.pnpm/@tiptap+core@3.20.6_@tiptap+pm@3.20.6/node_modules/@tiptap/core/dist/index.js |
Largest files eagerly shipped from src/scenes/session-recordings/detail/SessionRecordingDetail.tsx
| Size | File |
|---|---|
| 315.5 KiB | ../node_modules/.pnpm/posthog-js@1.435.5_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/rrweb.js |
| 306.2 KiB | ../node_modules/.pnpm/posthog-js@1.435.5_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/module.mjs |
| 280.3 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 220.3 KiB | ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 181.8 KiB | src/queries/validators.js |
| 153.7 KiB | ../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js |
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 112.1 KiB | ../packages/quill/packages/quill/dist/index.js |
| 99.7 KiB | src/lib/api.ts |
| 93.3 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
Posted automatically by check-eager-graph · sizes are eager output bytes (shipped, post-tree-shake) from the esbuild metafile · part of #32479
✅ Toolbar bundle — eager 2.21 MiB within budget
What the toolbar ships to customer pages, measured from the esbuild output (minified, post-tree-shake). The eager set is the entry plus everything statically imported from it — fetched before any feature runs; deferred chunks load lazily. The eager guardrail is 5.72 MiB. Each output file must also stay below 10 MB, where CloudFront stops compressing it. The module boundary is enforced separately by check-toolbar-graph.
| Metric | Size | Δ vs base | Budget |
|---|---|---|---|
| Eager (shipped) entry + static imports |
2.21 MiB · 19 files | no change | ████░░░░░░ 38.6% of 5.72 MiB |
| Deferred (lazy) | 2.11 MiB · 44 files | no change | n/a — loads on demand |
Loader dist/toolbar.js |
1.2 KiB | no change | █░░░░░░░░░ 6.0% of 19.5 KiB |
Largest eagerly-shipped chunks
| Size | File |
|---|---|
| 843.7 KiB | dist/toolbar/toolbar-app-5GD44WZJ.css |
| 657.3 KiB | dist/toolbar/chunk-chunk-S77RT4IT.js |
| 259.4 KiB | dist/toolbar/chunk-chunk-6KW5GTW2.js |
| 138.2 KiB | dist/toolbar/chunk-chunk-Z2KREENH.js |
| 131.8 KiB | dist/toolbar/chunk-chunk-FDH2IBXT.js |
| 75.2 KiB | dist/toolbar/toolbar-app-CT5ZXTZ5.js |
| 69.0 KiB | dist/toolbar/chunk-chunk-TSAL54PB.js |
| 35.6 KiB | dist/toolbar/chunk-chunk-OVFRMLST.js |
| 21.0 KiB | dist/toolbar/chunk-chunk-IGB2FOYE.js |
| 6.8 KiB | dist/toolbar/chunk-chunk-DV7IWQNF.js |
Posted automatically by check-toolbar-size · sizes are toolbar output bytes (shipped, post-tree-shake) from the esbuild metafile
✅ Dist folder size — 🔺 +5.0 KiB (+0.0%)
Total size of the built frontend/dist folder (all assets), compared against the base branch.
Total: 960.50 MiB · 🔺 +5.0 KiB (+0.0%)
ℹ️ MCP UI apps size — 32 app(s), 17180.5 KB JS
Built size of each MCP UI app (main.js + styles.css).
| App | JS | CSS |
|---|---|---|
| debug | 597.9 KB | 199.4 KB |
| action | 454.1 KB | 199.4 KB |
| action-list | 564.2 KB | 199.4 KB |
| cohort | 453.1 KB | 199.4 KB |
| cohort-list | 563.2 KB | 199.4 KB |
| email-template | 452.9 KB | 199.4 KB |
| error-details | 469.6 KB | 199.4 KB |
| error-issue | 454.5 KB | 199.4 KB |
| error-issue-list | 564.8 KB | 199.4 KB |
| experiment | 561.3 KB | 199.4 KB |
| experiment-list | 564.9 KB | 199.4 KB |
| experiment-results | 566.3 KB | 199.4 KB |
| feature-flag | 566.8 KB | 199.4 KB |
| feature-flag-list | 570.5 KB | 199.4 KB |
| feature-flag-testing | 457.3 KB | 199.4 KB |
| inline-scan | 453.6 KB | 199.4 KB |
| insight-actors | 562.3 KB | 199.4 KB |
| llm-costs | 559.3 KB | 199.4 KB |
| session-recording | 455.3 KB | 199.4 KB |
| survey | 454.7 KB | 199.4 KB |
| survey-global-stats | 561.9 KB | 199.4 KB |
| survey-list | 564.9 KB | 199.4 KB |
| survey-stats | 561.9 KB | 199.4 KB |
| trace-span | 453.5 KB | 199.4 KB |
| trace-span-list | 564.1 KB | 199.4 KB |
| vision-observation-list | 563.3 KB | 199.4 KB |
| workflow | 453.4 KB | 199.4 KB |
| workflow-list | 563.5 KB | 199.4 KB |
| loops-review | 457.8 KB | 199.4 KB |
| query-results | 774.3 KB | 199.4 KB |
| render-ui | 857.6 KB | 199.4 KB |
| visual-review-snapshots | 457.9 KB | 199.4 KB |
|
FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review |
A tool that lists thing:write is gated tighter than an API that needs thing:read, and MCP and the API both accept write for read. The exact string match warned about a 403 that cannot happen. The check now uses the same hasScope rule as the runtime.
|
@codex review |
|
Risk: No findings This increment removes PATCH from the annotation defaults in the MCP tool generator, so PATCH tools must now declare Sentinel reviewed |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bae52c9dad
ℹ️ 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".
A hook module that misspelled a hook name compiled and silently did nothing. Modules now default-export an object checked with `satisfies ToolHooks`, so a typo is a compile error. The generated import alias gets a hooks_ prefix, which a factory name cannot have, so a tool named like another tool plus "-hooks" no longer collides.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 📝 WalkthroughWalkthroughThe MCP tool generator now derives omitted scopes from OpenAPI security requirements and supplies annotation defaults for GET and DELETE. It supports YAML-configured hooks that can transform request parameters, transform successful responses, or handle errors. Feature-flag filter merging and session-recording not-found handling now use hooks on generated tools instead of handwritten tool overrides. Tests and implementation guidance are updated to reflect these changes. Priority: ⬇️ Low Merge Risk: 🔵 Low · up to This change moves scope and annotation derivation and tool hooks into codegen. The only open item is an outdated comment in a test file, so the merge risk is low. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The inspected changes preserve the existing authorization boundary and the two migrated tools’ behavior. Hook modules are selected by repository configuration, not runtime callers. No introduced security issue was established, but backend concurrency behavior and broader deployment coverage remain unverified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
A PATCH can be non-idempotent or a soft delete (cohorts add-persons, playlist deleted: true). A wrong default would skip the retry safety and the confirm prompt that destructiveHint and idempotentHint drive. Only GET and DELETE have a fixed meaning, so only they get defaults.
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
products/posthog_ai/eval_harness/test/test_feature_flags_scorers.py-100-101 (1)
100-101: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the stale comment. PATCH no longer gets default annotations.
The comment says GET, DELETE and PATCH have default annotations. The generator now gives defaults only to GET and DELETE. PATCH tools must declare their annotations.
Fix
- # tools.yaml may omit annotations (GET, DELETE and PATCH default), so read the generated + # tools.yaml may omit annotations (GET and DELETE default), so read the generated
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
- Review profile: QUIET
- Plan: Enterprise
- Run ID:
ec219e98-1561-4d93-9ad3-51b6270ee57a
⛔ Files ignored due to path filters (2)
services/mcp/src/tools/generated/feature_flags.tsis excluded by!**/generated/**services/mcp/src/tools/generated/replay.tsis excluded by!**/generated/**
📒 Files selected for processing (20)
.agents/skills/implementing-mcp-tools/SKILL.mddocs/published/handbook/engineering/ai/implementing-mcp-tools.mdproducts/feature_flags/mcp/tools.yamlproducts/posthog_ai/eval_harness/test/test_feature_flags_scorers.pyproducts/replay/mcp/tools.yamlservices/mcp/scripts/generate-tools.tsservices/mcp/scripts/yaml-config-schema.tsservices/mcp/src/tools/README.mdservices/mcp/src/tools/featureFlags/updateFeatureFlag.tsservices/mcp/src/tools/featureFlags/updateFeatureFlagHooks.tsservices/mcp/src/tools/index.tsservices/mcp/src/tools/replay/getSessionRecording.tsservices/mcp/src/tools/replay/sessionRecordingGetHooks.tsservices/mcp/src/tools/tool-hooks.tsservices/mcp/tests/tools/sessionReplays.integration.test.tsservices/mcp/tests/unit/generate-tools.test.tsservices/mcp/tests/unit/session-recording-get.test.tsservices/mcp/tests/unit/tool-hooks.test.tsservices/mcp/tests/unit/tool-name-validation.test.tsservices/mcp/tests/unit/update-feature-flag-preserving-groups.test.ts
💤 Files with no reviewable changes (3)
- services/mcp/src/tools/featureFlags/updateFeatureFlag.ts
- services/mcp/src/tools/index.ts
- services/mcp/src/tools/replay/getSessionRecording.ts
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 2 remain after this review.
There was a problem hiding this comment.
Approved.
Dev-tooling codegen change with explicit scopes/annotations still winning, existing generated output unchanged apart from the two hook-wrapped tools, and the moved logic preserved with tests. No unresolved substantive reviewer concerns on the current head; only a stale comment in a test remains.
- Author wrote 7% of the modified lines and has 21 merged PRs in these paths (familiarity MODERATE).
- coderabbitai[bot] reviewed the current head.
- Minor: a comment in products/posthog_ai/eval_harness/test/test_feature_flags_scorers.py still says PATCH gets default annotations, but only GET and DELETE do (CodeRabbit nit).
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 378L, 10F substantive, 876L/22F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (876L, 22F, cross-cutting, feat) |
| stamphog 2.3.1 | .stamphog/policy.yml @ d5393b3 · reviewed head d5393b3 |
|
😎 Stack merged successfully - details. |
Problem
An MCP tool can show up for a token and then fail with a 403 when called, because the scopes in tool YAML are typed by hand and drift from what the API enforces.
Authors also have to copy the same three annotation flags into every tool, even though they follow from the HTTP method.
The OpenAPI spec already lists the scopes each operation requires, and nothing reads them. A scan of the enabled tools found a handful of YAML lists that miss a required scope, so the drift is real.
Separately, customizing a generated tool means writing a hand-written tool with the same name, which silently replaces the generated one. Two of these "shadow" tools exist today (
update-feature-flag,session-recording-get). Each copies the generated tool's shape and has to be kept in sync by hand.Stacked on #111784 (docs cleanup). Part of a devex program on MCP codegen.
Changes
scopes:from a tool. Codegen then uses the scopes the API requires, so the tool matches the endpoint. Explicit YAML still wins, which keeps intentional gating.scopes:by hand.annotations:for GET (read-only) and DELETE (destructive). PATCH, POST and PUT vary too much to default (a PATCH can be a soft delete or non-idempotent), so codegen fails and asks for them.scopes:list that misses a scope the API requires now prints a warning, plus a GitHub annotation on the YAML file in CI. It only warns for now, so nothing breaks today. The current list is below.annotationsstraight from YAML, which breaks once a tool omits them. It reads the generated tool definitions now.hooks: <module under src/tools/>on a tool. The module exportsbeforeRequest,afterResponseandonError, and codegen wraps the generated handler with them. Name, schema and metadata stay generated. A missing module fails codegen, andhookswithconfirmed_actionis rejected because that path builds its own handlers.update-feature-flag(group targeting merge) andsession-recording-get(recording-not-found result) moved from shadow tools to hook modules with the same logic. The shadow files and theirTOOL_MAPentries are gone, and the pinned shadow list in the name validation test is empty.mergeToolFactoriesstays, since other code still merges hand-written tools. A third shadow in an open PR (fix(mcp): stop insight-update from reporting success when nothing persists #108314) could become a hook once this lands.hooks. The handbook's shadow-tool section is replaced.The scopes and annotations change leaves existing YAML and generated files byte-identical. Every enabled tool still declares both fields today, so it only opens the door for new tools.
Tools that trip the coverage warning today (owner follow-ups)
Each of these lists scopes in YAML that miss one the API requires. Add the scope, or drop
scopes:to use the API's.llma-evaluation-config-get,llma-evaluation-judge-models(missllm_analytics:read)loop-context-wiki-channel-resolve,loop-context-wiki-page-retrieve,task-context-wiki-channel-resolve,task-context-wiki-page-retrieve(missorganization:read);loop-context-wiki-page-update,task-context-wiki-page-propose,task-context-wiki-page-update(missorganization:write)warehouse-column-annotations-list(misseswarehouse_table:read);warehouse-column-annotations-create,warehouse-column-annotations-partial-update(misswarehouse_table:write)vision-scanners-affected-cohort-create,vision-scanners-impact-get,vision-scanners-impact-retrieve(misssession_recording:read)external-data-schemas-destinations-retrieve,external-data-sources-destinations-retrieve(missexternal_data_source:write)How did you test this code?
Unit tests in
generate-tools.test.tsrun the real category generation against small specs.Ran the MCP unit project and the touched feature flags eval tests locally. Typecheck only fails on unrelated
@posthog/quillimports.Ran the generator on the full spec after the first commit: output byte-identical (
git statusclean outside source edits), with the warnings above. After the hooks commit only the two hooked tools' generated files differ.The existing tests for both moved tools run unchanged apart from the import: they now take the tool from the generated map. Ran the MCP unit project and
test:honolocally.Checked derivation by hand with temporary YAML edits, since reverted:
scopes:andannotations:from one GET tool whose YAML matched the spec: output still identical.annotations:from a POST tool: codegen fails with a message that POST has no defaults.Not checked: a CI run of the new GitHub annotation. The unit test covers the line format only.
Test rationale: Hooks:
generate-tools.test.tscovers the emitted import and wrapper, tools without hooks staying unwrapped, a missing module failing, and theconfirmed_actionrejection.tool-hooks.test.tscovers hook order,onErrorhandling and rethrow, and a failedbeforeRequestskipping the request. Scopes and annotations: the tests cover derived scopes, the fail-when-spec-has-none path, the GET and DELETE defaults, PATCH/POST/PUT failing, explicit YAML winning, and the warning (missing vs broader list, CI annotation on and off). Existing tests did not touch scope or annotation resolution at all. A test at generation level is needed because the wiring ingenerateCategoryFileis the risk.👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Release status
Docs update
The authoring skill and the handbook guide are updated in this PR.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Agent: Claude Code. Code written by Claude Sonnet 5.5 (claude-sonnet-5-5) subagent, orchestrated by Claude Opus 5.5 (claude-opus-5-5).
--deep, base is the docs cleanup branch): no findings on either commit./reviewing-with-coderabbit,/writing-pr-descriptions./implementing-mcp-tools,/writing-testsand/writing-code-commentswere read for the edits.scopes/annotations, since they have no OpenAPI operation to derive from.