Repository navigation
feat(signals): verify fixes after merged pull requests #103555
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
8fad9f9
dd1a655
d588a87
30d2703
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| # TypeSafe egress | ||
|
|
||
| ## Identity | ||
|
|
||
| TypeSafe meters the account behind an API key. The limiter and metrics use a short SHA-256 fingerprint of the key, never the credential itself. | ||
|
|
||
| ## Budget | ||
|
|
||
| TypeSafe publishes 1,200 requests per minute for Jev. PostHog defaults to 1,000 requests per minute and 50,000 per hour so background verification cannot consume the provider limit or create unbounded spend. | ||
|
|
||
| ## Lanes and callers | ||
|
|
||
| Signals verification uses the `BATCH` lane. A denied call remains an errored, retryable check and does not become a verdict. | ||
|
|
||
| ## Rate-limit headers | ||
|
|
||
| The API documents `Retry-After` for HTTP 429 and 529 responses. The check runner does not sleep inside the coordinator tick; it records a retryable error and uses the check schedule for backoff. | ||
|
|
||
| ## Auth | ||
|
|
||
| Callers pass `TYPESAFE_API_KEY` as a bearer token. The transport is credential-agnostic and never stores it. | ||
|
|
||
| ## Sources | ||
|
|
||
| - [TypeSafe API](https://docs.typesafe.ai/api) | ||
| - [Jev models and limits](https://docs.typesafe.ai/models) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| from posthog.egress.typesafe.transport import TypeSafeEgressBudgetExhausted, typesafe_request | ||
|
|
||
| __all__ = ["TypeSafeEgressBudgetExhausted", "typesafe_request"] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| """TypeSafe egress budget keyed by the API credential's non-reversible fingerprint.""" | ||
|
|
||
| import hashlib | ||
|
|
||
| from posthog.egress.limiter.outbound import get_outbound_rate_limiter | ||
| from posthog.egress.limiter.policies import Priority, per_minute_and_hourly_policy, register_policy | ||
|
|
||
| TYPESAFE_DOMAIN = "typesafe" | ||
|
|
||
| register_policy( | ||
| TYPESAFE_DOMAIN, | ||
| per_minute_and_hourly_policy( | ||
| per_minute_setting="TYPESAFE_EGRESS_PER_MINUTE_BUDGET", | ||
| per_minute_default=1_000, | ||
| hourly_setting="TYPESAFE_EGRESS_HOURLY_BUDGET", | ||
| hourly_default=50_000, | ||
| ), | ||
| ) | ||
|
|
||
|
|
||
| def typesafe_account_id(api_key: str) -> str: | ||
| return hashlib.sha256(api_key.encode()).hexdigest()[:16] | ||
|
|
||
|
|
||
| def consume_typesafe_sync(account_id: str, *, priority: Priority = Priority.BATCH, source: str = "unknown") -> bool: | ||
| return get_outbound_rate_limiter().consume_sync( | ||
| f"{TYPESAFE_DOMAIN}:account:{account_id}", | ||
| 1, | ||
| priority=priority, | ||
| source=source, | ||
| ) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| from prometheus_client import Counter | ||
|
|
||
| from posthog.egress.observability.observability import EgressMetrics, EgressObservability | ||
|
|
||
| typesafe_egress = EgressObservability( | ||
| EgressMetrics( | ||
| request_counter=Counter( | ||
| "typesafe_api_requests", | ||
| "Number of TypeSafe API requests made through the TypeSafe egress client.", | ||
| labelnames=["account", "method", "endpoint", "status_code", "source"], | ||
| ), | ||
| ) | ||
| ) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| from typing import Any | ||
|
|
||
| import requests | ||
|
|
||
| from posthog.egress.limiter.policies import Priority | ||
| from posthog.egress.transport.transport import EgressBudgetExhausted, EgressClient | ||
| from posthog.egress.typesafe.limiter import consume_typesafe_sync, typesafe_account_id | ||
| from posthog.egress.typesafe.observability import typesafe_egress | ||
|
|
||
|
|
||
| class TypeSafeEgressBudgetExhausted(EgressBudgetExhausted): | ||
| pass | ||
|
|
||
|
|
||
| class TypeSafeClient(EgressClient): | ||
| observability = typesafe_egress | ||
|
|
||
| def _standard_headers(self) -> dict[str, str]: | ||
| return {"Accept": "application/json", "Content-Type": "application/json"} | ||
|
|
||
| def _consume(self, scope: str, priority: Priority, source: str, url: str) -> bool: | ||
| return consume_typesafe_sync(scope, priority=priority, source=source) | ||
|
|
||
| def _budget_exhausted_error(self, scope: str) -> TypeSafeEgressBudgetExhausted: | ||
| return TypeSafeEgressBudgetExhausted("TypeSafe egress budget exhausted; retry later") | ||
|
|
||
|
|
||
| _typesafe_client = TypeSafeClient() | ||
|
|
||
|
|
||
| def typesafe_request( | ||
| method: str, | ||
| url: str, | ||
| *, | ||
| api_key: str, | ||
| source: str, | ||
| endpoint: str, | ||
| priority: Priority = Priority.BATCH, | ||
| timeout: float | tuple[float, float] | None = None, | ||
| **kwargs: Any, | ||
| ) -> requests.Response: | ||
| return _typesafe_client.request( | ||
| method, | ||
| url, | ||
| source=source, | ||
| headers={"Authorization": f"Bearer {api_key}"}, | ||
| scope=typesafe_account_id(api_key), | ||
| priority=priority, | ||
| endpoint=endpoint, | ||
| timeout=timeout, | ||
| **kwargs, | ||
| ) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -914,6 +914,15 @@ def static_varies_origin(headers, path, url): | |
| FIRECRAWL_EGRESS_PER_MINUTE_BUDGET = get_from_env("FIRECRAWL_EGRESS_PER_MINUTE_BUDGET", 60, type_cast=int) | ||
| FIRECRAWL_EGRESS_HOURLY_BUDGET = get_from_env("FIRECRAWL_EGRESS_HOURLY_BUDGET", 1000, type_cast=int) | ||
|
|
||
| #### | ||
| # TypeSafe Jev (Signals verification, see posthog/egress/typesafe/) | ||
| TYPESAFE_API_KEY = get_from_env("TYPESAFE_API_KEY", "") | ||
| TYPESAFE_BASE_URL = get_from_env("TYPESAFE_BASE_URL", "https://api.typesafe.ai") | ||
| TYPESAFE_DEFAULT_MODEL = get_from_env("TYPESAFE_DEFAULT_MODEL", "jev-latest") | ||
| TYPESAFE_VERIFICATION_MIN_CONFIDENCE = get_from_env("TYPESAFE_VERIFICATION_MIN_CONFIDENCE", 0.6, type_cast=float) | ||
| TYPESAFE_EGRESS_PER_MINUTE_BUDGET = get_from_env("TYPESAFE_EGRESS_PER_MINUTE_BUDGET", 1000, type_cast=int) | ||
| TYPESAFE_EGRESS_HOURLY_BUDGET = get_from_env("TYPESAFE_EGRESS_HOURLY_BUDGET", 50000, type_cast=int) | ||
|
Comment on lines
+917
to
+924
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This adds six TypeSafe environment variables, including a production API key, but the repository requires every new secret or environment variable to also be added to Rule Used: When a new secret or environment variable is added (e.g. via Prompt To Fix With AIThis is a comment left during a code review.
Path: posthog/settings/web.py
Line: 917-924
Comment:
**TypeSafe settings lack deployment wiring**
This adds six TypeSafe environment variables, including a production API key, but the repository requires every new secret or environment variable to also be added to `posthog/charts` and provisioned through `posthog/secrets`. That deployment wiring must be added before merging; otherwise production keeps an empty API key and every verification attempt retries as an error.
**Rule Used:** When a new secret or environment variable is added (e.g. via `get_from_env` in settings files, or new entries in `.env.example`/`env.default`), warn that it must also be added to the posthog/charts repository and set via posthog/secrets to take effec... ([source](https://app.greptile.com/posthog-org-19734/github/PostHog/posthog/-/custom-context?memory=c7932f06-eec7-4fbe-b35f-ac93bc7b7a80))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! |
||
|
|
||
| #### | ||
| # Feature flag billing analytics | ||
| # Used to track feature flag requests for billing purposes. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,111 @@ | ||
| # Verification query artefacts | ||
|
|
||
| ## Goal | ||
|
|
||
| Teach Signals report research runs to leave behind a reproducible measurement of the problem they found. After an implementation pull request merges, the report check coordinator can run the same measurement over a comparable window and ask Jev whether the result supports that the fix worked. | ||
|
|
||
| The full evidence chain must stay inspectable: | ||
|
|
||
| 1. Research writes a `verification_query` artefact with the query, baseline result, decision criteria, and MCP provenance. | ||
| 2. A check binds that artefact to a specific pull request and soak window. | ||
| 3. The check runs the query over the current window and sends the baseline and current result to Jev. | ||
| 4. Evaluation writes a `verification_result` artefact that points to both the verification query and the pull request. | ||
|
|
||
| ## Artefact contract | ||
|
|
||
| `verification_query` is an append-only, latest-wins status artefact. The newest valid definition is the report's canonical verification recipe, while older definitions remain in the activity history. | ||
|
|
||
| The payload contains: | ||
|
|
||
| - A short description of what the query measures. | ||
| - An executable HogQL template with allowlisted UTC window placeholders. | ||
| - A normalized baseline result with its exact window. | ||
| - Success and inconclusive criteria. | ||
| - Opportunity evidence, normally returned as a denominator in the same query. | ||
| - MCP tool names and documentation references used to construct the query. | ||
|
|
||
| Results are bounded and JSON-only. Verification queries should return aggregate evidence rather than raw event or person rows. | ||
|
|
||
| ## Research behavior | ||
|
|
||
| The research protocol must tell the agent to prepare verification evidence while it investigates the report: | ||
|
|
||
| - Discover event and property shapes before querying them. | ||
| - Use the documented MCP tool schema rather than guessing arguments. | ||
| - Prefer a single aggregate query that returns both the symptom and its opportunity denominator. | ||
| - Use equal-length windows and explicit UTC bounds. | ||
| - Run the exact query before proposing it. | ||
| - Never emit a verification query from a failed, truncated, or hypothetical call. | ||
| - Fall back to the existing manual verification note for reports that are not meaningfully queryable. | ||
|
|
||
| The final structured turn serializes evidence already collected during research. It does not perform new research. | ||
|
|
||
| ## Trusted snapshots | ||
|
|
||
| The model nominates the successful MCP call that produced the query and result. Before persistence, the server re-runs the read-only query with the claimed snapshot bounds and requires the columns and rows to match. An invented, stale, or failed snapshot is dropped. | ||
|
|
||
| The pipeline rejects: | ||
|
|
||
| - Queries without both window placeholders. | ||
| - Queries that cannot be executed as HogQL by the report team's query engine. | ||
| - Results over the row or column limits. | ||
| - Results whose shape does not match the declared evidence columns. | ||
| - Results containing anything other than aggregate numbers, booleans, or nulls. | ||
|
|
||
| ## Access control | ||
|
|
||
| The artefact is generated by the research pipeline and is not writable through the generic artefact API. | ||
|
|
||
| The query and result follow the same access policy as report metric queries and snapshots. A viewer without access receives the artefact metadata and decision description, with the query and snapshot values redacted. | ||
|
|
||
| Only aggregate numeric, boolean, or null result cells may leave PostHog for Jev. Verification queries must not return identifiers, property values, URLs, names, free text, exception messages, or other row-level data. | ||
|
|
||
| ## Pull request binding and execution | ||
|
|
||
| The research artefact is report-level because it normally predates implementation. When a linked implementation pull request merges, the system creates a `verification_query` report check that references both the source artefact and the pull request. | ||
|
|
||
| The check: | ||
|
|
||
| 1. Starts one hour after merge and runs once an hour for up to 14 days. | ||
| 2. Runs the stored query over the available post-merge window, capped at the snapshot duration. Jev treats the shorter early windows as inconclusive when they are not yet comparable. | ||
| 3. Refuses to conclude success when the opportunity evidence is missing or too small. | ||
| 4. Sends the description, query, baseline result, current result, and decision criteria to Jev. | ||
| 5. Maps `solved` to `passed`, `not_solved` to `failed`, and `insufficient_evidence` to the retryable inconclusive path. | ||
| 6. Writes a `verification_result` associated with the exact pull request and source verification artefact. | ||
| 7. Stops early only when Jev returns a sufficiently confident `solved`; `not_solved`, inconclusive, and provider-error results continue hourly until the 14-day horizon. | ||
|
|
||
| When the previous verification result is still the report's newest artefact and belongs to the same query and pull request, the next attempt replaces that row. If another pull request or any other report activity arrived in between, the next attempt appends a new result instead. This keeps a quiet report's work log compact without erasing the boundary created by meaningful intervening work. | ||
|
|
||
| Low-confidence model answers are inconclusive. A deterministic metric comparison should continue to use the existing `metric_threshold` check kind instead of Jev. | ||
|
|
||
| ## Inbox presentation | ||
|
|
||
| The report activity log shows: | ||
|
|
||
| - `Verification query prepared` with the description and baseline window. | ||
| - The HogQL and captured row count. | ||
| - The later verification outcome, explanation, confidence, and model. | ||
|
|
||
| Restricted query data remains visibly redacted rather than disappearing. | ||
|
|
||
| ## Test coverage | ||
|
|
||
| Extend the nearest existing tests to cover: | ||
|
|
||
| - Artefact enum and schema parity. | ||
| - Query/result bounds and placeholder validation. | ||
| - Research output with a verified query and with the manual fallback. | ||
| - Latest-wins re-research behavior and task attribution. | ||
| - API write protection and restricted-data redaction. | ||
| - Check creation from a merged pull request. | ||
| - Jev response parsing, confidence gating, and retryable failures. | ||
| - Pull request and verification-query provenance on `verification_result` rows. | ||
| - Frontend rendering for complete and redacted artefacts. | ||
|
|
||
| Use invented fixtures that cover clear recovery, continued failure, missing opportunity, tiny baselines, schema drift, and fixes whose intended effect is a behavior change rather than complete disappearance. Do not commit production query results from the exploratory dataset. | ||
|
|
||
| ## Operational controls | ||
|
|
||
| The TypeSafe API key, base URL, model, minimum confidence, and egress budgets are environment settings. Requests use the shared egress limiter on the batch lane. Rate-limit and provider failures become retryable errored attempts rather than verdicts. | ||
|
|
||
| Research generation and snapshot-validation failures are logged. The latest `verification_result` for each uninterrupted verification period provides the durable Jev outcome, confidence, model, and explanation for later analysis. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔒 Security & Privacy | 🛡️ Analyzed with Security Review | 🟠 Major | ⚡ Quick win
Security Misconfiguration
Reachability: Internal
Exploitability: Difficult
CWE: CWE-319 — Cleartext Transmission of Sensitive Information
Require HTTPS before sending the bearer credential.
typesafe_requestsendsapi_keyto anyurlscheme. Anhttp://base URL override sends the TypeSafe credential in cleartext. Reject non-HTTPS URLs in this transport before it creates the Authorization header.🤖 Prompt for AI Agents