Repository navigation
feat(wizard): execute cloud runs - #91821
Conversation
🤖 CI report
|
| File | Size | Δ vs base |
|---|---|---|
posthog-app/src/scenes/AuthenticatedShell.js |
239.6 KiB | 🔺 +25.1 KiB (+11.7%) |
render-query/src/render-query/render-query.js |
21.93 MiB | 🔺 +20.5 KiB (+0.1%) |
posthog-app/src/scenes/experiments/Experiment.js |
275.1 KiB | 🔺 +5.3 KiB (+2.0%) |
posthog-app/src/scenes/models/ModelsScene.js |
18.3 KiB | 🟢 -4.4 KiB (-19.4%) |
exporter/src/exporter/scenes/ExporterNotebookScene.js |
3.46 MiB | 🔺 +2.5 KiB (+0.1%) |
posthog-app/_parent/products/signals/frontend/inbox/InboxScene.js |
781.0 KiB | 🟢 -1.3 KiB (-0.2%) |
posthog-app/_parent/products/posthog_ai/frontend/scenes/TaskTracker/components/SidePanelRunnerImpl.js |
21.9 KiB | 🔺 +1.1 KiB (+5.4%) |
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.50 MiB · 22 files | 🔺 +44 B (+0.0%) | ███░░░░░░░ 33.2% of 4.51 MiB |
logged-out boot: index + App + bootApp (preloaded by every page, including /login)src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts |
5.24 MiB · 668 files | 🔺 +653 B (+0.0%) | █████████░ 93.1% of 5.63 MiB |
authenticated shell (every logged-in page)src/scenes/AuthenticatedShell.tsx |
7.67 MiB · 2,680 files | 🔺 +16.0 KiB (+0.2%) | █████████░ 87.4% of 8.77 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
🟢 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
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/RootErrorBoundary.tsx |
| 1.0 KiB | src/index.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 |
|---|---|
| 281.8 KiB | ../node_modules/.pnpm/posthog-js@1.428.6_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/module.mjs |
| 267.7 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 |
| 255.4 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 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 |
| 104.2 KiB | src/lib/api.ts |
| 79.0 KiB | src/products.tsx |
| 68.8 KiB | src/lib/lemon-ui/icons/icons.tsx |
| 62.8 KiB | src/lib/utils/eventUsageLogic.ts |
| 38.8 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 |
Largest files eagerly shipped from src/scenes/AuthenticatedShell.tsx
| Size | File |
|---|---|
| 281.8 KiB | ../node_modules/.pnpm/posthog-js@1.428.6_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/module.mjs |
| 267.7 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 |
| 255.4 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 153.8 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 |
| 104.2 KiB | src/lib/api.ts |
| 95.8 KiB | ../packages/quill/packages/quill/dist/index.js |
| 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 |
| 79.0 KiB | src/products.tsx |
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.29 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.29 MiB · 18 files | 🔺 +439 B (+0.0%) | ████░░░░░░ 40.0% of 5.72 MiB |
| Deferred (lazy) | 2.11 MiB · 45 files | 🔺 +223 B (+0.0%) | n/a — loads on demand |
Loader dist/toolbar.js |
1.1 KiB | no change | █░░░░░░░░░ 5.8% of 19.5 KiB |
Largest eagerly-shipped chunks
| Size | File |
|---|---|
| 756.4 KiB | dist/toolbar/toolbar-app-6DCV5KV2.css |
| 608.8 KiB | dist/toolbar/chunk-chunk-Q2TIP7AE.js |
| 484.7 KiB | dist/toolbar/chunk-chunk-WMHPRUHK.js |
| 136.6 KiB | dist/toolbar/chunk-chunk-KP7ONKPB.js |
| 131.8 KiB | dist/toolbar/chunk-chunk-FDH2IBXT.js |
| 72.2 KiB | dist/toolbar/toolbar-app-V2OFE7DG.js |
| 69.0 KiB | dist/toolbar/chunk-chunk-TSAL54PB.js |
| 35.6 KiB | dist/toolbar/chunk-chunk-A2HGKOZX.js |
| 21.0 KiB | dist/toolbar/chunk-chunk-TBSJCMNI.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 — 🔺 +631.7 KiB (+0.0%)
Total size of the built frontend/dist folder (all assets), compared against the base branch.
Total: 1458.58 MiB · 🔺 +631.7 KiB (+0.0%)
⚠️ Backend coverage — 93.0% of changed backend lines covered — 132 uncovered
🧪 Backend test coverage
Patch coverage — changed backend lines (products + core): ███████████████████░ 93.0% (1,989 / 2,121)
| File | Patch | Uncovered changed lines |
|---|---|---|
products/wizard/backend/tasks/config.py |
0.0% | 1 |
products/wizard/backend/facade/temporal.py |
0.0% | 1, 3 |
products/tasks/backend/facade/repo_selection.py |
57.1% | 23, 27–28 |
products/wizard/backend/logic/workers/repository_publisher.py |
66.7% | 74–76, 80–82, 96, 101, 119, 121–122, 126, 131, 145–146, 154, 186, 193, 210, 238, 241–242, 244, 248, 251, 259, 274–277, 279–281, 283–284, 290–291, 293–294, 303–315, 322, 331–337, 349, 369 |
products/wizard/backend/temporal/activities/local_package.py |
75.0% | 21, 29–30, 35–36 |
products/wizard/backend/facade/api.py |
81.0% | 107, 115, 165, 169 |
products/wizard/backend/logic/runs/cancellation.py |
81.2% | 17–19 |
products/wizard/backend/temporal/activities/lifecycle.py |
85.3% | 39–40, 42–43, 45 |
products/wizard/backend/logic/runs/lifecycle.py |
89.6% | 67, 74–75, 77, 116, 150, 158, 162, 175, 197–198, 207 |
products/wizard/backend/logic/workers/lifecycle.py |
89.6% | 66–67, 77–78, 82 |
products/wizard/backend/logic/workers/service.py |
90.6% | 203, 281–282, 292, 296–302, 304–305, 307 |
products/wizard/backend/temporal/activities/workspace.py |
90.7% | 45, 51, 91, 136, 142 |
products/wizard/backend/temporal/activities/handoff.py |
91.3% | 31–32 |
products/wizard/backend/temporal/client.py |
93.5% | 47–48 |
products/wizard/backend/temporal/contracts.py |
94.3% | 24, 26 |
products/wizard/backend/temporal/errors.py |
96.0% | 22 |
products/wizard/backend/logic/runs/dispatch.py |
96.0% | 22 |
products/wizard/backend/temporal/workflows/execute_run.py |
97.0% | 65, 171 |
products/wizard/backend/logic/workers/commands.py |
98.0% | 86 |
🤖 Agents: add a test covering the lines above, or note why under "How did you test this code?". Machine-readable gap list: the patch-coverage artifact on this run (gh run download 34268766318 -n patch-coverage), or the coverage-data block at the end of this comment.
Per-product line coverage (touched products)
| Product | Coverage | Lines |
|---|---|---|
platform_features |
██░░░░░░░░░░░░░░░░░░ 12.1% |
7 / 58 |
warehouse_sources_queue |
██████░░░░░░░░░░░░░░ 29.1% |
92 / 316 |
data_tools |
████████████░░░░░░░░ 62.1% |
90 / 145 |
demo |
████████████░░░░░░░░ 62.1% |
1,658 / 2,671 |
ai_gateway |
███████████████░░░░░ 75.0% |
9 / 12 |
batch_exports |
████████████████░░░░ 79.9% |
19,472 / 24,368 |
apm |
█████████████████░░░ 83.3% |
1,234 / 1,481 |
cdp |
█████████████████░░░ 85.4% |
4,349 / 5,094 |
signals |
█████████████████░░░ 85.5% |
36,469 / 42,645 |
product_tours |
██████████████████░░ 88.0% |
1,312 / 1,491 |
notebooks |
██████████████████░░ 88.5% |
11,879 / 13,429 |
data_warehouse |
██████████████████░░ 88.9% |
12,824 / 14,424 |
business_knowledge |
██████████████████░░ 89.2% |
4,469 / 5,008 |
cohorts |
██████████████████░░ 89.3% |
7,565 / 8,468 |
dashboards |
██████████████████░░ 89.6% |
6,837 / 7,631 |
engineering_analytics |
██████████████████░░ 90.1% |
8,391 / 9,310 |
tasks |
██████████████████░░ 90.3% |
66,372 / 73,489 |
data_modeling |
██████████████████░░ 90.5% |
9,474 / 10,468 |
streamlit_apps |
██████████████████░░ 90.7% |
2,619 / 2,889 |
managed_warehouse |
██████████████████░░ 90.7% |
9,417 / 10,378 |
exports |
██████████████████░░ 90.8% |
9,124 / 10,050 |
alerts |
██████████████████░░ 90.9% |
5,242 / 5,765 |
links |
██████████████████░░ 91.0% |
192 / 211 |
wizard |
██████████████████░░ 91.1% |
4,911 / 5,393 |
mcp_analytics |
██████████████████░░ 91.2% |
4,411 / 4,837 |
canvas |
██████████████████░░ 91.3% |
5,033 / 5,514 |
conversations |
██████████████████░░ 91.3% |
21,778 / 23,859 |
visual_review |
██████████████████░░ 91.7% |
7,567 / 8,250 |
error_tracking |
██████████████████░░ 92.0% |
13,495 / 14,669 |
notifications |
██████████████████░░ 92.5% |
1,120 / 1,211 |
early_access_features |
███████████████████░ 92.6% |
1,331 / 1,438 |
managed_migrations |
███████████████████░ 92.7% |
1,576 / 1,700 |
slack_app |
███████████████████░ 92.7% |
12,684 / 13,676 |
surveys |
███████████████████░ 93.1% |
6,180 / 6,640 |
mcp_registry |
███████████████████░ 93.2% |
1,471 / 1,578 |
web_analytics |
███████████████████░ 93.3% |
19,324 / 20,706 |
stamphog |
███████████████████░ 93.4% |
6,543 / 7,009 |
context_layer |
███████████████████░ 93.4% |
2,616 / 2,801 |
mcp_store |
███████████████████░ 93.5% |
8,063 / 8,621 |
reminders |
███████████████████░ 93.5% |
477 / 510 |
marketing_analytics |
███████████████████░ 93.5% |
17,452 / 18,657 |
posthog_ai |
███████████████████░ 93.6% |
1,395 / 1,491 |
ai_observability |
███████████████████░ 93.8% |
18,255 / 19,463 |
approvals |
███████████████████░ 93.9% |
3,570 / 3,803 |
billing_alerts |
███████████████████░ 94.1% |
2,079 / 2,210 |
endpoints |
███████████████████░ 94.2% |
9,072 / 9,627 |
review_hog |
███████████████████░ 94.3% |
10,214 / 10,826 |
tracing |
███████████████████░ 94.5% |
2,726 / 2,885 |
legal_documents |
███████████████████░ 94.7% |
2,082 / 2,199 |
workflows |
███████████████████░ 94.7% |
11,801 / 12,455 |
autoresearch |
███████████████████░ 95.0% |
934 / 983 |
customer_analytics |
███████████████████░ 95.1% |
19,308 / 20,303 |
annotations |
███████████████████░ 95.3% |
804 / 844 |
product_analytics |
███████████████████░ 95.3% |
19,328 / 20,280 |
actions |
███████████████████░ 95.4% |
749 / 785 |
replay_vision |
███████████████████░ 95.5% |
22,833 / 23,898 |
skills |
███████████████████░ 95.6% |
5,536 / 5,791 |
experiments |
███████████████████░ 95.6% |
30,259 / 31,643 |
feature_flags |
███████████████████░ 95.7% |
20,361 / 21,270 |
access_control |
███████████████████░ 95.8% |
6,293 / 6,572 |
messaging |
███████████████████░ 95.9% |
3,680 / 3,839 |
growth |
███████████████████░ 96.0% |
7,728 / 8,053 |
logs |
███████████████████░ 96.0% |
13,950 / 14,532 |
revenue_analytics |
███████████████████░ 96.3% |
1,841 / 1,911 |
user_interviews |
███████████████████░ 96.5% |
2,651 / 2,747 |
warehouse_sources |
███████████████████░ 97.2% |
409,807 / 421,688 |
data_quality |
████████████████████ 97.6% |
4,229 / 4,335 |
analytics_platform |
████████████████████ 98.2% |
2,381 / 2,425 |
metrics |
████████████████████ 98.2% |
3,576 / 3,640 |
data_catalog |
████████████████████ 98.3% |
3,267 / 3,322 |
pulse |
████████████████████ 98.4% |
2,028 / 2,060 |
live_debugger |
████████████████████ 99.2% |
621 / 626 |
field_notes |
████████████████████ 99.4% |
167 / 168 |
Report-only. Patch coverage = changed backend lines covered vs origin/master. Sorted lowest first.
Known gaps: lines covered only by Temporal tests show as uncovered; core line numbers may drift if master changed the same file.
ℹ️ Docs preview — preview build triggered
Docs from this PR will be published at posthog.com.
| Project | Preview | Updated (UTC) |
|---|---|---|
| posthog.com | Open preview | Sep 8, 2026, 7:25 PM |
The preview should be ready in about 10 minutes. Open the preview at /handbook/engineering/.
Prompt To Fix All With AI### Issue 1
products/wizard/backend/tasks/__init__.py:2
**Missing reconciliation module breaks imports**
Importing `products.wizard.backend.tasks` executes this unconditional import, but `products.wizard.backend.tasks.reconciliation` is absent from the source tree, causing `ModuleNotFoundError` and preventing Wizard task registration or submodule loading.
### Issue 2
products/wizard/backend/temporal/workflows/execute_run.py:153-157
**Cleanup failures abandon active sandboxes**
When sandbox destruction fails through all cleanup attempts, this handler suppresses the activity error and allows the workflow to finalize without scheduling further cleanup, leaving the checked-out repository and `POSTHOG_WIZARD_API_KEY` active until the 75-minute sandbox TTL expires.
### Issue 3
products/wizard/backend/logic/runs/cancellation.py:12-14
**Workflow startup loses cancellation**
If cancellation overlaps dispatch after the run status check but before the workflow ID is persisted, this branch marks cancellation as dispatched without contacting Temporal; dispatch then stores the ID, leaving a run marked cancelled able to provision a worker, execute the Wizard, and publish repository changes.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "feat(wizard): execute cloud runs" | Re-trigger Greptile |
b65487c to
b35b472
Compare
PR overviewThis pull request enables the wizard backend worker to execute cloud runs, including configuring the sandbox environment and processing run output for GitHub handoff. Three issues have been addressed, but one credential-exposure risk remains. Repository-controlled code running in the sandbox can read and exfiltrate the run creator’s OAuth token, allowing an attacker to act with that user’s scoped PostHog access. The credential should be kept outside the repository-visible environment before the PR is considered secure. Open issues (1)
Fixed/addressed: 3 · PR risk: 6/10 |
b35b472 to
cf187e6
Compare
ef53413 to
eb6fbc7
Compare
🦔 PostHog Review reviewed this pull requestFound 10 must fix, 30 should fix, 10 consider. Published 50 findings. Resolved comments: 7 fixed, 1 declined, 11 left for you · couldn't handle 1 |
There was a problem hiding this comment.
Not approved yet — waiting on the conditions below.
Re-add the stamphog label to request another review once you have addressed this.
Gates denied for exceeding the size ceiling, and this is risky territory (sandbox provisioning, credential handling, repo publishing to Temporal workflows) with cross-team authorship and open bot-flagged concerns (possible missing import, cleanup/cancellation races) that lack clear resolution evidence.
- 👍 on the PR from greptile-apps[bot].
- Gate denied: 2102 substantive lines vs 800L ceiling for auto-review
- Cross-team change (author on team-growth, touches team-posthog-code files) with no independent approving review
- Greptile flagged a possibly-missing reconciliation module import, a cleanup failure path that could leave sandboxes/credentials active, and a cancellation race — no clear evidence these were fixed beyond a generic bot 'all issues addressed' comment
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✗ | too large for auto-review (2102L substantive in global — ceiling is 800L; 2102L, 35F total, 4603L/50F incl. docs/generated/snapshots) |
| tier | ✓ | T1-agent / T1d-complex (4603L, 50F, single-area, feat) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ 11b4505 · reviewed head 11b4505 |
|
PostHog Review alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
| _raise_for_failure( | ||
| "execution", | ||
| wizard_result.exit_code, | ||
| stdout=wizard_result.stdout, | ||
| stderr=wizard_result.stderr, | ||
| ) |
There was a problem hiding this comment.
Wizard failure output can leak credentials into Temporal history
Why we think it's a valid issue
- Checked: Every
_raise_for_failurecall site inservice.py, the_failure_detailredaction helper, the sandbox env built at provisioning, the activity that wraps the exception, the persistence path forerror_message, and the redaction helpers in the Tasks sandbox layer. - Found: The asymmetry is real and sits inside a control the authors built deliberately.
clone_repositorypassessensitive_values=(github_token,)atservice.py:158andservice.py:171, andproducts/wizard/backend/tests/runs/test_cloud_worker.py:125-145asserts the GitHub token is replaced with[REDACTED]in the raised exception text.execute_wizardatservice.py:219-224passes nosensitive_values, and that is the one stage wherePOSTHOG_WIZARD_API_KEYis live in the sandbox environment (service.py:346)._failure_detailatservice.py:325-330only replaces values it receives, so the wizard stage gets no redaction at all. - Found: The path to Temporal history is confirmed.
_failure_detailkeeps the last 2000 characters of combined stdout and stderr (WIZARD_ERROR_DETAIL_LENGTH = 2000inlogic/workers/config.py),WizardWorkerExecutionError.__init__appends that detail to the message (service.py:117-121), andproducts/wizard/backend/temporal/activities/execution.py:39-43passesstr(error)intoApplicationError. Raw sandbox output therefore lands in workflow history. - Found: The reviewer is correct that the existing helper does not cover this.
redact_sandbox_command(products/tasks/backend/logic/services/sandbox.py:258-260) matches onlyNAME=valueshapes and thex-access-token:clone URL form, and its callers pass command strings (docker_sandbox.py:650,modal_sandbox.py:1031,hogland_sandbox.py:325). A bare token printed on its own line survives it, so exact-value replacement — which_failure_detailalready supports — is the right mechanism. - Found: The fix needs a small amount of plumbing, not just one argument. The token is minted in
provision_wizard_worker(service.py:130) andWizardExecutionRequest(service.py:78-85) does not carry it, so the value is not in scope at the execute stage. - Impact: On a failing cloud run, any wizard or repository-script output that prints the environment or an authenticated request's headers is copied verbatim into Temporal workflow history, which outlives the credential and is readable by operators. This is the secret-leakage category, and the same file already treats this exact vector as worth blocking for the other credential.
- Priority: Lowered to
should_fix. The exposure is real but bounded: it needs a third party inside the sandbox to print the value, and nothing in this diff does. The destination is internal only —logic/runs/store.py:197derives the storederror_messagefrom the error code, so no raw output reaches the database or the run API. The token expires after 6 hours (posthog/temporal/oauth.py:197) and is scoped to one team for the run creator (oauth.py:380), so a reader of history almost always finds an expired, single-team credential. That is a defense-in-depth gap with a cheap fix, not a confirmed live credential exposure.
Issue description
execute_wizard passes raw output to _failure_detail without sensitive values. The activity puts the resulting exception text in a Temporal ApplicationError. A repository script can print POSTHOG_WIZARD_API_KEY. redact_sandbox_command only scrubs command text.
Suggested fix
Redact all Wizard output before it enters an exception. Use the exact token or a shared credential scanner. Keep raw diagnostics outside Temporal history. Add a failure test containing a fake pha_ token.
Prompt to fix with AI (copy-paste)
## Context
@products/wizard/backend/logic/workers/service.py#L219-224
@products/wizard/backend/logic/workers/service.py#L325-330
<issue_description>
`execute_wizard` passes raw output to `_failure_detail` without sensitive values. The activity puts the resulting exception text in a Temporal `ApplicationError`. A repository script can print `POSTHOG_WIZARD_API_KEY`. `redact_sandbox_command` only scrubs command text.
</issue_description>
<issue_validation>
- **Checked:** Every `_raise_for_failure` call site in `service.py`, the `_failure_detail` redaction helper, the sandbox env built at provisioning, the activity that wraps the exception, the persistence path for `error_message`, and the redaction helpers in the Tasks sandbox layer.
- **Found:** The asymmetry is real and sits inside a control the authors built deliberately. `clone_repository` passes `sensitive_values=(github_token,)` at `service.py:158` and `service.py:171`, and `products/wizard/backend/tests/runs/test_cloud_worker.py:125-145` asserts the GitHub token is replaced with `[REDACTED]` in the raised exception text. `execute_wizard` at `service.py:219-224` passes no `sensitive_values`, and that is the one stage where `POSTHOG_WIZARD_API_KEY` is live in the sandbox environment (`service.py:346`). `_failure_detail` at `service.py:325-330` only replaces values it receives, so the wizard stage gets no redaction at all.
- **Found:** The path to Temporal history is confirmed. `_failure_detail` keeps the last 2000 characters of combined stdout and stderr (`WIZARD_ERROR_DETAIL_LENGTH = 2000` in `logic/workers/config.py`), `WizardWorkerExecutionError.__init__` appends that detail to the message (`service.py:117-121`), and `products/wizard/backend/temporal/activities/execution.py:39-43` passes `str(error)` into `ApplicationError`. Raw sandbox output therefore lands in workflow history.
- **Found:** The reviewer is correct that the existing helper does not cover this. `redact_sandbox_command` (`products/tasks/backend/logic/services/sandbox.py:258-260`) matches only `NAME=value` shapes and the `x-access-token:` clone URL form, and its callers pass command strings (`docker_sandbox.py:650`, `modal_sandbox.py:1031`, `hogland_sandbox.py:325`). A bare token printed on its own line survives it, so exact-value replacement — which `_failure_detail` already supports — is the right mechanism.
- **Found:** The fix needs a small amount of plumbing, not just one argument. The token is minted in `provision_wizard_worker` (`service.py:130`) and `WizardExecutionRequest` (`service.py:78-85`) does not carry it, so the value is not in scope at the execute stage.
- **Impact:** On a failing cloud run, any wizard or repository-script output that prints the environment or an authenticated request's headers is copied verbatim into Temporal workflow history, which outlives the credential and is readable by operators. This is the secret-leakage category, and the same file already treats this exact vector as worth blocking for the other credential.
- **Priority:** Lowered to `should_fix`. The exposure is real but bounded: it needs a third party inside the sandbox to print the value, and nothing in this diff does. The destination is internal only — `logic/runs/store.py:197` derives the stored `error_message` from the error code, so no raw output reaches the database or the run API. The token expires after 6 hours (`posthog/temporal/oauth.py:197`) and is scoped to one team for the run creator (`oauth.py:380`), so a reader of history almost always finds an expired, single-team credential. That is a defense-in-depth gap with a cheap fix, not a confirmed live credential exposure.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Redact all Wizard output before it enters an exception. Use the exact token or a shared credential scanner. Keep raw diagnostics outside Temporal history. Add a failure test containing a fake `pha_` token.
</potential_solution>
There was a problem hiding this comment.
Escalating for a human decision rather than fixing this unattended — it is credential-redaction (secrets-handling) code, and the fix is a design choice with a real trap, not a mechanical change.
The report is accurate: the wizard execution stage calls the failure helper without any sensitive values, so if a repository script prints POSTHOG_WIZARD_API_KEY the raw output can reach the exception message and, from there, Temporal workflow history. The clone stage already guards its GitHub token; the wizard stage does not.
The catch is how to redact it safely. The wizard token is minted at provisioning and only exists as a sandbox environment variable — it is not in scope at the execute stage. The tempting plumbing (carry the token through the execution request) would route it through the provisioning activity's output and the execute activity's input, both of which Temporal records in history. That would write the credential into history on every run, which is worse than today's failure-only, someone-has-to-print-it exposure, and it contradicts this PR's stated goal of keeping credentials out of Temporal history.
A maintainer should choose between: (a) re-reading the token from the sandbox environment at execute time and passing it as a sensitive value to redact — keeps it out of history but adds a sandbox read; or (b) a shared output redactor that scrubs environment-variable values / known token formats regardless of which stage produced them. Either is reasonable; the security trade-off is what needs a human. Whatever is chosen must not carry the raw token across the workflow boundary.
| SENSITIVE_SANDBOX_ENV_NAMES: frozenset[str] = frozenset( | ||
| { | ||
| "GITHUB_TOKEN", | ||
| "POSTHOG_TASK_RUN_EVENT_INGEST_TOKEN", | ||
| "POSTHOG_TASK_RUN_SESSION_TOKEN", | ||
| "POSTHOG_WIZARD_API_KEY", | ||
| } |
There was a problem hiding this comment.
The redactor misses the GH_TOKEN alias
Why we think it's a valid issue
- Checked: Every producer of a
GH_TOKENenvironment entry, howSandboxConfig.environment_variablesreaches a logged string in each sandbox backend, the regex behavior for the two names, which backend runs in production, and the existing redaction tests. - Found: The alias assignment is real and sets the identical value.
products/tasks/backend/temporal/process_task/activities/provision_sandbox.py:508-509setsenvironment_variables["GITHUB_TOKEN"]andenvironment_variables["GH_TOKEN"]to the samegithub_token, andproducts/tasks/backend/temporal/process_task/utils.py:1298-1299does the same inbuild_sandbox_environment_variables.products/tasks/backend/temporal/process_task/activities/get_sandbox_for_repository.py:142also setsGH_TOKEN. - Found: The path into a redacted string is confirmed for the Docker backend.
products/tasks/backend/logic/services/docker_sandbox.py:486-491expands every entry into-e KEY=VALUEarguments, anddocker_sandbox.py:231logsredact_sandbox_command(' '.join(args)). So one log line carries both-e GITHUB_TOKEN=<secret>and-e GH_TOKEN=<secret>. - Found: No accidental match saves it. The pattern at
sandbox.py:241-243alternates over the names in the frozenset, andGH_TOKENis not a substring ofGITHUB_TOKEN. The redactor masks the first argument and prints the same secret verbatim in the second, so the control is fully defeated for that one credential rather than partially. - Found: No test catches the gap.
products/tasks/backend/logic/services/test_docker_sandbox.py:137-143parameterizes thedocker run -e NAME=...case overGITHUB_TOKENandPOSTHOG_WIZARD_API_KEYonly. - Found: The codebase already pairs the two names elsewhere, which supports the suggestion to centralize them:
products/tasks/backend/constants.py:521-522lists both inRESERVED_SANDBOX_ENVIRONMENT_VARIABLE_KEYS, andconstants.py:572-573lists both again in the blocked-keys tuple.products/tasks/backend/logic/services/agentsh.py:216andtemporal/process_task/sandbox_credentials.py:43treat them as one alias set too. - Impact: A GitHub installation token is written unmasked into the sandbox command log whenever the Docker backend starts a container, which nullifies the redaction applied to its twin argument on the same line.
- Priority: Lowered to
consider. The exposure does not reach production.get_sandbox_classdefaults to Modal (products/tasks/backend/logic/services/sandbox.py:838-841) and returns Docker only forSANDBOX_PROVIDER="docker"(sandbox.py:819-820), and the production backends never route env values through the redactor —modal_sandbox.py:796-797andhogland_sandbox.py:220hand the env to the provider API as a dict, while theirredact_sandbox_commandcalls cover execute commands only. The leaking call is alsologger.debug. The result is a real but one-line hygiene gap whose blast radius is local development and CI debug logs, in the same constant this PR edits.
Issue description
The sensitive-name set includes GITHUB_TOKEN but omits GH_TOKEN. Sandbox setup assigns the same credential to both names. Docker command logging therefore redacts one argument but exposes the other.
Suggested fix
Add GH_TOKEN to SENSITIVE_SANDBOX_ENV_NAMES. Centralize GitHub credential aliases in one constant. Add a redaction test for every alias used by sandbox configuration.
Prompt to fix with AI (copy-paste)
## Context
@products/tasks/backend/logic/services/sandbox.py#L233-239
<issue_description>
The sensitive-name set includes `GITHUB_TOKEN` but omits `GH_TOKEN`. Sandbox setup assigns the same credential to both names. Docker command logging therefore redacts one argument but exposes the other.
</issue_description>
<issue_validation>
- **Checked:** Every producer of a `GH_TOKEN` environment entry, how `SandboxConfig.environment_variables` reaches a logged string in each sandbox backend, the regex behavior for the two names, which backend runs in production, and the existing redaction tests.
- **Found:** The alias assignment is real and sets the identical value. `products/tasks/backend/temporal/process_task/activities/provision_sandbox.py:508-509` sets `environment_variables["GITHUB_TOKEN"]` and `environment_variables["GH_TOKEN"]` to the same `github_token`, and `products/tasks/backend/temporal/process_task/utils.py:1298-1299` does the same in `build_sandbox_environment_variables`. `products/tasks/backend/temporal/process_task/activities/get_sandbox_for_repository.py:142` also sets `GH_TOKEN`.
- **Found:** The path into a redacted string is confirmed for the Docker backend. `products/tasks/backend/logic/services/docker_sandbox.py:486-491` expands every entry into `-e KEY=VALUE` arguments, and `docker_sandbox.py:231` logs `redact_sandbox_command(' '.join(args))`. So one log line carries both `-e GITHUB_TOKEN=<secret>` and `-e GH_TOKEN=<secret>`.
- **Found:** No accidental match saves it. The pattern at `sandbox.py:241-243` alternates over the names in the frozenset, and `GH_TOKEN` is not a substring of `GITHUB_TOKEN`. The redactor masks the first argument and prints the same secret verbatim in the second, so the control is fully defeated for that one credential rather than partially.
- **Found:** No test catches the gap. `products/tasks/backend/logic/services/test_docker_sandbox.py:137-143` parameterizes the `docker run -e NAME=...` case over `GITHUB_TOKEN` and `POSTHOG_WIZARD_API_KEY` only.
- **Found:** The codebase already pairs the two names elsewhere, which supports the suggestion to centralize them: `products/tasks/backend/constants.py:521-522` lists both in `RESERVED_SANDBOX_ENVIRONMENT_VARIABLE_KEYS`, and `constants.py:572-573` lists both again in the blocked-keys tuple. `products/tasks/backend/logic/services/agentsh.py:216` and `temporal/process_task/sandbox_credentials.py:43` treat them as one alias set too.
- **Impact:** A GitHub installation token is written unmasked into the sandbox command log whenever the Docker backend starts a container, which nullifies the redaction applied to its twin argument on the same line.
- **Priority:** Lowered to `consider`. The exposure does not reach production. `get_sandbox_class` defaults to Modal (`products/tasks/backend/logic/services/sandbox.py:838-841`) and returns Docker only for `SANDBOX_PROVIDER="docker"` (`sandbox.py:819-820`), and the production backends never route env values through the redactor — `modal_sandbox.py:796-797` and `hogland_sandbox.py:220` hand the env to the provider API as a dict, while their `redact_sandbox_command` calls cover execute commands only. The leaking call is also `logger.debug`. The result is a real but one-line hygiene gap whose blast radius is local development and CI debug logs, in the same constant this PR edits.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Add `GH_TOKEN` to `SENSITIVE_SANDBOX_ENV_NAMES`. Centralize GitHub credential aliases in one constant. Add a redaction test for every alias used by sandbox configuration.
</potential_solution>
There was a problem hiding this comment.
Escalating for a maintainer rather than fixing unattended — this changes the credential-redaction control (secrets handling), which I don't modify without a human owning it, even for a change this small.
The finding is correct and reproducible in the current code: sandbox setup assigns the same GitHub token to both GITHUB_TOKEN and GH_TOKEN, but the sensitive-name set used for Docker command-log redaction lists only GITHUB_TOKEN. So a docker run log line masks -e GITHUB_TOKEN=... and prints -e GH_TOKEN=<secret> in the clear. As the report notes, the exposure is bounded to the Docker backend, which is local-dev/CI only (production uses Modal/Hogland, which don't route env values through this redactor), and the log is at debug level.
Recommended fix for whoever picks this up, so it's low-effort: add GH_TOKEN to SENSITIVE_SANDBOX_ENV_NAMES, and ideally source both names from the existing GITHUB_ENV_KEYS = ("GITHUB_TOKEN", "GH_TOKEN") in temporal/process_task/sandbox_credentials.py so the alias pair lives in one place. Then extend the redaction test in test_docker_sandbox.py (currently parameterized over GITHUB_TOKEN and POSTHOG_WIZARD_API_KEY only) to cover GH_TOKEN.
| @activity.defn(name="wizard_destroy_worker") | ||
| @asyncify | ||
| def destroy_worker(input: ProvisionedWizardWorker) -> None: | ||
| worker_lifecycle.cleanup_worker(input.team_id, input.run_id, input.sandbox_id) |
There was a problem hiding this comment.
Worker cleanup does not revoke its OAuth token
Why we think it's a valid issue
- Checked: the mint site (
provision_wizard_workerat products/wizard/backend/logic/workers/service.py:128-145,_build_sandbox_configat :342-350), the token helper (create_wizard_oauth_access_token_for_userand_mint_oauth_access_tokenat posthog/temporal/oauth.py:369-430), the cleanup chain (destroy_workerat products/wizard/backend/temporal/activities/workspace.py:132-135 →cleanup_workerat logic/workers/lifecycle.py:22-32 →_destroy_workerat :50-59), and everyOAuthAccessTokendelete site in the repository. - Found: the premise holds.
provision_wizard_workerreturns onlysandbox_idand resource usage (service.py:136-145), so no token row identifier survives the activity.cleanup_workermeasures usage, destroys the sandbox, and records state; it touches no token. No code underproducts/wizard/referencesOAuthAccessToken. - Found: no revoke helper exists to call.
posthog/temporal/oauth.pydefines minting only. The single sandbox-token deletion in a sibling product isproducts/tasks/backend/facade/api.py:5946, and it fires on task handoff (an ownership change, per the docstring at :5903), not on sandbox teardown. Tasks mints through the same_mint_oauth_access_tokenhelper and also lets its run tokens live out the expiry. The requested fix therefore means new persistence plus revoke calls on each path, which this product would be first to build. - Found: the blast radius stays inside one tenant.
_mint_oauth_access_tokensetsscoped_teams=[team_id](posthog/temporal/oauth.py:380) andTOKEN_EXPIRATION_SECONDSis 6 hours (:197).authorize_git_repository_access(workspace.py:98) resolves the repository through that same team's GitHub integration, so the project the token reaches and the repository the run edits belong to the same customer. - Impact: confirmed but mild. The token stays usable for up to 6 hours after the run ends, which widens the window a leaked credential is good for. To use it, an actor needs execution inside that run's sandbox while it runs, and that actor already sits inside the tenant that owns both the repository and the project. No cross-tenant escalation follows, and the expiry is the same mitigation every other sandbox token in this codebase relies on.
- Priority: lowered to
consider. Revoking a credential when its consumer dies is sound hardening and the finding names a genuine omission, so it stays on record. It is notmust_fix, because the code introduces no exploitable hole beyond the existing convention, the token is short-lived and team-scoped, and the fix adds new machinery rather than calling something that already exists.
Issue description
The provision service creates a Wizard OAuth token with a six-hour expiry. The activity does not retain a safe token identifier. destroy_worker removes the sandbox but does not revoke the token. A copied token remains valid after completion or cancellation.
Suggested fix
Bind each token to its Wizard run without storing the token value in Temporal. Persist only the token row ID. Revoke it on provisioning failure and every cleanup path. Revoke it even when sandbox destruction fails.
Prompt to fix with AI (copy-paste)
## Context
@products/wizard/backend/temporal/activities/workspace.py#L132-135
<issue_description>
The provision service creates a Wizard OAuth token with a six-hour expiry. The activity does not retain a safe token identifier. destroy_worker removes the sandbox but does not revoke the token. A copied token remains valid after completion or cancellation.
</issue_description>
<issue_validation>
- **Checked:** the mint site (`provision_wizard_worker` at products/wizard/backend/logic/workers/service.py:128-145, `_build_sandbox_config` at :342-350), the token helper (`create_wizard_oauth_access_token_for_user` and `_mint_oauth_access_token` at posthog/temporal/oauth.py:369-430), the cleanup chain (`destroy_worker` at products/wizard/backend/temporal/activities/workspace.py:132-135 → `cleanup_worker` at logic/workers/lifecycle.py:22-32 → `_destroy_worker` at :50-59), and every `OAuthAccessToken` delete site in the repository.
- **Found:** the premise holds. `provision_wizard_worker` returns only `sandbox_id` and resource usage (service.py:136-145), so no token row identifier survives the activity. `cleanup_worker` measures usage, destroys the sandbox, and records state; it touches no token. No code under `products/wizard/` references `OAuthAccessToken`.
- **Found:** no revoke helper exists to call. `posthog/temporal/oauth.py` defines minting only. The single sandbox-token deletion in a sibling product is `products/tasks/backend/facade/api.py:5946`, and it fires on task **handoff** (an ownership change, per the docstring at :5903), not on sandbox teardown. Tasks mints through the same `_mint_oauth_access_token` helper and also lets its run tokens live out the expiry. The requested fix therefore means new persistence plus revoke calls on each path, which this product would be first to build.
- **Found:** the blast radius stays inside one tenant. `_mint_oauth_access_token` sets `scoped_teams=[team_id]` (posthog/temporal/oauth.py:380) and `TOKEN_EXPIRATION_SECONDS` is 6 hours (:197). `authorize_git_repository_access` (workspace.py:98) resolves the repository through that same team's GitHub integration, so the project the token reaches and the repository the run edits belong to the same customer.
- **Impact:** confirmed but mild. The token stays usable for up to 6 hours after the run ends, which widens the window a leaked credential is good for. To use it, an actor needs execution inside that run's sandbox while it runs, and that actor already sits inside the tenant that owns both the repository and the project. No cross-tenant escalation follows, and the expiry is the same mitigation every other sandbox token in this codebase relies on.
- **Priority:** lowered to `consider`. Revoking a credential when its consumer dies is sound hardening and the finding names a genuine omission, so it stays on record. It is not `must_fix`, because the code introduces no exploitable hole beyond the existing convention, the token is short-lived and team-scoped, and the fix adds new machinery rather than calling something that already exists.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Bind each token to its Wizard run without storing the token value in Temporal. Persist only the token row ID. Revoke it on provisioning failure and every cleanup path. Revoke it even when sandbox destruction fails.
</potential_solution>
There was a problem hiding this comment.
Escalating for a maintainer rather than fixing unattended. This is OAuth token-lifecycle code (auth/secrets handling), and the fix is new machinery, not a contained change — so a human should own the design and the decision.
The finding is accurate against the current code: the provisioning step mints a Wizard OAuth token with a six-hour expiry, but nothing retains a token identifier and the cleanup path only tears down the sandbox, so the token stays usable for the rest of its TTL after the run finishes or is cancelled. As the report notes, the exposure is bounded — the token is scoped to the run's single team and expires in six hours, an attacker would need code execution inside that tenant's own sandbox, and the sibling Tasks product relies on the same expire-only convention.
What a maintainer needs to decide and build: (1) a revoke helper alongside the minting code in the shared OAuth module (none exists today); (2) how to bind each token to its run — persist only the token row ID, never the token value, and keep it out of Temporal history; and (3) revoke on provisioning failure and on every cleanup path, including when sandbox destruction itself fails (today that path raises before finishing). Because this is the first place in the codebase to revoke a sandbox token on teardown, it also sets a convention worth applying consistently, which is a call for the team rather than an automated edit.
| FINALIZATION_TIMEOUT = timedelta(minutes=1) | ||
| PROVISION_TIMEOUT = timedelta(minutes=5) | ||
| PREPARATION_TIMEOUT = timedelta(minutes=10) | ||
| EXECUTION_TIMEOUT = timedelta(minutes=50) | ||
| HANDOFF_TIMEOUT = timedelta(minutes=5) | ||
| LOCAL_PACKAGE_PREPARATION_TIMEOUT = timedelta(minutes=20) | ||
| CLEANUP_TIMEOUT = timedelta(minutes=5) | ||
| FINALIZATION_RETRY_POLICY = RetryPolicy(maximum_attempts=3) | ||
| PROVISION_RETRY_POLICY = RetryPolicy(maximum_attempts=1) | ||
| PREPARATION_RETRY_POLICY = RetryPolicy(maximum_attempts=3) | ||
| EXECUTION_RETRY_POLICY = RetryPolicy(maximum_attempts=1) | ||
| HANDOFF_RETRY_POLICY = RetryPolicy(maximum_attempts=3) | ||
| LOCAL_PACKAGE_PREPARATION_RETRY_POLICY = RetryPolicy(maximum_attempts=1) | ||
| CLEANUP_RETRY_POLICY = RetryPolicy(maximum_attempts=3) |
There was a problem hiding this comment.
Retry budgets exceed the sandbox lifetime
Why we think it's a valid issue
- Checked: every timeout and retry policy in
products/wizard/backend/temporal/config.py, the sandbox TTL, the stamped run deadline and what enforces it, the argumentsstart_workflowactually passes, and the repo's precedent for bounding queue wait. - Found: the arithmetic holds. Per-attempt
start_to_close_timeoutvalues multiplied bymaximum_attemptsgive provision 5x1, preparation 10x3 = 30, execution 50x1, handoff 5x3 = 15, cleanup 5x3 = 15, finalization 1x3 = 3 — 118 minutes worst case, before retry backoff, and 138 whenuse_local_wizard_sourceadds the 20 minute local package step. Noschedule_to_close_timeoutorschedule_to_start_timeoutappears anywhere in the product, so nothing caps the aggregate. - Found: both cited ceilings are real and lower than that sum.
SANDBOX_TTL_SECONDS = 75 * 60(products/wizard/backend/logic/workers/config.py:31) maps to Modal'stimeoutatproducts/tasks/backend/logic/services/modal_sandbox.py:808, so the provider kills the box at 75 minutes.WIZARD_RUN_DEADLINE = timedelta(minutes=85)(products/wizard/backend/facade/config.py:6) is stamped onto every cloud run asdeadline_atatproducts/wizard/backend/logic/runs/store.py:76. - Found: the missing workflow timeout is confirmed.
products/wizard/backend/temporal/client.py:24-30passesid,id_reuse_policy,task_queue, andretry_policyonly. The repo already treats this as a known hazard:products/mcp_analytics/backend/facade/api.py:179-189setsexecution_timeoutwith the comment that it "bounds the whole run including queue wait: with no worker polling the task queue, an unbounded workflow sits pending forever", and theCANVAS_BUILD_TASK_QUEUEnote inposthog/settings/temporal.pysays stranded work stays open "until the workflow execution timeout closes them". - Found: the 85 minute deadline is declarative today, which sharpens rather than weakens the point.
deadline_athas a partial index built for a sweeper (products/wizard/backend/models.py:162-164) and observability is wired (run_past_deadlineatproducts/wizard/backend/observability/service.py:215,posthog_wizard_runs_past_deadline_totalatobservability/metrics.py:85), but the only caller ofrun_past_deadlineis a test, and the intended enforcerwizard.reconcile_runsis still an unimplemented string atproducts/wizard/backend/tasks/config.py:1. So nothing stops a run at 85 minutes while the timeout budget allows 118. - Impact: a retried step can start against a sandbox the provider already destroyed. The concrete loss case is handoff:
create_run_artifactscaptures the diff and publishes the pull request, and its three 5 minute attempts can straddle the 75 minute TTL, so a run whose wizard execution succeeded fails with no diff and no pull request. - Impact: with no
execution_timeoutand noschedule_to_close_timeout,start_to_close_timeoutalone never fires while an activity task waits in the queue. A queue with no poller therefore leaves the run inRUNNINGindefinitely and it never reaches a terminal status — which matters here, because this stack does not yet defineWIZARD_TASK_QUEUEor register the wizard workflow instart_temporal_worker.py.
Issue description
The retry budgets permit 30 minutes for preparation, 50 for execution, 15 for handoff, and 15 for cleanup. This exceeds the sandbox's 75-minute TTL and the run's 85-minute deadline. Later attempts can start after the sandbox expires. The client also sets no workflow execution timeout.
Suggested fix
Set execution_timeout when starting the workflow. Add schedule_to_close_timeout to retried activities. Derive these limits from one run budget. Reserve time for finalization and cleanup before the sandbox TTL.
Prompt to fix with AI (copy-paste)
## Context
@products/wizard/backend/temporal/config.py#L5-18
<issue_description>
The retry budgets permit 30 minutes for preparation, 50 for execution, 15 for handoff, and 15 for cleanup. This exceeds the sandbox's 75-minute TTL and the run's 85-minute deadline. Later attempts can start after the sandbox expires. The client also sets no workflow execution timeout.
</issue_description>
<issue_validation>
- **Checked:** every timeout and retry policy in `products/wizard/backend/temporal/config.py`, the sandbox TTL, the stamped run deadline and what enforces it, the arguments `start_workflow` actually passes, and the repo's precedent for bounding queue wait.
- **Found:** the arithmetic holds. Per-attempt `start_to_close_timeout` values multiplied by `maximum_attempts` give provision 5x1, preparation 10x3 = 30, execution 50x1, handoff 5x3 = 15, cleanup 5x3 = 15, finalization 1x3 = 3 — 118 minutes worst case, before retry backoff, and 138 when `use_local_wizard_source` adds the 20 minute local package step. No `schedule_to_close_timeout` or `schedule_to_start_timeout` appears anywhere in the product, so nothing caps the aggregate.
- **Found:** both cited ceilings are real and lower than that sum. `SANDBOX_TTL_SECONDS = 75 * 60` (`products/wizard/backend/logic/workers/config.py:31`) maps to Modal's `timeout` at `products/tasks/backend/logic/services/modal_sandbox.py:808`, so the provider kills the box at 75 minutes. `WIZARD_RUN_DEADLINE = timedelta(minutes=85)` (`products/wizard/backend/facade/config.py:6`) is stamped onto every cloud run as `deadline_at` at `products/wizard/backend/logic/runs/store.py:76`.
- **Found:** the missing workflow timeout is confirmed. `products/wizard/backend/temporal/client.py:24-30` passes `id`, `id_reuse_policy`, `task_queue`, and `retry_policy` only. The repo already treats this as a known hazard: `products/mcp_analytics/backend/facade/api.py:179-189` sets `execution_timeout` with the comment that it "bounds the whole run *including* queue wait: with no worker polling the task queue, an unbounded workflow sits pending forever", and the `CANVAS_BUILD_TASK_QUEUE` note in `posthog/settings/temporal.py` says stranded work stays open "until the workflow execution timeout closes them".
- **Found:** the 85 minute deadline is declarative today, which sharpens rather than weakens the point. `deadline_at` has a partial index built for a sweeper (`products/wizard/backend/models.py:162-164`) and observability is wired (`run_past_deadline` at `products/wizard/backend/observability/service.py:215`, `posthog_wizard_runs_past_deadline_total` at `observability/metrics.py:85`), but the only caller of `run_past_deadline` is a test, and the intended enforcer `wizard.reconcile_runs` is still an unimplemented string at `products/wizard/backend/tasks/config.py:1`. So nothing stops a run at 85 minutes while the timeout budget allows 118.
- **Impact:** a retried step can start against a sandbox the provider already destroyed. The concrete loss case is handoff: `create_run_artifacts` captures the diff and publishes the pull request, and its three 5 minute attempts can straddle the 75 minute TTL, so a run whose wizard execution succeeded fails with no diff and no pull request.
- **Impact:** with no `execution_timeout` and no `schedule_to_close_timeout`, `start_to_close_timeout` alone never fires while an activity task waits in the queue. A queue with no poller therefore leaves the run in `RUNNING` indefinitely and it never reaches a terminal status — which matters here, because this stack does not yet define `WIZARD_TASK_QUEUE` or register the wizard workflow in `start_temporal_worker.py`.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Set `execution_timeout` when starting the workflow. Add `schedule_to_close_timeout` to retried activities. Derive these limits from one run budget. Reserve time for finalization and cleanup before the sandbox TTL.
</potential_solution>
| except asyncio.CancelledError: | ||
| await self._destroy_worker(worker) | ||
|
|
||
| await self._finalize_run( | ||
| WizardRunFinalizationActivityInput( | ||
| team_id=input.team_id, | ||
| run_id=input.run_id, | ||
| status=WizardRunStatus.CANCELLED, | ||
| ) | ||
| ) | ||
| raise | ||
| except ActivityError as error: | ||
| await self._destroy_worker(worker) | ||
|
|
||
| await self._finalize_run( | ||
| WizardRunFinalizationActivityInput( | ||
| team_id=input.team_id, | ||
| run_id=input.run_id, | ||
| status=WizardRunStatus.FAILED, | ||
| error_code=wizard_run_error_code(error), | ||
| ) | ||
| ) | ||
| raise | ||
| except Exception: | ||
| workflow.logger.exception( | ||
| "wizard_run_workflow_failed", | ||
| extra={"team_id": input.team_id, "run_id": str(input.run_id)}, | ||
| ) | ||
| await self._destroy_worker(worker) | ||
|
|
||
| await self._finalize_run( | ||
| WizardRunFinalizationActivityInput( | ||
| team_id=input.team_id, | ||
| run_id=input.run_id, | ||
| status=WizardRunStatus.FAILED, | ||
| error_code=WizardRunErrorCode.EXECUTION_FAILED, | ||
| ) | ||
| ) | ||
| raise |
There was a problem hiding this comment.
Finalization failure can leave runs active
Why we think it's a valid issue
- Checked: the three failure handlers and their
raisestatements,_finalize_runversus_destroy_worker, the real retry budget behindFINALIZATION_RETRY_POLICY, the transition rules, and every path that could later write a terminal status. - Found: the asymmetry is in the code.
_destroy_workerwraps its activity intry/except ActivityErrorand logs withteam_idandrun_id(execute_run.py:148-159), while_finalize_run(execute_run.py:136-142) has no guard and no logging. So a finalization failure raises out of the handler at line 96, 107, or 123, theraiseon the following line never runs, and the workflow closes with the finalizationActivityErrorinstead of the original cause. In the cancellation branch that also makes Temporal record the run as Failed rather than Canceled. - Found: the retry budget is far narrower than "three attempts" suggests.
RetryPolicy(maximum_attempts=3)inheritsinitial_interval=1sandbackoff_coefficient=2.0, which I confirmed by constructing it — so the retries land about 1 and 2 seconds after each failure. Roughly 3 seconds of backoff covers the whole budget, which a Postgres failover, a deploy-time connection reset, pool exhaustion, or a lock wait exceeds routinely. - Found: nothing repairs the row afterwards.
WORKFLOW_RETRY_POLICY = RetryPolicy(maximum_attempts=1)atproducts/wizard/backend/temporal/client.py:17rules out a workflow retry.wizard.reconcile_runsis only a string atproducts/wizard/backend/tasks/config.py:1, with no task and no beat entry.run_past_deadlineatproducts/wizard/backend/observability/service.py:215has a test as its only caller, andruns/store.pydefines no stale-run or past-deadline selector, so the partial index atproducts/wizard/backend/models.py:162-164feeds nothing. - Found: there is also a deterministic exhaustion path the finding does not name.
_ALLOWED_STATUS_TRANSITIONSatproducts/wizard/backend/logic/runs/transitions.py:4-13omits(CANCELLED, FAILED), so CANCELLED is terminal.cancel_runwrites CANCELLED to the row before the workflow reacts (products/wizard/backend/logic/runs/lifecycle.py:178-183), and cancellation cannot stop the threadedexecute_wizard, so that activity can still fail afterwards and drive_finalize_run(FAILED).transition_cloud_runthen raisesIllegalStatusTransitionErroron every attempt and burns all three. That case strands nothing, because the row is already terminal, but it masks the original error and fails the workflow spuriously. - Impact: a database blip of more than a few seconds during finalization leaves the row in
RUNNINGpermanently. The run shows as in progress forever with no artifacts and no failure, and becausetransition_runemits observability only after a successful transition (lifecycle.py:214-223), the terminal counters and events never fire either, so the stuck run is invisible to the product's own metrics.
Issue description
_finalize_run stops after three attempts. Its ActivityError then replaces the original execution error or cancellation. The workflow closes without writing a terminal run status. No current reconciliation task repairs that state, so the run can remain running.
Suggested fix
Retry terminal-state persistence until the workflow deadline, or ship the run reconciler before activation. Log finalization failures with team_id and run_id. Re-raise the original workflow error after finalization succeeds.
Prompt to fix with AI (copy-paste)
## Context
@products/wizard/backend/temporal/workflows/execute_run.py#L93-131
@products/wizard/backend/temporal/workflows/execute_run.py#L135-142
<issue_description>
`_finalize_run` stops after three attempts. Its `ActivityError` then replaces the original execution error or cancellation. The workflow closes without writing a terminal run status. No current reconciliation task repairs that state, so the run can remain `running`.
</issue_description>
<issue_validation>
- **Checked:** the three failure handlers and their `raise` statements, `_finalize_run` versus `_destroy_worker`, the real retry budget behind `FINALIZATION_RETRY_POLICY`, the transition rules, and every path that could later write a terminal status.
- **Found:** the asymmetry is in the code. `_destroy_worker` wraps its activity in `try` / `except ActivityError` and logs with `team_id` and `run_id` (`execute_run.py:148-159`), while `_finalize_run` (`execute_run.py:136-142`) has no guard and no logging. So a finalization failure raises out of the handler at line 96, 107, or 123, the `raise` on the following line never runs, and the workflow closes with the finalization `ActivityError` instead of the original cause. In the cancellation branch that also makes Temporal record the run as Failed rather than Canceled.
- **Found:** the retry budget is far narrower than "three attempts" suggests. `RetryPolicy(maximum_attempts=3)` inherits `initial_interval=1s` and `backoff_coefficient=2.0`, which I confirmed by constructing it — so the retries land about 1 and 2 seconds after each failure. Roughly 3 seconds of backoff covers the whole budget, which a Postgres failover, a deploy-time connection reset, pool exhaustion, or a lock wait exceeds routinely.
- **Found:** nothing repairs the row afterwards. `WORKFLOW_RETRY_POLICY = RetryPolicy(maximum_attempts=1)` at `products/wizard/backend/temporal/client.py:17` rules out a workflow retry. `wizard.reconcile_runs` is only a string at `products/wizard/backend/tasks/config.py:1`, with no task and no beat entry. `run_past_deadline` at `products/wizard/backend/observability/service.py:215` has a test as its only caller, and `runs/store.py` defines no stale-run or past-deadline selector, so the partial index at `products/wizard/backend/models.py:162-164` feeds nothing.
- **Found:** there is also a deterministic exhaustion path the finding does not name. `_ALLOWED_STATUS_TRANSITIONS` at `products/wizard/backend/logic/runs/transitions.py:4-13` omits `(CANCELLED, FAILED)`, so CANCELLED is terminal. `cancel_run` writes CANCELLED to the row before the workflow reacts (`products/wizard/backend/logic/runs/lifecycle.py:178-183`), and cancellation cannot stop the threaded `execute_wizard`, so that activity can still fail afterwards and drive `_finalize_run(FAILED)`. `transition_cloud_run` then raises `IllegalStatusTransitionError` on every attempt and burns all three. That case strands nothing, because the row is already terminal, but it masks the original error and fails the workflow spuriously.
- **Impact:** a database blip of more than a few seconds during finalization leaves the row in `RUNNING` permanently. The run shows as in progress forever with no artifacts and no failure, and because `transition_run` emits observability only after a successful transition (`lifecycle.py:214-223`), the terminal counters and events never fire either, so the stuck run is invisible to the product's own metrics.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Retry terminal-state persistence until the workflow deadline, or ship the run reconciler before activation. Log finalization failures with `team_id` and `run_id`. Re-raise the original workflow error after finalization succeeds.
</potential_solution>
|
|
||
| _stage_all(sandbox, repository_path) |
There was a problem hiding this comment.
Publishing can linearize an unfinished Git operation
Why we think it's a valid issue
- Checked: what the sandbox git guard actually blocks, what refs exist in the wizard's clone, what runs inside the sandbox between clone and publish, and whether a sibling signed-commit path in this repo already makes this check.
- Found: the guard description is accurate but not the operative factor.
products/tasks/backend/sandbox/images/git-guard.shintercepts onlycommitandpushand passes every other subcommand through, so it neither causes nor prevents a pending operation. - Found: the mechanism is correct if the state ever arose.
git add --allsucceeds during a conflicted or--no-commitmerge,_head_shaatrepository_publisher.py:77-81returns the pre-merge HEAD, and_staged_changesatrepository_publisher.py:88-96diffs the merged index against that HEAD, so the incoming branch's content becomes additions.createCommitOnBranchtakes no parent list, so_commit_staged_changesatrepository_publisher.py:143-158would flatten it into one ordinary commit on the wizard branch. - Found: the precondition cannot arise from the clone.
clone_repositorypassesshallow=Trueatproducts/wizard/backend/logic/workers/service.py:151, andSandboxBase.clone_repositorybuildsgit clone --single-branch --depth 1atproducts/tasks/backend/logic/services/sandbox.py:485-496. The working copy therefore holds one branch and one commit, with no second ref to merge, rebase onto, or cherry-pick from. A script would first have to fetch one. - Found: nothing in the run performs such an operation.
build_wizard_commandatproducts/wizard/backend/logic/workers/commands.py:21-67installs the wizard outside the repository with--ignore-scripts, then runs the headless setup CLI withcd <repo>. Its job is to write SDK configuration. The finding names no script that starts a merge, and I found none. - Found: the check is cheap and already exists elsewhere in this repo.
products/desktop/packages/git/src/queries.ts:459-466detectsrebase-merge,rebase-apply,MERGE_HEAD, andCHERRY_PICK_HEAD, andproducts/desktop/packages/git/src/signed-commit.ts:135refuses a signed commit while one is in progress. That path guards a developer's own machine, where mid-merge state is routine, which is the opposite of this sandbox. - Impact: confirmed as a real gap in reasoning, with no reachable trigger today. If it ever fired, a customer would get a pull request carrying code from another branch under a wizard commit message, which is why the mechanism is worth recording.
- Priority: lowered to
consider.must_fixdoes not hold, because the clone contains nothing to merge and no component in the run starts a merge, rebase, or cherry-pick. It is not dismissed outright, because the sandbox does execute the user's own project install scripts, so the invariant rests on behavior rather than on structure, and because the marker check is a few lines that a sibling signed-commit implementation already makes.
Issue description
The sandbox Git guard permits merge, rebase, and cherry-pick. A repository script can leave one operation after it stages the resolved files. _stage_all then succeeds. createCommitOnBranch creates only a single-parent commit. This converts the pending operation into a normal Wizard commit. The pull request can then include unrelated changes from another branch.
Suggested fix
Before _stage_all, use git rev-parse --git-path to resolve the operation markers. Reject the publish if MERGE_HEAD, CHERRY_PICK_HEAD, rebase-merge, or rebase-apply exists. Add regression tests with git merge --no-commit and an interrupted rebase.
Prompt to fix with AI (copy-paste)
## Context
@products/wizard/backend/logic/workers/repository_publisher.py#L48-49
<issue_description>
The sandbox Git guard permits merge, rebase, and cherry-pick. A repository script can leave one operation after it stages the resolved files. `_stage_all` then succeeds. `createCommitOnBranch` creates only a single-parent commit. This converts the pending operation into a normal Wizard commit. The pull request can then include unrelated changes from another branch.
</issue_description>
<issue_validation>
- **Checked:** what the sandbox git guard actually blocks, what refs exist in the wizard's clone, what runs inside the sandbox between clone and publish, and whether a sibling signed-commit path in this repo already makes this check.
- **Found:** the guard description is accurate but not the operative factor. `products/tasks/backend/sandbox/images/git-guard.sh` intercepts only `commit` and `push` and passes every other subcommand through, so it neither causes nor prevents a pending operation.
- **Found:** the mechanism is correct if the state ever arose. `git add --all` succeeds during a conflicted or `--no-commit` merge, `_head_sha` at `repository_publisher.py:77-81` returns the pre-merge HEAD, and `_staged_changes` at `repository_publisher.py:88-96` diffs the merged index against that HEAD, so the incoming branch's content becomes additions. `createCommitOnBranch` takes no parent list, so `_commit_staged_changes` at `repository_publisher.py:143-158` would flatten it into one ordinary commit on the wizard branch.
- **Found:** the precondition cannot arise from the clone. `clone_repository` passes `shallow=True` at `products/wizard/backend/logic/workers/service.py:151`, and `SandboxBase.clone_repository` builds `git clone --single-branch --depth 1` at `products/tasks/backend/logic/services/sandbox.py:485-496`. The working copy therefore holds one branch and one commit, with no second ref to merge, rebase onto, or cherry-pick from. A script would first have to fetch one.
- **Found:** nothing in the run performs such an operation. `build_wizard_command` at `products/wizard/backend/logic/workers/commands.py:21-67` installs the wizard outside the repository with `--ignore-scripts`, then runs the headless setup CLI with `cd <repo>`. Its job is to write SDK configuration. The finding names no script that starts a merge, and I found none.
- **Found:** the check is cheap and already exists elsewhere in this repo. `products/desktop/packages/git/src/queries.ts:459-466` detects `rebase-merge`, `rebase-apply`, `MERGE_HEAD`, and `CHERRY_PICK_HEAD`, and `products/desktop/packages/git/src/signed-commit.ts:135` refuses a signed commit while one is in progress. That path guards a developer's own machine, where mid-merge state is routine, which is the opposite of this sandbox.
- **Impact:** confirmed as a real gap in reasoning, with no reachable trigger today. If it ever fired, a customer would get a pull request carrying code from another branch under a wizard commit message, which is why the mechanism is worth recording.
- **Priority:** lowered to `consider`. `must_fix` does not hold, because the clone contains nothing to merge and no component in the run starts a merge, rebase, or cherry-pick. It is not dismissed outright, because the sandbox does execute the user's own project install scripts, so the invariant rests on behavior rather than on structure, and because the marker check is a few lines that a sibling signed-commit implementation already makes.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Before `_stage_all`, use `git rev-parse --git-path` to resolve the operation markers. Reject the publish if `MERGE_HEAD`, `CHERRY_PICK_HEAD`, `rebase-merge`, or `rebase-apply` exists. Add regression tests with `git merge --no-commit` and an interrupted rebase.
</potential_solution>
| try: | ||
| result = cloud_worker.create_git_repository_handoff( | ||
| GitRepositoryHandoffRequest( | ||
| team_id=input.team_id, | ||
| run_id=input.run_id, | ||
| sandbox_id=input.sandbox_id, | ||
| workspace_path=input.root_path, | ||
| github_integration_id=input.github_integration_id, | ||
| repository=input.repository, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Prevent overlapping handoff attempts
Why we think it's a valid issue
- Checked: every per-call timeout in the publish path, how many sandbox round trips the file loop makes, the activity's own budget and retry policy, whether
asyncifycan stop a timed-out call, whether any activity heartbeats, and what two overlapping attempts do to the same branch. - Found: the per-call ceilings are 60 seconds and no deadline spans the sequence.
GIT_COMMAND_TIMEOUT_SECONDS = 60andGITHUB_MUTATION_TIMEOUT_SECONDS = 60(products/wizard/backend/logic/workers/repository_publisher.py:10-11)._run_gitapplies the first to every sandbox exec (:330-332), and_staged_file_contentsissues one such exec per changed file (:110-122, driven by the loop in_staged_changes:89-109). The only overall bound is the activity'sstart_to_close_timeoutof 5 minutes (temporal/config.py:9). - Found: the work is unbounded in file count and the size guard comes too late.
_assert_payload_sizeallows up toMAX_COMMIT_PAYLOAD_BYTES = 35 * 1024 * 1024(:14, :124-129) and runs only after every file has already been read one round trip at a time. - Found: a timed-out attempt is not stopped.
asyncifyends inawait sync_to_async(thread_sensitive=False)(...)()(posthog/temporal/common/utils.py:102), and asgiref cannot interrupt that thread. No wizard activity heartbeats, so Temporal has no liveness signal and simply fails the attempt on the timeout and schedules the next underHANDOFF_RETRY_POLICY=maximum_attempts=3with default sub-second backoff (temporal/config.py:10). The first thread keeps issuing sandbox execs and GitHub mutations while the second attempt runs. - Found: overlapping attempts do collide on the same branch.
create_signed_commitreads the branch tip, then either creates the ref or commits withexpectedHeadOid(repository_publisher.py:54-69, :131-160). Two attempts that both readtip=Noneboth callcreateRef, and two attempts sharing a tip both commit against the same expected oid._graphql_requestraisesRepositoryPublishingErroron any GraphQL error array (:222-228), whichcreate_git_repository_handoffconverts toWizardWorkerExecutionError(logic/workers/service.py:275-276) and handoff.py:31-36 marksnon_retryable=True, so the loser ends the run with no further attempts. - Found: the repository itself cannot be corrupted.
expectedHeadOidmakes the commit a compare-and-swap,create_pull_requestreturns an existing pull request matching the head branch (:259-266), and GitHub rejects a second pull request for the same head and base. A collision therefore yields a spurious failure, not a double commit or a duplicate pull request. - Impact: a handoff with slow provider round trips or many changed files overruns the 5-minute budget; the retry then races a thread that is still publishing, and the loser's GraphQL error permanently fails a run whose commit and pull request already exist, leaving the branch unreferenced by the product. The leaked thread also keeps its executor slot until its remaining 60-second execs drain, reducing what else that worker can run.
- Priority: lowered to
should_fix. The mechanism is confirmed, but it needs the 5-minute budget to be exceeded, which a typical setup diff of a few small files does not approach, and the confirmed damage is a mislabeled run with an orphan pull request rather than wrong repository state. Therun_idlock the finding asks for adds new cross-process machinery; the bounded bulk read and one overall deadline are the parts that remove the trigger.
Issue description
The handoff runs many sequential sandbox and GitHub calls inside one synchronous activity. Reading each changed file can wait 60 seconds. The activity has a five-minute timeout and three attempts. A timed-out asyncify call leaves its worker thread running. Temporal can start another attempt while the first attempt still writes the same branch. The attempts can race on branch creation or expectedHeadOid. A valid run can fail after a commit or pull request exists.
Suggested fix
Use one bounded bulk read and enforce one overall handoff deadline. Ensure provider work stops before Temporal retries. Add a lock keyed by run_id to prevent concurrent publication.
Prompt to fix with AI (copy-paste)
## Context
@products/wizard/backend/temporal/activities/handoff.py#L20-30
<issue_description>
The handoff runs many sequential sandbox and GitHub calls inside one synchronous activity. Reading each changed file can wait 60 seconds. The activity has a five-minute timeout and three attempts. A timed-out `asyncify` call leaves its worker thread running. Temporal can start another attempt while the first attempt still writes the same branch. The attempts can race on branch creation or `expectedHeadOid`. A valid run can fail after a commit or pull request exists.
</issue_description>
<issue_validation>
- **Checked:** every per-call timeout in the publish path, how many sandbox round trips the file loop makes, the activity's own budget and retry policy, whether `asyncify` can stop a timed-out call, whether any activity heartbeats, and what two overlapping attempts do to the same branch.
- **Found:** the per-call ceilings are 60 seconds and no deadline spans the sequence. `GIT_COMMAND_TIMEOUT_SECONDS = 60` and `GITHUB_MUTATION_TIMEOUT_SECONDS = 60` (products/wizard/backend/logic/workers/repository_publisher.py:10-11). `_run_git` applies the first to every sandbox exec (:330-332), and `_staged_file_contents` issues one such exec per changed file (:110-122, driven by the loop in `_staged_changes` :89-109). The only overall bound is the activity's `start_to_close_timeout` of 5 minutes (temporal/config.py:9).
- **Found:** the work is unbounded in file count and the size guard comes too late. `_assert_payload_size` allows up to `MAX_COMMIT_PAYLOAD_BYTES = 35 * 1024 * 1024` (:14, :124-129) and runs only after every file has already been read one round trip at a time.
- **Found:** a timed-out attempt is not stopped. `asyncify` ends in `await sync_to_async(thread_sensitive=False)(...)()` (posthog/temporal/common/utils.py:102), and asgiref cannot interrupt that thread. No wizard activity heartbeats, so Temporal has no liveness signal and simply fails the attempt on the timeout and schedules the next under `HANDOFF_RETRY_POLICY` = `maximum_attempts=3` with default sub-second backoff (temporal/config.py:10). The first thread keeps issuing sandbox execs and GitHub mutations while the second attempt runs.
- **Found:** overlapping attempts do collide on the same branch. `create_signed_commit` reads the branch tip, then either creates the ref or commits with `expectedHeadOid` (repository_publisher.py:54-69, :131-160). Two attempts that both read `tip=None` both call `createRef`, and two attempts sharing a tip both commit against the same expected oid. `_graphql_request` raises `RepositoryPublishingError` on any GraphQL error array (:222-228), which `create_git_repository_handoff` converts to `WizardWorkerExecutionError` (logic/workers/service.py:275-276) and handoff.py:31-36 marks `non_retryable=True`, so the loser ends the run with no further attempts.
- **Found:** the repository itself cannot be corrupted. `expectedHeadOid` makes the commit a compare-and-swap, `create_pull_request` returns an existing pull request matching the head branch (:259-266), and GitHub rejects a second pull request for the same head and base. A collision therefore yields a spurious failure, not a double commit or a duplicate pull request.
- **Impact:** a handoff with slow provider round trips or many changed files overruns the 5-minute budget; the retry then races a thread that is still publishing, and the loser's GraphQL error permanently fails a run whose commit and pull request already exist, leaving the branch unreferenced by the product. The leaked thread also keeps its executor slot until its remaining 60-second execs drain, reducing what else that worker can run.
- **Priority:** lowered to `should_fix`. The mechanism is confirmed, but it needs the 5-minute budget to be exceeded, which a typical setup diff of a few small files does not approach, and the confirmed damage is a mislabeled run with an orphan pull request rather than wrong repository state. The `run_id` lock the finding asks for adds new cross-process machinery; the bounded bulk read and one overall deadline are the parts that remove the trigger.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Use one bounded bulk read and enforce one overall handoff deadline. Ensure provider work stops before Temporal retries. Add a lock keyed by `run_id` to prevent concurrent publication.
</potential_solution>
| cloud_worker.execute_wizard( | ||
| WizardExecutionRequest( | ||
| sandbox_id=input.sandbox_id, | ||
| workspace_path=input.root_path, | ||
| team_id=input.team_id, | ||
| wizard_version=run.program.wizard_version, | ||
| program_command=run.program.command, | ||
| use_local_wizard_source=input.use_local_wizard_source, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Bound Wizard process output before loading it
Why we think it's a valid issue
- Checked: which sandbox API this call uses, how long that call can run, whether either sandbox backend caps the returned output, what the buffered output is actually used for, and whether a streaming alternative already exists.
- Found: the call is non-streaming and holds open for 47 minutes.
execute_wizardpassestimeout_seconds=SANDBOX_EXECUTION_TIMEOUT_SECONDS(products/wizard/backend/logic/workers/service.py:205-217), and that constant isWIZARD_TIMEOUT_SECONDS + 120whereWIZARD_TIMEOUT_SECONDS = 45 * 60(logic/workers/config.py:7, :30). The command it runs is annpm installfollowed by the headless agent (logic/workers/commands.py:39-67), so the output is process logs, not a bounded payload. - Found: neither backend caps it.
ModalSandbox.executecallsprocess.stdout.read()afterprocess.wait()and then.decode("utf-8")(products/tasks/backend/logic/services/modal_sandbox.py:1034-1041);HoglandSandbox.executereturnsresult.stdoutwhole (hogland_sandbox.py:327-347). The only truncation helper in that layer,_truncate_output, is used for error strings in the docker backend and never forexecute's return (docker_sandbox.py:113-118). - Found: the buffer earns almost nothing. On a zero exit code
execute_wizardreturnsNoneand the whole buffer is discarded. On a nonzero code the code keeps 2000 characters:_failure_detailstrips stdout and stderr, joins them, and slices the tail againstWIZARD_ERROR_DETAIL_LENGTH(service.py:325-330). Those strips and the join each allocate another full-size copy before the slice. - Found: the error parser adds one more full pass and copy.
wizard_error_code_from_stderrdoesreversed(stderr.splitlines())(logic/workers/wizard_error_output.py:8), materializing a list of every line in stderr to find onephw-error:line. - Found: the streaming path the finding asks for already exists in the shared API.
SandboxBase.execute_streamis declared at products/tasks/backend/logic/services/sandbox.py:349 and implemented by both backends (hogland_sandbox.py:349-361, modal_sandbox.py:1067). The fix needs no new machinery. - Impact: every cloud run ends with its entire agent log resident in the Temporal worker process at once, and the accumulation window is 47 minutes rather than seconds, so a verbose or looping agent scales that with nothing to stop it. The worker serves many concurrent activities, so memory pressure or an OOM there takes unrelated in-flight work down too, and the cost is paid on successful runs where the output is thrown away. Whether a given volume actually OOMs depends on the worker container's memory limit, which is not set in this repository, so the confirmed defect is the unbounded buffering rather than a specific failure threshold.
Issue description
cloud_worker.execute_wizard calls the non-streaming sandbox API. That API buffers all stdout and stderr before this activity returns. A noisy 45-minute run can exhaust memory in the shared Temporal worker. The 2,000-character error limit applies only after the full output is buffered.
Suggested fix
Use execute_stream and retain a fixed-size tail for error parsing. Alternatively, redirect output inside the sandbox and read only a capped tail. Preserve the structured phw-error: line separately.
Prompt to fix with AI (copy-paste)
## Context
@products/wizard/backend/temporal/activities/execution.py#L23-32
<issue_description>
`cloud_worker.execute_wizard` calls the non-streaming sandbox API. That API buffers all stdout and stderr before this activity returns. A noisy 45-minute run can exhaust memory in the shared Temporal worker. The 2,000-character error limit applies only after the full output is buffered.
</issue_description>
<issue_validation>
- **Checked:** which sandbox API this call uses, how long that call can run, whether either sandbox backend caps the returned output, what the buffered output is actually used for, and whether a streaming alternative already exists.
- **Found:** the call is non-streaming and holds open for 47 minutes. `execute_wizard` passes `timeout_seconds=SANDBOX_EXECUTION_TIMEOUT_SECONDS` (products/wizard/backend/logic/workers/service.py:205-217), and that constant is `WIZARD_TIMEOUT_SECONDS + 120` where `WIZARD_TIMEOUT_SECONDS = 45 * 60` (logic/workers/config.py:7, :30). The command it runs is an `npm install` followed by the headless agent (logic/workers/commands.py:39-67), so the output is process logs, not a bounded payload.
- **Found:** neither backend caps it. `ModalSandbox.execute` calls `process.stdout.read()` after `process.wait()` and then `.decode("utf-8")` (products/tasks/backend/logic/services/modal_sandbox.py:1034-1041); `HoglandSandbox.execute` returns `result.stdout` whole (hogland_sandbox.py:327-347). The only truncation helper in that layer, `_truncate_output`, is used for error strings in the docker backend and never for `execute`'s return (docker_sandbox.py:113-118).
- **Found:** the buffer earns almost nothing. On a zero exit code `execute_wizard` returns `None` and the whole buffer is discarded. On a nonzero code the code keeps 2000 characters: `_failure_detail` strips stdout and stderr, joins them, and slices the tail against `WIZARD_ERROR_DETAIL_LENGTH` (service.py:325-330). Those strips and the join each allocate another full-size copy before the slice.
- **Found:** the error parser adds one more full pass and copy. `wizard_error_code_from_stderr` does `reversed(stderr.splitlines())` (logic/workers/wizard_error_output.py:8), materializing a list of every line in stderr to find one `phw-error:` line.
- **Found:** the streaming path the finding asks for already exists in the shared API. `SandboxBase.execute_stream` is declared at products/tasks/backend/logic/services/sandbox.py:349 and implemented by both backends (hogland_sandbox.py:349-361, modal_sandbox.py:1067). The fix needs no new machinery.
- **Impact:** every cloud run ends with its entire agent log resident in the Temporal worker process at once, and the accumulation window is 47 minutes rather than seconds, so a verbose or looping agent scales that with nothing to stop it. The worker serves many concurrent activities, so memory pressure or an OOM there takes unrelated in-flight work down too, and the cost is paid on successful runs where the output is thrown away. Whether a given volume actually OOMs depends on the worker container's memory limit, which is not set in this repository, so the confirmed defect is the unbounded buffering rather than a specific failure threshold.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Use `execute_stream` and retain a fixed-size tail for error parsing. Alternatively, redirect output inside the sandbox and read only a capped tail. Preserve the structured `phw-error:` line separately.
</potential_solution>
The stored run error code can be an open PHW_* value from a pinned Wizard version, and that value flowed straight into the error_code label of posthog_wizard_runs_finished_total and posthog_wizard_run_failures_total. Because prometheus_client never evicts a label set, each distinct code minted a permanent series and made error_code non-enumerable for alerts. Collapse any code outside the known WizardRunErrorCode set to a single "other" bucket at the metric-emission site. The raw code still lands in the run row and the wizard_run_finished log line, so the diagnostics forward-compatibility path is unchanged. Generated-By: PostHog Desktop Task-Id: 9895cb6d-50e0-4b4c-a529-4af542468bcf
The handoff captured the full git diff through the sandbox, and both sandbox backends buffer all stdout in the shared Temporal worker before the 30 MiB artifact cap ran. A working tree with large untracked or generated content could push hundreds of megabytes into worker memory in several copies and take unrelated workflows down with it. Stage the diff to a file inside the sandbox and return at most MAX_GIT_DIFF_BYTES + 1 bytes, following the existing repo-commit-activity and handoff-body pattern. The && chain preserves git's exit code, and the sentinel extra byte lets the existing len(content) > MAX_GIT_DIFF_BYTES check omit an oversized diff exactly as before. Under-limit diffs return in full, so line counts and the pull request are unchanged. Generated-By: PostHog Desktop Task-Id: 9895cb6d-50e0-4b4c-a529-4af542468bcf
temporal/client.py reads settings.WIZARD_TASK_QUEUE when starting the run workflow, but the setting was never defined. That raised AttributeError on the dispatch path and failed the client test this layer adds. Define WIZARD_TASK_QUEUE following the canvas-build pattern: default to the general-purpose fleet (which has a live worker) with a WIZARD_TASK_QUEUE env override, so nothing strands on a pollerless queue. Registering a worker to poll the queue stays with the deferred scheduler-registration layer. Generated-By: PostHog Desktop Task-Id: 9895cb6d-50e0-4b4c-a529-4af542468bcf
The clone activity marked every WizardWorkerExecutionError from the clone step non_retryable=True, which defeated the workflow's deliberate PREPARATION_RETRY_POLICY (maximum_attempts=3). A single transient network blip (DNS, TLS, GitHub transport) therefore ended the whole onboarding run on the first attempt. Drop non_retryable on the clone-execution branch so the existing 3-attempt policy applies. The clone is idempotent — the sandbox command clears any partial checkout with rm -rf before cloning — and the activity's other steps (stage update, access re-check, remote sanitize) are repeatable. The configuration and access-error branches stay non_retryable, since those do not recover on retry. Generated-By: PostHog Desktop Task-Id: 9895cb6d-50e0-4b4c-a529-4af542468bcf
cancel_run commits CANCELLED before Temporal receives the cancel, so a finalizer can arrive against an already-cancelled record. transition_cloud_run only short-circuited on an exact status+error_code match, so a FAILED or COMPLETED finalizer against a CANCELLED run raised IllegalStatusTransitionError, exhausted the finalization retries, and recorded a failed workflow while the product record correctly read CANCELLED. Treat a user cancellation as terminal: when the run is already CANCELLED and a FAILED or COMPLETED finalizer arrives, transition_cloud_run returns without attempting the transition, in both the pre-check and the post-exception re-check. The guard is scoped to finalizers so the provisioning RUNNING transition still fails fast on a cancelled run rather than letting it proceed to execution. Generated-By: PostHog Desktop Task-Id: 9895cb6d-50e0-4b4c-a529-4af542468bcf
cancel_run committed CANCELLED in transition_run's transaction and only afterwards wrote cancellation_requested_at in a separate statement, then dispatched the Temporal cancel inline. A process exit between the two writes (routine during a deploy) left a run with status CANCELLED but cancellation_requested_at NULL, so the wizard_run_cancel_idx recovery index could never find it and the cancel was neither delivered nor repairable. Write the status change and cancellation_requested_at in one atomic block, holding the run lock, and schedule dispatch_cancellation with transaction.on_commit so the external Temporal call runs only after the durable commit. A crash now leaves both writes or neither, keeping the run recoverable. request_cloud_run_cancellation had this single caller and is folded in. Generated-By: PostHog Desktop Task-Id: 9895cb6d-50e0-4b4c-a529-4af542468bcf
b4d740a to
7e6ad18
Compare
| from products.wizard.backend.models import WizardRun | ||
|
|
||
|
|
||
| def enforce_cloud_run_creation_policy(team_id: int, created_by_id: int, idempotency_key: str | None = None) -> None: |
There was a problem hiding this comment.
Removing the team-row lock lets a user bypass Wizard cloud-run rate limits via concurrent requests
enforce_cloud_run_creation_policy() (products/wizard/backend/logic/runs/admission.py:18-32) enforces "only one active cloud run per user" and hourly/daily quotas by querying existing WizardRun rows and raising if the count/existence check fails, with no SELECT ... FOR UPDATE of its own. Commit 1c906f9e ("chore: remove team lock") dropped the Team.objects.select_for_update().only("id").get(id=team_id) call that used to precede this check inside create_run_with_result's database_transaction.atomic() block (products/wizard/backend/logic/runs/lifecycle.py:100-101). That lock was the only thing serializing concurrent admission checks for the same team/user under Postgres's default READ COMMITTED isolation; with it gone, two (or more) concurrent create_run calls with distinct idempotency_keys can each read "no active run" / "under quota" before any of them commits its INSERT, and each then proceeds to create its own CLOUD run — bypassing ActiveWizardRunError, WizardRunHourlyLimitError, and WizardRunDailyLimitError entirely.
An authenticated user who fires several concurrent "create cloud run" requests (each with a fresh idempotency key) can spin up many parallel Wizard Workers — each provisioning a sandbox with a live, team-scoped OAuth token and cloning/executing against the connected GitHub repository — well past the one-active-run and per-hour/per-day admission limits meant to bound this cost and blast radius. Restore serialization before the read-then-decide check, e.g. reintroduce a Team.objects.select_for_update() (or lock/upsert against the requesting user's active-run row) inside the same transaction as enforce_cloud_run_creation_policy and store.create_run, so concurrent admission checks for the same team/user are serialized again.
Severity: medium | Confidence: 65% | React with 👍 if useful or 👎 if not
|
/trunk merge |
Problem
A cloud run needs an isolated worker that can change a connected GitHub repository without exposing credentials in logs or Temporal history.
A Wizard Worker is a PostHog-provisioned sandbox. It resolves short-lived credentials inside an activity, clones the repository, runs the headless setup agent, captures the diff, publishes a signed commit and pull request, then destroys the sandbox.
This is layer 4 of 6 for #85854. It follows durable storage and precedes API activation in #91822.
Changes
How did you test this code?
Automatic notifications
Docs update
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Pi created the stack. Skills:
/stacking-prs,/writing-pr-descriptions, and/asd-ste100.