From 49c32d8720a833c20eaaceb07f6ce0f41c78d375 Mon Sep 17 00:00:00 2001 From: Georges-Antoine Assi Date: Mon, 3 Aug 2026 08:32:21 -0400 Subject: [PATCH] fix(ci): stop a comment in tach.toml from truncating a module's deps parseTachModules scanned for the closing bracket of depends_on with a non-greedy match, so a `]` inside a comment ended the list early and every entry below it was dropped. products.review_hog annotates its facade-only edge with the name of the tach block that enforces it, and the two entries after that comment, products.stamphog and products.tasks, were never parsed. The drop was silent because the fail-closed check discarded comments before looking for unsupported syntax, so a truncated list looked well-formed. Both consumers lost the same edges: turbo-discover skipped review_hog's suite when stamphog or tasks changed, and the merge queue put the two products in parallel lanes. Ten of the last 500 merged PRs should have claimed py:product:review_hog and did not. Comments are now stripped before the block scan, which also lets the fail-closed check see the raw list again. Co-Authored-By: Claude Opus 5 (1M context) --- .../scripts/turbo-discover-cascade.test.js | 20 ++++++++++ .github/scripts/turbo-discover.js | 39 ++++++++++++++++--- 2 files changed, 54 insertions(+), 5 deletions(-) diff --git a/.github/scripts/turbo-discover-cascade.test.js b/.github/scripts/turbo-discover-cascade.test.js index 25f0511ef1a1..70d9bab0864a 100644 --- a/.github/scripts/turbo-discover-cascade.test.js +++ b/.github/scripts/turbo-discover-cascade.test.js @@ -207,6 +207,26 @@ layer = "modules" assert.deepEqual(dependents, []) }) +// tach.toml annotates facade-only edges with prose that names the tach block +// enforcing them, so a depends_on list can carry a `]` inside a comment. The +// scan for the list's closing bracket used to stop there and drop every entry +// below it, and the fail-closed check could not see it because that check +// discarded comments too. +test('a comment inside depends_on does not truncate the list', () => { + const toml = ` +[[modules]] +path = "products.a" +depends_on = [ + "products.b", + # Facade-only (enforced by c's [[interfaces]] block): a queues c's work. + "products.c", + "products.d", # direct import +] +layer = "modules" +` + assert.deepEqual(parseTachModules(toml).get('a'), ['b', 'c', 'd']) +}) + test('fail closed: a depends_on entry that is not a double-quoted string throws instead of silently dropping the edge', () => { const toml = ` [[modules]] diff --git a/.github/scripts/turbo-discover.js b/.github/scripts/turbo-discover.js index d0354976f46f..17deeb45a294 100644 --- a/.github/scripts/turbo-discover.js +++ b/.github/scripts/turbo-discover.js @@ -244,20 +244,47 @@ const moduleToProduct = (module) => module.replace(/_/g, '-') // aren't products.* so they fall out of the startsWith filter for free. See // tachDependents for why routing through them would be wrong, not just // inconvenient. +// TOML comments run to the end of the line, and a `#` inside a double-quoted +// string does not start one. Comments have to go before the block scan below, +// because a comment inside a depends_on list can carry a `]`: tach.toml +// documents a facade-only edge as "enforced by stamphog's [[interfaces]] +// block", and that bracket ends the non-greedy scan early, dropping every +// entry after it. +function stripTomlComments(tomlText) { + let out = '' + let inString = false + for (let index = 0; index < tomlText.length; index++) { + const char = tomlText[index] + if (char === '"' && tomlText[index - 1] !== '\\') { + inString = !inString + } + if (!inString && char === '#') { + while (index < tomlText.length && tomlText[index] !== '\n') { + index++ + } + out += '\n' + continue + } + out += char + } + return out +} + function parseTachModules(tomlText) { const graph = new Map() // Each `[[modules]]` block holds exactly one `path` and one `depends_on` // before the next block starts — split on the marker and take the first - // match of each within a block. depends_on entries are plain quoted - // strings with no nested brackets, so a non-greedy scan to the first `]` - // is safe even across multi-line lists or lists split across shared lines. + // match of each within a block. With comments stripped, depends_on entries + // are plain quoted strings with no nested brackets, so a non-greedy scan to + // the first `]` is safe even across multi-line lists or lists split across + // shared lines. // // Only double-quoted strings are supported. Other valid TOML (single-quoted // literals, inline tables) would be dropped by the regexes without error, // silently shrinking the cascade — so any entry the regexes can't represent // throws instead, which loadTachModuleGraph turns into "test all products". // A false trip over-tests; a silent drop under-tests, so err on throwing. - const blocks = tomlText.split('[[modules]]').slice(1) + const blocks = stripTomlComments(tomlText).split('[[modules]]').slice(1) for (const block of blocks) { const pathMatch = block.match(/path\s*=\s*"([^"]+)"/) if (!pathMatch) { @@ -273,7 +300,9 @@ function parseTachModules(tomlText) { } continue } - const leftover = dependsMatch[1].replace(/"[^"]*"/g, '').replace(/#[^\n]*/g, '') + // Comments are already gone, so anything left beside the quoted entries + // is an entry shape these regexes cannot represent. + const leftover = dependsMatch[1].replace(/"[^"]*"/g, '') if (/[^\s,]/.test(leftover)) { throw new Error( `unsupported \`depends_on\` entry for ${pathMatch[1]} in tach.toml (expected double-quoted strings): ${leftover.trim().slice(0, 80)}`