feat: generic artifact shapes — shape-based save_artifact + backend migration (#796 PR A) - #2313
Conversation
Introduce a domain-agnostic, closed SHAPE vocabulary (link, commit_set, check, metric, decision, note) with a freeform KIND semantic hint. A PR, an issue, and a preview page are all 'link' with different kinds; infra never names domain kinds. The new artifact-shapes module is pure (no zod/I/O) so daemon and web can share it, and provides: - ARTIFACT_SHAPES / ArtifactShape closed set + per-shape data contracts - isArtifactShape / validateArtifactShape (per-shape required-field checks) - deriveArtifactKey (shape-aware identity: note→single upsert, link→one per kind, check/metric→name, decision→key|kind|'current') - resolveLegacyShape (data-aware legacy type→shape router; result is overloaded so it picks link vs decision by content) - normalizeLinkData (copy pr_url/review_url onto data.url for link rows) Task #796 (PR A — backend foundation).
…ackend
Replace the freeform save_artifact type system with the closed shape
vocabulary. save_artifact now takes { shape, kind?, key?, summary?, data? },
validates the payload against the per-shape contract, rejects unknown shapes,
and derives a shape-aware identity key so a 'note' is a single rolling-status
upsert (no per-round growth) and a 'link' is one-per-kind.
A data-aware legacy shim keeps in-flight agents working: { type } is mapped
to a shape (progress→note, result→link|decision by content, review→decision,
pr→link kind:pr) and bypasses strict validation since it predates the
contracts; unknown legacy types are still rejected.
Migrate every backend reader/writer off legacy types:
- review auto-save (review-posted-gate) → decision kind:review, keyed round-N
- list_peers rolling status → note (data.text ?? summary)
- resolvePrUrlForRun (x3) → link kind:pr, data.url, with legacy fallback
- terminal result-summary readers (mark_complete, run completion, Forge gap
detection) → decision summaries
Update the infra-level Runtime Execution Contract prompts to the shape API.
Migration 164 backfills existing rows: legacy type→shape (data-aware), link
url normalization, and collapses per-(run,node) note rows to the single most
recent 'current' row so storage stops growing per round. Idempotent.
Coding-workflow prompt text and physical relocation of coding-specific
writers/readers out of daemon core are deferred to follow-up PRs (B: UI
shape rendering, C: coding-layer mapping + prompts).
Task #796 (PR A — backend foundation).
…-shapes-shape-based-save-artifact-rendering # Conflicts: # packages/daemon/src/storage/schema/migrations.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0546268414
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 129c8d3160
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Twelve review findings, all fixed: - c1: migration now dedupes legacy rows by (run,node,shape,key) keeping the latest BEFORE writing, so two legacy types mapping to the same key (e.g. review + URL-less result sharing a key) cannot violate UNIQUE and abort startup. - c2: the note collapse targets only ex-progress rows (the append-log bloat); unknown freeform types (merge_blocked, merge_conflict_loop, etc.) are preserved as distinct notes keyed by their original type, not collapsed. - c3: unknown legacy types are accepted as a note tagged _legacyType (with a distinct key) so active post-approval prompts that still emit merge_conflict_loop/merge_blocked/cleanup_warning keep recording state instead of erroring. - c4: a legacy result with a summary now stays a decision (summary visible to completion readers) even when it also carries a pr_url; only URL-only results become links. - c5: resolvePrUrlForRun (4 copies) no longer treats an arbitrary data.url as a PR URL -- only a link kind:pr (data.url) or an explicit legacy pr_url/prUrl qualifies, so an issue/preview link cannot be injected as PR_URL. - c6: terminal-result readers exclude kind:review decisions (review-round feedback is not a terminal outcome) across space-runtime, task-agent-manager, and the evolution gap detector. - c7: deriveArtifactKey honors an explicit key only for decision; note/link/ check/metric/commit_set always derive, so a caller cannot smuggle in a key to create unlimited notes. - c8: a shape NAME passed via the legacy type alias is validated strictly (no bypass) -- only genuine legacy semantic types skip validation. - c9: list_artifacts maps legacy type filters to their shape sets (result -> decision+link, progress -> note, review -> decision, pr -> link) so migrated rows stay visible to in-flight agents. - c10: metric validation rejects non-scalar values (array/object/boolean). - c11: note validation accepts a bare timestamp (data.ts). - c12: a legacy-artifact reconciliation runs outside the one-shot marker (mirroring migration 163) so legacy rows written by an older binary after a rollback are caught on every startup.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c9c51d4a4b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (NeoKai)
Model: glm-5.1 | Client: NeoKai | Provider: GLM
Recommendation: REQUEST_CHANGES — one in-scope correctness regression in a reader this PR modifies, plus two robustness items I'm concurring with from the other review. The foundation itself is solid.
Verified good
- All 12 prior findings are genuinely resolved (read each fix in source, not just the replies).
- Migration 166 is sound: legacy rows are deduped by (run,node,shape,key) with losers deleted before any UPDATE, so two legacy types mapping to the same key can't violate UNIQUE and abort startup. Idempotent; the outside-marker reconcile mirrors migration 163, is a cheap
COUNT(*)no-op once clean, and safely catches rollback stragglers. The note collapse is correctly limited to ex-progressrows — unknown freeform types are preserved as distinct notes. - Every backend reader is migrated off legacy types; no orphaned
artifactType:'progress'|'result'|'review'|'pr'reads remain in daemon/web source (only the intentionallist_artifactsshim and web test fixtures). Terminal-result readers correctly excludekind:'review'. The fourresolvePrUrlForRuncopies no longer treat an arbitrarydata.urlas a PR URL. - CI: daemon unit/online + lint/typecheck/knip all green. The lone Web Tests failure is
ui/Dropdown > should close when Escape key is pressed— a jsdom keyboard flake in a component this PR does not touch; it passes locally (246/246 files). Not a regression.
P2 (blocking) — list_peers reports the wrong note after migration — see inline comment on node-agent-tools.ts. This reader was changed by this PR (progress→note), so it's in scope and a ~2-line fix.
P2 — concur: terminal-result reader is a one-kind denylist (space-runtime.ts:9568, also evolution-episode-service.ts and task-agent-manager.ts)
kind !== 'review' is a denylist. DecisionArtifactData also models gate approvals; once any non-terminal decision with a summary and a different kind is written, it gets persisted as the task's terminal result. Not a regression vs. the old result-with-summary behavior, but the point of the shape split is a positive contract — please add an explicit terminal discriminator (e.g. a kind:'outcome'/terminal flag) before agents freely use decisions in PR C.
P2 — concur: resolvePrUrlForRun ignores recency across forms (space-runtime.ts:4333 + 3 copies)
The first pass returns the first link kind:pr it finds and never reaches the legacy pr_url/prUrl fallback. While the built-in prompts still emit legacy result/pr_url rows, a run with an older link kind:pr and a newer legacy pr_url (e.g. a superseded PR) will route the merge off the stale PR. Suggestion: collect both eligible forms and choose by updatedAt, as the old recency walk did.
P3 (notes)
- Review auto-save key
round-${count}(node-agent-tools.ts:1101) can collide with a caller-authoredround-N; safe today because cycles are sequential, and pre-existing — not introduced here. deriveArtifactKey('decision', …, explicitKey)ignoreskindwhen an explicit key is given (artifact-shapes.ts:229), so two decision streams sharing an explicit key but differing in kind would collide — no practical collision today (only review uses explicit keys).- Transition: the legacy
append:truepath still inserts a new row per call formerge_blocked/merge_conflict_loop/result-append, so per-round bloat for those types persists until PR C migrates the prompts. The "storage stops growing per-round" criterion is fully met only after PR C.
Five follow-up findings from the second review pass: - c13 (P1): terminal-result readers used a one-kind denylist (kind != 'review'). Switched to a positive discriminator — only a kind-less decision (the bare terminal form; legacy result -> decision carries no kind, review/gate carry a kind) counts as terminal. Applied across space-runtime, task-agent-manager, and the evolution gap detector. - c14 (P1): resolvePrUrlForRun (4 copies) prioritized an older link kind:pr over a newer legacy pr_url row. Now collects every eligible candidate (link kind:pr data.url + legacy pr_url/prUrl) and returns the most recently updated, so a superseded PR is never routed into the merge. - c15 (P2): list_peers read the last note by created_at, but a node can hold several notes (rolling 'current' + migrated unknown types); the appended merge_blocked/merge_conflict_loop notes won. Now selects the 'current'-keyed note, falling back to max(updatedAt). - c16 (P2): review auto-save used a count-based cycle that could upsert onto an existing round-N. Now derives the next round from the max numeric suffix among existing review decisions (handles sparse keys and both legacy cycle-N and namespaced review:round-N), guaranteeing an unused key. - c17 (P2): deriveArtifactKey ignored kind under an explicit decision key, so kind:review round-0 and kind:gate round-0 collided. Explicit decision keys are now namespaced by kind (review:round-0 vs gate:round-0).
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (NeoKai)
Model: glm-5.1 | Client: NeoKai | Provider: GLM
Recommendation: APPROVE — all second-pass findings addressed and verified; zero open findings.
Verified each fix on ad0fcde30:
- list_peers note selection (c15): now reads the
current-keyed note, falling back tomax(updatedAt)— appendedmerge_blocked/merge_conflict_loopnotes no longer shadow the rolling status. - terminal-result discriminator (c13): all three readers require a kind-less decision with a summary (legacy
result→decisionis kind-less; review/gate are excluded). Confirmed no current path writes a terminal result carrying a kind. - resolvePrUrlForRun recency (c14): all four copies collect eligible candidates (link
kind:'pr'viadata.url, or legacypr_url/prUrl) and return the max-updatedAt; a genericdata.urlstill never qualifies. - review-round keying (c16): next round derived from the max numeric suffix across existing review keys (handles legacy
cycle-Nand namespacedreview:round-N) — no overwrite of sparse/caller-authored rounds. - decision key namespacing (c17):
deriveArtifactKeynamespaces explicit decision keys by kind, so two streams (review:round-0vsgate:round-0) never collide.
CI on ad0fcde30 is fully green (Web Tests, All Tests Pass, coverage gate, lint/typecheck, all daemon unit/online shards). PR is open, mergeable, and all 18 review threads are resolved. Migration 166 remains sound (dedup-before-write, idempotent, rollback-safe reconcile).
One non-blocking note carried to PR C: the legacy append:true path still grows a row per call for merge_blocked/merge_conflict_loop/result-append until the coding-workflow prompts migrate — expected and out of scope for PR A.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad0fcde304
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (NeoKai)
Model: glm-5.1 | Client: NeoKai | Provider: GLM
Recommendation: REQUEST_CHANGES (against ad0fcde30) — three valid findings from the latest pass. I can see fixes for all three are already in progress in the worktree; this review records them against the committed head. Will re-approve once they land as a new commit + CI is green. Human: please do not merge ad0fcde30 — wait for the follow-up commit.
-
(P2)
resolvePrUrlForRunmust require thelinkshape, not justkind:'pr'. All four copies readdata.urlfor any artifact withdata.kind === 'pr'.CheckArtifactDatalegitimately allows bothkindandurl, so acheck { kind:'pr', url:<CI run URL> }would have its CI URL misread as the run's PR and injected into gate eval /gh props. Fix: gate ona.artifactType === 'link' && a.data.kind === 'pr'(non-linkkind:'pr'falls through to the legacypr_url/prUrlcheck only). (Already applied in worktree at all 5 sites.) -
(P2, most important) The registered
save_artifacttool description is stale.tool('save_artifact', …)still advertises "type is fully generic: use 'progress'/'result'/'review' or any custom label" and thatappendcontrols identity — contradicting the new closedshapeschema. Agents following the tool description (not the schema) will emittype:'test_result'etc., silently stored asnoteand invisible to shape consumers. This is the agent-facing contract; it should ship in PR A. Fix: advertise the shape set +kind, demotetypeto a compat alias (and updatelist_artifacts). (Already applied in worktree.) -
(P2)
commit_setidentity ignoresrepo.deriveArtifactKeykeys onbranchonly, so two repos sharing a branch (repo-a/main,repo-b/main) collapse. Fix: key on${repo}:${branch}with the existing fallbacks. (Already applied in worktree.)
The c13–c17 fixes from the prior round remain correct and verified; CI on ad0fcde30 was fully green. These three are the only open items.
… commit_set
Third-pass review findings (all agent-facing/correctness):
- c18 (P1): resolvePrUrlForRun (5 copies) read data.url for any artifact with
kind:'pr', but CheckArtifactData also allows kind+url, so a check
{kind:'pr', url:<CI url>} would be misread as the PR URL. Now requires
artifactType === 'link' && kind === 'pr' before reading data.url; non-links
fall through to legacy pr_url/prUrl fields only.
- c19 (P2): the registered tool('save_artifact') description still advertised
freeform type/append, contradicting the shape schema — agents following it
wrote type:'test_result' which silently became a note. Rewrote the
save_artifact + list_artifacts tool descriptions to advertise shapes/kind and
demote type to a compat alias.
- c20 (P2): deriveArtifactKey for commit_set used branch only, so repo-a/main
and repo-b/main collided. Identity is now repo:branch (falling back to either,
then 'default').
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (NeoKai)
Model: glm-5.1 | Client: NeoKai | Provider: GLM
Recommendation: APPROVE — final re-approval against 1675ab7b5.
Round-4 verification (fresh pass)
All three round-3 findings are addressed in committed 1675ab7b5 and re-verified independently:
- c18 — PR-kind URLs restricted to the
linkshape.resolvePrUrlForRun/dispatchPostApprovalPR resolver now requireartifactType === 'link' && data.kind === 'pr'before a non-link artifact's straydata.urlis treated as a PR URL. Confirmed at all 5 resolver sites (node-agent-tools.ts,space-runtime.ts×2,task-agent-manager.ts,space-runtime-service.ts). Closes the latent leak where acheck/decisioncarrying bothkindandurlcould masquerade as a PR link. - c19 — Tool descriptions advertise shapes.
save_artifactnow documents the closed shape vocabulary (link/commit_set/check/metric/decision/note) + freeformkind, withtypedemoted to a compat alias.list_artifactsdescription updated to match. This is the agent-facing surface, so it matters for correct caller behavior going forward. - c20 —
commit_setidentity isrepo:branch.deriveArtifactKey('commit_set')returns${repo}:${branch}when both are present, sorepo-a/mainandrepo-b/mainno longer collapse onto one row. Falls back tobranch || repo || 'default'.
Verification evidence
- Local tests: migration-166 backfill 8/8 ✓;
node-agent-tools(save/list/list_peers) 171/171 ✓. - CI on
1675ab7b5: Web Tests ✓, Lint/Knip/Format/Type ✓, all Daemon Unit + Online shards ✓. OnlyCoverage Quality Gatestillin_progress(informational —Finalize Coverallsalready succeeded; not a test gate).mergeStateStatus: CLEAN. - Review threads: 21/21 resolved, 0 unresolved.
- Shape invariant holds: infra vocabulary stays closed;
pr/reviewnever named in infra — only askindonlink/decision. Legacy shim + backfill migration remain idempotent and UNIQUE-safe.
Zero P0–P3 findings. Ready for the human to squash-merge into dev.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1675ab7b54
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (NeoKai)
Model: glm-5.1 | Client: NeoKai | Provider: GLM
Post-approval triage of 5 new inline findings (chatgpt-codex bot) on 1675ab7b5. I validated each independently against the committed code and the pre-shape history on origin/dev. Recommendation: APPROVE for merge stands — the single P1 is invalid; the two real P2s are non-correctness quality/fidelity gaps routed to Coding for a tiny follow-up.
P1 — "review/result key collision" → INVALID (debunked)
The bot's premise — "a node has a legacy review and terminal result with the same non-empty key" — cannot occur. On origin/dev, review rows were auto-keyed cycle-${cycle} (node-agent-tools.ts:1091) and terminal result rows were always written append:true → random ${Date.now()}-${rand} keys (node-agent-tools.ts:23, :1511). The two schemes never overlap, so the migration's originalKey || deriveArtifactKey (migrations.ts:11195) cannot collapse a review onto a result. No data loss. The bot's code-reading (that the migration preserves originalKey) is accurate; the claimed impact is not.
P2 — extractArtifactDetail misses new-shape fields → REAL (non-blocking)
evolution-scope-service.ts:936-949 reads only summary / pr_url / review_url / status / ..., not url / text / recommendation. Migrated rows survive (normalizeLinkData keeps pr_url alongside the new url), but a fresh { shape:'link', data:{ url, kind } } yields "no artifact detail" in persisted evolution evidence. Fix: add url, text, recommendation to the field list (~1 line). Routed to Coding.
P2 — list_artifacts legacy filter over-returns by kind → REAL (non-blocking)
node-agent-tools.ts:1703-1706 maps type:'review'→['decision'], type:'pr'→['link'] without post-filtering on data.kind, so { type:'pr' } now also returns issue/preview/doc links (and { type:'review' } returns the terminal result decision). Compat-shim fidelity drift from the old exact-type semantics. Fix: post-filter the mapped-shape results by the legacy semantic kind. Routed to Coding.
P3 — two unknown legacy types sharing a key collide (migrations.ts:11173) → theoretical
Requires two different unknown types with the same non-empty key. The old code defaulted unknown-type keys to '' → distinct-by-type (originalKey || originalType), so collision needs a caller that passed identical explicit keys across two unknown types. Not observed.
P3 — unknown-legacy write ignores keyArg (node-agent-tools.ts:1648) → theoretical
Only affects an in-flight caller using one unknown type with multiple explicit keys (e.g. audit/phase-1, audit/phase-2); the active post-approval prompts (merge_blocked, cleanup_warning) write one rolling status per type, so this is keyed-by-type intentionally.
Net
The headline P1 is debunked (no data loss). The two real P2s are reader-migration completeness items with no correctness or data-loss impact; Coding has been notified for a small follow-up (round-5 or PR C). The foundation — closed shape vocabulary, per-shape validation, UNIQUE-safe dedup-before-write migration, and the migrated readers — remains sound and CI-green.
Dev advanced by one commit (49d3719, PR #2313 generic artifact shapes) which introduced its own migration 166 — colliding with the space_tasks migration I had just renumbered to 166. Resolution: - migrations.ts: renumber the space_tasks rate/usage-limited migration 166 -> 167 (table space_tasks_m166_new -> space_tasks_m167_new, marker migration_167). Dev's M166 (artifact-shape backfill + legacy reconcile) kept intact and registered before 167. - space-runtime.ts / space-runtime-service.ts / task-agent-manager.ts / space.ts auto-merged cleanly (artifact-shape additions don't overlap the rate-limit paths). Verified: lint/format/knip/typecheck/db-schema-parity/space-task-handler-tests clean; daemon shards 4-space-migrations-a/b, 4-space-storage, 5-space-runtime-a/b, 5-space-agent-other all pass.
Replace the detectRenderer(data) data-shape sniffing (and the hardcoded GITHUB_PR_RE GitHub-PR special-case) with a dispatch on artifact.artifactType, which after the generic-shapes migration holds a value from the closed ArtifactShape vocabulary. Per-shape renderers: link (icon/label by data.kind — kind:'pr' is a PR row, kind:'issue' an issue row, etc.), commit_set (commit list + +/- totals), check (status chip + counts), metric (name value unit -> target), decision (recommendation badge + summary/counts), note (status text line). A default renderer handles any shape, known or not. Tests now seed shape-typed artifacts instead of relying on data-shape detection. TaskArtifactsPanel is unchanged — it already renders ArtifactCard. Depends on the closed shape vocabulary from #2313 (PR A), included here as commit 4e26d69 since PR A is unmerged.
Replace the detectRenderer(data) data-shape sniffing (and the hardcoded GITHUB_PR_RE GitHub-PR special-case) with a dispatch on artifact.artifactType, which after the generic-shapes migration holds a value from the closed ArtifactShape vocabulary. Per-shape renderers: link (icon/label by data.kind — kind:'pr' is a PR row, kind:'issue' an issue row, etc.), commit_set (commit list + +/- totals), check (status chip + counts), metric (name value unit -> target), decision (recommendation badge + summary/counts), note (status text line). A default renderer handles any shape, known or not. Tests now seed shape-typed artifacts instead of relying on data-shape detection. TaskArtifactsPanel is unchanged — it already renders ArtifactCard. Depends on the closed shape vocabulary from #2313 (PR A), included here as commit 4e26d69 since PR A is unmerged.
…2314) * feat(web): render ArtifactCard by shape, with kind as icon/label Replace the detectRenderer(data) data-shape sniffing (and the hardcoded GITHUB_PR_RE GitHub-PR special-case) with a dispatch on artifact.artifactType, which after the generic-shapes migration holds a value from the closed ArtifactShape vocabulary. Per-shape renderers: link (icon/label by data.kind — kind:'pr' is a PR row, kind:'issue' an issue row, etc.), commit_set (commit list + +/- totals), check (status chip + counts), metric (name value unit -> target), decision (recommendation badge + summary/counts), note (status text line). A default renderer handles any shape, known or not. Tests now seed shape-typed artifacts instead of relying on data-shape detection. TaskArtifactsPanel is unchanged — it already renders ArtifactCard. Depends on the closed shape vocabulary from #2313 (PR A), included here as commit 4e26d69 since PR A is unmerged. * fix(web): address review — safe-URL links, commit_set guard, minimal-PR id, review evidence Addresses the four review findings on #2314 (security P1 + three P2s): 1. Safe URL schemes (P1, security): LinkCard/CheckCard/DecisionCard now bind href only for http(s) URLs via safeHref(); agent-controlled javascript:/data:/ custom-scheme URLs render as plain text. Defense-in-depth: validateArtifactShape ('link') also rejects non-http(s) URLs at save time. 2. commit_set crash guard (P2): filter null/non-object commit entries before dereferencing, so {commits:[null]} no longer throws and blanks the panel. 3. Minimal PR identifier (P2): a {url, kind:'pr'} link with no number/title now falls back to the URL as its label instead of a bare 'Pull Request'. 4. Legacy review evidence (P2): DecisionCard surfaces data.url/data.review_url as a 'review' link so review-history rows (mapped review->decision, carrying review_url not recommendation) are not rendered blank. Tests cover all four behaviors plus the new link scheme validation. * fix(web): normalize legacy artifact types at the UI boundary Until the backend producer + DB migration (PR A commit 2) land, daemon rows still carry pre-shape types (progress/result/pr/review) with no backfill, so the shape-based dispatch rendered every real artifact as GenericCard — a regression vs. the old data-shape detection. ArtifactCard now resolves the effective shape at the render boundary: isArtifactShape(artifactType) for post-migration rows, falling back to resolveLegacyShape (progress->note, result->link|decision, pr->link, review->decision) for legacy rows. Link rows also get normalizeLinkData so a pr_url/review_url is copied onto data.url. Truly unknown types still fall to the default renderer. Idempotent: once the backend stores shapes, the legacy branch is never taken. Covered by a new 'legacy type normalization' test block (6 cases). * fix(web): preserve mixed-content results + reject non-scalar metric values Two more automated review findings: 1. Mixed-content legacy results (P2): the full-stack QA workflow writes a result artifact carrying pr_url + summary + test_output + browser-validation evidence. resolveLegacyShape('result') was mapping any URL-bearing result to a pure link, so LinkCard hid the QA summary/output. Refined so a result with a URL AND content maps to a decision (summary renders); URL-only results stay links. DecisionCard also surfaces pr_url (label 'view') so the PR link survives. 2. Non-scalar metric values (P2): validateArtifactShape('metric') accepted {value:{...}} (only checked non-null) and MetricCard stringified it to '[object Object]'. Validator now requires number|string; MetricCard only renders scalar values so unvalidated/legacy data can't show [object Object]. Tests: shared (result-with-content->decision, metric rejects non-scalar) + web (mixed QA result preserves summary + view link; metric object value not stringified). * fix(shared): recognize merged_pr_url (and any *_url field) for legacy link routing Round-7 review finding (P2): the post-approval merge audit writes a legacy result { merged_pr_url, merged_at, approval_source }, but resolveLegacyShape's url check only knew url/pr_url/prUrl/review_url, so the row routed to decision and rendered a bare badge — dropping the merged PR URL. migrations.ts already treats merged_pr_url as a URL field, so this was an inconsistency. Rather than re-enumerate field names (the codebase has several *_url variants in artifact data: pr_url, prUrl, review_url, merged_pr_url, image_url, ...), both resolveLegacyShape('result') and normalizeLinkData now use a shared findLinkUrl helper that matches any key that is 'url' or ends in '_url'/'Url'. Covers the migrations.ts list and stops the per-variant whack-a-mole. Existing url-only tests still pass. Secondary audit metadata (merged_at/approval_source) and the full producer split stay with PR C. Tests: shared result+merged_pr_url→link and normalize merged_pr_url; web legacy merge-audit result renders the merged PR link, not an empty card. * fix(web): DecisionCard — validate each evidence URL independently Greptile round-11 findings (both real, edge-case): 1. Label/href mismatch: when a decision carried both data.url and review_url, href resolved to data.url (won the ||) but the label read 'review'. 2. Non-http data.url dropped the evidence link: safeHref(url || reviewUrl || pr_url) validated only the first non-empty candidate, so a non-http data.url returned null and the valid review_url/pr_url fallbacks were never tried. Fix: compute safeHref per candidate and pick the first http(s) one, tracking which won so the link label matches the selected URL. Two new tests cover both cases.
1 participant
Replaces the freeform
save_artifacttype system with a closed, domain-agnostic shape vocabulary (link,commit_set,check,metric,decision,note) plus a freeformkindsemantic hint — PR A of #796. A PR, an issue, and a preview page are alllinkwith differentkind; infra never namespr/review/etc.What changed
save_artifact({ shape, kind?, key?, summary?, data? })validates against the closed set, rejects unknown shapes, and derives a shape-aware identity key:noteis a single rolling-status upsert (stops per-round bloat),linkis one-per-kind,check/metrickeyed by name,decisionsingle-terminal or multi-round via explicit key.{ type }maps to a shape (progress→note, result→link|decision by content, review→decision, pr→link kind:pr) and bypasses strict validation; unknown legacy types are rejected.current) — idempotent.Deferred to follow-up PRs: UI shape-based rendering (PR B), and relocating coding-specific writers/readers out of daemon core + coding-workflow prompt text (PR C). The UI keeps working today because
ArtifactCardalready detects by data shape.Verified: shared/daemon/web suites green; the only daemon failures (3 in
space-mcp-handlers.test.ts) are pre-existing (reproduce on the clean base, unrelated to artifacts). Note:check:test-qualityhas one pre-existing issue inprovider-registry.test.ts(also untouched here).