Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 41 additions & 10 deletions packages/shared/src/artifact-shapes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -149,11 +149,10 @@ export function resolveLegacyShape(
case 'result': {
const d = data ?? {};
const hasSummary = typeof d.summary === 'string' && d.summary.length > 0;
const hasUrl =
typeof d.url === 'string' ||
typeof d.pr_url === 'string' ||
typeof d.prUrl === 'string' ||
typeof d.review_url === 'string';
// findLinkUrl recognizes any *_url field (pr_url, merged_pr_url,
// review_url, …), so the post-approval merge audit (merged_pr_url) routes
// to a link like other URL-bearing results.
const hasUrl = findLinkUrl(d) !== null;
// A summary is the important payload (QA outcome / completion note) and
// must stay visible to decision readers, so prefer decision when present.
// Only a URL-only result becomes a pure link.
Expand All @@ -168,17 +167,34 @@ export function isArtifactShape(value: unknown): value is ArtifactShape {
return typeof value === 'string' && (ARTIFACT_SHAPES as readonly string[]).includes(value);
}

/**
* Find a URL-bearing field in legacy data. Pre-shape producers stored the URL
* under various keys (`url`, `pr_url`, `prUrl`, `review_url`, `merged_pr_url`,
* `image_url`, `external_url`, …), so match any key that is `url` or ends in
* `_url`/`Url`. Returns the first non-empty string value found, else null.
*/
function findLinkUrl(data: Record<string, unknown> | undefined): string | null {
if (!data) return null;
for (const [key, value] of Object.entries(data)) {
if (
(key === 'url' || key.endsWith('_url') || key.endsWith('Url')) &&
typeof value === 'string' &&
value
) {
return value;
}
}
return null;
}
Comment thread
lsm marked this conversation as resolved.

/**
* For a legacy row being treated as a `link`, copy the URL-bearing field onto
* `data.url` so link readers (which key off `data.url`) find it. Returns a new
* data object; no-op when `data.url` is already set or no URL field is present.
*/
export function normalizeLinkData(data: Record<string, unknown>): Record<string, unknown> {
if (typeof data.url === 'string' && data.url) return data;
const url =
(typeof data.pr_url === 'string' && data.pr_url) ||
(typeof data.prUrl === 'string' && data.prUrl) ||
(typeof data.review_url === 'string' && data.review_url);
const url = findLinkUrl(data);
if (!url) return data;
return { ...data, url };
}
Expand Down Expand Up @@ -259,11 +275,26 @@ export function validateArtifactShape(
data: Record<string, unknown>
): ArtifactValidation {
switch (shape) {
case 'link':
case 'link': {
if (!nonEmptyString(data.url)) {
return { ok: false, error: "shape 'link' requires data.url (the URL)." };
}
// Defense-in-depth: link URLs are agent-controlled and rendered as
// clickable anchors, so restrict storage to http(s). This prevents
// `javascript:` / custom-scheme URLs from ever reaching a renderer.
try {
const parsed = new URL(data.url as string);
if (parsed.protocol !== 'http:' && parsed.protocol !== 'https:') {
return {
ok: false,
error: `shape 'link' requires an http(s) URL (got '${parsed.protocol}').`,
};
}
} catch {
return { ok: false, error: "shape 'link' requires a valid http(s) URL." };
}
return { ok: true };
}
case 'check':
if (!nonEmptyString(data.name)) {
return { ok: false, error: "shape 'check' requires data.name (the check identity)." };
Expand Down
18 changes: 18 additions & 0 deletions packages/shared/tests/artifact-shapes.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -77,10 +77,18 @@ describe('artifact-shapes: deriveArtifactKey (identity rules)', () => {
describe('artifact-shapes: validateArtifactShape', () => {
test('link requires data.url', () => {
expect(validateArtifactShape('link', { url: 'https://x' })).toEqual({ ok: true });
expect(validateArtifactShape('link', { url: 'http://x.example' })).toEqual({ ok: true });
const bad = validateArtifactShape('link', { title: 'no url' });
expect(bad.ok).toBe(false);
});

test('link rejects non-http(s) URLs (defense-in-depth against agent-controlled schemes)', () => {
expect(validateArtifactShape('link', { url: 'javascript:alert(1)' }).ok).toBe(false);
expect(validateArtifactShape('link', { url: 'data:text/html,<script>' }).ok).toBe(false);
expect(validateArtifactShape('link', { url: 'ftp://example.com/x' }).ok).toBe(false);
expect(validateArtifactShape('link', { url: 'not a url' }).ok).toBe(false);
});

test('check requires name + status', () => {
expect(validateArtifactShape('check', { name: 'ci', status: 'pass' })).toEqual({ ok: true });
expect(validateArtifactShape('check', { name: 'ci' }).ok).toBe(false);
Expand Down Expand Up @@ -125,6 +133,8 @@ describe('artifact-shapes: resolveLegacyShape (data-aware router)', () => {
expect(resolveLegacyShape('result', { pr_url: 'u' })).toBe('link');
expect(resolveLegacyShape('result', { url: 'u' })).toBe('link');
expect(resolveLegacyShape('result', { review_url: 'u' })).toBe('link');
// The post-approval merge audit stores the URL under merged_pr_url.
expect(resolveLegacyShape('result', { merged_pr_url: 'u' })).toBe('link');
});

test('result with a summary → decision (summary preserved even alongside a URL)', () => {
Expand Down Expand Up @@ -154,6 +164,14 @@ describe('artifact-shapes: normalizeLinkData', () => {
});
});

test('copies merged_pr_url onto data.url when missing', () => {
expect(normalizeLinkData({ merged_pr_url: 'https://x', merged_at: '2026-01-01' })).toEqual({
merged_pr_url: 'https://x',
merged_at: '2026-01-01',
url: 'https://x',
});
});

test('leaves data.url untouched when already set', () => {
expect(normalizeLinkData({ url: 'https://x', pr_url: 'https://y' })).toEqual({
url: 'https://x',
Expand Down
Loading