feat(control-plane): client-settable run display name, labels and links - #1032
Merged
Conversation
…adata POST /api/ui/v2/workflow-runs/:run_id/golden wrote the caller-supplied name and tag list into workflow_runs.metadata with no bounds at all. The name was only TrimSpace'd, so a 1 MiB name persisted verbatim; sanitizeStringList trimmed and de-duped but capped neither the entry count nor the entry length, and it preallocated its output slice (and an unbounded de-dupe map) straight from the attacker-controlled input length. That row is re-read and re-serialised on every runs-list page that contains the run, so both are stored amplification vectors (#944). Cap tags at 20 entries of at most 64 runes each, and truncate the name at 200 runes. Over-long tags are dropped rather than truncated: byte-slicing can land mid-rune and json.Marshal silently rewrites the invalid UTF-8 to U+FFFD. Lengths are counted with utf8.RuneCountInString so a 64-rune CJK tag survives. The output slice and de-dupe map are now sized min(len(values), maxCount) and the loop stops once maxCount survivors are collected, so a multi-million-entry tag array cannot force a large allocation before the cap applies. This route is UI-private and its only caller sends one hard-coded tag, so oversized input is bounded silently rather than rejected — a 400 would break the existing "Save as golden run" button. The name fallback is unchanged: an empty name still falls back to run_id, and run_id itself is not truncated. Forward-only. Nothing re-validates on read, so rows that already hold oversized golden metadata keep reading back exactly as they do today. The caps are named constants so the follow-up run-metadata endpoint can reuse the same bounds and the same helper. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…y reads Adds the behaviour tests for the golden-run caps: - sanitizeStringList: 100 inputs cap to the first 20; a 65-rune ASCII entry is dropped while its 64-rune neighbour survives; a 64-rune CJK entry is kept byte-identical (proving it is not truncated into U+FFFD) while a 65-rune one is dropped; the legacy trim/de-dupe/order behaviour is pinned unchanged; the maxCount<=0 and maxRunes<=0 guards are exercised. - A 100k-entry input asserts cap(out) <= 20, which is the allocation contract — a length assertion alone would still pass with the old preallocation. - truncateRunes: cut on a rune boundary, result always valid UTF-8 with no replacement character, and the non-positive cap guard. - Handler round-trip: a POST with 50 tags plus one over-long tag stores exactly 20, a 1 MiB name stores 200 runes and keeps the whole metadata blob under 4 KiB, and a blank name still falls back to run_id. - Read-back: a pre-seeded row holding 50 tags and a 500-rune name still surfaces in full on both the runs list and the run detail, pinning that this change is forward-only and read paths do not re-validate. The two pre-existing golden-route tests are left untouched as the behaviour-unchanged regression guard. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A 'run' namespace in workflow_runs.metadata, written only through a namespace-merging transactional primitive (never the full-row upsert, which clobbers golden/lineage and resets state columns). POST /api/v1/runs/:run_id/metadata read-merge-writes it with strict caps (display_name<=200, labels<=20x64, links<=10, url<=2048, http/https only, no embedded credentials) and creates the carrier row on first write; an optional run_metadata execute field seeds it at dispatch, excluded from the replay dedupe key; restart lineage now writes through the same primitive. The run list, run detail, DAG and agentic overview surface it. external_status is deliberately out of scope. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Write transactions now take the write reservation at BEGIN instead of on first write, so the read-merge-write metadata primitive (and every other BeginTx writer) cannot hit the read->write upgrade deadlock; WAL and the 60s busy timeout were already in place. Global, deliberate change — the full suite gates it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… links Display name takes precedence in the run list row, labels render as chips, links render only after scheme re-validation (http/https, host required) with rel="noopener noreferrer"; nothing is treated as trusted HTML. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…-matrix coverage Concurrent different-namespace writers both survive; lineage seed and metadata merge interleave without clobbering; two executes differing only in run_metadata replay-hit through the real findReplayHit path; byte-identity of untouched namespaces; endpoint-level negatives for every cap and for javascript:/data:/file:/credentialed/scheme-less URLs with storage untouched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
📊 Coverage gateThresholds from
✅ Gate passedNo surface regressed past the allowed threshold and the aggregate stayed above the floor. |
Contributor
📐 Patch coverage gateThreshold: 80% on lines this PR touches vs
✅ Patch gate passedEvery surface whose lines were touched by this PR has patch coverage at or above the threshold. |
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
1 participant
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.
Stacked on #1025 — merge that first; this branch contains its two commits and rebases clean once it lands.
Summary
The v1 of first-class run identity from the #944 discussion:
display_name,labelsandlinksas arunnamespace inworkflow_runs.metadata.POST /api/v1/runs/:run_id/metadataread-merge-writes the namespace (never the full-row upsert, which clobbers golden/lineage metadata and resetsstate_version/last_event_sequence), creates the carrier row on first write, and enforces the caps from that thread server-side:display_name≤ 200, ≤ 20 labels × ≤ 64 chars, ≤ 10 links with URL ≤ 2048, http/https only, host required, no embedded credentials. An optionalrun_metadatafield on async execute seeds it at dispatch — and stays out of the replay dedupe key, proven by a test that replays through the realfindReplayHitpath. The run list, run detail, DAG and the agentic overview all surface it (soaf wait's polling endpoint carries run identity), and the dashboard renders display names, label chips and scheme-revalidated links withrel="noopener noreferrer".external_statusis deliberately deferred, per the issue thread.Why
Refs #944. The design shape was settled publicly in the issue; this implements exactly that reduced v1. One correction to the earlier comment discovered while building: an ordinary run has no
workflow_runsrow (only restarts and golden saves created one), so the endpoint seeds the row on first write instead of assuming it exists.Changes
_txlock=immediateValidation contract
TestSetRunMetadataHandlerRoundTripAndRejectsBeforeWrite,TestWorkflowRunListAndDetailCarryRunMetadataAlongsideLineage,TestWorkflowDAGRunMetadataBothModes,TestRunOverviewRunMetadataPresenceAndEnvelopeLocationworkflow_runsrow; state/status/count/version columns never change →TestUpdateWorkflowRunMetadataCreatesAndMergesWithoutChangingStaterun_metadatastill replay-hit →TestExecuteHandler_RunMetadataDifferenceStillReturnsReplayHitjavascript:,data:,file:, scheme-less, embedded credentials) rejected at the endpoint with storage untouched → table-driven negative matrixTestRunMetadataIsNotInheritedByARestartedRunRunsPage/RunDetailPage/safeExternalUrltestsHow it was tested
CI-literal gates in the worktree: control-plane build/gofmt/vet/full suite (
-tags sqlite_fts5, minusinternal/packages), web-uinpm ci/lint/build/vitest coverage,coverage-surface+patch-coverage-gate(≥80 % on touched lines) — see the gate log summary in the checks. An adversarial review ran mid-flight; all four blocking findings it raised (Postgres first-write race, SQLite deferred-transaction upgrade hazard, restart lineage still using the full-row upsert, and a replay test that never executedfindReplayHit) are fixed in the final commits.Notes / follow-ups
_txlock=immediateDSN change is global to the control plane's SQLite connections — deliberate (write reservation atBEGINavoids read→write upgrade deadlocks; WAL + 60 s busy timeout were already set) and gated by the full suite, but worth a reviewer's eye.external_status(mutable, integration-owned lifecycle) is the deferred second slice from Proposal: first-class run display names, labels, links, and external status #944.npm run lintreports 440 pre-existing errors repo-wide onmain; the touched files carry exactly the same per-file error counts before and after this change (verified file-by-file), and lint is not a CI gate for the web client. No new lint debt added.🤖 Generated with Claude Code