Fix post-merge Devin Review findings on #72

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

cloud-run/ffai-runner/bin/server.dart

@@ -586,9 +586,6 @@ List<String> _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 = <String>{};
   for (final raw in value) {

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) {

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), []);
+});

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;

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);
+});