feat(autoresearch): teach the training agent what its feature query costs - #110916
posthog[bot] wants to merge 8 commits into
Conversation
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
🦔 PostHog Review reviewed this pull requestFound 0 must fix, 1 should fix, 0 consider. Published 1 finding (view the review). Resolved comments: 1 fixed |
|
Risk: High · 1 high, 1 medium This increment threads ClickHouse query contexts through the scoring helpers (the Temporal activity correctly passes BATCH_QUERY) and adds a flag-gated report-notebook step to the training agent's brief, granting the sandbox agent notebook:read/write MCP scopes and a new complete-run field. The step is only prose-guarded: the granted scopes let the untrusted agent run Python in notebook kernel sandboxes that have network egress and overwrite/delete any notebook in the project, both of which the training sandbox's own guarantees were meant to close. Sentinel reviewed |
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
🤖 CI report
|
| Root | Eager (shipped) | Δ vs base | Budget |
|---|---|---|---|
entry (logged-out pages, app bootstrap)src/index.tsx |
1.63 MiB · 22 files | no change | █████████░ 88.7% 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 · 661 files | no change | █████████░ 92.3% of 4.03 MiB |
authenticated shell (every logged-in page)src/scenes/AuthenticatedShell.tsx |
7.59 MiB · 2,409 files | no change | █████████░ 91.0% of 8.34 MiB |
dashboard scenesrc/scenes/dashboard/Dashboard.tsx |
9.67 MiB · 3,392 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.61 MiB · 2,417 files | no change | █████████░ 88.6% of 8.58 MiB |
events scenesrc/scenes/activity/explore/EventsScene.tsx |
9.29 MiB · 3,244 files | no change | ███████░░░ 73.5% of 12.64 MiB |
replay detail scenesrc/scenes/session-recordings/detail/SessionRecordingDetail.tsx |
12.12 MiB · 4,130 files | no change | ████████░░ 77.1% 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 |
| 854 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 |
| 100.5 KiB | src/lib/api.ts |
| 92.7 KiB | src/products.tsx |
| 69.4 KiB | src/lib/lemon-ui/icons/icons.tsx |
| 40.1 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 |
| 279.9 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 |
| 110.1 KiB | ../packages/quill/packages/quill/dist/index.js |
| 100.5 KiB | src/lib/api.ts |
| 93.3 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
| 92.7 KiB | src/products.tsx |
| 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/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 |
| 279.9 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 |
| 110.1 KiB | ../packages/quill/packages/quill/dist/index.js |
| 100.5 KiB | src/lib/api.ts |
| 93.3 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
| 92.7 KiB | src/products.tsx |
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 |
| 279.9 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 |
| 110.1 KiB | ../packages/quill/packages/quill/dist/index.js |
| 100.5 KiB | src/lib/api.ts |
| 93.3 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
| 92.7 KiB | src/products.tsx |
| 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/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 |
| 279.9 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 |
| 110.1 KiB | ../packages/quill/packages/quill/dist/index.js |
| 100.5 KiB | src/lib/api.ts |
| 93.3 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
| 92.7 KiB | src/products.tsx |
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 |
| 279.9 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 |
| 110.1 KiB | ../packages/quill/packages/quill/dist/index.js |
| 100.5 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.20 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.20 MiB · 19 files | no change | ████░░░░░░ 38.4% 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 |
|---|---|
| 835.5 KiB | dist/toolbar/toolbar-app-FSDWO46I.css |
| 657.5 KiB | dist/toolbar/chunk-chunk-SLUTN3OK.js |
| 259.4 KiB | dist/toolbar/chunk-chunk-7JWMBALG.js |
| 138.2 KiB | dist/toolbar/chunk-chunk-JZ43POKQ.js |
| 131.8 KiB | dist/toolbar/chunk-chunk-FDH2IBXT.js |
| 75.2 KiB | dist/toolbar/toolbar-app-LJ7FFVU6.js |
| 69.0 KiB | dist/toolbar/chunk-chunk-TSAL54PB.js |
| 35.6 KiB | dist/toolbar/chunk-chunk-P52PYKZI.js |
| 21.0 KiB | dist/toolbar/chunk-chunk-BO2MEODF.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 — 🔺 +1008 B (+0.0%)
Total size of the built frontend/dist folder (all assets), compared against the base branch.
Total: 960.34 MiB · 🔺 +1008 B (+0.0%)
ℹ️ MCP UI apps size — 33 app(s), 17633.6 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 |
| invite-email-preview | 452.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 | 858.4 KB | 199.4 KB |
| visual-review-snapshots | 457.9 KB | 199.4 KB |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 🧰 Additional context used📚 Code guidelines (15)No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: PostHog/posthog/.coderabbit.yaml Review profile: QUIET Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughClickHouse query statistics now track bytes read. Feature materialization returns elapsed time, rows read, and bytes read from the feature query. The training brief updates its inference cutoff guidance and worked SQL example, and adds instructions to compare query costs. MCP tool descriptions and generated types include the new metrics. Priority: ➖ Normal Merge Risk: 🔵 Low · up to The change appears mergeable with awareness that reported feature-query costs could include other work if materialization begins issuing additional queries or sharing its statistics scope concurrently. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to This adds cost feedback to an existing operation rather than granting new access. No new security issue was identified in the inspected path, although interrupted-run cleanup and live deployment behavior were not fully verified. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
products/autoresearch/backend/inference/sandbox.py (1)
408-416: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueElapsed time is summed from per-query ClickHouse durations, not wall-clock, and may include concurrent queries.
stats.duration_mssumselapsed_nsfrom each ClickHouse query that runs under the shared scope. The delta covers the whole_materialize_rowscall. If_materialize_rowsever runs a second query (for example, a HogQL metadata lookup), or if the scope is nested in a request scope that other threads write to, the delta includes unrelated queries.QueryStatsexplicitly supports multi-thread recording, so another thread in an enclosing scope can inflate the delta.This is a low-likelihood case today. Consider documenting that the figure is the sum of ClickHouse-reported durations for all queries in the scope. Do not read it as the feature query's own cost if concurrent work is possible.
products/autoresearch/backend/training/runner.py (1)
325-341: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueWorked example
active_dayscomment and expression do not match.The comment above
pageviewsreads "recent browsing", but the expression counts all pageviews in the whole lookback window, not recent ones. This misleads the agent about what the feature measures. The agent copies this example, so the mismatch propagates into uploadedfeatures.sqlfiles that non-technical users read.Also,
uniqIf(toDate(e.timestamp), e.event != '')counts days with any event. The filtere.event != ''is also true for theNULL-to-default row produced byLEFT JOINwhen a person has no events. In ClickHouse, unmatchedLEFT JOINrows get default values (empty string forevent), so this filter excludes them correctly. The condition is therefore valid, but it reads as a no-op to a human. Add a short comment that it excludes the unmatched-join default row.Proposed fix for the comment
- -- recent browsing: people who come back often convert more + -- browsing volume over the lookback window: heavy browsers convert more countIf(e.event = '$pageview') AS pageviews,
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: cc711909-b9b0-4702-b493-295639229340
⛔ Files ignored due to path filters (1)
products/autoresearch/frontend/generated/api.schemas.tsis excluded by!**/generated/**
📒 Files selected for processing (17)
posthog/clickhouse/client/connection.pyposthog/clickhouse/client/execute.pyposthog/clickhouse/client/test/test_execute.pyposthog/hogql/query_stats.pyproducts/autoresearch/backend/facade/api.pyproducts/autoresearch/backend/facade/contracts.pyproducts/autoresearch/backend/inference/sandbox.pyproducts/autoresearch/backend/inference/test_materialize_features.pyproducts/autoresearch/backend/presentation/views/serializers.pyproducts/autoresearch/backend/presentation/views/views.pyproducts/autoresearch/backend/training/AGENTS.mdproducts/autoresearch/backend/training/runner.pyproducts/autoresearch/backend/training/test_training.pyproducts/autoresearch/mcp/tools.yamlservices/mcp/schema/generated-tool-definitions.jsonservices/mcp/schema/tool-definitions-all.jsonservices/mcp/src/api/generated.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.
A new stamphog review started for this PR — the fresh verdict replaces this approval.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
A new stamphog review started for this PR — the fresh verdict replaces this approval.
andrewm4894
left a comment
andrewm4894
left a comment
There was a problem hiding this comment.
Matches #110889: pre-filtered worked example, cost guidance as advice, corrected inference cutoff, and a cost readout from the query's own stats (the shared query_stats change is additive, defaulting to 0).
Non-blocking nit: the cutoff paragraph ends with "do not build one". We prefer nudges over prohibitions in the brief, so something like "so it is not worth building" reads better. Fine to fix here or in a follow-up.
|
PostHog Review alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
| AND person_id IN (SELECT person_id FROM {{anchors}}) | ||
| AND timestamp >= (SELECT fromUnixTimestamp(min(cutoff_ts)) FROM {{anchors}}) - toIntervalDay({{lookback_days}}) | ||
| AND timestamp < (SELECT fromUnixTimestamp(max(cutoff_ts)) FROM {{anchors}}) |
There was a problem hiding this comment.
Materialize anchors before reusing them in the new filters
Issue description
These filters add three references to {anchors}. Training substitutes a non-materialized labeled_anchors CTE, while inference substitutes the full population subquery each time. ClickHouse evaluates these relations separately for each reference. The new filters therefore repeat expensive population scans and aggregations. Sampled training also repeats the label calculation. This increases query work under the execution time limit, even when the selected population is small.
Why we think it's a valid issue
- Checked: The worked
features.sqlinproducts/autoresearch/backend/training/runner.py:322-347. I also checked how{anchors}gets replaced inproducts/autoresearch/backend/dataset/labeling.py(_substitute_anchorsatlabeling.py:1058-1072,build_training_features_sqlatlabeling.py:1075-1131,build_inference_features_sqlatlabeling.py:1134-1166), thelabeled_usersCTE builder (labeling.py:645-800), and how the HogQL ClickHouse printer emits CTEs (posthog/hogql/printer/clickhouse.py:154-165). - Found: The new subquery adds three more references to
{anchors}:person_id IN (SELECT person_id FROM {{anchors}}), amin(cutoff_ts)scalar subquery and amax(cutoff_ts)scalar subquery (runner.py:339-341). TheFROM {{anchors}} areference was already there, so the example now has four._substitute_anchorscopies the replacement text into every reference. - Found: At inference, the replacement is the whole
build_inference_anchors_sql()query, inlined each time (labeling.py:1162-1165). That query scanseventsover the feature lookback for population members and joinsraw_persons. The anchor work goes from one evaluation to four. All inference anchors share the same midnight cutoff, so theminandmaxsubqueries compute one constant, but ClickHouse still runs the full population query to get it. - Found: At training, the replacement is
(SELECT person_id, t0_ts AS cutoff_ts FROM labeled_anchors)(labeling.py:1109).labeled_anchorsandlabeled_usersare plain CTEs with noMATERIALIZEDhint (labeling.py:1112-1131). ClickHouse runs a plain CTE again for each reference. Each run oflabeled_usersdoes twoeventsscans over the training lookback (theuser_windowaggregate and the label join withGROUP BY,labeling.py:741-783). The wrapper's outerLEFT JOIN labeled_anchorsis one more reference, so one materialization goes from about 2 to about 5 runs of the labeling CTE. - Found: The fix is available. HogQL parses and prints
AS MATERIALIZEDfor ClickHouse (clickhouse.py:163-164,HogQLParser.g4:349), andproducts/product_analytics/backend/hogql_queries/funnels/funnel_correlation_query_runner.pyalready uses it. - Impact: The PR gives this example as the cost-safe shape that the agent copies. The brief says the same SQL runs on every scoring cadence over the whole inference population, under a query time limit (
runner.py:354-356). The new references add three full population scans at inference and three full labeling passes at training, so the query does several times the anchor work it needs. On a large team this can push scoring or materialization past the time limit, which is the failure the PR tries to prevent. The new shape still beats the old direct events join, which ran out of memory. But the extra cost comes from the lines this PR adds, and a small change in the builders removes it.
Suggested fix
Materialize the framework's anchors once in both build_training_features_sql() and build_inference_features_sql(). Reuse that relation for the filters and the outer FROM. For training, use labeled_anchors AS MATERIALIZED (...). Use an equivalent shared materialized CTE for inference. The marketing attribution runners already use this pattern to prevent repeated scans. Keep the per-anchor timestamp predicates.
Prompt to fix with AI (copy-paste)
## Context
@products/autoresearch/backend/training/runner.py#L339-341
<issue_description>
These filters add three references to {anchors}. Training substitutes a non-materialized labeled_anchors CTE, while inference substitutes the full population subquery each time. ClickHouse evaluates these relations separately for each reference. The new filters therefore repeat expensive population scans and aggregations. Sampled training also repeats the label calculation. This increases query work under the execution time limit, even when the selected population is small.
</issue_description>
<issue_validation>
- **Checked:** The worked `features.sql` in `products/autoresearch/backend/training/runner.py:322-347`. I also checked how `{anchors}` gets replaced in `products/autoresearch/backend/dataset/labeling.py` (`_substitute_anchors` at `labeling.py:1058-1072`, `build_training_features_sql` at `labeling.py:1075-1131`, `build_inference_features_sql` at `labeling.py:1134-1166`), the `labeled_users` CTE builder (`labeling.py:645-800`), and how the HogQL ClickHouse printer emits CTEs (`posthog/hogql/printer/clickhouse.py:154-165`).
- **Found:** The new subquery adds three more references to `{anchors}`: `person_id IN (SELECT person_id FROM {{anchors}})`, a `min(cutoff_ts)` scalar subquery and a `max(cutoff_ts)` scalar subquery (`runner.py:339-341`). The `FROM {{anchors}} a` reference was already there, so the example now has four. `_substitute_anchors` copies the replacement text into every reference.
- **Found:** At inference, the replacement is the whole `build_inference_anchors_sql()` query, inlined each time (`labeling.py:1162-1165`). That query scans `events` over the feature lookback for population members and joins `raw_persons`. The anchor work goes from one evaluation to four. All inference anchors share the same midnight cutoff, so the `min` and `max` subqueries compute one constant, but ClickHouse still runs the full population query to get it.
- **Found:** At training, the replacement is `(SELECT person_id, t0_ts AS cutoff_ts FROM labeled_anchors)` (`labeling.py:1109`). `labeled_anchors` and `labeled_users` are plain CTEs with no `MATERIALIZED` hint (`labeling.py:1112-1131`). ClickHouse runs a plain CTE again for each reference. Each run of `labeled_users` does two `events` scans over the training lookback (the `user_window` aggregate and the label join with `GROUP BY`, `labeling.py:741-783`). The wrapper's outer `LEFT JOIN labeled_anchors` is one more reference, so one materialization goes from about 2 to about 5 runs of the labeling CTE.
- **Found:** The fix is available. HogQL parses and prints `AS MATERIALIZED` for ClickHouse (`clickhouse.py:163-164`, `HogQLParser.g4:349`), and `products/product_analytics/backend/hogql_queries/funnels/funnel_correlation_query_runner.py` already uses it.
- **Impact:** The PR gives this example as the cost-safe shape that the agent copies. The brief says the same SQL runs on every scoring cadence over the whole inference population, under a query time limit (`runner.py:354-356`). The new references add three full population scans at inference and three full labeling passes at training, so the query does several times the anchor work it needs. On a large team this can push scoring or materialization past the time limit, which is the failure the PR tries to prevent. The new shape still beats the old direct events join, which ran out of memory. But the extra cost comes from the lines this PR adds, and a small change in the builders removes it.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Materialize the framework's anchors once in both build_training_features_sql() and build_inference_features_sql(). Reuse that relation for the filters and the outer FROM. For training, use labeled_anchors AS MATERIALIZED (...). Use an equivalent shared materialized CTE for inference. The marketing attribution runners already use this pattern to prevent repeated scans. Keep the per-anchor timestamp predicates.
</potential_solution>
There was a problem hiding this comment.
Escalated. The cost is real, but the fix changes the framework's query builders, and a person must choose the trade-off.
- The worked example now reads
{anchors}four times. Each read runs the full anchors query again: the population scan at inference, the labeling CTE at training. - Option 1: materialize the anchors once in
build_training_features_sql()andbuild_inference_features_sql(). This changes every scoring run, including existing champions, and the new CTE name must go on the validator's deny list. - Option 2: drop the min/max anchor reads from the example. There are fewer anchor scans, but the events subquery reads a wider window.
- I did not choose because the gain shows only in live query cost on large teams, and I cannot measure that here.
How this was verified
No code change. I read labeling.py (anchor substitution and both feature SQL builders), the ClickHouse printer's CTE handling, and the worked example in runner.py at the current head.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
A new stamphog review started for this PR — the fresh verdict replaces this approval.
There was a problem hiding this comment.
Not approved — this change needs a human reviewer.
Re-add the stamphog label to request another review once you have addressed this.
The brief tells the training agent that person.properties.* is already on events and avoids a persons join. That's wrong: the labeler modifiers resolve it through the persons table at its latest values. The advice can leak the outcome into training features, and it's the posthog[bot] must-fix thread that is still unresolved on the current diff.
- products/autoresearch/backend/training/runner.py, cost bullet on
person.properties.*: the text says these are columns already on events and that using them avoidsLEFT JOIN persons.LABELER_QUERY_MODIFIERSin dataset/labeling.py sets PERSON_ID_OVERRIDE_PROPERTIES_JOINED, and its own comment says aperson.*column resolves through the persons table. So the advice is factually wrong, it adds the persons join it claims to avoid, and it reads current property values, which can leak the label into training and inflate holdout AUC. This is the unresolved must-fix thread from @PostHog[bot]. - The worked example references
{anchors}three more times in subqueries. The unresolved should-fix thread from @PostHog[bot] says training's labeled_anchors CTE and the inference population subquery are not materialized, so each reference repeats the scans. That can add cost under the scoring time limit, which is the problem this PR is meant to fix. - Smaller points: the
do not build onewording in rule 1 is a nit from @andrewm4894, and @coderabbitai raised a docstring caveat about summed ClickHouse durations. Neither blocks.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 154L, 12F substantive, 207L/18F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (207L, 18F, cross-cutting, feat) |
| stamphog 2.3.1 | .stamphog/policy.yml @ 0a2ce6f · reviewed head 0a2ce6f |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
A new stamphog review started for this PR — the fresh verdict replaces this approval.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
A new stamphog review started for this PR — the fresh verdict replaces this approval.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…osts The brief's worked features.sql now reads events through a pre-filtered subquery instead of a direct events join. The brief adds cost guidance and states the real inference cutoff (start of the prediction date in UTC). materialize-features returns the feature query's elapsed time, rows read and bytes read, taken from query_stats, which now also adds up bytes read. Closes #110889 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 46c088a6-987e-457f-9f66-ef60757b7361
…ompute Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 46c088a6-987e-457f-9f66-ef60757b7361
A training cutoff falls at any time of day, so the daily or hourly bucket that holds it also holds events after it. The brief now tells the agent to join only whole buckets before the cutoff and to read the cutoff's own day from raw events with the strict `<` of rule 2. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 8bba9ed5-6a9c-45d1-a8bd-0ca446bf5129
Under the labeler's query modifiers, `person.properties.*` joins the persons table and returns current values. That costs the same join as `LEFT JOIN persons`, and at training it leaks values set by the outcome. The brief now tells the agent to read the snapshot on each event, `poe.properties.*`, and take the latest value before the cutoff. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 8bba9ed5-6a9c-45d1-a8bd-0ca446bf5129
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 46c088a6-987e-457f-9f66-ef60757b7361
…ction The brief told the agent to prefer a cheaper query when two iterations score about the same AUC. The backend promotes the kept iteration with the highest holdout AUC and rejects a bundle whose features.sql differs from that iteration's query, so following the guidance could fail completion. The brief now states the server's rule and uses cost to steer hypotheses, not to pick the winner. Generated-By: PostHog Desktop Task-Id: 2059e301-c671-4e0e-9555-0ca3d29c9499
531fa7b to
8e7a179
Compare
A new stamphog review started for this PR — the fresh verdict replaces this approval.
…nces Generated-By: PostHog Desktop Task-Id: 594f197f-93bd-49c8-bdc4-1fc41943c54d
There was a problem hiding this comment.
Approved.
The change is mostly prompt and brief text plus additive, default-zero byte counters on query stats and extra response fields on a feature-flagged autoresearch endpoint. I found no risky-territory change. The one open reviewer thread is a query-cost trade-off in the worked example, not a correctness or safety issue.
- The open inline thread notes the worked features.sql reads {anchors} several times, which could repeat expensive scans on large teams. This is a performance trade-off for follow-up, not a blocker.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 157L, 12F substantive, 210L/18F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (210L, 18F, cross-cutting, feat) |
| stamphog 2.3.1 | .stamphog/policy.yml @ 3bb5f0a · reviewed head 3bb5f0a |
![parameterai[bot]](https://avatars.githubusercontent.com/in/2665233?s=60&v=4){kind=small}
| cutoff's time of day or hour varies in training but is constant at scoring. It teaches the | ||
| model nothing it can use, so it is not worth building. | ||
| 2. Join events with `e.timestamp < fromUnixTimestamp(a.cutoff_ts)` — strict `<`. The leakage guard. | ||
| 3. Window the lookback: `e.timestamp >= fromUnixTimestamp(a.cutoff_ts) - toIntervalDay({{lookback_days}})`. |
There was a problem hiding this comment.
The report-notebook step added to the agent brief in this PR (runner.py:151, _report_notebook_step, rendered into the brief at runner.py:638) directs the agent to add and run Python cells in a notebook — but notebook kernel sandboxes do not block network egress, unlike the training sandbox, which is created with block_network=True (products/autoresearch/backend/inference/sandbox.py:717). build_notebook_sandbox_config (products/notebooks/backend/kernel_runtime.py:206-222) never sets block_network, and SandboxConfig defaults it to False (products/tasks/backend/logic/services/sandbox.py:240). The hand-written notebooks-add-cell MCP tool runs python cells immediately ("'sql' ... and 'python' run immediately", services/mcp/src/tools/notebooks/addCell.ts:40) by POSTing to notebooks/{short_id}/sql_v2/run/, which requires only notebook:write + query:read (products/notebooks/backend/presentation/views/notebook.py:1990) — both granted by REPORT_NOTEBOOK_MCP_SCOPES (runner.py:81, applied to the token at runner.py:744-745). The brief's rules "No network access: no requests, urllib, http, socket, or subprocess" and "no package installs" are prose only; the product's own docs state everything the agent sends is untrusted. A prompt-injected or confused agent can run Python that exfiltrates the team's event data (readable via its SQL cells and query:read) to arbitrary external hosts, or installs packages in a paid kernel.
How: 1. A user with the autoresearch-report-notebook flag on starts a training run; the agent token gains notebook:read/notebook:write. 2. Injected instructions make the agent call notebooks-add-cell with cell_type python whose code does urllib.request.urlopen(...) with the dataframes from earlier SQL cells. 3. The cell dispatches a run into a notebook kernel sandbox that has live network egress. 4. Team data leaves PostHog through a path the training sandbox's network block was built to close.
Fix: Set block_network=True in build_notebook_sandbox_config (or refuse kernel runs from agent/task tokens), so notebook cells can never give the training agent egress.
React with 👍 if useful or 👎 if not
| date in UTC (midnight) for every person. Same SQL, two tables. A feature derived from the | ||
| cutoff's time of day or hour varies in training but is constant at scoring. It teaches the | ||
| model nothing it can use, so it is not worth building. | ||
| 2. Join events with `e.timestamp < fromUnixTimestamp(a.cutoff_ts)` — strict `<`. The leakage guard. |
There was a problem hiding this comment.
When the report-notebook flag is on, the agent's MCP token gains notebook:write (runner.py:81, REPORT_NOTEBOOK_MCP_SCOPES, applied at runner.py:744-745) — but that scope covers notebooks-partial-update and notebooks-destroy on every notebook in the project: both are enabled MCP tools gated only on notebook:write (products/notebooks/mcp/tools.yaml, entries notebooks-partial-update and notebooks-destroy, both enabled: true). The comment above the constant admits this and falls back to prose: "notebook:write also exposes notebooks-partial-update and notebooks-destroy, so the brief limits the agent to the notebook it creates in this run" (runner.py:78-79). The product's own rules say everything the agent sends is untrusted, and a brief instruction is not enforcement — a confused or injected agent can rewrite or delete teammates' notebooks (dashboards-as-notebooks, reports) in the same project, with no object-level restriction.
How: 1. Start a training run as a user with the flag on; the token gains notebook:write. 2. Injected or mistaken agent behavior calls notebooks-partial-update (or notebooks-destroy) on another notebook's short_id from notebooks-list. 3. The write succeeds — the endpoint is team-scoped, not restricted to the agent's created notebook. 4. The victim notebook's content is replaced or deleted.
Fix: After the run creates its notebook, narrow the grant — e.g. an object-scoped permission for that one notebook, or a server-side guard on the training token that refuses notebook writes whose short_id is not the run's own notebook.
React with 👍 if useful or 👎 if not
andrewm4894
added
the
ci-backend-github
PostHog
This branch has not been deployed
1 participant
Problem
features.sql. That example joins raweventsdirectly, so ClickHouse reads all of the team's events into memory before the anchor filter applies. On a large team, the query runs out of memory.cutoff_ts = now(). Scoring actually binds the start of the prediction date in UTC (ScoringWindow). The wrong statement invites time-of-day features: train/serve skew.Closes #110889
Origin
Changes
<cutoff and lookback stay in the join. All hard rules still hold.$pageviewwindows short, preferperson.properties.*toLEFT JOIN persons, compare iterations on cost as well as AUC.materialize-featuresreturnsfeature_query_elapsed_ms,feature_query_rows_readandfeature_query_bytes_read. The numbers come from the query's own statistics, not a second query.query_statsandQuerySummarynow also add up bytes read (native and HTTP clients). Existing callers are unchanged; the new argument defaults to 0.tools.yamldescription andtraining/AGENTS.md.How did you test this code?
validate_feature_sql(), materializes throughmaterialize_training_data()(train + holdout) and through the inference path, and reports non-zero rows and bytes read. The fullmaterialize-featuresHTTP path needs a live Tasks sandbox, so that was not exercised end to end.test_materialize_features.py,test_training.py,test_sandbox_inference.py,clickhouse/client/test/test_execute.pyandhogql/test/test_query_stats.pylocally.Test rationale:
test_writes_parquet_and_returns_pathsnow asserts the three cost fields, so it fails if the facade or serializer drops them.test_sync_execute_records_what_clickhouse_readnow asserts bytes read for native, killed and HTTP clients. It is the existing test for the executor's stats recording.test_prompt_drives_materialize_features_not_execute_sql_pullnow checks the brief names the cost fields and no longer teaches the direct events join orcutoff_ts = now().Release status
Automatic notifications
Docs update
None.
products/autoresearch/backend/training/AGENTS.mdis updated in this PR.🤖 Agent context
Autonomy: Fully autonomous
Agent: Claude Code, Claude Opus 5.5 (
claude-opus-5-5)/improving-drf-endpoints,/writing-tests.system.query_log.Created with PostHog Desktop from this inbox report.
🤖 Generated with Claude Code