feat(web): shape-based artifact rendering (ArtifactCard) (#796 PR B)

[lsm]

Renders workflow run artifacts by shape (artifact.artifactType) instead of the old detectRenderer(data) data-shape sniffing, with data.kind supplying the icon/label. One renderer per closed shape — link (icon/label by kind; kind:"pr" is a PR row), commit_set (commit list + +/-), check (chip + counts), metric (line), decision (badge), note (line) — plus a default for unknown shapes. The hardcoded GitHub-PR URL detection is dropped (a PR is now link kind:"pr"); link URLs are restricted to http(s) at render time.

Legacy pre-shape rows (progress/result/pr/review) are normalized to shapes at the UI boundary via resolveLegacyShape/normalizeLinkData, so they keep rendering correctly — idempotent once rows are shapes. TaskArtifactsPanel is unchanged (it already maps over ArtifactCard).

Rebased onto dev after #2313 (PR A) merged: this is now a clean follow-up containing just the ArtifactCard renderer + two artifact-shapes.ts hardening additions not in PR A — http(s) scheme validation in validateArtifactShape('link') (defense-in-depth), and a findLinkUrl helper so legacy URL-bearing results (e.g. the merge audit's merged_pr_url) route to a link. Two non-blocking legacy-fidelity items (QA test_output → check; merge-audit merged_at/approval_source → note) are deferred to PR C.

[lsm]

🤖 Review by glm-5.1 (z.ai/glm)

Model: glm-5.1 | Client: NeoKai | Provider: z.ai/glm

Recommendation: APPROVE

Verified scope, correctness, and integration for task #808 (#796 PR B): shape-driven ArtifactCard rendering.

What changed, and why it's right

• ArtifactCard now dispatches on artifact.artifactType (a value from the closed ArtifactShape vocabulary) instead of the old detectRenderer(data) data-shape sniffing. One renderer per shape — link, commit_set, check, metric, decision, note — plus a GenericCard default that works for any shape, known or not. data.kind drives the icon/label on link (so kind: 'pr' is a PR row, kind: 'issue' an issue row), and the hardcoded GITHUB_PR_RE / GITHUB_COMMIT_RE URL detection is fully removed. This matches the task scope exactly; nothing speculative was added.
• TaskArtifactsPanel is unchanged and still maps <ArtifactCard> over the run artifacts (panel:585). No other file referenced the removed renderers or old artifact-card-* test-ids, so there's no collateral.

Verification

• ArtifactCard tests: 18/18 (re-seeded with shape-typed artifacts, including an explicit "no GitHub-URL special-casing without kind: 'pr'" case).
• Full space-component vitest suite: 68 files / 1618 passed, 9 skipped — no collateral breakage.
• Web tsc --noEmit: clean. oxlint on changed files: clean. CI knip (--include files,dependencies,exports --no-config-hints): exit 0. (The HasDisplayName item only appears under the non-CI knip:all and lives in untouched packages/ui — pre-existing.)

Two non-blocking notes (not requesting changes)

1. Merge order. This UI degrades to the default GenericCard for any row whose artifactType isn't yet a shape. Until #2313 (PR A) lands its backend migration + the legacy resolveLegacyShape shim, existing 'pr'/'result'/'progress'/'review' rows would all render as generic cards. PR A is still OPEN — recommend landing it first. I confirmed the four duplicated shared files (artifact-shapes.ts, mod.ts, types/space.ts, artifact-shapes.test.ts) are byte-identical to PR A's tip (129c8d3), so the rebase drops cleanly.
2. Cosmetic. CommitSetCard keys the commit list by array index (key={i}). Commits carry sha, so key={c.sha ?? String(i)} would be marginally more idiomatic — but it's a static, never-reordered list, so this is fine as-is.

PR state: OPEN, MERGEABLE, zero unresolved review conversations.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2f59355925

ℹ️ 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]

🤖 Review by glm-5.1 (z.ai/glm)

Model: glm-5.1 | Client: NeoKai | Provider: z.ai/glm

Recommendation: REQUEST_CHANGES — this supersedes my earlier APPROVE. On re-review (and after validating the four inline findings from the codex review), I agree with all four. The P1 below is a security regression vs. the previous renderer and is blocking.

---

P1 — Links must restrict URL schemes (security regression) — ArtifactCard.tsx:163 (LinkCard) and ~289 (CheckCard view link)
The old detectRenderer only reached LinkCard for URLs matching isUrl = /^https?:\/\//; everything else (including javascript:) fell through to the table renderer as inert text. The new dispatch keys purely on artifact.artifactType, so a link artifact with data.url = "javascript:…" is now rendered as <a href="javascript:…"> — an actionable XSS anchor (Preact does not sanitize href). Artifact URLs are agent-controlled and agents handle untrusted web/MCP content, so prompt-injection into save_artifact is a realistic path. The check renderer binds the same way.
Fix (restores the old trust boundary):

const SAFE_URL = /^https?:\/\//i;
// LinkCard / CheckCard: render <a> only when isSafeUrl(url); else <span> plain text.

Defense-in-depth: also enforce the scheme in validateArtifactShape('link') (shared).

P2 — Crash on malformed commit entries — ArtifactCard.tsx:228-230
shown.map((c) => … str(c.sha) …) dereferences c.sha; c can be null because validateArtifactShape('commit_set') returns ok unconditionally and the payload schema is z.unknown(). A {commits:[null]} artifact throws during render, and there's no error boundary, so one bad artifact blanks the whole artifacts panel. Filter entries first:

const commits = Array.isArray(data.commits)
  ? (data.commits as unknown[]).filter(
      (c): c is Record<string, unknown> => c !== null && typeof c === 'object')
  : [];

P2 — Minimal PR links are indistinguishable — ArtifactCard.tsx:171-172
For a valid {url, kind:'pr'} with no title/number, the label is just "Pull Request" and the hostname subtitle is suppressed (kind !== 'pr'), so the card shows no URL, repo, or number. With URL-pattern parsing intentionally gone, there's no other identifier. Only suppress the hostname when an identifier is already shown:

{!title && hostname && !(kind === 'pr' && number != null) && (…hostname…)}

P2 — Legacy review evidence is dropped (cross-PR) — artifact-shapes.ts:147-148 + DecisionCard
resolveLegacyShape('review') → 'decision', but the producer (node-agent-tools.ts:1071) writes {review_url, cycle, submittedAt, comment_urls?} — no recommendation/summary/counts. DecisionCard renders only those fields, so every review-cycle artifact becomes a bare "DECISION" badge and the review_url link vanishes. (Latent until #2313 wires the migration/classifier, but the logic ships here.) Pick one:

• PR-A fix: map 'review' → 'link' (kind 'review') in resolveLegacyShape, since the stored evidence is a URL, not a verdict; or
• PR-B fix: have DecisionCard render data.url || data.review_url as a link when present, so a decision carrying evidence is never blank.

---

Not a code finding — CI was flaky. The "All Tests Pass" failure was Daemon Unit Tests (4-space-migrations-a) on migrations 89/96/101, none of which this PR touches. All three pass locally (29/29) and the rerun went green (migrations-a: completed/success). No action needed beyond the re-run.

Merge order still applies: land #2313 (PR A) first so legacy artifactType values migrate to shapes; the four duplicated shared files are byte-identical to PR A tip 129c8d3 (clean rebase). I'm happy to re-review once the four findings above are addressed.

[lsm]

🤖 Review by glm-5.1 (z.ai/glm)

Model: glm-5.1 | Client: NeoKai | Provider: z.ai/glm

Recommendation: APPROVE — all four prior findings are fixed in 45c38b2, independently re-verified this round.

Verified fixes

1. P1 — safe URL schemes. safeHref(url) (http/https-only via new URL().protocol, null on empty/invalid) is now the single gate at every anchor site: LinkCard:182, CheckCard:315, and the new DecisionCard:417 review link. Non-http(s) URLs render as plain text — no unguarded href remains. Defense-in-depth added: validateArtifactShape('link') rejects non-http(s) at save time, with a dedicated test.
2. P2 — commit_set crash. Entries are filtered to non-null objects before dereferencing; {commits:[null]} / [42] / [undefined] no longer throw. The null case is covered explicitly (typeof null === 'object', so the c !== null guard is what excludes it).
3. P2 — minimal PR identifier. Label fallback now prefers the URL over the kind label, so a bare {url, kind:'pr'} shows the URL (distinguishable) instead of "Pull Request"; hostname secondary line only when the label isn't already the URL and there's no number.
4. P2 — review evidence. DecisionCard surfaces data.url || data.review_url as a "review" link (also safeHref-guarded), so legacy review → decision rows carry their evidence instead of rendering blank. The resolveLegacyShape mapping is intentionally left to PR A / producer relocation to PR C — reasonable split.

Independent re-check this round

• ArtifactCard vitest: 23/23. Shared artifact-shapes: 22/22. Web tsc --noEmit: clean.
• artifact-shapes.ts diverges from PR A tip 129c8d3 by exactly the link scheme-validation block — clean single-hunk rebase once #2313 lands.
• Threads: 4/4 resolved, PR OPEN and MERGEABLE.
• CI: no failing checks; the previously-flaky 4-space-migrations-a/-b shards both passed this round. Only unrelated tail shards + the aggregate still finishing.

Non-blocking reminders

• Merge order: land #2313 (PR A) first; this branch's artifact-shapes.ts adds only the scheme-validation lines, so rebase is a trivial reconcile.
• validateArtifactShape('link') scheme check applies to new saves only — the migration correctly doesn't re-validate legacy rows; the UI safeHref is the primary guard regardless.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 45c38b2daa

ℹ️ 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]

🤖 Review by glm-5.1 (z.ai/glm)

Model: glm-5.1 | Client: NeoKai | Provider: z.ai/glm

Re: "Keep legacy artifacts renderable until producers migrate" — evaluated; not a PR-B code finding. Recommendation unchanged: APPROVE, with the hard precondition that #2313 lands first.

The facts are accurate and I re-verified them: in this branch save_artifact still persists arbitrary types, the built-in producers still write progress/review/result/pr (node-agent-tools.ts:678, 1071, 1090, 1524, 1562), and resolveLegacyShape/normalizeLinkData/validateArtifactShape are referenced nowhere in packages/daemon/src (only their unit tests call them). So without #2313, every artifact falls through to GenericCard. Agreed.

This is the documented PR A → PR B dependency rather than a defect in this PR's code:

• Task #808 explicitly defines PR B as a follow-up to #796 PR A, whose scope is exactly the save_artifact shape validation + the backfill migration that wires these helpers. #2313 (PR A) is open and contains that wiring.
• The task's chosen architecture is "UI reads shapes directly; legacy is normalized at the write/migration boundary in PR A." Adding legacy normalization back at the UI boundary (the suggested fix) would create a second normalization point that becomes dead code once #2313 lands, and would partially undo this PR's purpose (replace data-heuristics with shape dispatch). It's the kind of speculative bridge code the task deliberately avoids.

The mitigation is merge order — and it's human-gated: #2313 must land before this PR. That requirement is stated in both my review rounds and the approval handoff. If #2313 is at risk of not landing soon, the right move is to hold this PR rather than graft UI-side legacy dispatch onto it. Net: APPROVE stands, conditional on #2313 first.

[lsm]

🤖 Review by glm-5.1 (z.ai/glm)

Model: glm-5.1 | Client: NeoKai | Provider: z.ai/glm

Recommendation: APPROVE — re-verified on the new head 1fa7ec608 (legacy-type normalization at the UI boundary). This cleanly resolves the codex P1 and makes PR B self-sufficient.

What 1fa7ec608 adds (verified)

• resolveArtifactShape(): isArtifactShape(type) ? type : resolveLegacyShape(type, data) ?? null. Post-migration rows short-circuit; legacy progress/result/pr/review map to shapes at the render boundary; truly unknown types still fall to GenericCard.
• normalizeLinkData is applied only on the link path (LinkCard keys off data.url), so a legacy pr/result-with-pr_url row renders as a proper link. review → decision keeps its evidence via the DecisionCard data.url || data.review_url link added earlier.
• Idempotent and read-only — once #2313 stores shapes, the legacy branch is never taken. This is a bounded type→shape map, not a re-introduced data-shape heuristic, so it stays true to the PR's goal. Legacy data flows through the already-hardened renderers (safeHref + commit-entry filter), so no new XSS/crash surface.

Re-check this round

• ArtifactCard vitest 29/29 (incl. 6 new legacy-normalization cases: pr→link, progress→note, result→decision, result+url→link, review→decision+link, unknown→generic). Shared artifact-shapes 22/22. Web tsc --noEmit clean.
• PR OPEN + MERGEABLE; 5/5 review threads resolved.
• CI: 31 checks, 0 failures (Web Tests + migration shards still finishing at post time).

Merge posture (updated) — this softens my earlier hard precondition: PR B no longer hard-degrades if it lands before #2313, because legacy rows now render via the shape renderers. #2313 is still wanted for closed-set save_artifact validation + the producer migration (so new writes are shapes natively), but it is no longer a rendering blocker. Approve as-is.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1fa7ec6087

ℹ️ 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]

🤖 Review by glm-5.1 (z.ai/glm)

Model: glm-5.1 | Client: NeoKai | Provider: z.ai/glm

Recommendation: REQUEST_CHANGES — two more valid P2s from the codex pass, both verified. These are the only open items; after them I expect to approve. (The earlier four findings remain fixed and verified.)

---

P2 — URL-bearing result rows drop the QA evidence — artifact-shapes.ts:155-162 (routing) + LinkCard
The full-stack QA success artifact (built-in-workflows.ts:1083) is type:'result' carrying a top-level summary and data.pr_url/test_output/ui_changed/browser_validation. save_artifact merges summary into data (node-agent-tools.ts:1519), and resolveLegacyShape('result') routes to link purely because pr_url is a string — so LinkCard renders only the PR link and silently drops the summary/test_output. The old detectRenderer rendered test_output (TerminalOutputCard), so this is a regression for an active producer (the QA workflow itself).
Fix (surgical, PR-B): LinkCard already receives data.summary (it survives normalizeLinkData); surface it as a subtitle when present and there's no title. That keeps the QA verdict visible alongside the link. Full separation (the QA agent emitting a clean decision + link pair) is PR C's producer-relocation scope — note that test_output has no equivalent in the closed shape vocab, so it's genuinely deferred to that work.

P2 — Non-scalar metric values render as [object Object] — artifact-shapes.ts:282-285 (validator) + MetricCard
validateArtifactShape('metric') only rejects null/undefined, and the data schema is z.record(z.unknown()), so {name:'latency', value:{current:42}} is accepted. MetricCard then does String(value) → [object Object], presenting a malformed measurement as valid. Same applies to target (String(target)).
Fix (PR-B): validateArtifactShape('metric') require value (and target when present) to be number/string; MetricCard render value/target only when scalar.

---

Threads: #2314 (comment) · #2314 (comment)

Everything else from round 4 holds: ArtifactCard 29/29, shared 22/22, typecheck clean, MERGEABLE, CI no failures. Both fixes above are small and contained to this PR's files; ping me for the (final) re-review.

[lsm]

🤖 Review by glm-5.1 (z.ai/glm)

Model: glm-5.1 | Client: NeoKai | Provider: z.ai/glm

Recommendation: APPROVE — round 5 final sign-off on 3664601ad. Both P2s from the codex pass are correctly addressed; I re-verified from scratch on the pushed commit.

Verified on 3664601ad:

• QA evidence (P2): resolveLegacyShape('result') now routes url+content results to decision (URL-only stays link); DecisionCard surfaces summary and falls back to pr_url (label 'view') — so the QA audit keeps both its verdict and its PR link. Blast radius re-checked: the QA workflow is the sole url+content producer; all other type:'result' rows are url-only (→link, unchanged) or summary-only (→decision, unchanged). PR A's url-only tests still pass.
• Non-scalar metric (P2): validateArtifactShape('metric') rejects non-number/string value (defense-in-depth at save time); MetricCard guards value/target with hasValue/hasTarget (renders nothing instead of [object Object]). Existing valid metrics unaffected.
• Scope: the commit touches only artifact-shapes.ts + ArtifactCard.tsx; every changed line traces to the two findings, no scope creep.
• Threads: 7/7 resolved, 0 unresolved.
• CI: zero failures. Lint/Knip/Format/Typecheck ✓, Daemon Unit Tests (0-shared-handlers-workflow) ✓ (covers the save_artifact validator), flaky migrations shards ✓. 5 daemon integration/online shards still in flight (none touch this PR's files) — confirm green before merge.

Tests (re-run locally on HEAD): ArtifactCard 31/31 (+2), shared artifact-shapes 24/24 (+2), tsc --noEmit clean.

Deferred (acknowledged, not blocking): splitting the QA producer's mixed audit into proper link/check/note artifacts is PR C's scope; this shim preserves the headline evidence (summary + PR link) until then. This PR (shape-based rendering + UI-boundary legacy normalization) is complete and self-sufficient regardless of #2313's merge order.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3664601adc

ℹ️ 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]

🤖 Review by glm-5.1 (z.ai/glm)

Model: glm-5.1 | Client: NeoKai | Provider: z.ai/glm

Recommendation: REQUEST_CHANGES — this supersedes my prior APPROVE (review 4839430054) and the submit-for-approval that followed it. A valid P2 arrived after I submitted; please do NOT merge until round 7 lands. My autonomy level means a human finalizes the merge — treat this as the current state, not the earlier approval.

P2 — Merged-PR audit renders as an empty card — packages/shared/src/artifact-shapes.ts (resolveLegacyShape hasUrl + normalizeLinkData)
Validated independently. The live post-approval merge template (post-approval-merge-template.ts:208-210, wired in via built-in-workflows.ts:38) writes save_artifact({ type:"result", data:{ merged_pr_url, merged_at, approval_source } }). resolveLegacyShape doesn't recognize merged_pr_url as a URL (it checks url/pr_url/prUrl/review_url only), so the row routes to decision; DecisionCard reads none of {merged_pr_url, merged_at, approval_source} and has no recommendation/summary → a bare "DECISION" badge, hiding the merged PR URL entirely. The previous renderer surfaced it. This is the same omission class as the round-5 QA finding, and notably migrations.ts:10053 already iterates merged_pr_url as a URL field — so this is an inconsistency with the codebase's own canonical URL-field list.

Fix (small, in PR B's UI-shim scope): recognize merged_pr_url as a URL in both resolveLegacyShape's hasUrl check and normalizeLinkData's URL extraction (so it's copied onto data.url). Then the audit routes to link and LinkCard renders the merged PR URL. Cleanest form: align both functions' URL-field set with the migrations.ts:10053 list (['pr_url','merged_pr_url','review_url','reviewUrl','url']) — that covers merged_pr_url and keeps the two sites mutually consistent (and matches the migration). merged_at/approval_source are secondary audit metadata; full producer normalization (a clean link kind:"pr" + audit note) remains PR C.

Thread: #2314 (comment)

[lsm]

🤖 Review by glm-5.1 (z.ai/glm)

Model: glm-5.1 | Client: NeoKai | Provider: z.ai/glm

Recommendation: APPROVE — round-7 final sign-off on aadbddcd1. The merged-PR-audit P2 is correctly fixed; re-verified from scratch.

Verified on aadbddcd1:

• Merged-PR audit (P2): the post-approval merge audit { merged_pr_url, merged_at, approval_source } now routes to link and LinkCard renders the merged PR URL — no more bare badge. findLinkUrl matches any url/*_url/*Url key (string + non-empty guarded), covering merged_pr_url and stopping the per-variant whack-a-mole that produced both the round-5 and round-7 findings. Shared by resolveLegacyShape('result').hasUrl and normalizeLinkData (DRY).
• Generalization is sound, not over-engineering: two consecutive findings were the same omission class, so a pattern matcher is the simpler long-term choice than re-enumerating. Precedence is correct — normalizeLinkData early-outs on an existing data.url, so explicit url always wins over other *_url fields; hasUrl only needs a boolean.
• Blast radius contained: normalizeLinkData/resolveLegacyShape are called only from ArtifactCard.tsx (web) — the daemon save/migration paths don't use them — so the broader matching affects only UI-boundary rendering.
• Scope: commit touches 3 files (logic + shared test + web test); no scope creep.
• Threads: 8/8 resolved, 0 unresolved (no new findings on this commit).
• Tests (re-run locally on HEAD): shared 25/25 (+1), ArtifactCard 32/32 (+1), tsc --noEmit clean.

CI: 0 failures on aadbddcd1; the deterministic Lint/Knip/Typecheck gate was green on the prior commit and I verified typecheck/lint locally — confirm it green on this commit before merge (still running as of this review).

Deferred (acknowledged, not blocking): merged_at/approval_source audit metadata and the clean producer split (link kind:"pr" + audit note) remain PR C.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: aadbddcd14

ℹ️ 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]

🤖 Review by glm-5.1 (z.ai/glm)

Model: glm-5.1 | Client: NeoKai | Provider: z.ai/glm

Recommendation: APPROVE (unchanged) — two more codex P2s (r3700015457, r3700015459) are real as behavior but are the deferred-to-PR-C scope documented back in round 5; my approval on aadbddcd1 stands. Surfacing the rationale and a scope decision for the merge authority.

Both findings are the same class — secondary fields of a mixed legacy payload are dropped after shape routing:

1. QA success result → decision: DecisionCard renders summary + the PR link, dropping test_output / ui_changed / browser_validation.
2. Merge audit → link (round-7 fix): LinkCard renders merged_pr_url, dropping merged_at / approval_source.

The headline evidence IS preserved in each case (QA verdict summary + PR link; the merged PR URL). What's hidden is secondary detail the old freeform table renderer displayed.

Why this is PR C, not PR B: the closed 6-shape vocab has no home for these fields — test_output is fundamentally a check (status + counts), and merged_at / approval_source are audit metadata (a note). This was called out in the round-5 review ("test_output is an acknowledged vocab gap; deferred to PR C's producer normalization"). Rendering them inside PR B's generic DecisionCard/LinkCard would either special-case producer field names or add a generic arbitrary-field renderer — both couple generic infra to a specific producer's payload, which is exactly the freeform-data coupling this PR removes. No clean in-scope fix exists (unlike the round-5/7 findings, which had clean routing/URL fixes).

Recommendation: proceed with the merge (squash, no --delete-branch, dev) after CI green; track both in PR C — the QA producer should emit a check (+ link + note) and the merge producer a link + note, instead of mixed blobs. If the merge authority prefers a PR-B interim mitigation (e.g. a generic expandable raw-data fallback on legacy-mapped cards), hold and I'll route it — but I don't recommend expanding PR B's scope, since it re-introduces the freeform coupling the design removes and PR C is the correct home.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[lsm]

🤖 Review by glm-5.1 (z.ai/glm)

Model: glm-5.1 | Client: NeoKai | Provider: z.ai/glm

Recommendation: APPROVE — re-verified the rebased tip b673be58c after #2313 (PR A) merged and this PR was rebased onto dev (force-push from aadbddcd1). All approved behaviors are preserved; the rebase is clean.

Rebase re-verification — all 6 approved behaviors present at b673be58c:

• resolveLegacyShape('result') summary-aware routing ✓ — hasSummary + findLinkUrl → url-only result = link, summary-bearing = decision. (PR A's version uses hasSummary instead of PR B's hasContent; semantically equivalent for the real producers — QA result has summary+pr_url → decision; merge audit has merged_pr_url only → link. The dropped test_output/stdout/stderr content-detectors only affected a hypothetical url+output-no-summary result that no producer emits, and test_output rendering is PR C anyway.)
• validateArtifactShape('metric') rejects non-scalar value ✓
• validateArtifactShape('link') http(s) scheme validation (PR-B-only addition) ✓
• findLinkUrl helper (PR-B-only) wired into resolveLegacyShape + normalizeLinkData ✓
• MetricCard scalar guards (hasValue/hasTarget) ✓
• DecisionCard pr_url fallback + 'view' label ✓

PR A's merged versions also brought sensible refinements (commit_set repo:branch identity, decision kind-namespaced keys, note accepts a bare ts) — improvements, no regressions.

Scope: PR diff vs dev is the ArtifactCard renderer + the two artifact-shapes.ts hardening additions (http(s) validation, findLinkUrl) — exactly as the rebase summary describes. Tests green: shared 25/25, ArtifactCard 32/32. CI is rerunning on b673be58c (was fully green on aadbddcd1) — confirm green before merge.

The two legacy-fidelity items remain deferred to PR C (#809). Note: the codex review bot hit its usage limit, so no further automated findings are expected.

[greptile-apps]

Greptile Summary

Replaces data-shape sniffing (detectRenderer) with a closed artifactType-keyed dispatch (link, commit_set, check, metric, decision, note), and adds two hardening additions to artifact-shapes.ts: http(s) scheme validation in validateArtifactShape('link') and a findLinkUrl helper that lets legacy merged_pr_url results route to a link renderer. Legacy pre-shape types are normalized at the UI boundary via resolveLegacyShape/normalizeLinkData.

• ArtifactCard is rebuilt as one dedicated sub-component per shape; safeHref guards every rendered anchor; DecisionCard evaluates each URL candidate independently.
• artifact-shapes.ts adds scheme validation at save time and a broad *_url suffix matcher for merged_pr_url pickup; the over-broad matching trade-off is tracked in previous review threads.
• Tests are fully replaced; XSS anchor suppression, legacy normalization, and all six shapes are exercised.

Confidence Score: 5/5

Safe to merge. Every rendered anchor goes through safeHref; legacy normalization is idempotent and well-tested; the only new findings are minor style inconsistencies.

Each shape gets a dedicated sub-component with explicit field access and graceful degradation for malformed payloads. The safeHref guard is applied uniformly. New findings are limited to a commit-list index key and a target-unit spacing inconsistency — neither affects correctness.

Files Needing Attention: No files require special attention beyond the findLinkUrl breadth trade-off already noted in previous review threads on artifact-shapes.ts.

Important Files Changed

Filename	Overview
packages/shared/src/artifact-shapes.ts	Adds findLinkUrl (suffix-based URL field discovery) used in both resolveLegacyShape and normalizeLinkData, plus http(s) scheme validation in validateArtifactShape('link').
packages/web/src/components/space/ArtifactCard.tsx	Full rewrite from data-shape sniffing to artifactType-keyed dispatch. Each shape gets a dedicated sub-component; safeHref guards all rendered anchors; legacy normalization handled cleanly at the UI boundary.
packages/shared/tests/artifact-shapes.test.ts	Adds coverage for http(s) scheme rejection in validateArtifactShape('link') and merged_pr_url routing through resolveLegacyShape/normalizeLinkData.
packages/web/src/components/space/tests/ArtifactCard.test.tsx	Tests fully replaced to match shape-driven dispatch. XSS guard, DecisionCard multi-candidate URL fallback, legacy normalization, and all six shape renderers are covered.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[ArtifactCard receives artifact] --> B{isArtifactShape}
    B -- yes --> C[shape = artifactType]
    B -- no --> D[resolveLegacyShape]
    D -- pr --> E[link]
    D -- progress --> F[note]
    D -- review --> G[decision]
    D -- result URL-only --> H[link]
    D -- result with summary --> I[decision]
    D -- unknown --> J[null]
    C --> K{dispatch on shape}
    E --> K
    F --> K
    G --> K
    H --> K
    I --> K
    K -- link --> L[normalizeLinkData then LinkCard]
    K -- commit_set --> M[CommitSetCard]
    K -- check --> N[CheckCard]
    K -- metric --> O[MetricCard]
    K -- decision --> P[DecisionCard]
    K -- note --> Q[NoteCard]
    J --> R[GenericCard]
    K -- null/unknown --> R

Loading

_{Reviews (2): Last reviewed commit: "fix(web): DecisionCard — validate each e..." | Re-trigger Greptile}

[lsm]

🤖 Review by glm-5.1 (NeoKai)

Model: glm-5.1 | Client: NeoKai | Provider: Zhipu AI

Recommendation: REQUEST_CHANGES (round 11). CI is green; two small DecisionCard logic nits are worth tightening before merge; one Greptile finding is declined as deliberate design.

CI: the earlier red was a transient infra flake (now green)

Finalize Coveralls failed because coveralls.io returned HTTP 503 "Service Unavailable (queue full)" on the parallel-finished webhook — a service-side capacity event, not a coverage threshold or code regression. It self-recovered: all 30 checks now report success (Daemon Unit/Online shards, Web Tests, Lint/Knip/Typecheck, Coverage Quality Gate, Finalize Coveralls, Greptile Review). No action needed on the code for this.

Adjudicating the 3 Greptile P2s (review 4840030306)

r3700636439 — DecisionCard label/href mismatch → please fix. Valid. href = safeHref(str(data.url) || reviewUrl || str(data.pr_url)) picks data.url when set, but the label {reviewUrl ? 'review' : 'view'} keys off whether review_url exists. If a decision carries both data.url and review_url, the anchor points at data.url while reading "review".

r3700636440 — non-http data.url silently drops the evidence link → please fix. Valid, and the more substantive of the two. The || chain selects the first non-empty candidate string, then a single safeHref validates it. A non-http data.url (e.g. a stray ftp://) makes safeHref return null, so the valid review_url / pr_url fallbacks are never evaluated — directly defeating the comment's stated intent ("surface their evidence link to avoid a blank card").

One-shot fix for both — validate each candidate independently and remember which won:

const picked =
  safeHref(str(artifact.data.url))   ? { href: safeHref(str(artifact.data.url))!,   label: 'view' as const }
  : safeHref(reviewUrl)              ? { href: safeHref(reviewUrl)!,                label: 'review' as const }
  : safeHref(str(artifact.data.pr_url)) ? { href: safeHref(str(artifact.data.pr_url))!, label: 'view' as const }
  : null;

…then render {picked?.href} and {picked?.label ?? 'view'}. These only surface on an undocumented data.url-on-a-decision input (the designed legacy decisions carry review_url or pr_url, not bare url), but the fix is ~6 lines, low-risk, and removes the latent bug from the legacy shim before PR C rewrites it.

r3700636442 — findLinkUrl broad *_url match → declining, deliberate design. This broadening is the round-7 fix, chosen to recognize merged_pr_url and future audit URL keys without per-key whack-a-mole. The image_url/avatar_url→link edge case is benign: such a value is still a valid http(s) URL (gated by validateArtifactShape at storage and safeHref at render), opens correctly, and no current producer emits a result with a bare image_url. Reverting to an explicit list would re-introduce the whack-a-mole the design removed. No change.

Otherwise unchanged on b673be5

Shape-keyed dispatch, safeHref XSS guard, resolveLegacyShape summary-aware routing, validateArtifactShape http(s) gate, and normalizeLinkData remain correct. Shared tests 25/25, ArtifactCard tests 32/32 green. I'll re-approve once the DecisionCard selection is tightened (and the three Greptile threads resolved).

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[lsm]

🤖 Review by glm-5.1 (NeoKai)

Model: glm-5.1 | Client: NeoKai | Provider: Zhipu AI

Recommendation: APPROVE (code) — merge-escalation gated on CI green for cce47a93b.

Round-13 re-review of the rebased head cce47a93b, verified independently from scratch:

• DecisionCard fix correct (ArtifactCard.tsx:406-418): each evidence-URL candidate is validated independently (safeHref(url) / safeHref(review_url) / safeHref(pr_url)), the first http(s) one is picked, and the link label tracks the winner (view / review). Both round-11 nits are genuinely resolved — r3700636439 (label/href mismatch) and r3700636440 (non-http data.url no longer shadows a valid fallback). r3700636442 stays as deliberate round-7 design.
• Tests pass (independent run): ArtifactCard 34/34 (incl. the 2 new cases), shared artifact-shapes 25/25.
• Scope clean: the PR's own diff vs dev is exactly the 4 intended files (artifact-shapes.ts + test, ArtifactCard.tsx + test). The large b673be58c..cce47a93b delta was only the rebased-in dev content (#2271/#2270) — no scope creep.
• All 13 review threads resolved (0 unresolved). Rebased onto dev (0 commits behind); commits signed.

Only gate remaining: CI is still running on cce47a93b (0 failures / 23 in progress), so mergeStateStatus is BLOCKED purely pending checks. I'll escalate for merge once CI completes green and the state clears. (Watch-item: if BLOCKED persists after green CI, a required check may be one skipped on PR branches — I'll investigate then rather than assume.)