feat(tasks): let users dismiss files uploaded by a run - #78654
Conversation
|
😎 Stack merged successfully - details. |
|
Hey @adboio! 👋 It looks like your git author email on this PR isn't your
You can fix it for this repo with: git config user.email "you@posthog.com"Or set it globally with |
🤖 CI report✅ Bundle size — no changeUncompressed size of every built Total: 65.67 MiB · no change No file changed by more than 1000 B. Posted automatically by build-bundle-size-report · uncompressed bytes from dist-report ✅ Eager graph — within budgetHow much code each root ships on the eager path — downloaded and parsed before the surface is interactive. Measured from the esbuild output chunks (post-tree-shake, static imports only); lazy
🟢 Largest files eagerly shipped from
|
| 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 |
|---|---|
| 285.5 KiB | ../node_modules/.pnpm/posthog-js@1.410.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 |
| 238.4 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 231.5 KiB | ../node_modules/.pnpm/posthog-js@1.410.1/node_modules/posthog-js/dist/module.js |
| 154.3 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 |
| 105.0 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.20 MiB within budget
What the toolbar ships to customer pages, measured from the esbuild output (minified, post-tree-shake). The eager set is the entry plus everything statically imported from it — fetched before any feature runs; deferred chunks load lazily. The eager guardrail is 5.72 MiB. Each output file must also stay below 10 MB, where CloudFront stops compressing it. The module boundary is enforced separately by check-toolbar-graph.
| Metric | Size | Δ vs base | Budget |
|---|---|---|---|
| Eager (shipped) entry + static imports |
2.20 MiB · 17 files | no change | ████░░░░░░ 38.4% of 5.72 MiB |
| Deferred (lazy) | 2.08 MiB · 33 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 |
|---|---|
| 723.0 KiB | dist/toolbar/toolbar-app-M7B2P2W4.css |
| 552.3 KiB | dist/toolbar/chunk-chunk-LINCPQWW.js |
| 484.6 KiB | dist/toolbar/chunk-chunk-5HONEO7J.js |
| 133.6 KiB | dist/toolbar/chunk-chunk-PSGMVXK7.js |
| 131.8 KiB | dist/toolbar/chunk-chunk-T5KY5WYR.js |
| 71.0 KiB | dist/toolbar/toolbar-app-BIMPVI2A.js |
| 69.0 KiB | dist/toolbar/chunk-chunk-27JL52RE.js |
| 35.6 KiB | dist/toolbar/chunk-chunk-QQAXO5G5.js |
| 20.9 KiB | dist/toolbar/chunk-chunk-B5MOUYTE.js |
| 12.2 KiB | dist/toolbar/chunk-chunk-PIK3PADE.js |
Posted automatically by check-toolbar-size · sizes are toolbar output bytes (shipped, post-tree-shake) from the esbuild metafile
✅ Dist folder size — 🔺 +5.2 KiB (+0.0%)
Total size of the built frontend/dist folder (all assets), compared against the base branch.
Total: 1394.75 MiB · 🔺 +5.2 KiB (+0.0%)
ℹ️ MCP UI apps size — 32 app(s), 17074.3 KB JS
Built size of each MCP UI app (main.js + styles.css).
| App | JS | CSS |
|---|---|---|
| debug | 599.6 KB | 187.7 KB |
| action | 457.8 KB | 187.7 KB |
| action-list | 564.4 KB | 187.7 KB |
| cohort | 456.8 KB | 187.7 KB |
| cohort-list | 563.4 KB | 187.7 KB |
| email-template | 456.6 KB | 187.7 KB |
| error-details | 472.4 KB | 187.7 KB |
| error-issue | 457.5 KB | 187.7 KB |
| error-issue-list | 564.3 KB | 187.7 KB |
| experiment | 561.5 KB | 187.7 KB |
| experiment-list | 565.2 KB | 187.7 KB |
| experiment-results | 563.2 KB | 187.7 KB |
| feature-flag | 567.2 KB | 187.7 KB |
| feature-flag-list | 570.9 KB | 187.7 KB |
| feature-flag-testing | 461.0 KB | 187.7 KB |
| insight-actors | 562.2 KB | 187.7 KB |
| invite-email-preview | 456.0 KB | 187.7 KB |
| llm-costs | 559.5 KB | 187.7 KB |
| session-recording | 458.6 KB | 187.7 KB |
| session-summary | 463.9 KB | 187.7 KB |
| survey | 458.4 KB | 187.7 KB |
| survey-global-stats | 562.2 KB | 187.7 KB |
| survey-list | 565.1 KB | 187.7 KB |
| survey-stats | 562.2 KB | 187.7 KB |
| trace-span | 457.2 KB | 187.7 KB |
| trace-span-list | 564.3 KB | 187.7 KB |
| workflow | 457.1 KB | 187.7 KB |
| workflow-list | 563.7 KB | 187.7 KB |
| loops-review | 461.4 KB | 187.7 KB |
| query-results | 749.3 KB | 187.7 KB |
| render-ui | 830.0 KB | 187.7 KB |
| visual-review-snapshots | 461.6 KB | 187.7 KB |
⚠️ Backend coverage — 97.0% of changed backend lines covered — 2 uncovered
🧪 Backend test coverage
Patch coverage — changed backend lines (products + core): ███████████████████░ 97.0% (86 / 88)
| File | Patch | Uncovered changed lines |
|---|---|---|
products/tasks/backend/presentation/views/api.py |
90.0% | 1705 |
products/tasks/backend/facade/api.py |
96.0% | 3008 |
🤖 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 31131639831 -n patch-coverage), or the coverage-data block at the end of this comment.
Per-product line coverage (touched products)
| Product | Coverage | Lines |
|---|---|---|
demo |
███████████░░░░░░░░░ 56.3% |
1,497 / 2,661 |
tasks |
██████████████░░░░░░ 70.5% |
35,174 / 49,927 |
signals |
████████████████░░░░ 82.5% |
27,025 / 32,777 |
cdp |
█████████████████░░░ 84.2% |
3,914 / 4,646 |
notebooks |
█████████████████░░░ 86.4% |
8,029 / 9,296 |
data_modeling |
█████████████████░░░ 86.6% |
8,451 / 9,761 |
actions |
█████████████████░░░ 86.6% |
717 / 828 |
managed_warehouse |
█████████████████░░░ 87.1% |
6,219 / 7,141 |
cohorts |
██████████████████░░ 87.6% |
6,482 / 7,400 |
data_warehouse |
██████████████████░░ 87.8% |
11,281 / 12,843 |
product_tours |
██████████████████░░ 87.9% |
1,303 / 1,482 |
exports |
██████████████████░░ 89.2% |
7,429 / 8,328 |
engineering_analytics |
██████████████████░░ 89.2% |
6,716 / 7,527 |
dashboards |
██████████████████░░ 89.4% |
5,983 / 6,693 |
alerts |
██████████████████░░ 90.3% |
4,482 / 4,966 |
conversations |
██████████████████░░ 90.4% |
18,026 / 19,932 |
canvas |
██████████████████░░ 90.6% |
2,075 / 2,291 |
streamlit_apps |
██████████████████░░ 90.7% |
2,630 / 2,901 |
error_tracking |
██████████████████░░ 91.1% |
11,153 / 12,243 |
stamphog |
██████████████████░░ 91.3% |
4,505 / 4,936 |
slack_app |
██████████████████░░ 91.6% |
10,627 / 11,602 |
marketing_analytics |
██████████████████░░ 92.1% |
13,388 / 14,539 |
mcp_analytics |
███████████████████░ 92.7% |
3,810 / 4,112 |
ai_observability |
███████████████████░ 92.9% |
17,086 / 18,399 |
early_access_features |
███████████████████░ 92.9% |
1,347 / 1,450 |
web_analytics |
███████████████████░ 93.1% |
15,963 / 17,154 |
surveys |
███████████████████░ 93.2% |
5,858 / 6,286 |
posthog_ai |
███████████████████░ 93.3% |
1,327 / 1,423 |
reminders |
███████████████████░ 93.4% |
468 / 501 |
approvals |
███████████████████░ 93.5% |
3,491 / 3,734 |
product_analytics |
███████████████████░ 93.5% |
7,027 / 7,514 |
endpoints |
███████████████████░ 94.3% |
8,771 / 9,306 |
workflows |
███████████████████░ 94.3% |
7,900 / 8,380 |
review_hog |
███████████████████░ 94.6% |
8,246 / 8,715 |
skills |
███████████████████░ 94.8% |
3,478 / 3,669 |
logs |
███████████████████░ 95.5% |
11,202 / 11,732 |
experiments |
███████████████████░ 95.7% |
27,557 / 28,806 |
annotations |
███████████████████░ 96.2% |
732 / 761 |
revenue_analytics |
███████████████████░ 96.3% |
1,887 / 1,960 |
feature_flags |
███████████████████░ 96.4% |
17,580 / 18,235 |
replay_vision |
███████████████████░ 96.5% |
17,225 / 17,854 |
user_interviews |
███████████████████░ 96.5% |
2,638 / 2,734 |
customer_analytics |
███████████████████░ 97.1% |
10,748 / 11,070 |
warehouse_sources |
███████████████████░ 97.4% |
381,535 / 391,701 |
data_catalog |
████████████████████ 97.9% |
2,676 / 2,734 |
pulse |
████████████████████ 98.4% |
2,017 / 2,049 |
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.
adboio
added
the
run-ci-frontend
PostHog
b7b052b to
f3ac6ba
Compare
🦔 ReviewHog reviewed this pull requestFound 0 must fix, 2 should fix, 0 consider. Published 2 findings (view the review). |
|
ReviewHog 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}
There was a problem hiding this comment.
ReviewHog Report
Changes
Issues: 2 issues
Files (8)
products/tasks/backend/facade/api.pyproducts/tasks/backend/presentation/serializers.pyproducts/tasks/backend/presentation/views/api.pyproducts/tasks/frontend/generated/api.schemas.tsproducts/tasks/frontend/generated/api.tsproducts/tasks/frontend/generated/api.zod.tsproducts/tasks/mcp/tools.yamlservices/mcp/src/api/generated.ts
| def artifacts_dismiss(self, request, pk=None, **kwargs): | ||
| task_id = self._ensure_task_accessible() | ||
| manifest, error = tasks_facade.set_task_run_artifacts_dismissed( | ||
| pk, | ||
| task_id, | ||
| self.team_id, | ||
| artifact_ids=request.validated_data["artifact_ids"], | ||
| dismissed=request.validated_data["dismissed"], | ||
| ) | ||
| if error == "not_found": | ||
| raise NotFound("Artifact not found on this run") | ||
| if manifest is None: | ||
| raise NotFound() | ||
| return Response(TaskRunArtifactsDismissResponseSerializer({"artifacts": manifest}).data) |
There was a problem hiding this comment.
New dismiss endpoint's "artifact not found on this run" 404 uses a different response shape than its sibling artifact actions
Why we think it's a valid issue
- Checked: Compared how the three artifact sub-resource actions in this viewset handle the identical facade
error == "not_found"sentinel —artifacts_presign,artifacts_download, and the newartifacts_dismiss— and surveyed where bareraise NotFound()vsTaskRunErrorResponseSerializer({"error": ...})is used across the file. - Found: For the exact same condition,
artifacts_presignreturnsResponse(TaskRunErrorResponseSerializer({"error": "Artifact not found on this run"}), 404)(views/api.py:1564-1568) andartifacts_downloaddoes the identical thing (1607-1611). The newartifacts_dismiss(PR diff) doesraise NotFound("Artifact not found on this run"), which DRF serializes as{"detail": ...}. Same facade sentinel, same verbatim message string, different JSON key — a real, verified divergence, not a subjective preference. - Found: The convention is genuinely nuanced but consistent: bare
raise NotFound()is the baseline for a missing/invisible run (used throughout, e.g. 1563, 1606), while the differentiated artifact-not-found-on-run error goes throughTaskRunErrorResponseSerializerin both siblings. Dismiss is the sole action that routes that specific sub-resource error through the run-missing shape. The copied message string signals the author intended to mirror the siblings, so this reads as an accidental mechanism mismatch. - Found: The 404 OpenAPI response is declared with only a bare
description(noresponse=schema), sopnpm typescript:check/generated types cannot catch the shape drift — it surfaces only at runtime. - Impact: A client with a shared
{"error"}parser across artifact actions (the stacked desktop client is the intended consumer) readsundefinedfor this one action's 404 and cannot render its message; the status is still a correct 404, so it is a contract wart, not a functional break. Worth fixing before this new endpoint (also scaffolded as an MCP tool) ships and locks the inconsistent shape in — the fix is a one-line change to match the two siblings. Not overengineering (it aligns with existing code, adds no abstraction) and not pure style (JSON body key is an observable contract).
Issue description
In artifacts_dismiss, when set_task_run_artifacts_dismissed reports error == "not_found", the view does raise NotFound("Artifact not found on this run"), which DRF serializes as {"detail": "Artifact not found on this run"}. Its two closest siblings in the very same viewset, artifacts_presign and artifacts_download, handle the identical "artifact not found on this run" condition by returning Response(TaskRunErrorResponseSerializer({"error": "Artifact not found on this run"}).data, status=status.HTTP_404_NOT_FOUND) — an {"error": ...} body built from the shared TaskRunErrorResponseSerializer that this same file already uses consistently for every other artifact-related 400/404. The new endpoint reuses the exact same English message string, showing the intent was to mirror that existing convention, but the implementation mechanism (raising a bare DRF exception vs. building an explicit Response from TaskRunErrorResponseSerializer) produces a different JSON key (detail vs error) for what is, from a caller's perspective, the same logical error on the same sub-resource. Any client code (including the stacked desktop client in #78644) that centralizes error handling for this artifact API around the {"error": ...} shape — which is what every other artifact action in this file returns — will get undefined/a missing field for this one action's 404, and has to special-case it. Because the OpenAPI 404 response for this action is declared with only a bare description (no response= schema, matching the pattern used for artifacts_presign/artifacts_download), this mismatch is invisible to generated-type checking on the frontend, so it won't be caught by pnpm typescript:check — it will only surface as an actual runtime shape difference when a client tries to read .error off a 404 response from this endpoint.
Suggested fix
Return the same shape as artifacts_presign/artifacts_download for this case: return Response(TaskRunErrorResponseSerializer({"error": "Artifact not found on this run"}).data, status=status.HTTP_404_NOT_FOUND) instead of raise NotFound("Artifact not found on this run"). This keeps a single, predictable error contract ({"error": string}) across all artifact-not-found-on-run responses in the viewset, and lets a shared client-side error parser work for every artifact action, including this new one.
Prompt to fix with AI (copy-paste)
## Context
@products/tasks/backend/presentation/views/api.py#L1601-1614
<issue_description>
In `artifacts_dismiss`, when `set_task_run_artifacts_dismissed` reports `error == "not_found"`, the view does `raise NotFound("Artifact not found on this run")`, which DRF serializes as `{"detail": "Artifact not found on this run"}`. Its two closest siblings in the very same viewset, `artifacts_presign` and `artifacts_download`, handle the identical "artifact not found on this run" condition by returning `Response(TaskRunErrorResponseSerializer({"error": "Artifact not found on this run"}).data, status=status.HTTP_404_NOT_FOUND)` — an `{"error": ...}` body built from the shared `TaskRunErrorResponseSerializer` that this same file already uses consistently for every other artifact-related 400/404. The new endpoint reuses the exact same English message string, showing the intent was to mirror that existing convention, but the implementation mechanism (raising a bare DRF exception vs. building an explicit `Response` from `TaskRunErrorResponseSerializer`) produces a different JSON key (`detail` vs `error`) for what is, from a caller's perspective, the same logical error on the same sub-resource. Any client code (including the stacked desktop client in #78644) that centralizes error handling for this artifact API around the `{"error": ...}` shape — which is what every other artifact action in this file returns — will get `undefined`/a missing field for this one action's 404, and has to special-case it. Because the OpenAPI `404` response for this action is declared with only a bare `description` (no `response=` schema, matching the pattern used for `artifacts_presign`/`artifacts_download`), this mismatch is invisible to generated-type checking on the frontend, so it won't be caught by `pnpm typescript:check` — it will only surface as an actual runtime shape difference when a client tries to read `.error` off a 404 response from this endpoint.
</issue_description>
<issue_validation>
- **Checked:** Compared how the three artifact sub-resource actions in this viewset handle the identical facade `error == "not_found"` sentinel — `artifacts_presign`, `artifacts_download`, and the new `artifacts_dismiss` — and surveyed where bare `raise NotFound()` vs `TaskRunErrorResponseSerializer({"error": ...})` is used across the file.
- **Found:** For the exact same condition, `artifacts_presign` returns `Response(TaskRunErrorResponseSerializer({"error": "Artifact not found on this run"}), 404)` (views/api.py:1564-1568) and `artifacts_download` does the identical thing (1607-1611). The new `artifacts_dismiss` (PR diff) does `raise NotFound("Artifact not found on this run")`, which DRF serializes as `{"detail": ...}`. Same facade sentinel, same verbatim message string, different JSON key — a real, verified divergence, not a subjective preference.
- **Found:** The convention is genuinely nuanced but consistent: bare `raise NotFound()` is the baseline for a missing/invisible *run* (used throughout, e.g. 1563, 1606), while the differentiated *artifact-not-found-on-run* error goes through `TaskRunErrorResponseSerializer` in both siblings. Dismiss is the sole action that routes that specific sub-resource error through the run-missing shape. The copied message string signals the author intended to mirror the siblings, so this reads as an accidental mechanism mismatch.
- **Found:** The 404 OpenAPI response is declared with only a bare `description` (no `response=` schema), so `pnpm typescript:check`/generated types cannot catch the shape drift — it surfaces only at runtime.
- **Impact:** A client with a shared `{"error"}` parser across artifact actions (the stacked desktop client is the intended consumer) reads `undefined` for this one action's 404 and cannot render its message; the status is still a correct 404, so it is a contract wart, not a functional break. Worth fixing before this new endpoint (also scaffolded as an MCP tool) ships and locks the inconsistent shape in — the fix is a one-line change to match the two siblings. Not overengineering (it aligns with existing code, adds no abstraction) and not pure style (JSON body key is an observable contract).
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Return the same shape as `artifacts_presign`/`artifacts_download` for this case: `return Response(TaskRunErrorResponseSerializer({"error": "Artifact not found on this run"}).data, status=status.HTTP_404_NOT_FOUND)` instead of `raise NotFound("Artifact not found on this run")`. This keeps a single, predictable error contract (`{"error": string}`) across all artifact-not-found-on-run responses in the viewset, and lets a shared client-side error parser work for every artifact action, including this new one.
</potential_solution>
There was a problem hiding this comment.
valid — fixed in a5c0775. Now returns TaskRunErrorResponseSerializer({"error": ...}) with a 404, matching artifacts_presign and artifacts_download.
Your note that generated types could not catch this turned out to matter more than the shape itself: chasing it surfaced that dismissed_at was declared allow_null=True, and DRF checks allow_null before required in get_attribute, so a missing key serialized as null on every artifact — including staged user attachments that have no dismissal concept. That was breaking two existing tests. Restoring a file now drops the key instead of nulling it, so the field is only ever a string or absent, and the generated types were regenerated to match.
adboio
added
the
run-ci-backend
PostHog
Prompt To Fix All With AI### Issue 1
products/tasks/backend/facade/api.py:3003-3021
**Unlocked writers erase dismissals**
If loop-run artifact seeding or warm-run staging overlaps a dismissal, those writers can save a stale whole-manifest snapshot without participating in this row-locking protocol, removing the newly committed `dismissed_at` value and making the artifact visible again.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "chore(tasks): pin that restoring a file ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| with transaction.atomic(): | ||
| locked_run = TaskRun.objects.select_for_update().get(pk=run.pk) | ||
| manifest = list(locked_run.artifacts or []) | ||
| requested = set(artifact_ids) | ||
| if not requested.issubset({entry.get("id") for entry in manifest}): | ||
| return None, "not_found" | ||
|
|
||
| # Restoring drops the key rather than nulling it, so a manifest entry only ever carries | ||
| # ``dismissed_at`` while it is dismissed and the response shape stays a plain optional. | ||
| dismissed_at = django_timezone.now().isoformat() | ||
| manifest = [ | ||
| ( | ||
| ({**entry, "dismissed_at": dismissed_at} if dismissed else _without_dismissal(entry)) | ||
| if entry.get("id") in requested | ||
| else entry | ||
| ) | ||
| for entry in manifest | ||
| ] | ||
| _save_artifact_manifest(locked_run, manifest) |
There was a problem hiding this comment.
If loop-run artifact seeding or warm-run staging overlaps a dismissal, those writers can save a stale whole-manifest snapshot without participating in this row-locking protocol, removing the newly committed dismissed_at value and making the artifact visible again.
Prompt To Fix With AI
This is a comment left during a code review.
Path: products/tasks/backend/facade/api.py
Line: 3003-3021
Comment:
**Unlocked writers erase dismissals**
If loop-run artifact seeding or warm-run staging overlaps a dismissal, those writers can save a stale whole-manifest snapshot without participating in this row-locking protocol, removing the newly committed `dismissed_at` value and making the artifact visible again.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
🦔 Hogbox preview · ❌ build failedThe preview didn't come up for commit Previews are optional and never block merging. A failure here is often a hogland or tailnet hiccup rather than anything in your PR, so the check stays green and this comment is the status. |
A cloud agent's uploaded files pile up on a run with no way to clear one out. Adds POST .../runs/<run_id>/artifacts/dismiss/, which stamps dismissed_at on the manifest ids it is given so a client can hide a file, and clears it when passed dismissed: false. Nothing leaves object storage, so a file dismissed by mistake can be restored. Callers pass every version of a file together: uploads sharing a name are versions of one file, and dismissing only the newest would resurface the copy it replaced. dismissed_at joins the artifact response so clients can tell what is hidden. No migration — dismissed_at is a key on the existing TaskRun.artifacts JSON manifest. Generated-By: PostHog Code Task-Id: 57deec6a-7831-4922-a4ef-6d0c9b86831b
artifact_ids accepted an unbounded list of 200-character strings. Caps it at 100 ids and aligns the id length with pending_user_artifact_ids, the other place a caller passes run artifact ids. Generated-By: PostHog Code Task-Id: 57deec6a-7831-4922-a4ef-6d0c9b86831b
finalize_task_run_artifact_uploads read the manifest, did S3 verification I/O,
then blind-wrote the whole array back, so a dismissal committing in that window
was silently reverted. It now merges its new entries under the same row lock the
sibling upload path already takes, after the I/O rather than across it.
Restoring a file drops the dismissed_at key instead of nulling it, so the field
is only ever a string or absent. Serializing it as nullable made DRF emit
dismissed_at on every artifact, including staged user attachments that have no
dismissal concept.
The not-found 404 now returns {"error": ...} like the sibling artifact actions
rather than DRF's {"detail": ...}.
Generated-By: PostHog Code
Task-Id: 57deec6a-7831-4922-a4ef-6d0c9b86831b
The assertion read dismissed_at with .get(), so it passed whether the key was absent or explicitly null — the distinction the response contract now rests on. Generated-By: PostHog Code Task-Id: 57deec6a-7831-4922-a4ef-6d0c9b86831b
fe28059 to
8119bd7
Compare
|
Stacked PR 78644 was cancelled: a member PR was pushed to. |
2 participants
Problem
Files a cloud agent uploads pile up on a task run with no way to clear one out. There is no dismiss, hide, or delete — every artifact a run ever produced stays in the list forever.
This is the backend half of that. The client that uses it is stacked on top: #78644.
Changes
POST /api/projects/:team_id/tasks/:task_id/runs/:run_id/artifacts/dismiss/takesartifact_idsand adismissedboolean, stampsdismissed_aton those manifest entries, and returns the updated manifest.dismissed: falseclears the stamp. Nothing is deleted from object storage, so a file dismissed by mistake can be restored.dismissed_atjoinsTaskRunArtifactResponseSerializerso clients can tell what is hidden.No migration:
dismissed_atis a key on the existingTaskRun.artifactsJSON manifest.How did you test this code?
New tests in
products/tasks/backend/tests/test_api.pycover the two ways this endpoint can go wrong: it must flag only the requested ids and leave the rest of the manifest alone (and undo cleanly), and an unknown id must 404 without half-writing the manifest. Both pass locally against Postgres and ClickHouse.Also ran
mypyoverproducts/tasks— clean.Automatic notifications
Docs update
Documented with the client half in #78644, in
products/desktop/docs/cloud-task-artifacts.md.🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Built with PostHog Code (Claude). Skills invoked:
/improving-drf-endpoints,/implementing-mcp-tools,/writing-tests,/stacking-prs.Originally one PR with the desktop client. The "Desktop backend coupling" check rejected that, correctly — the dismiss button calls this endpoint, so shipping an auto-updating desktop release before this deploys would give users a button that 404s. Split into this stack instead of taking the
skip-desktop-backend-checklabel, since the two halves are not safe to ship in either order.Generated OpenAPI types, the MCP client, and the tasks MCP tool list (new entry scaffolded disabled) were regenerated with
hogli build:openapi.Created with PostHog Code