Skip to content
Closed
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
39 changes: 37 additions & 2 deletions src/review/content-lane/source-evidence.ts
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,31 @@ function listSourceUrlValues(source: string, spec: ContentRepoSpec): SubmittedSo
return values;
}

// A SCALAR source field authored as a YAML block sequence (`field:` with an empty inline value, then `- item`
// lines) must yield one URL per item — exactly as listSourceUrlValues already does for a list field. Otherwise
// parseSimpleFrontmatter collapses the items into one comma-joined string that `new URL()` cannot parse, so two
// live sources hard-fail a valid submission (#9668). Returns null for every NON-sequence form (an inline
// scalar, a flow `[a, b]` list, or a block scalar `|`/`>`), which the scalar reader already handles correctly;
// only a bare block sequence needs the per-item split. Mirrors listSourceUrlValues' `- item` grammar.
function scalarSequenceItems(source: string, field: string): string[] | null {
const lines = frontmatterBlock(source).split(/\r?\n/);
for (let i = 0; i < lines.length; i += 1) {
const head = /^([A-Za-z][A-Za-z0-9_]*):\s*(.*?)\s*$/.exec(lines[i] ?? "");
if (!head || head[1] !== field) continue;
if ((head[2] ?? "") !== "") return null; // an inline value present ⇒ scalar / flow / block-scalar header, not a bare sequence
const items: string[] = [];
for (let j = i + 1; j < lines.length; j += 1) {
const line = lines[j] ?? "";
if (line.trim() === "") break; // a blank line ends the sequence
const item = /^\s*-\s*(.*?)\s*$/.exec(line);
if (!item) break; // the next top-level key (or any non-`-` line) ends the sequence
items.push(item[1] ?? "");
}
return items.length > 0 ? items : null;
}
return null;
}

function isAbsoluteHttpUrl(url: string): boolean {
try {
const protocol = new URL(url).protocol;
Expand All @@ -244,8 +269,18 @@ export function extractSubmittedSourceUrls(
const fields = parseSimpleFrontmatter(source);
const urls: SubmittedSourceUrl[] = [];
for (const field of spec.sourceUrlFields) {
for (const url of scalarSourceUrlValues(fields[field] || "")) {
urls.push({ field, url });
const sequenceItems = scalarSequenceItems(source, field);
if (sequenceItems) {
// One URL per block-sequence item (mirrors listSourceUrlValues), so a multi-item scalar sequence is not
// fused into one unparseable string; a single-item sequence still yields exactly one URL as before.
for (const item of sequenceItems) {
const url = unquoteYamlValue(item);
if (url) urls.push({ field, url });
}
} else {
for (const url of scalarSourceUrlValues(fields[field] || "")) {
urls.push({ field, url });
}
}
}
urls.push(...listSourceUrlValues(source, spec));
Expand Down
33 changes: 33 additions & 0 deletions test/unit/content-lane-source-evidence.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,24 @@ describe("checkSubmittedSourceEvidence", () => {
expect(report.status).toBe("passed");
});

it("REGRESSION (#9668): fetch-verifies BOTH URLs of a two-item scalar sequence instead of hard-failing the joined string", async () => {
// Before the fix the two items were joined into "https://a.example/1, https://b.example/2", which new URL()
// rejects → outcome invalid_url → the whole report "failed" on a submission whose sources are both live.
const a = "https://a.example/1";
const b = "https://b.example/2";
const src = ["---", "documentationUrl:", ` - ${a}`, ` - ${b}`, "---", "", "body"].join("\n");
const report = await checkSubmittedSourceEvidence(src, fakeFetch({ [a]: 200, [b]: 200 }));
expect(report.urls.map((u) => u.url).sort()).toEqual([a, b]);
expect(report.status).toBe("passed");
});

it("still hard-fails a genuinely malformed scalar source value — the false-positive removal must not weaken the real check", async () => {
const report = await checkSubmittedSourceEvidence(mdx({ documentationUrl: "notaurl" }), fakeFetch({}));
const item = report.urls.find((u) => u.field === "documentationUrl") ?? report.warnings.find((u) => u.field === "documentationUrl");
expect(item?.outcome).toBe("invalid_url");
expect(item?.status).toBe("hard_failure");
});

it("is retryable (not hard) on a 403/429/5xx canonical source", async () => {
const src = mdx({ githubUrl: "https://github.com/acme/x" });
const report = await checkSubmittedSourceEvidence(src, fakeFetch({ "https://github.com/acme/x": 403 }));
Expand Down Expand Up @@ -359,6 +377,21 @@ describe("extractSubmittedSourceUrls — frontmatter parsing edge cases", () =>
expect(urls.map((u) => `${u.field}:${u.url}`)).toContain("documentationUrl:https://docs.acme.example/guide");
});

it("regression (#9668): a scalar source field authored as a MULTI-item block sequence yields one URL per item", () => {
const a = "https://a.example/1";
const b = "https://b.example/2";
const src = ["---", "documentationUrl:", ` - ${a}`, ` - ${b}`, "---", "", "body"].join("\n");
const urls = extractSubmittedSourceUrls(src).filter((u) => u.field === "documentationUrl");
expect(urls.map((u) => u.url)).toEqual([a, b]); // two distinct URLs, not one comma-joined string
});

it("regression (#9668): a single-item scalar sequence is byte-identical — exactly one URL, its raw value", () => {
const url = "https://docs.acme.example/only";
const src = ["---", "documentationUrl:", ` - ${url}`, "---", "", "body"].join("\n");
const urls = extractSubmittedSourceUrls(src).filter((u) => u.field === "documentationUrl");
expect(urls.map((u) => u.url)).toEqual([url]);
});

it("does not surface any block-scalar header on a list field as a bogus URL (both indicator orders)", () => {
// `retrievalSources` is a list field; a block-scalar header is not a URL. The old guard only skipped the bare
// `|`/`>`, so `|-` leaked as the literal url "|-". YAML allows chomping and the indentation digit in EITHER
Expand Down
Loading