Repository navigation
feat(tasks): skip CI follow-up reruns when PR is closed or unchanged - #93641
Closed
sortafreel wants to merge 19 commits into
Closed
sortafreel wants to merge 19 commits into
sortafreel wants to merge 19 commits into
Conversation
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…_context.py Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: graphite-app[bot] <96075541+graphite-app[bot]@users.noreply.github.com>
…low-up Adds a sequence-driven mock and a new test that walks the workflow through fire→skip→fire→fire ticks, asserting the persisted fingerprint causes the middle tick to skip. Also moves the state-reset fixture onto TestFollowupGuards so it doesn't inherit leftover state from earlier classes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Prompt To Fix All With AI### Issue 1
products/tasks/backend/temporal/process_task/workflow.py:207-212
**PR context activity is unregistered**
When a workflow reaches its first scheduled CI follow-up, `_should_run_ci_follow_up` schedules `get_pr_context`, but the production task worker does not register that activity type, causing the activity to retry and fail instead of deciding whether to send the follow-up.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "Merge branch 'master' into ryan/prevent_..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+207
to
+212
| pr_context = await workflow.execute_activity( | ||
| get_pr_context, | ||
| GetPrContextInput(context=self.context), | ||
| start_to_close_timeout=timedelta(minutes=5), | ||
| retry_policy=RetryPolicy(maximum_attempts=3), | ||
| ) |
Contributor
There was a problem hiding this comment.
When a workflow reaches its first scheduled CI follow-up, _should_run_ci_follow_up schedules get_pr_context, but the production task worker does not register that activity type, causing the activity to retry and fail instead of deciding whether to send the follow-up.
Prompt To Fix With AI
This is a comment left during a code review.
Path: products/tasks/backend/temporal/process_task/workflow.py
Line: 207-212
Comment:
**PR context activity is unregistered**
When a workflow reaches its first scheduled CI follow-up, `_should_run_ci_follow_up` schedules `get_pr_context`, but the production task worker does not register that activity type, causing the activity to retry and fail instead of deciding whether to send the follow-up.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Contributor
🦔 PostHog Review reviewed this pull requestFound 3 must fix, 2 should fix, 0 consider. Published 5 findings (view the review). |
Contributor
|
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}
Comment on lines
+72
to
+74
| pull_request = github_integration.get_pull_request_from_url(pr_url) # Validate PR URL and permissions | ||
| if not pull_request.get("success"): | ||
| return None |
Contributor
There was a problem hiding this comment.
GitHub failures bypass the activity retry policy
Why we think it's a valid issue
- Checked: the full call chain from the activity to the GitHub client and back to the workflow gate:
get_pr_context(products/tasks/backend/temporal/process_task/activities/get_pr_context.py:71-81),GitHubIntegration.get_pull_request/get_pull_request_from_url(posthog/models/integration.py:3210-3270),_installation_authenticated_get(posthog/models/integration.py:2246-2278), the caller_should_run_ci_follow_up(products/tasks/backend/temporal/process_task/workflow.py:206-243), and the exception classes in products/tasks/backend/temporal/exceptions.py. - Found:
_installation_authenticated_getreturnsNonefor any request exception (posthog/models/integration.py:2276-2278) and after a failed token refresh on 401 (posthog/models/integration.py:2271-2273).get_pull_requestturns thatNoneinto{"success": False, ...}(posthog/models/integration.py:3214-3215) and does the same for every non-200 status, which includes 429 and 5xx (posthog/models/integration.py:3216-3221). No exception reaches the activity, so theexcept Exceptionblock at get_pr_context.py:76 never runs for these failures. - Found: the activity returns
Noneat get_pr_context.py:73-74 with no log line. Temporal marks the activity successful, soretry_policy=RetryPolicy(maximum_attempts=3)at workflow.py:211 never applies. The author configured that retry policy, so the intent to retry exists but the code defeats it. - Found: the workflow then takes the
if not pr_contextbranch at workflow.py:213-218, logs"PR context is missing, skipping CI follow-up", and returnsFalse. A GitHub outage, a rate limit, and a real "no PR URL" case all produce the same message, so an operator cannot tell them apart. - Found: the suggestion to re-raise outside the broad handler is correct for this code. A
ProcessTaskTransientErrorraised inside thetrywould be caught at get_pr_context.py:76 and rewrapped asTaskInvalidStateError, which subclassesProcessTaskFatalErrorand setsnon_retryable=True(products/tasks/backend/temporal/exceptions.py:22-40). - Impact: before this PR the CI follow-up ran on every CI event. Now one transient GitHub failure suppresses the follow-up for that event, and the agent does not react to that CI result. The stored fingerprint stays unchanged, so a later CI event can still trigger the follow-up, but if the failed fetch happens on the last CI event of a run, the follow-up is lost and the run waits for the inactivity timeout. This is a swallowed error that hides a failure mode which will occur in normal operation.
Issue description
get_pull_request() converts network failures and all non-200 responses into unsuccessful results. This activity converts each result to None. Temporal records a successful activity and does not apply the configured retry policy. A short GitHub outage, 429, or 5xx response can suppress the CI follow-up for that check. The workflow log also reports the failure as missing PR context, which hides the cause.
Suggested fix
Raise a retryable ProcessTaskTransientError for network failures, 429 responses, and 5xx responses. Re-raise that error outside the broad exception handler. Return None only for permanent cases, and log the status and reason for those cases.
Prompt to fix with AI (copy-paste)
## Context
@products/tasks/backend/temporal/process_task/activities/get_pr_context.py#L72-74
<issue_description>
`get_pull_request()` converts network failures and all non-200 responses into unsuccessful results. This activity converts each result to `None`. Temporal records a successful activity and does not apply the configured retry policy. A short GitHub outage, 429, or 5xx response can suppress the CI follow-up for that check. The workflow log also reports the failure as missing PR context, which hides the cause.
</issue_description>
<issue_validation>
- **Checked:** the full call chain from the activity to the GitHub client and back to the workflow gate: `get_pr_context` (products/tasks/backend/temporal/process_task/activities/get_pr_context.py:71-81), `GitHubIntegration.get_pull_request` / `get_pull_request_from_url` (posthog/models/integration.py:3210-3270), `_installation_authenticated_get` (posthog/models/integration.py:2246-2278), the caller `_should_run_ci_follow_up` (products/tasks/backend/temporal/process_task/workflow.py:206-243), and the exception classes in products/tasks/backend/temporal/exceptions.py.
- **Found:** `_installation_authenticated_get` returns `None` for any request exception (posthog/models/integration.py:2276-2278) and after a failed token refresh on 401 (posthog/models/integration.py:2271-2273). `get_pull_request` turns that `None` into `{"success": False, ...}` (posthog/models/integration.py:3214-3215) and does the same for every non-200 status, which includes 429 and 5xx (posthog/models/integration.py:3216-3221). No exception reaches the activity, so the `except Exception` block at get_pr_context.py:76 never runs for these failures.
- **Found:** the activity returns `None` at get_pr_context.py:73-74 with no log line. Temporal marks the activity successful, so `retry_policy=RetryPolicy(maximum_attempts=3)` at workflow.py:211 never applies. The author configured that retry policy, so the intent to retry exists but the code defeats it.
- **Found:** the workflow then takes the `if not pr_context` branch at workflow.py:213-218, logs `"PR context is missing, skipping CI follow-up"`, and returns `False`. A GitHub outage, a rate limit, and a real "no PR URL" case all produce the same message, so an operator cannot tell them apart.
- **Found:** the suggestion to re-raise outside the broad handler is correct for this code. A `ProcessTaskTransientError` raised inside the `try` would be caught at get_pr_context.py:76 and rewrapped as `TaskInvalidStateError`, which subclasses `ProcessTaskFatalError` and sets `non_retryable=True` (products/tasks/backend/temporal/exceptions.py:22-40).
- **Impact:** before this PR the CI follow-up ran on every CI event. Now one transient GitHub failure suppresses the follow-up for that event, and the agent does not react to that CI result. The stored fingerprint stays unchanged, so a later CI event can still trigger the follow-up, but if the failed fetch happens on the last CI event of a run, the follow-up is lost and the run waits for the inactivity timeout. This is a swallowed error that hides a failure mode which will occur in normal operation.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Raise a retryable `ProcessTaskTransientError` for network failures, 429 responses, and 5xx responses. Re-raise that error outside the broad exception handler. Return `None` only for permanent cases, and log the status and reason for those cases.
</potential_solution>
Comment on lines
+32
to
+35
| pr_url = pr.get("url", "") | ||
| updated_at = pr.get("updated_at", "") | ||
| fingerprint_source = f"{pr_url}|{updated_at}" | ||
| return hashlib.sha256(fingerprint_source.encode()).hexdigest() |
Contributor
There was a problem hiding this comment.
Fingerprint ignores CI result changes
Why we think it's a valid issue
- Checked: how the CI follow-up is triggered, what the gate compares, and what the fingerprint covers:
_wait_for_ci_follow_upandCI_FOLLOW_UP_DELAY(products/tasks/backend/temporal/process_task/workflow.py:77, 138-152),DEFAULT_CI_MESSAGE(workflow.py:80-85),_should_run_ci_follow_up(workflow.py:206-243),compute_pr_fingerprint(products/tasks/backend/temporal/process_task/activities/get_pr_context.py:28-35), and the fieldsGitHubIntegration.get_pull_requestreturns (posthog/models/integration.py:3237-3262). - Found: the CI follow-up is not webhook-driven. It is a repeating timer that fires 15 minutes after the last agent activity (workflow.py:77, 138-152), so the gate runs as a poll. Each poll asks one question: did
updated_atmove since the last follow-up. A pull requestupdated_attracks issue-level activity (commits, comments, reviews, state). A check run belongs to the commit, not to the pull request record, so its transition tocompleted/failureleavesupdated_atunchanged. - Found: the gate at workflow.py:227-242 stores the fingerprint when a follow-up fires and skips every later poll that sees the same value. Nothing in the run resets it. The new test
test_skips_ci_follow_up_when_fingerprint_unchanged(products/tasks/backend/temporal/process_task/tests/test_followup.py:473-500) codifies this: with a constant fingerprint the workflow dispatches exactly one follow-up for the whole run. - Found: the reachable sequence needs no unusual conditions. The agent pushes and opens the PR, so
updated_atis T0. Fifteen minutes later poll 1 sees a new fingerprint, sends the CI message, and stores F(T0). Checks are often still running at that point, so the agent finds nothing to fix and pushes nothing — and reading logs or comments does not changeupdated_at. Checks then finish red. Poll 2 and every later poll compute F(T0) again, log "PR context has not changed", and skip. The remaining budget ofMAX_CI_REPETITIONS = 3(workflow.py:78) is never used. - Found:
compute_pr_fingerprintuses onlyurlandupdated_at(get_pr_context.py:32-34), although the activity already receiveshead_sha(posthog/models/integration.py:3248) andstate(posthog/models/integration.py:3243) in the same response. - Impact: the gate can permanently disable the CI-fix loop for a run in the exact case the loop exists for — a red check on an unchanged head commit. The failure is silent: the workflow logs a normal "no change" skip and ends at the inactivity timeout with a failing pull request. The loop only keeps working when each follow-up produces a push, which is the case that needs it least.
Issue description
GitHub does not update a pull request's updated_at when a check run changes state or finishes. A failed check on the same commit therefore produces the same fingerprint. _should_run_ci_follow_up then skips the follow-up that should fix the failure.
Suggested fix
Fetch checks for head_sha. Hash the head SHA and each relevant check's status, conclusion, and completion time. Add a test where checks change while updated_at stays constant.
Prompt to fix with AI (copy-paste)
## Context
@products/tasks/backend/temporal/process_task/activities/get_pr_context.py#L32-35
<issue_description>
GitHub does not update a pull request's `updated_at` when a check run changes state or finishes. A failed check on the same commit therefore produces the same fingerprint. `_should_run_ci_follow_up` then skips the follow-up that should fix the failure.
</issue_description>
<issue_validation>
- **Checked:** how the CI follow-up is triggered, what the gate compares, and what the fingerprint covers: `_wait_for_ci_follow_up` and `CI_FOLLOW_UP_DELAY` (products/tasks/backend/temporal/process_task/workflow.py:77, 138-152), `DEFAULT_CI_MESSAGE` (workflow.py:80-85), `_should_run_ci_follow_up` (workflow.py:206-243), `compute_pr_fingerprint` (products/tasks/backend/temporal/process_task/activities/get_pr_context.py:28-35), and the fields `GitHubIntegration.get_pull_request` returns (posthog/models/integration.py:3237-3262).
- **Found:** the CI follow-up is not webhook-driven. It is a repeating timer that fires 15 minutes after the last agent activity (workflow.py:77, 138-152), so the gate runs as a poll. Each poll asks one question: did `updated_at` move since the last follow-up. A pull request `updated_at` tracks issue-level activity (commits, comments, reviews, state). A check run belongs to the commit, not to the pull request record, so its transition to `completed`/`failure` leaves `updated_at` unchanged.
- **Found:** the gate at workflow.py:227-242 stores the fingerprint when a follow-up fires and skips every later poll that sees the same value. Nothing in the run resets it. The new test `test_skips_ci_follow_up_when_fingerprint_unchanged` (products/tasks/backend/temporal/process_task/tests/test_followup.py:473-500) codifies this: with a constant fingerprint the workflow dispatches exactly one follow-up for the whole run.
- **Found:** the reachable sequence needs no unusual conditions. The agent pushes and opens the PR, so `updated_at` is T0. Fifteen minutes later poll 1 sees a new fingerprint, sends the CI message, and stores F(T0). Checks are often still running at that point, so the agent finds nothing to fix and pushes nothing — and reading logs or comments does not change `updated_at`. Checks then finish red. Poll 2 and every later poll compute F(T0) again, log "PR context has not changed", and skip. The remaining budget of `MAX_CI_REPETITIONS = 3` (workflow.py:78) is never used.
- **Found:** `compute_pr_fingerprint` uses only `url` and `updated_at` (get_pr_context.py:32-34), although the activity already receives `head_sha` (posthog/models/integration.py:3248) and `state` (posthog/models/integration.py:3243) in the same response.
- **Impact:** the gate can permanently disable the CI-fix loop for a run in the exact case the loop exists for — a red check on an unchanged head commit. The failure is silent: the workflow logs a normal "no change" skip and ends at the inactivity timeout with a failing pull request. The loop only keeps working when each follow-up produces a push, which is the case that needs it least.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Fetch checks for `head_sha`. Hash the head SHA and each relevant check's status, conclusion, and completion time. Add a test where checks change while `updated_at` stays constant.
</potential_solution>
Comment on lines
+318
to
+324
| if await self._should_run_ci_follow_up(): | ||
| self._ci_repetitions += 1 | ||
| ci_message = self.context.ci_prompt or DEFAULT_CI_MESSAGE | ||
|
|
||
| self._last_active_time = workflow.now() # Reset inactivity timer on CI follow-up | ||
|
|
||
| await self._send_followup_to_sandbox(ci_message, []) |
Contributor
There was a problem hiding this comment.
Workflow command change breaks replay of active runs
Why we think it's a valid issue
- Checked: what command the CI branch emitted before this change, whether the tasks worker can isolate old histories, whether this repository already treats replay compatibility as a real constraint, and how long a
process-taskrun can stay open. - Found: the new call sits inside the
TaskEvent.CI_FOLLOW_UPbranch and runs before the existing one._should_run_ci_follow_upschedulesget_pr_context(products/tasks/backend/temporal/process_task/workflow.py:207-212) and only then can_send_followup_to_sandboxschedulesend_followup_to_sandbox(products/tasks/backend/temporal/process_task/workflow.py:318-324, 805-814). A history recorded before the deploy hasActivityTaskScheduled(send_followup_to_sandbox)at that position, so the replayed command does not match. - Found: nothing isolates old histories from new workers. A repository-wide grep for
use_worker_versioning,build_id, andWorkerDeploymentreturns no Temporal worker versioning, the worker is built without version options (posthog/temporal/common/worker.py:290-325), and every task run goes to the single queueTASKS_TASK_QUEUE(posthog/settings/temporal.py:79, posthog/management/commands/start_temporal_worker.py:250, products/tasks/backend/temporal/client.py:117). - Found: the repository already applies the exact remedy elsewhere, so this is a known constraint here and not a general Temporal caution.
posthog/temporal/subscriptions/workflows.py:50-68documents the rolling-deploy reason and the two-steppatched()→deprecate_patch()cleanup, andposthog/temporal/llm_analytics/run_evaluation.py:1023, 1040, 1167, 1185gates command changes withtemporalio.workflow.patched(...). No patch guard exists anywhere in products/tasks. - Found: affected runs are long-lived and reachable. With a CI follow-up scheduled the inactivity window widens to 16 minutes (products/tasks/backend/temporal/process_task/workflow.py:163-167), the follow-up activity allows 35 minutes (workflow.py:813), and up to
MAX_CI_REPETITIONS = 3cycles can run (workflow.py:78).client.start_workflowsets noexecution_timeout(products/tasks/backend/temporal/client.py:112-119), so a run stays open across a deploy. - Impact: for a run that already dispatched one CI follow-up when a deploy lands, replay raises a nondeterminism error. The worker is not configured with
workflow_failure_exception_types, so this fails the workflow task and Temporal retries it forever instead of failing the run. The task hangs in progress, the sandbox stays up, and no completion status is written, so an operator must terminate it by hand. - Impact: the affected population is bounded by the
tasks-pr-loopflag, since only flagged runs execute this branch. That narrows the blast radius but does not remove it, and the fix is oneworkflow.patchedgate with a precedent already in the repository.
Issue description
This change inserts get_pr_context before the existing follow-up activity command. A workflow that started before deployment replays history that schedules send_followup_to_sandbox at this point. The new worker schedules a different first command, which causes a Temporal nondeterminism failure. The tasks worker uses one task queue and has no worker-version routing.
Suggested fix
Guard the command change with workflow.patched("tasks-pr-context-ci-follow-up"). Preserve the original follow-up command sequence on the legacy branch. Run the new PR check only on the patched branch. Keep the patch until all old histories drain, then use the repository's deprecate_patch cleanup sequence.
Prompt to fix with AI (copy-paste)
## Context
@products/tasks/backend/temporal/process_task/workflow.py#L318-324
<issue_description>
This change inserts `get_pr_context` before the existing follow-up activity command. A workflow that started before deployment replays history that schedules `send_followup_to_sandbox` at this point. The new worker schedules a different first command, which causes a Temporal nondeterminism failure. The tasks worker uses one task queue and has no worker-version routing.
</issue_description>
<issue_validation>
- **Checked:** what command the CI branch emitted before this change, whether the tasks worker can isolate old histories, whether this repository already treats replay compatibility as a real constraint, and how long a `process-task` run can stay open.
- **Found:** the new call sits inside the `TaskEvent.CI_FOLLOW_UP` branch and runs before the existing one. `_should_run_ci_follow_up` schedules `get_pr_context` (products/tasks/backend/temporal/process_task/workflow.py:207-212) and only then can `_send_followup_to_sandbox` schedule `send_followup_to_sandbox` (products/tasks/backend/temporal/process_task/workflow.py:318-324, 805-814). A history recorded before the deploy has `ActivityTaskScheduled(send_followup_to_sandbox)` at that position, so the replayed command does not match.
- **Found:** nothing isolates old histories from new workers. A repository-wide grep for `use_worker_versioning`, `build_id`, and `WorkerDeployment` returns no Temporal worker versioning, the worker is built without version options (posthog/temporal/common/worker.py:290-325), and every task run goes to the single queue `TASKS_TASK_QUEUE` (posthog/settings/temporal.py:79, posthog/management/commands/start_temporal_worker.py:250, products/tasks/backend/temporal/client.py:117).
- **Found:** the repository already applies the exact remedy elsewhere, so this is a known constraint here and not a general Temporal caution. `posthog/temporal/subscriptions/workflows.py:50-68` documents the rolling-deploy reason and the two-step `patched()` → `deprecate_patch()` cleanup, and `posthog/temporal/llm_analytics/run_evaluation.py:1023, 1040, 1167, 1185` gates command changes with `temporalio.workflow.patched(...)`. No patch guard exists anywhere in products/tasks.
- **Found:** affected runs are long-lived and reachable. With a CI follow-up scheduled the inactivity window widens to 16 minutes (products/tasks/backend/temporal/process_task/workflow.py:163-167), the follow-up activity allows 35 minutes (workflow.py:813), and up to `MAX_CI_REPETITIONS = 3` cycles can run (workflow.py:78). `client.start_workflow` sets no `execution_timeout` (products/tasks/backend/temporal/client.py:112-119), so a run stays open across a deploy.
- **Impact:** for a run that already dispatched one CI follow-up when a deploy lands, replay raises a nondeterminism error. The worker is not configured with `workflow_failure_exception_types`, so this fails the workflow task and Temporal retries it forever instead of failing the run. The task hangs in progress, the sandbox stays up, and no completion status is written, so an operator must terminate it by hand.
- **Impact:** the affected population is bounded by the `tasks-pr-loop` flag, since only flagged runs execute this branch. That narrows the blast radius but does not remove it, and the fix is one `workflow.patched` gate with a precedent already in the repository.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Guard the command change with `workflow.patched("tasks-pr-context-ci-follow-up")`. Preserve the original follow-up command sequence on the legacy branch. Run the new PR check only on the patched branch. Keep the patch until all old histories drain, then use the repository's `deprecate_patch` cleanup sequence.
</potential_solution>
Comment on lines
+59
to
+60
| pr_url = (task_run.output or {}).get("pr_url") | ||
| if not pr_url: |
Contributor
There was a problem hiding this comment.
PR context assumes task output is a dictionary
Why we think it's a valid issue
- Checked: the
outputfield definition, every write path that can set it, how the repository's other readers treat it, and where an exception at this line lands. - Found: nothing constrains
outputto a dictionary. The model field ismodels.JSONField(blank=True, null=True)(products/tasks/backend/models.py:477-481), the write serializer is a bareserializers.JSONField()(products/tasks/backend/serializers.py:416-419), andset_outputassigns the value straight through after only an optionaljsonschema.validateagainst the task's own schema (products/tasks/backend/api.py:1151-1165).resolve_schemareturns a raw dict schema unchanged (products/tasks/backend/models.py:45-48), so a task can carry a schema with a non-object root and store a list, string, or number as a valid output. No backend code writespr_url; the sandbox agent writes it through these API endpoints with atask:writekey, so the shape is client-controlled. - Found: the repository already treats this field as untyped at read time. The same
pr_urlread is guarded three times in products/tasks/backend/api.py:905, 967, and 974 withisinstance(task_run.output, dict), and the merge branch at api.py:908-912 also checksisinstance(value, dict)before merging. - Found: the sibling activity has the same unguarded expression at products/tasks/backend/temporal/process_task/activities/post_slack_update.py:36, but it sits inside a
try(line 31) whoseexcept Exceptionlogs and returns (lines 77-78). Line 59 of the new activity is above bothtryblocks (get_pr_context.py:62 and 71), so nothing catches it. - Impact: a non-dict output raises
AttributeErrorin the activity. The activity exhaustsRetryPolicy(maximum_attempts=3)(products/tasks/backend/temporal/process_task/workflow.py:211), the error reaches_should_run_ci_follow_up, escapes the event loop, and lands in the outer handler at workflow.py:402, which sets the run tofailed(workflow.py:425). The intended behavior for a run with no usablepr_urlis to skip the follow-up and continue, so this turns a benign skip into a failed task run — the exact case the new guard exists to handle.
Issue description
TaskRun.output accepts any JSON value, but this line assumes every truthy value is a dictionary. A valid string, number, or list raises AttributeError before the guarded GitHub call. The activity then fails instead of treating the PR context as missing. Other task-run access paths already use an isinstance(..., dict) check.
Suggested fix
Read task_run.output into a local value. Return None unless it is a dictionary and pr_url is a nonempty string. Add parameterized tests for list, string, and numeric outputs.
Prompt to fix with AI (copy-paste)
## Context
@products/tasks/backend/temporal/process_task/activities/get_pr_context.py#L59-60
<issue_description>
`TaskRun.output` accepts any JSON value, but this line assumes every truthy value is a dictionary. A valid string, number, or list raises `AttributeError` before the guarded GitHub call. The activity then fails instead of treating the PR context as missing. Other task-run access paths already use an `isinstance(..., dict)` check.
</issue_description>
<issue_validation>
- **Checked:** the `output` field definition, every write path that can set it, how the repository's other readers treat it, and where an exception at this line lands.
- **Found:** nothing constrains `output` to a dictionary. The model field is `models.JSONField(blank=True, null=True)` (products/tasks/backend/models.py:477-481), the write serializer is a bare `serializers.JSONField()` (products/tasks/backend/serializers.py:416-419), and `set_output` assigns the value straight through after only an optional `jsonschema.validate` against the task's own schema (products/tasks/backend/api.py:1151-1165). `resolve_schema` returns a raw dict schema unchanged (products/tasks/backend/models.py:45-48), so a task can carry a schema with a non-object root and store a list, string, or number as a valid output. No backend code writes `pr_url`; the sandbox agent writes it through these API endpoints with a `task:write` key, so the shape is client-controlled.
- **Found:** the repository already treats this field as untyped at read time. The same `pr_url` read is guarded three times in products/tasks/backend/api.py:905, 967, and 974 with `isinstance(task_run.output, dict)`, and the merge branch at api.py:908-912 also checks `isinstance(value, dict)` before merging.
- **Found:** the sibling activity has the same unguarded expression at products/tasks/backend/temporal/process_task/activities/post_slack_update.py:36, but it sits inside a `try` (line 31) whose `except Exception` logs and returns (lines 77-78). Line 59 of the new activity is above both `try` blocks (get_pr_context.py:62 and 71), so nothing catches it.
- **Impact:** a non-dict output raises `AttributeError` in the activity. The activity exhausts `RetryPolicy(maximum_attempts=3)` (products/tasks/backend/temporal/process_task/workflow.py:211), the error reaches `_should_run_ci_follow_up`, escapes the event loop, and lands in the outer handler at workflow.py:402, which sets the run to `failed` (workflow.py:425). The intended behavior for a run with no usable `pr_url` is to skip the follow-up and continue, so this turns a benign skip into a failed task run — the exact case the new guard exists to handle.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Read `task_run.output` into a local value. Return `None` unless it is a dictionary and `pr_url` is a nonempty string. Add parameterized tests for list, string, and numeric outputs.
</potential_solution>
Comment on lines
+206
to
+245
| async def _should_run_ci_follow_up(self) -> bool: | ||
| pr_context = await workflow.execute_activity( | ||
| get_pr_context, | ||
| GetPrContextInput(context=self.context), | ||
| start_to_close_timeout=timedelta(minutes=5), | ||
| retry_policy=RetryPolicy(maximum_attempts=3), | ||
| ) | ||
| if not pr_context: | ||
| workflow.logger.info( | ||
| "PR context is missing, skipping CI follow-up", | ||
| run_id=self.context.run_id, | ||
| ) | ||
| return False | ||
| if pr_context.pr_state == "closed": | ||
| workflow.logger.info( | ||
| "PR is closed, skipping CI follow-up", | ||
| run_id=self.context.run_id, | ||
| pr_url=pr_context.pr_url, | ||
| pr_state=pr_context.pr_state, | ||
| ) | ||
| return False | ||
| if self._pr_fingerprint != pr_context.fingerprint: | ||
| workflow.logger.info( | ||
| "PR context has changed, running CI follow-up", | ||
| run_id=self.context.run_id, | ||
| pr_url=pr_context.pr_url, | ||
| pr_state=pr_context.pr_state, | ||
| ) | ||
| self._pr_fingerprint = pr_context.fingerprint | ||
| return True | ||
| else: | ||
| workflow.logger.info( | ||
| "PR context has not changed, skipping CI follow-up", | ||
| run_id=self.context.run_id, | ||
| pr_url=pr_context.pr_url, | ||
| pr_state=pr_context.pr_state, | ||
| ) | ||
| return False | ||
|
|
||
| @workflow.run |
Contributor
There was a problem hiding this comment.
Skipped PR checks can spin forever and leak the sandbox
Why we think it's a valid issue
- Checked: every write to
_last_active_timeand_ci_repetitions, the sleep computation in the CI timer, how the three event tasks are rebuilt each iteration, and what the new test asserts. - Found: the CI timer returns with no sleep once the clock is already past the delay.
_wait_for_ci_follow_upcomputesremaining = CI_FOLLOW_UP_DELAY - elapsedand only sleepsif remaining.total_seconds() > 0(products/tasks/backend/temporal/process_task/workflow.py:138-152). At the moment any CI tick fires,elapsedis at least 15 minutes. - Found: the skip paths advance no clock and no counter. All three
return Falsebranches (workflow.py:213-218 missing context, 219-226 closed PR, 236-243 unchanged fingerprint) only log. Both state updates sit inside theifbody at workflow.py:318-324, so a skip leaves_last_active_timefrozen at its old value and_ci_repetitionsunchanged._ci_repetitions < MAX_CI_REPETITIONStherefore stays true (workflow.py:155-159) and the CI task is recreated on the next pass. - Found: with
_last_active_timefrozen at 15+ minutes old, each following iteration re-enters_wait_for_ci_follow_up, computesremaining <= 0, returns at once, and runsget_pr_contextagain — one activity and one GitHub API request per pass (products/tasks/backend/temporal/process_task/activities/get_pr_context.py:72, posthog/models/integration.py:3213). The only writes to_last_active_timeare a sent CI follow-up, a user follow-up, and anagent_activeheartbeat (workflow.py:322, 332, 780), so an idle agent never breaks the cycle. - Found: the inactivity timer cannot end the run during the cycle.
_wait_for_eventbuilds fresh tasks on every call and cancels the pending ones (workflow.py:169-177), so the 16-minute_wait_for_inactivitysleep set at workflow.py:164-168 restarts from zero each pass and is always cancelled first.TIMEOUT_REACHEDbecomes unreachable, and only acomplete_tasksignal can exit the loop. - Found: the closed-PR and missing-context branches are terminal states. They will not change on a later poll, so for those two the cycle has no natural end at all.
- Impact: the run spins on
get_pr_contextwith no delay. It burns the installation's GitHub API budget, which is shared with every other GitHub feature for that team, and it grows the Temporal history by several events per pass until the run hits the history limit. The sandbox stays provisioned the whole time because the loop never reaches the cleanup path after the event loop (workflow.py:357-363). - Impact: this is a regression the change introduces. The previous CI branch always set
_last_active_timeand incremented_ci_repetitions, so afterMAX_CI_REPETITIONSthe CI task stopped being scheduled, the inactivity window fell back to 5 minutes, and the run ended. - Impact: the new coverage does not bound it.
test_skips_ci_follow_up_when_fingerprint_unchangedasserts onlylen(_ci_followup_calls) == 1(products/tasks/backend/temporal/process_task/tests/test_followup.py:499), which stays true no matter how many timesget_pr_contextruns.
Issue description
The false paths change no scheduling state. After one follow-up, _last_active_time is already 15 minutes old when an unchanged check finishes. The next CI timer returns immediately and schedules get_pr_context in a tight loop. MAX_CI_REPETITIONS cannot stop this path because the code increments it only after a sent follow-up. A missing or closed PR can also keep the workflow and sandbox alive without a bound. This loop can exhaust GitHub limits and grow the Temporal history until the workflow fails.
Suggested fix
Track a separate CI check time and advance it after every check. Use a fixed inactivity deadline or a bounded poll count that skipped checks cannot reset. Disable CI polling when the PR is closed. Keep _ci_repetitions only for sent follow-ups. Add a test that checks the activity-call count and workflow completion for a stable fingerprint.
Prompt to fix with AI (copy-paste)
## Context
@products/tasks/backend/temporal/process_task/workflow.py#L206-245
@products/tasks/backend/temporal/process_task/workflow.py#L318-324
<issue_description>
The false paths change no scheduling state. After one follow-up, `_last_active_time` is already 15 minutes old when an unchanged check finishes. The next CI timer returns immediately and schedules `get_pr_context` in a tight loop. `MAX_CI_REPETITIONS` cannot stop this path because the code increments it only after a sent follow-up. A missing or closed PR can also keep the workflow and sandbox alive without a bound. This loop can exhaust GitHub limits and grow the Temporal history until the workflow fails.
</issue_description>
<issue_validation>
- **Checked:** every write to `_last_active_time` and `_ci_repetitions`, the sleep computation in the CI timer, how the three event tasks are rebuilt each iteration, and what the new test asserts.
- **Found:** the CI timer returns with no sleep once the clock is already past the delay. `_wait_for_ci_follow_up` computes `remaining = CI_FOLLOW_UP_DELAY - elapsed` and only sleeps `if remaining.total_seconds() > 0` (products/tasks/backend/temporal/process_task/workflow.py:138-152). At the moment any CI tick fires, `elapsed` is at least 15 minutes.
- **Found:** the skip paths advance no clock and no counter. All three `return False` branches (workflow.py:213-218 missing context, 219-226 closed PR, 236-243 unchanged fingerprint) only log. Both state updates sit inside the `if` body at workflow.py:318-324, so a skip leaves `_last_active_time` frozen at its old value and `_ci_repetitions` unchanged. `_ci_repetitions < MAX_CI_REPETITIONS` therefore stays true (workflow.py:155-159) and the CI task is recreated on the next pass.
- **Found:** with `_last_active_time` frozen at 15+ minutes old, each following iteration re-enters `_wait_for_ci_follow_up`, computes `remaining <= 0`, returns at once, and runs `get_pr_context` again — one activity and one GitHub API request per pass (products/tasks/backend/temporal/process_task/activities/get_pr_context.py:72, posthog/models/integration.py:3213). The only writes to `_last_active_time` are a sent CI follow-up, a user follow-up, and an `agent_active` heartbeat (workflow.py:322, 332, 780), so an idle agent never breaks the cycle.
- **Found:** the inactivity timer cannot end the run during the cycle. `_wait_for_event` builds fresh tasks on every call and cancels the pending ones (workflow.py:169-177), so the 16-minute `_wait_for_inactivity` sleep set at workflow.py:164-168 restarts from zero each pass and is always cancelled first. `TIMEOUT_REACHED` becomes unreachable, and only a `complete_task` signal can exit the loop.
- **Found:** the closed-PR and missing-context branches are terminal states. They will not change on a later poll, so for those two the cycle has no natural end at all.
- **Impact:** the run spins on `get_pr_context` with no delay. It burns the installation's GitHub API budget, which is shared with every other GitHub feature for that team, and it grows the Temporal history by several events per pass until the run hits the history limit. The sandbox stays provisioned the whole time because the loop never reaches the cleanup path after the event loop (workflow.py:357-363).
- **Impact:** this is a regression the change introduces. The previous CI branch always set `_last_active_time` and incremented `_ci_repetitions`, so after `MAX_CI_REPETITIONS` the CI task stopped being scheduled, the inactivity window fell back to 5 minutes, and the run ended.
- **Impact:** the new coverage does not bound it. `test_skips_ci_follow_up_when_fingerprint_unchanged` asserts only `len(_ci_followup_calls) == 1` (products/tasks/backend/temporal/process_task/tests/test_followup.py:499), which stays true no matter how many times `get_pr_context` runs.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Track a separate CI check time and advance it after every check. Use a fixed inactivity deadline or a bounded poll count that skipped checks cannot reset. Disable CI polling when the PR is closed. Keep `_ci_repetitions` only for sent follow-ups. Add a test that checks the activity-call count and workflow completion for a stable fingerprint.
</potential_solution>
Contributor
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, please remove the |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The
ProcessTaskWorkflowfires a CI follow-up on every CI event for the task's PR. That causes spurious reruns when the PR hasn't actually changed (e.g. duplicate webhook deliveries, unrelated check-runs) and pointless work when the PR has already been closed. Each spurious follow-up wakes the sandbox and consumes the agent's CI retry budget.Changes
GitHubIntegration.get_pull_request/get_pull_request_from_url/parse_pull_request_urlhelpers onposthog/models/integration.pyso we can fetch PR state via the installation token.get_pr_contextTemporal activity underproducts/tasks/backend/temporal/process_task/activities/that loads the task run, resolves itspr_url, fetches the PR from GitHub, and returns(pr_url, pr_state, fingerprint)where the fingerprint is a SHA-256 ofpr_url|updated_at.ProcessTaskWorkflow, gated the CI follow-up branch behind a new_should_run_ci_follow_upcheck that:closed,How did you test this code?
Manual verification against a test workflow. No automated tests added yet — happy to add coverage for
_should_run_ci_follow_upand the newGitHubIntegrationhelpers if reviewers would like it in this PR.Publish to changelog?
no
Docs update
skip-inkeep-docs