From b4276f4c99a9454c0c7712154f06d22ecc50f4a4 Mon Sep 17 00:00:00 2001 From: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Date: Mon, 28 Sep 2026 15:56:19 +0000 Subject: [PATCH] fix: consume multiline strings and YAML comments during directive and flow-map scans - SCAN_PATTERN and stripComments now consume '''...''' and """..." literals whole; a triple-quoted string quoting import text no longer invents a dependency (Devin Review on #72). - findFlowMappingEnd and parseFlowSourceDirective skip YAML comments: a '}' inside a comment between flow-map members no longer closes the map early and the comment text no longer reads as an unreadable member. - DEPLOYMENT.md now describes the merged scoped-skip + approximation policy instead of the dropped graph-level skip. - server.dart: drop the duplicated maxDependenciesPerRequest check whose second branch was unreachable and hardcoded the wrong field name. Co-Authored-By: Stuart Gardoll --- DEPLOYMENT.md | 21 ++++++++------- cloud-run/ffai-runner/bin/server.dart | 3 --- src/dartPackageImports.js | 23 +++++++++++----- src/dartPackageImports.test.js | 26 ++++++++++++++++++ src/pubspecSync.js | 38 +++++++++++++++++++++++---- src/pubspecSync.test.js | 17 ++++++++++++ 6 files changed, 104 insertions(+), 24 deletions(-) diff --git a/DEPLOYMENT.md b/DEPLOYMENT.md index c659a8d..21bc98e 100644 --- a/DEPLOYMENT.md +++ b/DEPLOYMENT.md @@ -97,15 +97,18 @@ reported rather than silently dropped: package would resolve different code, and a check against the wrong version reports a result that does not describe what ships. -The second case poisons the whole resolution graph, so it is decided **at the -graph level**. A git or path dependency's own pubspec still constrains every -package it shares with a class's imports - even ones the class never names - -and only the project's `pubspec.lock`, which is never uploaded, records which -versions actually resolved. A check that cannot reproduce the graph cannot -distinguish "clean" from "clean under different code", so any class importing -a `package:` URI is reported unverified. A class using only `dart:` imports -resolves nothing through pub and is still compiled, and a class naming the -unreproducible entry directly gets a reason that says so. +The second case is handled **by scope, not breadth**. A class that imports +the unreproducible entry directly cannot compile without it at all, so it is +reported with a reason that names the import as its own problem. Every other +class is still compiled - but against a scratch graph that omits the +unreproducible package, whose own pubspec may still constrain packages those +classes reach transitively (only the project's `pubspec.lock`, never +uploaded, records which versions actually resolved). Because that drift +cannot be ruled out, the deploy surfaces one **approximation notice** per +unreproducible package saying the check ran against a reduced graph - the +limitation is disclosed rather than silently trusted. A class using only +`dart:` imports resolves nothing through pub, so the approximation does not +reach it and it is compiled normally. **Which packages the Flutter SDK supplies is read from the project's own pubspec** - a dependency written in block form under `sdk:` is an SDK package by diff --git a/cloud-run/ffai-runner/bin/server.dart b/cloud-run/ffai-runner/bin/server.dart index 8eec571..c33a5fa 100644 --- a/cloud-run/ffai-runner/bin/server.dart +++ b/cloud-run/ffai-runner/bin/server.dart @@ -586,9 +586,6 @@ List _normalizeSdkPackages(Object? value, String field) { if (value.length > maxDependenciesPerRequest) { throw FormatException('Too many entries in verification.$field.'); } - if (value.length > maxDependenciesPerRequest) { - throw const FormatException('Too many entries in verification.sdkPackages.'); - } final result = {}; for (final raw in value) { diff --git a/src/dartPackageImports.js b/src/dartPackageImports.js index e157614..7974127 100644 --- a/src/dartPackageImports.js +++ b/src/dartPackageImports.js @@ -38,10 +38,15 @@ function stripComments(code) { i += 2; out += " "; } else if (code[i] === "'" || code[i] === '"') { - const quote = code[i]; - out += code[i]; - i++; - while (i < code.length && code[i] !== quote) { + // A triple quote opens a multiline string, which ends only at the next + // matching triple - a lone quote inside it is content, not a delimiter. + const quote = + code.slice(i, i + 3) === "'''" || code.slice(i, i + 3) === '"""' + ? code.slice(i, i + 3) + : code[i]; + out += quote; + i += quote.length; + while (i < code.length && !code.startsWith(quote, i)) { if (code[i] === "\\") { out += code[i]; i++; @@ -50,8 +55,8 @@ function stripComments(code) { i++; } if (i < code.length) { - out += code[i]; - i++; + out += quote; + i += quote.length; } } else { out += code[i]; @@ -91,7 +96,11 @@ const DIRECTIVE_PATTERN = // a directive. A raw `r"..."` literal needs no alternative of its own - `r` // matches nothing, and the quote after it opens the string alternative, whose // escape-aware body ends at the same quote a raw literal would. -const STRING_PATTERN = /'(?:\\.|[^'\\\n])*'|"(?:\\.|[^"\\\n])*"/.source; +// Triple-quoted alternatives come first: a `'`/`"` literal cannot hold a +// newline, so without them `'''example\nimport 'package:x/x.dart';\n'''` +// scans its second line as a directive and invents a dependency on `x`. +// Their bodies are lazy `'`/`"`-tolerant runs to the matching triple quote. +const STRING_PATTERN = /'''(?:\\.|[^\\])*?'''|"""(?:\\.|[^\\])*?"""|'(?:\\.|[^'\\\n])*'|"(?:\\.|[^"\\\n])*"/.source; const SCAN_PATTERN = new RegExp(`${DIRECTIVE_PATTERN.source}|${STRING_PATTERN}`, "g"); function* directiveMatches(code) { diff --git a/src/dartPackageImports.test.js b/src/dartPackageImports.test.js index d3f86e0..ab6c8e6 100644 --- a/src/dartPackageImports.test.js +++ b/src/dartPackageImports.test.js @@ -222,3 +222,29 @@ test("still detects a real import on the line after a comment mentioning a diffe assert.deepEqual(extractPackageImports(code), ["geolocator"]); }); + +test("a multiline string quoting an import is not a dependency", () => { + const code = [ + "final example = '''", + "import 'package:nonexistent_demo/demo.dart';", + "''';", + "import 'package:geolocator/geolocator.dart';", + "class Plain {}", + ].join("\n"); + + // The triple-quoted string holds display text: without consuming it whole + // the scanner would invent a `nonexistent_demo` dependency and add it to + // the project's pubspec, breaking `pub get` after deploy. + assert.deepEqual(extractPackageImports(code), ["geolocator"]); +}); + +test("a raw multiline double-quoted string is consumed whole too", () => { + const code = [ + 'final example = r"""', + "export 'package:ghost_pkg/ghost.dart';", + '""";', + "class Plain {}", + ].join("\n"); + + assert.deepEqual(extractPackageImports(code), []); +}); diff --git a/src/pubspecSync.js b/src/pubspecSync.js index 15e5e51..9b894a2 100644 --- a/src/pubspecSync.js +++ b/src/pubspecSync.js @@ -199,14 +199,35 @@ function parseFlowSourceDirective(value) { if (end === -1) return { key: null, value: null, version: null }; // Split the members on top-level commas only - a comma inside a nested map - // or a quote belongs to the member, not the map. + // or a quote belongs to the member, not the map. Comments are excised + // first: a `#` preceded by whitespace runs to end of line (a wrapped map + // can carry comments between members), and its text is neither a member + // nor structure - left in place it would read as an unreadable member. const inner = text.slice(1, end); + let cleaned = ""; + { + let quote = null; + for (let i = 0; i < inner.length; i += 1) { + const char = inner[i]; + if (quote) { + cleaned += char; + if (char === quote) quote = null; + continue; + } + if (char === "#" && (i === 0 || /\s/.test(inner[i - 1]))) { + while (i < inner.length && inner[i] !== "\n") i += 1; + continue; + } + if (char === "'" || char === '"') quote = char; + cleaned += char; + } + } const segments = []; let quote = null; let depth = 0; let start = 0; - for (let i = 0; i < inner.length; i += 1) { - const char = inner[i]; + for (let i = 0; i < cleaned.length; i += 1) { + const char = cleaned[i]; if (quote) { if (char === quote) quote = null; continue; @@ -218,11 +239,11 @@ function parseFlowSourceDirective(value) { if (char === "{" || char === "[") depth += 1; else if (char === "}" || char === "]") depth -= 1; else if (char === "," && depth === 0) { - segments.push(inner.slice(start, i)); + segments.push(cleaned.slice(start, i)); start = i + 1; } } - segments.push(inner.slice(start)); + segments.push(cleaned.slice(start)); let version = null; for (const segment of segments) { @@ -255,6 +276,13 @@ function findFlowMappingEnd(text) { if (char === quote) quote = null; continue; } + // A `#` preceded by whitespace starts a YAML comment that runs to end of + // line - a `}` inside it is text, not structure, so a comment like + // `# close } later` must not end the map before its members are read. + if (char === "#" && (i === 0 || /\s/.test(text[i - 1]))) { + while (i < text.length && text[i] !== "\n") i += 1; + continue; + } if (char === "'" || char === '"') { quote = char; continue; diff --git a/src/pubspecSync.test.js b/src/pubspecSync.test.js index f469930..9c1708c 100644 --- a/src/pubspecSync.test.js +++ b/src/pubspecSync.test.js @@ -374,3 +374,20 @@ test("reads a source from dependency_overrides too", () => { assert.equal(overrides.get("flutter_web_plugins").sourceKey, "sdk"); }); + +test("a brace inside a flow-map comment does not close the mapping", () => { + const pubspec = `dependencies: + flutter_web_plugins: { + # close } later + sdk: flutter + } + intl: ^0.20.3 +`; + const declared = parseExistingDependencies(pubspec); + + // YAML flow mappings permit comments between members; counting the `}` in + // the comment would end the map before `sdk:` and classify the package + // unrepresentable instead of SDK-sourced. + assert.equal(declared.get("flutter_web_plugins").sourceKey, "sdk"); + assert.equal(declared.get("flutter_web_plugins").isScalar, false); +});