feat(tasks): return short stable download URLs for run artifacts - #79176
Conversation
|
😎 Merged successfully - details. |
🦔 ReviewHog reviewed this pull requestFound 0 must fix, 0 should fix, 2 consider. Published 2 findings (view the review). |
|
ReviewHog Alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
There was a problem hiding this comment.
ReviewHog Report
Changes
Issues: 2 issues
Files (7)
products/desktop/packages/agent/src/adapters/local-tools/tools/upload-artifact.tsproducts/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.tsservices/mcp/src/api/generated.ts
|
|
||
| filename = str(entry.get("name") or "artifact").replace('"', "") | ||
| url = object_storage.get_presigned_url( | ||
| entry["storage_path"], | ||
| content_type=str(entry.get("content_type") or "") or None, | ||
| content_disposition=f'attachment; filename="{filename}"', |
There was a problem hiding this comment.
Sanitize artifact names before placing them in Content-Disposition
Why we think it's a valid issue
- Checked: Traced the name from upload to header.
name = serializers.CharField()(serializers.py:268) only blocks null bytes by DRF default, so CR/LF, backslashes and non-ASCII pass.get_safe_artifact_name(logic/services/staged_artifacts.py:23) isos.path.basename(name).strip()— removes path separators and edge whitespace only, preserving internal control/non-ASCII chars — and that value is stored as the manifestname(facade/api.py:2834).presign_task_run_artifact_download(facade/api.py:3004) then readsentry['name'], strips only", and buildsattachment; filename="{filename}". - Found: The disposition string is passed to
get_presigned_url→params['ResponseContentDisposition']→boto3.generate_presigned_url(storage/object_storage.py:192-198), which percent-encodes it into the presigned URL query string. The URL is then returned viaHttpResponseRedirect(presentation/views/api.py). So no raw CR/LF ever reaches PostHog'sLocationor any PostHog response header (Django rejects raw newlines regardless). Real response-splitting would require the storage backend to re-emit raw control bytes in its own header; PostHog runs SeaweedFS (Go net/http rejects invalid header values) and AWS S3 rejects invalid override values, so the injection path is not reachable on deployed backends. The endpoint is same-tenant (required_scopes=['task:read']+_get_visible_run), and the name originates from the tenant's own agent — no cross-tenant vector. - Impact: The 'response-header injection' framing does not hold, but a real low-severity residue remains: non-ASCII filenames (common for agent-produced files) produce a Content-Disposition that isn't RFC 6266-encoded (
filename*), yielding a garbled download filename, and pathological CR/LF names likely 400 at the storage layer. The suggested fix is accurate —content_disposition_header(as_attachment=True, filename=...)is the canonical helper already used atposthog/api/uploaded_media.py:76. - Priority: Lowered from must_fix to consider — the concrete security consequence (header injection) is blocked by boto3 URL-encoding and the storage backends, and impact is a same-tenant filename-display/robustness nit rather than an exploitable vulnerability, so it does not meet the must_fix security bar but is a cheap, canonical hardening worth keeping on record.
Issue description
Artifact names originate from user-controlled upload metadata. Removing only double quotes leaves carriage returns, line feeds, backslashes, and other invalid header characters. Passing this value as S3's response Content-Disposition can produce malformed download responses and may enable response-header injection, depending on the object-storage backend.
Suggested fix
Build the header with Django's content_disposition_header(as_attachment=True, filename=filename) helper, as posthog/api/uploaded_media.py does. Also reject control characters when validating artifact names so unsafe values never enter the manifest.
Prompt to fix with AI (copy-paste)
## Context
@products/tasks/backend/facade/api.py#L3005-3010
<issue_description>
Artifact names originate from user-controlled upload metadata. Removing only double quotes leaves carriage returns, line feeds, backslashes, and other invalid header characters. Passing this value as S3's response Content-Disposition can produce malformed download responses and may enable response-header injection, depending on the object-storage backend.
</issue_description>
<issue_validation>
- **Checked:** Traced the name from upload to header. `name = serializers.CharField()` (serializers.py:268) only blocks null bytes by DRF default, so CR/LF, backslashes and non-ASCII pass. `get_safe_artifact_name` (logic/services/staged_artifacts.py:23) is `os.path.basename(name).strip()` — removes path separators and edge whitespace only, preserving internal control/non-ASCII chars — and that value is stored as the manifest `name` (facade/api.py:2834). `presign_task_run_artifact_download` (facade/api.py:3004) then reads `entry['name']`, strips only `"`, and builds `attachment; filename="{filename}"`.
- **Found:** The disposition string is passed to `get_presigned_url` → `params['ResponseContentDisposition']` → `boto3.generate_presigned_url` (storage/object_storage.py:192-198), which percent-encodes it into the presigned URL query string. The URL is then returned via `HttpResponseRedirect` (presentation/views/api.py). So no raw CR/LF ever reaches PostHog's `Location` or any PostHog response header (Django rejects raw newlines regardless). Real response-splitting would require the storage backend to re-emit raw control bytes in its own header; PostHog runs SeaweedFS (Go net/http rejects invalid header values) and AWS S3 rejects invalid override values, so the injection path is not reachable on deployed backends. The endpoint is same-tenant (`required_scopes=['task:read']` + `_get_visible_run`), and the name originates from the tenant's own agent — no cross-tenant vector.
- **Impact:** The 'response-header injection' framing does not hold, but a real low-severity residue remains: non-ASCII filenames (common for agent-produced files) produce a Content-Disposition that isn't RFC 6266-encoded (`filename*`), yielding a garbled download filename, and pathological CR/LF names likely 400 at the storage layer. The suggested fix is accurate — `content_disposition_header(as_attachment=True, filename=...)` is the canonical helper already used at `posthog/api/uploaded_media.py:76`.
- **Priority:** Lowered from must_fix to consider — the concrete security consequence (header injection) is blocked by boto3 URL-encoding and the storage backends, and impact is a same-tenant filename-display/robustness nit rather than an exploitable vulnerability, so it does not meet the must_fix security bar but is a cheap, canonical hardening worth keeping on record.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Build the header with Django's `content_disposition_header(as_attachment=True, filename=filename)` helper, as `posthog/api/uploaded_media.py` does. Also reject control characters when validating artifact names so unsafe values never enter the manifest.
</potential_solution>
There was a problem hiding this comment.
Fixed in 7ad5d93: the disposition is now built with Django's content_disposition_header(as_attachment=True, filename=...) — the same helper posthog/api/uploaded_media.py uses — so non-ASCII names get RFC 6266 filename* encoding and control characters can't produce a malformed header value. Left upload-side name validation as is: it predates this PR, and per the validation notes here the injection path isn't reachable, so tightening it is out of scope for this change.
| entry_id = entry.get("id") | ||
| response_entries.append( | ||
| {**entry, "url": absolute_uri(_build_artifact_download_path(run, entry_id))} if entry_id else dict(entry) | ||
| ) |
There was a problem hiding this comment.
Stable links still dead-end after artifact retention expires
Why we think it's a valid issue
- Checked: Whether the 30-day tag actually bounds the link lifetime, whether it was introduced here, and whether the PR's claim is exposed to consumers. Read
_tag_artifact_object(facade/api.py:2608), thettl_dayslifecycle convention (docs/internal/workflows/s3-query-cache-setup.md),RUN_ARTIFACT_TTL_DAYS = "30"(logic/services/staged_artifacts.py:17), the finalize comment, and the serializer help text. - Found: The retention is real and deliberate — objects are tagged
ttl_days: 30and thettl_daystag drives S3 lifecycle deletion (the doc explicitly warns objects expire when a matching lifecycle rule exists), andRUN_ARTIFACT_TTL_DAYS = "30"confirms intent. The diff shows_tag_artifact_objectand its calling loop are context, not additions — the 30-day retention predates this PR. The premise is therefore correct: after ~30 days the object is gone and the download endpoint still presigns +HttpResponseRedirects to a key that 404s at S3. - Found: The overstatement is confined to the internal comment (facade/api.py:2853 "never expires") and the PR description. The consumer-facing contract — the
urlserializer help text (presentation/serializers.py:283) — says only "Stable download URL... redirects to a fresh presigned URL on each request," which is accurate and does not promise permanence. - Impact: The shipped code is functionally correct and a strict improvement (1h → ~30 days); no correctness regression. The genuine residue is (a) an inaccurate internal "never expires" comment/PR wording, and (b) a rare-case UX nit where a >30-day-old link yields a raw S3 error instead of a clean 404. The suggested per-request existence check adds an S3 HEAD to every download to serve an infrequent expired case, which is defensive for the common path.
- Priority: Lowered from should_fix to consider — the retention is pre-existing and out of scope, the user-facing API doc is already accurate, and the actionable part is a comment/description reword plus an optional defensive 404; real and worth noting, but below the should_fix bar.
Issue description
The response now presents this app URL as a non-expiring download link, but finalized objects are tagged with ttl_days: 30 by _tag_artifact_object. After deletion, this endpoint still creates a presigned URL, which leads to a missing object. Older messages therefore continue to dead-end, only after 30 days instead of one hour.
Suggested fix
Align storage retention with the promised link lifetime, or describe the URL as stable only for the artifact's retention period. Consider checking object existence so expired artifacts return a clear 404 instead of redirecting to a broken storage URL.
Prompt to fix with AI (copy-paste)
## Context
@products/tasks/backend/facade/api.py#L2857-2860
<issue_description>
The response now presents this app URL as a non-expiring download link, but finalized objects are tagged with `ttl_days: 30` by `_tag_artifact_object`. After deletion, this endpoint still creates a presigned URL, which leads to a missing object. Older messages therefore continue to dead-end, only after 30 days instead of one hour.
</issue_description>
<issue_validation>
- **Checked:** Whether the 30-day tag actually bounds the link lifetime, whether it was introduced here, and whether the PR's claim is exposed to consumers. Read `_tag_artifact_object` (facade/api.py:2608), the `ttl_days` lifecycle convention (docs/internal/workflows/s3-query-cache-setup.md), `RUN_ARTIFACT_TTL_DAYS = "30"` (logic/services/staged_artifacts.py:17), the finalize comment, and the serializer help text.
- **Found:** The retention is real and deliberate — objects are tagged `ttl_days: 30` and the `ttl_days` tag drives S3 lifecycle deletion (the doc explicitly warns objects expire when a matching lifecycle rule exists), and `RUN_ARTIFACT_TTL_DAYS = "30"` confirms intent. The diff shows `_tag_artifact_object` and its calling loop are context, not additions — the 30-day retention predates this PR. The premise is therefore correct: after ~30 days the object is gone and the download endpoint still presigns + `HttpResponseRedirect`s to a key that 404s at S3.
- **Found:** The overstatement is confined to the internal comment (facade/api.py:2853 "never expires") and the PR description. The consumer-facing contract — the `url` serializer help text (presentation/serializers.py:283) — says only "Stable download URL... redirects to a fresh presigned URL on each request," which is accurate and does not promise permanence.
- **Impact:** The shipped code is functionally correct and a strict improvement (1h → ~30 days); no correctness regression. The genuine residue is (a) an inaccurate internal "never expires" comment/PR wording, and (b) a rare-case UX nit where a >30-day-old link yields a raw S3 error instead of a clean 404. The suggested per-request existence check adds an S3 HEAD to every download to serve an infrequent expired case, which is defensive for the common path.
- **Priority:** Lowered from should_fix to consider — the retention is pre-existing and out of scope, the user-facing API doc is already accurate, and the actionable part is a comment/description reword plus an optional defensive 404; real and worth noting, but below the should_fix bar.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Align storage retention with the promised link lifetime, or describe the URL as stable only for the artifact's retention period. Consider checking object existence so expired artifacts return a clear 404 instead of redirecting to a broken storage URL.
</potential_solution>
There was a problem hiding this comment.
Fixed the wording in 7ad5d93: the comment and the PR description now say the link works for the artifact's retention window rather than "never expires" (the 30-day ttl_days tag predates this PR). Skipped the per-request existence check: it adds an S3 HEAD to every download to improve a rare >30-day case from an S3 404 to an app 404, which — as the validation notes say — is defensive for the common path.
|
Reviews (1): Last reviewed commit: "fix(tasks): build artifact Content-Dispo..." | Re-trigger Greptile |
The finalize-upload response handed the upload_artifact tool a raw presigned S3 URL, which is extremely long and expires after an hour. Return a short app URL instead, backed by a new GET endpoint that redirects to a freshly presigned URL per request. Generated-By: PostHog Code Task-Id: dceada9c-7368-4176-99e1-769fde0d02fa
content_disposition_header handles RFC 6266 filename* encoding for non-ASCII names and control characters, matching uploaded_media. Also reword the finalize comment: links last for the artifact's retention window, not forever. Generated-By: PostHog Code Task-Id: dceada9c-7368-4176-99e1-769fde0d02fa
d082055 to
183a799
Compare
|
React Doctor found no issues in the changed files. 🎉 Reviewed by React Doctor for commit |
Generated-By: PostHog Code Task-Id: dceada9c-7368-4176-99e1-769fde0d02fa
🦔 Hogbox preview · ✅ ready▶ Open the preview
commit |
🤖 CI report
|
| Root | Eager (shipped) | Δ vs base | Budget |
|---|---|---|---|
entry (logged-out pages, app bootstrap)src/index.tsx |
1.26 MiB · 22 files | no change | ███░░░░░░░ 27.9% of 4.51 MiB |
authenticated shell (every logged-in page)src/scenes/AuthenticatedShell.tsx |
8.27 MiB · 3,104 files | 🔺 +28 B (+0.0%) | █████████░ 85.1% 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 |
|---|---|
| 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 |
| 104.5 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 |
|---|---|
| 724.0 KiB | dist/toolbar/toolbar-app-DKENCNQJ.css |
| 552.1 KiB | dist/toolbar/chunk-chunk-5APP6ML7.js |
| 484.6 KiB | dist/toolbar/chunk-chunk-UYYUQJSY.js |
| 133.6 KiB | dist/toolbar/chunk-chunk-PH6M6VEG.js |
| 131.8 KiB | dist/toolbar/chunk-chunk-T5KY5WYR.js |
| 71.0 KiB | dist/toolbar/toolbar-app-FHOT2G2O.js |
| 69.0 KiB | dist/toolbar/chunk-chunk-27JL52RE.js |
| 35.6 KiB | dist/toolbar/chunk-chunk-FI2IQ23E.js |
| 20.9 KiB | dist/toolbar/chunk-chunk-Q6C75LDA.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 — 🔺 +9.5 KiB (+0.0%)
Total size of the built frontend/dist folder (all assets), compared against the base branch.
Total: 1396.07 MiB · 🔺 +9.5 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 — 93.0% of changed backend lines covered — 3 uncovered
🧪 Backend test coverage
Patch coverage — changed backend lines (products + core): ███████████████████░ 93.0% (46 / 49)
| File | Patch | Uncovered changed lines |
|---|---|---|
products/tasks/backend/facade/api.py |
90.5% | 3066, 3079 |
products/tasks/backend/presentation/views/api.py |
90.9% | 1805 |
🤖 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 31186310029 -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.4% |
35,265 / 50,092 |
signals |
█████████████████░░░ 82.6% |
27,212 / 32,964 |
cdp |
█████████████████░░░ 84.2% |
3,914 / 4,649 |
notebooks |
█████████████████░░░ 86.4% |
8,029 / 9,296 |
data_modeling |
█████████████████░░░ 86.6% |
8,451 / 9,761 |
actions |
█████████████████░░░ 86.6% |
717 / 828 |
cohorts |
██████████████████░░ 87.6% |
6,482 / 7,400 |
managed_warehouse |
██████████████████░░ 87.8% |
5,928 / 6,752 |
data_warehouse |
██████████████████░░ 87.8% |
11,281 / 12,843 |
product_tours |
██████████████████░░ 87.9% |
1,303 / 1,482 |
engineering_analytics |
██████████████████░░ 89.2% |
6,716 / 7,527 |
exports |
██████████████████░░ 89.3% |
7,484 / 8,378 |
dashboards |
██████████████████░░ 89.3% |
5,995 / 6,710 |
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,135 / 12,225 |
stamphog |
██████████████████░░ 91.3% |
4,505 / 4,936 |
slack_app |
██████████████████░░ 91.6% |
10,627 / 11,602 |
mcp_analytics |
███████████████████░ 92.7% |
3,810 / 4,112 |
ai_observability |
███████████████████░ 92.9% |
17,116 / 18,426 |
early_access_features |
███████████████████░ 92.9% |
1,347 / 1,450 |
marketing_analytics |
███████████████████░ 92.9% |
14,833 / 15,967 |
web_analytics |
███████████████████░ 93.1% |
15,963 / 17,154 |
surveys |
███████████████████░ 93.1% |
5,858 / 6,290 |
posthog_ai |
███████████████████░ 93.3% |
1,327 / 1,423 |
reminders |
███████████████████░ 93.4% |
468 / 501 |
product_analytics |
███████████████████░ 93.5% |
7,027 / 7,517 |
approvals |
███████████████████░ 93.5% |
3,491 / 3,734 |
workflows |
███████████████████░ 94.3% |
7,951 / 8,436 |
endpoints |
███████████████████░ 94.3% |
8,771 / 9,306 |
review_hog |
███████████████████░ 94.6% |
8,246 / 8,715 |
skills |
███████████████████░ 94.8% |
3,478 / 3,669 |
replay_vision |
███████████████████░ 95.7% |
18,103 / 18,923 |
experiments |
███████████████████░ 95.7% |
27,865 / 29,109 |
logs |
███████████████████░ 95.8% |
11,623 / 12,135 |
annotations |
███████████████████░ 96.2% |
732 / 761 |
revenue_analytics |
███████████████████░ 96.3% |
1,887 / 1,960 |
feature_flags |
███████████████████░ 96.4% |
17,580 / 18,238 |
user_interviews |
███████████████████░ 96.5% |
2,638 / 2,734 |
customer_analytics |
███████████████████░ 97.1% |
10,838 / 11,160 |
warehouse_sources |
███████████████████░ 97.5% |
380,177 / 390,026 |
data_catalog |
████████████████████ 97.9% |
2,677 / 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.
Problem
upload_artifact, the download link it pastes into the chat is a raw presigned S3 URL — over a thousand characters of signature and session token that make streaming text unreadable.Streaming-side rendering is handled separately in #79142; this PR shortens the URL itself.
Changes
/api/projects/:id/tasks/:task_id/runs/:run_id/artifacts/:artifact_id/download/) on each artifact instead of a presigned URL. Theupload_artifacttool already reads this field, so agents surface the short link with no tool changes.GET .../artifacts/:artifact_id/download/action onTaskRunViewSetredirects to a freshly presigned URL per request, so links carry no transport credentials and keep working for the artifact's retention window instead of one presign TTL. Access is gated like the other run-read actions (task:readscope, task visibility).Content-Disposition: attachment, so downloads keep the artifact's clean filename instead of the storage key.How did you test this code?
pytest products/tasks/backend/tests/test_api.py -k "finalize_artifact_uploads or download_artifact_by_id or presign_artifact"— new coverage: the redirect endpoint 302s to the presigned URL with the attachment disposition (404 for an unknown artifact id), and the finalize response pins the app download URL shape so it can't silently revert to an expiring presigned URL.uv run mypy --cache-fine-grained .,tach check --dependencies --interfaces, andhogli ci:preflight --fixall pass.Automatic notifications
Docs update
None.
🤖 Agent context
Autonomy: Agent-driven (human-reviewed)
Authored by PostHog Code from a request to shorten artifact download URLs in agent replies. Skills invoked:
/improving-drf-endpoints,/writing-tests,/writing-pr-descriptions.Created with PostHog Code