Repository navigation
feat(wizard): add run registry foundation - #85921
Conversation
|
😎 Stack merged successfully - details. |
🤖 CI report
|
| Root | Eager (shipped) | Δ vs base | Budget |
|---|---|---|---|
entry (logged-out pages, app bootstrap)src/index.tsx |
1.37 MiB · 22 files | no change | ███░░░░░░░ 30.3% of 4.51 MiB |
authenticated shell (every logged-in page)src/scenes/AuthenticatedShell.tsx |
8.82 MiB · 3,245 files | no change | █████████░ 90.8% of 9.71 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/scenes/AuthenticatedShell.tsx
🟢 src/lib/components/ActivityLog/describers 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 |
| 912 B | ../node_modules/.pnpm/ieee754@1.2.1/node_modules/ieee754/index.js |
| 789 B | src/scenes/ChunkLoadErrorBoundary.tsx |
| 762 B | src/index.tsx |
Largest files eagerly shipped from src/scenes/AuthenticatedShell.tsx
| Size | File |
|---|---|
| 307.0 KiB | ../node_modules/.pnpm/posthog-js@1.422.5_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/rrweb.js |
| 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 |
| 263.5 KiB | ../node_modules/.pnpm/posthog-js@1.422.5_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/module.js |
| 252.9 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 154.2 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.7 KiB | src/lib/api.ts |
| 95.2 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 |
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.26 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.26 MiB · 18 files | no change | ████░░░░░░ 39.5% of 5.72 MiB |
| Deferred (lazy) | 2.11 MiB · 45 files | no change | 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 |
|---|---|
| 749.4 KiB | dist/toolbar/toolbar-app-MMYGYX5P.css |
| 588.2 KiB | dist/toolbar/chunk-chunk-UPPTUOWK.js |
| 484.7 KiB | dist/toolbar/chunk-chunk-FOUIAG54.js |
| 133.9 KiB | dist/toolbar/chunk-chunk-EK7WYTNP.js |
| 131.8 KiB | dist/toolbar/chunk-chunk-FDH2IBXT.js |
| 71.3 KiB | dist/toolbar/toolbar-app-MPMXPNK5.js |
| 69.0 KiB | dist/toolbar/chunk-chunk-TSAL54PB.js |
| 35.6 KiB | dist/toolbar/chunk-chunk-4GSGOFRW.js |
| 21.0 KiB | dist/toolbar/chunk-chunk-UAN2CCY6.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 — 🔺 +16.9 KiB (+0.0%)
Total size of the built frontend/dist folder (all assets), compared against the base branch.
Total: 1435.60 MiB · 🔺 +16.9 KiB (+0.0%)
⚠️ Backend coverage — 94.0% of changed backend lines covered — 25 uncovered
🧪 Backend test coverage
Patch coverage — changed backend lines (products + core): ███████████████████░ 94.0% (431 / 456)
| File | Patch | Uncovered changed lines |
|---|---|---|
products/wizard/backend/metrics.py |
66.7% | 55 |
products/wizard/backend/facade/validation.py |
69.1% | 22, 42, 46–50, 54, 63, 66, 72, 83–88 |
products/wizard/backend/logic/registry/service.py |
82.8% | 40–44 |
products/wizard/backend/logic/programs.py |
91.7% | 40 |
products/wizard/backend/logic/registry/parser.py |
95.8% | 17 |
🤖 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 33799385152 -n patch-coverage), or the coverage-data block at the end of this comment.
Per-product line coverage (touched products)
| Product | Coverage | Lines |
|---|---|---|
wizard |
██████████████████░░ 88.7% |
1,507 / 1,699 |
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.
⚠️ Django migration SQL — 3 new migrations to review
We've detected new migrations on this PR. Review the SQL output for each migration:
products/wizard/backend/migrations/0006_create_wizard_run.py
BEGIN;
--
-- Create model WizardRun
--
CREATE TABLE "wizard_wizardrun" ("created_at" timestamp with time zone NOT NULL, "updated_at" timestamp with time zone NULL, "id" uuid NOT NULL PRIMARY KEY, "environment" varchar(20) NOT NULL, "workspace_type" varchar(30) NOT NULL, "workspace" jsonb NOT NULL, "program" jsonb NOT NULL, "status" varchar(20) NOT NULL, "error_code" varchar(50) NULL, "idempotency_key" varchar(255) NULL, "request_fingerprint" varchar(64) NULL, "dispatch_status" varchar(20) NULL, "dispatch_attempts" smallint NOT NULL CHECK ("dispatch_attempts" >= 0), "dispatch_error" varchar(255) NULL, "dispatch_next_attempt_at" timestamp with time zone NULL, "workflow_id" varchar(255) NULL, "stage" varchar(30) NULL, "stage_started_at" timestamp with time zone NULL, "error_message" varchar(255) NULL, "started_at" timestamp with time zone NULL, "finished_at" timestamp with time zone NULL, "deadline_at" timestamp with time zone NULL, "cancellation_requested_at" timestamp with time zone NULL, "cancellation_dispatched_at" timestamp with time zone NULL, "created_by_id" integer NULL, "team_id" integer NOT NULL);
--
-- Create constraint unique_wizard_run_idempotency_key_per_team on model wizardrun
--
CREATE UNIQUE INDEX "unique_wizard_run_idempotency_key_per_team" ON "wizard_wizardrun" ("team_id", "idempotency_key") WHERE "idempotency_key" IS NOT NULL;
CREATE INDEX "wizard_wizardrun_created_by_id_d7863361" ON "wizard_wizardrun" ("created_by_id");
CREATE INDEX "wizard_wizardrun_team_id_32ea8c03" ON "wizard_wizardrun" ("team_id");
CREATE INDEX "wizard_run_dispatch_idx" ON "wizard_wizardrun" ("dispatch_next_attempt_at", "created_at") WHERE ("dispatch_status" = 'pending' AND "status" = 'created');
CREATE INDEX "wizard_run_cancel_idx" ON "wizard_wizardrun" ("id") WHERE ("cancellation_dispatched_at" IS NULL AND "cancellation_requested_at" IS NOT NULL AND "status" IN ('cancelled', 'failed'));
CREATE INDEX "wizard_run_deadline_idx" ON "wizard_wizardrun" ("deadline_at") WHERE ("deadline_at" IS NOT NULL AND "status" IN ('created', 'running'));
COMMIT;products/wizard/backend/migrations/0007_create_wizard_run_artifact.py
BEGIN;
--
-- Create model WizardRunArtifact
--
CREATE TABLE "wizard_wizardrunartifact" ("id" uuid NOT NULL PRIMARY KEY, "type" varchar(30) NOT NULL, "storage_path" varchar(1024) NULL, "external_url" varchar(1024) NULL, "metadata" jsonb NULL, "size_bytes" bigint NULL CHECK ("size_bytes" >= 0), "content_hash" varchar(64) NULL, "created_at" timestamp with time zone NOT NULL, "run_id" uuid NOT NULL, "team_id" integer NOT NULL);
--
-- Create constraint unique_wizard_artifact_type_per_run on model wizardrunartifact
--
ALTER TABLE "wizard_wizardrunartifact" ADD CONSTRAINT "unique_wizard_artifact_type_per_run" UNIQUE ("run_id", "type");
ALTER TABLE "wizard_wizardrunartifact" ADD CONSTRAINT "wizard_wizardrunartifact_run_id_72605ebb_fk_wizard_wizardrun_id" FOREIGN KEY ("run_id") REFERENCES "wizard_wizardrun" ("id") DEFERRABLE INITIALLY DEFERRED;
CREATE INDEX "wizard_wizardrunartifact_run_id_72605ebb" ON "wizard_wizardrunartifact" ("run_id");
CREATE INDEX "wizard_wizardrunartifact_team_id_4d5dc2eb" ON "wizard_wizardrunartifact" ("team_id");
COMMIT;products/wizard/backend/migrations/0008_create_wizard_worker.py
BEGIN;
--
-- Create model WizardWorker
--
CREATE TABLE "wizard_wizardworker" ("updated_at" timestamp with time zone NULL, "id" uuid NOT NULL PRIMARY KEY, "created_at" timestamp with time zone NOT NULL, "sandbox_id" varchar(255) NULL UNIQUE, "resource_usage" jsonb NULL, "cleanup_status" varchar(20) NOT NULL, "cleanup_attempts" smallint NOT NULL CHECK ("cleanup_attempts" >= 0), "cleanup_error" varchar(255) NULL, "cleaned_at" timestamp with time zone NULL, "run_id" uuid NOT NULL UNIQUE, "team_id" integer NOT NULL);
ALTER TABLE "wizard_wizardworker" ADD CONSTRAINT "wizard_wizardworker_run_id_7d56184d_fk_wizard_wizardrun_id" FOREIGN KEY ("run_id") REFERENCES "wizard_wizardrun" ("id") DEFERRABLE INITIALLY DEFERRED;
CREATE INDEX "wizard_wizardworker_sandbox_id_f863b5fe_like" ON "wizard_wizardworker" ("sandbox_id" varchar_pattern_ops);
CREATE INDEX "wizard_wizardworker_team_id_52433f71" ON "wizard_wizardworker" ("team_id");
CREATE INDEX "wizard_worker_cleanup_idx" ON "wizard_wizardworker" ("cleanup_attempts") WHERE ("cleanup_status" IN ('active', 'pending') AND "sandbox_id" IS NOT NULL);
COMMIT;Last updated: 2026-09-03 20:02 UTC (14d48f1)
✅ Django migration risk — migration analysis complete
We've analyzed your migrations for potential risks.
Summary: 3 Safe | 0 Needs Review | 0 Blocked
✅ Safe
Brief or no lock, backwards compatible
wizard.0006_create_wizard_run
└─ #1 ✅ CreateModel
Creating new table is safe
model: WizardRun
│
└──> ℹ️ INFO:
ℹ️ Skipped operations on newly created tables (empty tables
don't cause lock contention).
wizard.0007_create_wizard_run_artifact
└─ #1 ✅ CreateModel
Creating new table is safe
model: WizardRunArtifact
│
└──> ℹ️ INFO:
ℹ️ Skipped operations on newly created tables (empty tables
don't cause lock contention).
wizard.0008_create_wizard_worker
└─ #1 ✅ CreateModel
Creating new table is safe
model: WizardWorker
│
└──> ℹ️ INFO:
ℹ️ Skipped operations on newly created tables (empty tables
don't cause lock contention).
Last updated: 2026-09-03 20:02 UTC (14d48f1)
✅ ClickHouse migration SQL — none
No ClickHouse migrations in the latest push.
ℹ️ Docs preview — preview build triggered
Docs from this PR will be published at posthog.com.
| Project | Preview | Updated (UTC) |
|---|---|---|
| posthog.com | Open preview | Sep 3, 2026, 7:57 PM |
The preview should be ready in about 10 minutes. Open the preview at /handbook/engineering/.
996198a to
7d99005
Compare
a702fe7 to
55bcf0e
Compare
c350daf to
0d98957
Compare
fercgomes
changed the title
Prompt To Fix All With AI### Issue 1
products/wizard/backend/logic/registry/service.py:10
**Undefined metric breaks registry import**
When the registry service is imported directly or through `logic.registry`, it imports `report_registry_fallback`, which is not defined by `products.wizard.backend.metrics`, causing an `ImportError` before `get_registry` or `get_program` can run.
### Issue 2
products/wizard/backend/facade/errors.py:1-2
**Exception move breaks existing imports**
Moving `WizardSessionOwnershipError` out of `facade.contracts` without retaining a re-export or updating its importers causes `logic.sessions`, `presentation.views`, and `test_logic` to fail during module loading with `ImportError`.
### Issue 3
products/wizard/backend/logic/registry/__init__.py:1-3
**Registry facade remains unwired**
The new tests import `facade.api` and call `wizard_facade.get_registry`, but these exports exist only in the logic package and are not exposed by `facade.api`, causing the registry tests to raise `AttributeError` instead of exercising the service.
---
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): add run registry foundatio..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
🦔 PostHog Review reviewed this pull requestFound 1 must fix, 4 should fix, 2 consider. Published 7 findings (view the review). Resolved comments: 6 fixed, 1 left for you |
|
PostHog Review alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
| try: | ||
| payload = posthoganalytics.get_feature_flag_payload( | ||
| REGISTRY_FEATURE_FLAG, | ||
| distinct_id=distinct_id, | ||
| groups={"organization": organization_id}, | ||
| group_properties={"organization": {"id": organization_id}}, | ||
| only_evaluate_locally=False, | ||
| send_feature_flag_events=False, | ||
| ) | ||
| except (APIError, RequestsConnectionError, RequestsTimeout): | ||
| logger.warning("wizard_registry_fallback", extra={"reason": "request_failed"}, exc_info=True) | ||
| report_registry_fallback("request_failed") | ||
| return FALLBACK_REGISTRY.programs | ||
|
|
||
| try: | ||
| registry = parse_registry_payload(payload) | ||
| except ValueError: | ||
| logger.warning("wizard_registry_fallback", extra={"reason": "invalid_payload"}, exc_info=True) | ||
| report_registry_fallback("invalid_payload") |
There was a problem hiding this comment.
Request failures are recorded as invalid payloads
Why we think it's a valid issue
- Checked: the pinned SDK's flag path end to end (
posthoganalytics==7.44.2,pyproject.toml:103), the parser's handling ofNone, and the PR's own failure tests. - Found: The SDK never lets transport errors escape.
_get_feature_flag_resultwraps the server call (client.py:3354) in a try that catchesQuotaLimitError,RequestsTimeout,RequestsConnectionError,APIError, and bareException(client.py:3384-3403). Each branch falls back to a stale cache and continues, and the method returnsflag_resultatclient.py:3439without re-raising.get_feature_flag_payloadthen returnsNonewhen that result is falsy (client.py:3671). - Found: The local path swallows too, so no second route exists.
_locally_evaluate_flagcatches everything atclient.py:3588-3593. - Found:
Noneroutes straight to the wrong label._decode_payloadpasses a non-string through unchanged (logic/registry/parser.py:24-25), then theisinstance(decoded, dict)guard raisesValueError(parser.py:12-13), whichget_registryrecords asinvalid_payload(logic/registry/service.py:30-34). The suite already encodes this:("missing_payload", None)sits in the invalid-payload cases (tests/test_registry.py:128). - Found: The existing test passes only by bypassing the SDK.
tests/test_registry.py:188patchesposthoganalytics.get_feature_flag_payloadwithside_effect=APIError(503, "Unavailable")and assertsrequest_failed(test_registry.py:194). Replacing the whole function skips the internal handlers, so the test asserts a path production cannot produce. That is why a lower-level transport mock is the right test. - Found: One narrow escape survives, so "remove the branch" needs care.
_locally_evaluate_flagcallsload_feature_flags()outside any try (client.py:3566), and that method re-raisesAPIErrorfor 401 and 402 only whenself.debugis set (client.py:3028,client.py:3046-3049). The handler is therefore dead for transport failures, not for every case. - Impact: During a flags API outage or timeout, the registry degrades to the single fallback program for every affected user, and the metric blames a malformed payload. An engineer who sees an
invalid_payloadspike would audit the flag JSON while the real cause is API health, and therequest_failedseries stays flat through the exact incident it exists to surface. The two failure modes are also now indistinguishable, since a genuinely bad payload emits the same label. - Priority: Kept at
should_fix. The fallback keeps the product working and no data is lost, so this is notmust_fix. It is well aboveconsider, because the dedicated label cannot fire for its stated purpose and a green test asserts behavior the pinned SDK cannot generate, which will mislead the next reader.
Issue description
The pinned SDK catches these request exceptions and returns None. This outer handler cannot observe real transport failures. The parser then records them as invalid_payload. The request_failed metric therefore cannot report actual network failures.
Suggested fix
Handle None before parsing and record payload_unavailable. Remove the unreachable exception branch. Add a test that mocks the SDK's lower-level flags transport.
Prompt to fix with AI (copy-paste)
## Context
@products/wizard/backend/logic/registry/service.py#L16-34
<issue_description>
The pinned SDK catches these request exceptions and returns None. This outer handler cannot observe real transport failures. The parser then records them as invalid_payload. The request_failed metric therefore cannot report actual network failures.
</issue_description>
<issue_validation>
- **Checked:** the pinned SDK's flag path end to end (`posthoganalytics==7.44.2`, `pyproject.toml:103`), the parser's handling of `None`, and the PR's own failure tests.
- **Found:** The SDK never lets transport errors escape. `_get_feature_flag_result` wraps the server call (`client.py:3354`) in a try that catches `QuotaLimitError`, `RequestsTimeout`, `RequestsConnectionError`, `APIError`, and bare `Exception` (`client.py:3384-3403`). Each branch falls back to a stale cache and continues, and the method returns `flag_result` at `client.py:3439` without re-raising. `get_feature_flag_payload` then returns `None` when that result is falsy (`client.py:3671`).
- **Found:** The local path swallows too, so no second route exists. `_locally_evaluate_flag` catches everything at `client.py:3588-3593`.
- **Found:** `None` routes straight to the wrong label. `_decode_payload` passes a non-string through unchanged (`logic/registry/parser.py:24-25`), then the `isinstance(decoded, dict)` guard raises `ValueError` (`parser.py:12-13`), which `get_registry` records as `invalid_payload` (`logic/registry/service.py:30-34`). The suite already encodes this: `("missing_payload", None)` sits in the invalid-payload cases (`tests/test_registry.py:128`).
- **Found:** The existing test passes only by bypassing the SDK. `tests/test_registry.py:188` patches `posthoganalytics.get_feature_flag_payload` with `side_effect=APIError(503, "Unavailable")` and asserts `request_failed` (`test_registry.py:194`). Replacing the whole function skips the internal handlers, so the test asserts a path production cannot produce. That is why a lower-level transport mock is the right test.
- **Found:** One narrow escape survives, so "remove the branch" needs care. `_locally_evaluate_flag` calls `load_feature_flags()` outside any try (`client.py:3566`), and that method re-raises `APIError` for 401 and 402 only when `self.debug` is set (`client.py:3028`, `client.py:3046-3049`). The handler is therefore dead for transport failures, not for every case.
- **Impact:** During a flags API outage or timeout, the registry degrades to the single fallback program for every affected user, and the metric blames a malformed payload. An engineer who sees an `invalid_payload` spike would audit the flag JSON while the real cause is API health, and the `request_failed` series stays flat through the exact incident it exists to surface. The two failure modes are also now indistinguishable, since a genuinely bad payload emits the same label.
- **Priority:** Kept at `should_fix`. The fallback keeps the product working and no data is lost, so this is not `must_fix`. It is well above `consider`, because the dedicated label cannot fire for its stated purpose and a green test asserts behavior the pinned SDK cannot generate, which will mislead the next reader.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Handle None before parsing and record payload_unavailable. Remove the unreachable exception branch. Add a test that mocks the SDK's lower-level flags transport.
</potential_solution>
There was a problem hiding this comment.
Confirmed — this is a real bug. I traced the pinned SDK (posthoganalytics 7.44.2): get_feature_flag_payload swallows APIError, RequestsConnectionError, RequestsTimeout, quota, and generic errors internally, falling back to a stale cache and returning None rather than raising. So the except (APIError, RequestsConnectionError, RequestsTimeout) branch in get_registry almost never runs for a real outage; the outage instead surfaces as a None payload, which the parser rejects and we record as invalid_payload. The request_failed metric therefore can't fire for the incident it exists to surface, and the existing test_registry_falls_back_when_feature_flag_fetch_fails asserts a path the SDK doesn't actually produce because it patches the whole function.
I'm not landing this unattended because the fix is a set of judgment calls rather than a mechanical change, and I'd rather a maintainer decide them:
- Label taxonomy — add a distinct reason for an absent payload (e.g.
payload_unavailable), and decide whether aNonefrom an org that simply hasn't set the flag (the normal case, not an error) should be counted as a fallback at all or stay neutral. - Whether to remove the now-effectively-dead
except (APIError, ...)branch. It isn't strictly dead —load_feature_flags()can still re-raiseAPIErrorfor 401/402 when the client's debug flag is set — and removing it couples us to SDK internals, so a future SDK change could turn an outage into an unhandled exception out ofget_registry. Keeping it as cheap insurance is also defensible. - Reworking the two tests that encode the current contract, including replacing the whole-function
APIErrormock with a lower-level SDK-transport mock so the test exercises a path production can actually reach.
Happy to implement whichever taxonomy and branch decision you prefer once it's chosen.
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
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 on migrations, size, and tier, and the diff adds new team-scoped models not yet covered by the IDOR semgrep scoping rule — an unresolved must-fix security finding, not just documentation nitpicks.
- 👍 on the PR from greptile-apps[bot].
- New WizardRun, WizardRunArtifact, and WizardWorker models are team-scoped but absent from .semgrep/rules/security/idor-team-scoped-models.yaml, leaving them outside IDOR coverage (posthog[bot] must-fix, unresolved).
- Greptile flagged likely import-breaking changes (undefined metric import, moved exception without re-export, unwired facade exports) that could break module loading, unaddressed.
- Gate deny-list matched migration files and the PR exceeds size/tier ceilings for auto-review (1003L substantive, 20 files, cross-cutting, T2-never).
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✗ | matches: migrations |
| size | ✗ | too large for auto-review (1003L substantive in global — ceiling is 800L; 1003L, 17F total, 1266L/20F incl. docs/generated/snapshots) |
| tier | ✗ | classified as T2-never: T2-never (1266L, 20F, cross-cutting, feat) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ 1be94f4 · reviewed head 1be94f4 |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| if not isinstance(value, dict): | ||
| raise ValueError("Invalid Wizard program") | ||
| fields = set(value) | ||
| if not _PROGRAM_FIELDS.issubset(fields) or (not allow_latest_version and fields != _PROGRAM_FIELDS): |
There was a problem hiding this comment.
Logic bug: The allow_latest_version parameter controls both wizard version validation AND whether extra fields are allowed in the mapping. When allow_latest_version=True, the condition (not allow_latest_version and fields != _PROGRAM_FIELDS) becomes False, allowing extra fields to pass validation. This means stored programs with wizard_version="latest" could have arbitrary extra fields that bypass validation.
# Current (buggy):
if not _PROGRAM_FIELDS.issubset(fields) or (not allow_latest_version and fields != _PROGRAM_FIELDS):
# Should be:
if not _PROGRAM_FIELDS.issubset(fields) or fields != _PROGRAM_FIELDS:The allow_latest_version parameter should only affect wizard_version validation at line 48-52, not field set validation.
| if not _PROGRAM_FIELDS.issubset(fields) or (not allow_latest_version and fields != _PROGRAM_FIELDS): | |
| if not _PROGRAM_FIELDS.issubset(fields) or fields != _PROGRAM_FIELDS: | |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
79cae89 to
512beff
Compare
| @frozen | ||
| class LocalFolderWorkspace: | ||
| project_name: str | ||
| type: Literal["local_folder"] = "local_folder" | ||
|
|
||
|
|
||
| @frozen | ||
| class GitRepositoryWorkspace: | ||
| repository: str | ||
| type: Literal["git_repository"] = "git_repository" | ||
|
|
||
|
|
||
| type WizardWorkspace = LocalFolderWorkspace | GitRepositoryWorkspace |
There was a problem hiding this comment.
Don't know what's more common in Python: having a base class you inherit from or this. Also, Github doesn't seem to handle the type keyword all that well, so maybe this means this is not the best way to do it?
There was a problem hiding this comment.
Don't know what's more common in Python: having a base class you inherit from or this.
i don't think it matters honestly
Github doesn't seem to handle the type keyword all that well, so maybe this means this is not the best way to do it?
can you elaborate?
There was a problem hiding this comment.
You know, you hate AI comments, but some heredocs here would have been quite useful!
| class WizardRunErrorCode(StrEnum): | ||
| TIMEOUT = "timeout" | ||
| PROVISIONING_FAILED = "provisioning_failed" | ||
| REPOSITORY_ACCESS_FAILED = "repository_access_failed" | ||
| WORKSPACE_PREPARATION_FAILED = "workspace_preparation_failed" | ||
| EXECUTION_FAILED = "execution_failed" | ||
| ARTIFACT_CREATION_FAILED = "artifact_creation_failed" | ||
| DISPATCH_FAILED = "dispatch_failed" |
There was a problem hiding this comment.
It's a bit weird we have these here, and then the errors.py file but I guess this is fine?
There was a problem hiding this comment.
this is not an error class (which extends Exception), it's just an enum that's used for errors
| return value | ||
|
|
||
|
|
||
| def validate_pinned_wizard_version(value: object) -> str: |
There was a problem hiding this comment.
I've never in my whole life seen the type object. Is that any better than Any?
There was a problem hiding this comment.
object still offers type-safety, whereas Any bypasses type-checking
| def get_registry(*, distinct_id: str, organization_id: str) -> tuple[WizardProgram, ...]: | ||
| try: | ||
| payload = posthoganalytics.get_feature_flag_payload( | ||
| REGISTRY_FEATURE_FLAG, | ||
| distinct_id=distinct_id, | ||
| groups={"organization": organization_id}, | ||
| group_properties={"organization": {"id": organization_id}}, | ||
| only_evaluate_locally=False, | ||
| send_feature_flag_events=False, | ||
| ) | ||
| except (APIError, RequestsConnectionError, RequestsTimeout): | ||
| logger.warning("wizard_registry_fallback", extra={"reason": "request_failed"}, exc_info=True) | ||
| report_registry_fallback("request_failed") | ||
| return FALLBACK_REGISTRY.programs | ||
|
|
||
| try: | ||
| registry = parse_registry_payload(payload) | ||
| except ValueError: | ||
| logger.warning("wizard_registry_fallback", extra={"reason": "invalid_payload"}, exc_info=True) | ||
| report_registry_fallback("invalid_payload") | ||
| return FALLBACK_REGISTRY.programs | ||
| return registry.programs |
There was a problem hiding this comment.
Why do we need this? I don't think there's an explanation anywhere
There was a problem hiding this comment.
yes i could've added a comment to it (and will)
but tl;dr: i'm using a remote config flag to store the available wizard programs, so we can dinamically show/hide them for users
| class Migration(migrations.Migration): | ||
| dependencies = [ | ||
| ("posthog", "1319_team_posthog_team_widget_token_idx"), | ||
| ("wizard", "0005_wizardsession_handoff_text"), | ||
| migrations.swappable_dependency(settings.AUTH_USER_MODEL), | ||
| ] |
There was a problem hiding this comment.
Kinda weird? In theory should only depend on 0005, right?
| WIZARD_REGISTRY_FALLBACK_TOTAL = Counter( | ||
| "posthog_wizard_registry_fallback_total", | ||
| "Wizard registry requests that used the built-in fallback", | ||
| labelnames=["reason"], | ||
| ) |
There was a problem hiding this comment.
Why do we need the fallback? And maybe better, why do we need that feature flag to define these? Should we not be reading it from the context-mill somehow?
There was a problem hiding this comment.
Or, even better, just have them in the database and something syncs them over from context mill to DB from time to time?
WizardRun, WizardRunArtifact, and WizardWorker are team-scoped models (each declares a Team FK) but were absent from the team-scoped IDOR semgrep regexes. The check-idor-model-coverage CI step failed because these models were neither covered nor excluded. Add all three to both team-scoped rules (idor-taint-user-input-to-model-get and idor-lookup-without-team), mirroring the existing WizardSession entry, so future unsafe id lookups on these models are flagged. Generated-By: PostHog Desktop Task-Id: f923b6a0-fb45-46b5-99a6-25fb285d0a45
The registry parser requires each program to declare a pinned wizard_version and rejects the whole payload otherwise (allow_latest_version is False for the feature-flag path). The "Remote configuration" example in docs/internal/wizard-registry.md omitted the field, so an operator copying it would have the entire registry rejected and silently served the fallback program. Add a pinned wizard_version to the example and state that the field is required and cannot be "latest". Generated-By: PostHog Desktop Task-Id: f923b6a0-fb45-46b5-99a6-25fb285d0a45
This PR lands only the data models and registry parsing. It registers no run or registry HTTP routes (routes.py registers only wizard/sessions) and adds no cloud workers. The doc described the "API" endpoints and "Cloud execution" workers in present tense, so an internal reader who followed the documented registry route would hit a 404. Add a status line, matching the convention in neighboring docs/internal documents, that scopes the document as design for the Wizard run stack and names which sections are live today versus planned for later layers. Generated-By: PostHog Desktop Task-Id: f923b6a0-fb45-46b5-99a6-25fb285d0a45
The registry stores a pinned wizard_version on each program snapshot and the parser rejects "latest" for registry programs (validate_pinned_wizard_version). The "Cloud execution" section told workers to run @posthog/wizard@latest, a mutable tag, which contradicts the same section's promise that workers do not evaluate the registry again and would let one snapshot run different releases over time. Point the documented command at the snapshot's pinned wizard_version and state that workers must use it, never a mutable tag. Generated-By: PostHog Desktop Task-Id: f923b6a0-fb45-46b5-99a6-25fb285d0a45
The pinned-version pattern used a permissive prerelease group ([0-9A-Za-z-]+), so it accepted numeric prerelease identifiers with leading zeros such as 1.2.3-01 and 1.2.3-alpha.01. SemVer clause 9 and npm reject those, so the registry accepted an unusable package version instead of applying its invalid-payload fallback. Tighten the prerelease group to the canonical SemVer identifier grammar, which forbids leading zeros in numeric identifiers while still allowing alphanumeric ones. Build metadata is unchanged. Extend the invalid-program cases and add a positive case for a valid prerelease (1.2.3-alpha.1). semantic-version is already available, but a regex tightening is the smaller, more predictable change and keeps the facade validator dependency-free. Generated-By: PostHog Desktop Task-Id: f923b6a0-fb45-46b5-99a6-25fb285d0a45
The API section's summary of a program's contents omitted the pinned Wizard version, the one field a reader could not infer from the corrected example alone. Add it to the enumeration so the required field is named in the prose as well as the payload example. The example itself and the required / no-"latest" rule were corrected earlier in this review pass. Generated-By: PostHog Desktop Task-Id: f923b6a0-fb45-46b5-99a6-25fb285d0a45
512beff to
14d48f1
Compare
Problem
People need a reliable record of each Wizard program run before PostHog can execute the setup agent in the cloud.
Wizard is an npm setup agent. The program registry selects programs for the signed-in user and selected organization. A run stores the selected program snapshot so later registry changes cannot change an in-progress run.
This is layer 1 of 6 for #85854. It defines data only. The later layers add session code, storage, workers, APIs, and the UI.
Changes
How did you test this code?
hogli test products/wizard/backend/tests/test_registry.py.Automatic notifications
Docs update
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Pi created the stack. Skills:
/stacking-prs,/writing-pr-descriptions, and/asd-ste100.