test(integ): harden the aws-command extractor against review findings

Review of the first pass found three real defects, all verified against the
current tree.

FALSE POSITIVES (the worst of the three). Only `#` comments were stripped, so
prose and data parsed as invocations:
  echo "==> Verifying deploy via aws cli (sanity)"   -> aws cli sanity
  arn:aws:s3:::my-bucket                              -> aws s3 my-bucket
  Tags[?Key==`aws:cdk:path`]                          -> aws cdk path
Harmless only because those verbs are not in the removal table — a fixture's
own `echo "... aws emr list-instance-groups ..."` explanation would have become
a CI-blocking violation curable only by DELETING the explanation, the exact
outcome comment-stripping exists to prevent. They also inflated the coverage
floors with non-invocations (2687 -> 2640 real; services 70 -> 68, verbs
362 -> 355).

Fixed by blanking quoted spans while PRESERVING $( ) command substitutions —
both halves load-bearing, since the capture form lives inside double quotes and
is the largest shape in the tree (1028 invocations, unchanged by the fix) — and
by keeping `:` inside tokens so an ARN or a JMESPath backtick literal stays one
token instead of splitting out a bare `aws`.

FALSE NEGATIVE: `aws --region us-east-1 emr list-instance-groups` vanished
SILENTLY — the service slot held `--region`, NAME rejected it, and the call went
unchecked with no diagnostic. Now steps past leading global options, consuming
the value of the value-taking ones.

ESCAPE-HATCH LEAKS: the marker was matched on raw line text, so it counted
inside a string literal, and "the line above" accepted a marker trailing the
PREVIOUS command. Now read only from the stripped comment, and the line-above
form requires a whole-line comment.

Also in refresh-aws-cli-removals.ts: `command -v aws` THROWS when absent so the
actionable not-found message was unreachable; `--check` threw on a corrupt
fixture instead of reporting stale; `--aws-root` with no value silently fell
back to PATH (or took `--check` as the path); and the parser now reconciles
against an independent count of building-command-table events, so a reordered
remove() call fails loudly instead of dropping a service silently.

Real-code fail probe re-run through the hardened parser: injecting BOTH the
capture form and the leading-global-option form into the real
emr-cluster/verify.sh flagged both, each named by file:line; restored after.

Author: go-to-k

scripts/check-integ-aws-commands.ts

@@ -86,7 +86,96 @@ const NAME = /^[a-z][a-z0-9-]*$/;
  * being read as a verb; keeping `/` is what stops `s3://bucket` from splitting
  * into a service-looking `s3`.
  */
-const TOKEN_SEPARATOR = /[^A-Za-z0-9._/-]+/;
+const TOKEN_SEPARATOR = /[^A-Za-z0-9._/:-]+/;
+
+/**
+ * AWS CLI global options that CONSUME the following token, so a leading
+ * `aws --region us-east-1 emr <verb>` is not read as service `us-east-1`.
+ * Boolean globals (`--debug`, `--no-cli-pager`, ...) need no entry — they are
+ * skipped by the generic "starts with `-`" rule.
+ */
+const VALUE_TAKING_GLOBAL_OPTIONS: ReadonlySet<string> = new Set([
+  '--region',
+  '--profile',
+  '--endpoint-url',
+  '--output',
+  '--query',
+  '--color',
+  '--ca-bundle',
+  '--cli-read-timeout',
+  '--cli-connect-timeout',
+  '--cli-binary-format',
+]);
+
+/**
+ * Blanks out quoted spans so their contents are not mistaken for commands,
+ * while PRESERVING `$( ... )` command substitutions inside them.
+ *
+ * Both halves are load-bearing. Without blanking, prose and data parse as
+ * invocations — confirmed live in this tree: `echo "==> Verifying deploy via
+ * aws cli (sanity)"` read as `aws cli sanity`, and `arn:aws:s3:::bucket` read
+ * as `aws s3 bucket`. That inflates the coverage floors with non-invocations
+ * and, worse, would make a fixture's own `echo "... aws emr
+ * list-instance-groups ..."` explanation a CI-blocking violation whose only
+ * cure is deleting the explanation — the outcome comment-stripping exists to
+ * prevent. And without preserving `$( )`, the capture form
+ * `IDS="$(aws emr list-clusters)"` — over a thousand invocations, the single
+ * largest shape in the tree — would vanish wholesale.
+ *
+ * Note `:` is INSIDE {@link TOKEN_SEPARATOR}'s allowed set for the same
+ * family of reasons: it keeps `arn:aws:...` and a JMESPath `` `aws:cdk:path` ``
+ * as ONE token rather than splitting out a bare `aws`.
+ */
+export function blankQuotedSpans(line: string): string {
+  let out = '';
+  let i = 0;
+  while (i < line.length) {
+    const ch = line[i]!;
+    if (ch !== '"' && ch !== "'") {
+      out += ch;
+      i++;
+      continue;
+    }
+    const quote = ch;
+    out += ch;
+    i++;
+    while (i < line.length && line[i] !== quote) {
+      // `\"` inside a double-quoted span is an escaped quote, not the closer.
+      if (quote === '"' && line[i] === '\\' && i + 1 < line.length) {
+        out += '  ';
+        i += 2;
+        continue;
+      }
+      // A command substitution runs a real command — keep it verbatim and let
+      // the tokenizer see it. Single quotes suppress expansion, so only the
+      // double-quoted case substitutes.
+      if (quote === '"' && line[i] === '$' && line[i + 1] === '(') {
+        let depth = 0;
+        while (i < line.length) {
+          if (line[i] === '(') depth++;
+          else if (line[i] === ')') {
+            depth--;
+            if (depth === 0) {
+              out += line[i]!;
+              i++;
+              break;
+            }
+          }
+          out += line[i]!;
+          i++;
+        }
+        continue;
+      }
+      out += ' ';
+      i++;
+    }
+    if (i < line.length) {
+      out += line[i]!;
+      i++;
+    }
+  }
+  return out;
+}
 
 /**
  * Pulls each `aws <service> <verb>` out of a verify.sh.
@@ -109,23 +198,50 @@ export function extractAwsInvocations(content: string): AwsInvocation[] {
 
   for (let i = 0; i < joined.length; i++) {
     const { text, line } = joined[i]!;
-    // The hatch lives in a COMMENT, so it has to be read off the raw text
-    // before the comment is stripped. Accept it on the invocation's own line or
-    // the line immediately above (the idiomatic placement for a long rationale).
-    const allowed = text.includes(ALLOW_MARKER) || (joined[i - 1]?.text.includes(ALLOW_MARKER) ?? false);
-
+    // The hatch lives in a COMMENT, so it is read from the part
+    // `stripTrailingComment` removes — NOT from the raw line, which would also
+    // honor the marker when it merely appears inside a string literal.
     const stripped = stripTrailingComment(text);
+    const ownComment = text.slice(stripped.length);
+    const prevText = joined[i - 1]?.text ?? '';
+    // On the line above, require a WHOLE-line comment: a marker trailing a
+    // previous command is about that command, not this one.
+    const prevIsCommentLine = prevText.trimStart().startsWith('#');
+    const allowed =
+      ownComment.includes(ALLOW_MARKER) ||
+      (prevIsCommentLine && prevText.includes(ALLOW_MARKER));
 
-    for (const segment of splitShellCommands(stripped)) {
+    for (const segment of splitShellCommands(blankQuotedSpans(stripped))) {
       const tokens = segment.split(TOKEN_SEPARATOR).filter(Boolean);
       for (let t = 0; t < tokens.length - 2; t++) {
         if (tokens[t] !== 'aws') continue;
-        const service = tokens[t + 1]!;
-        const verb = tokens[t + 2]!;
-        // A flag or a variable right after `aws` means this is not a plain
-        // `aws <service> <verb>` we can classify statically — skip rather than
-        // guess (an unclassifiable invocation is reported by the coverage
-        // floors, not by a wrong verdict).
+
+        // Step past any leading GLOBAL options — `aws --region us-east-1 emr
+        // list-clusters` must resolve to (emr, list-clusters), not
+        // (us-east-1, emr). Without this the invocation silently vanishes:
+        // `NAME` rejects `--region` and the whole call goes unchecked.
+        let k = t + 1;
+        while (k < tokens.length && tokens[k]!.startsWith('-')) {
+          const flag = tokens[k]!.split('=')[0]!;
+          k++;
+          // `--flag=value` already carries its value; a bare value-taking flag
+          // consumes the next token.
+          if (
+            VALUE_TAKING_GLOBAL_OPTIONS.has(flag) &&
+            !tokens[k - 1]!.includes('=') &&
+            k < tokens.length &&
+            !tokens[k]!.startsWith('-')
+          ) {
+            k++;
+          }
+        }
+
+        const service = tokens[k];
+        const verb = tokens[k + 1];
+        // A variable or otherwise unclassifiable token means this is not a
+        // plain `aws <service> <verb>` we can judge statically — skip rather
+        // than guess.
+        if (service === undefined || verb === undefined) continue;
         if (!NAME.test(service) || !NAME.test(verb)) continue;
         invocations.push({ line, service, verb, raw: segment.trim(), allowed });
       }

scripts/refresh-aws-cli-removals.ts

@@ -78,7 +78,15 @@ export function findAwsCliPackageRoot(explicitRoot?: string): string {
     return explicitRoot;
   }
 
-  const awsBin = execFileSync('sh', ['-c', 'command -v aws'], { encoding: 'utf8' }).trim();
+  // `command -v` EXITS 1 when the binary is absent, so execFileSync throws
+  // before any empty-string check could run — catch it and surface the
+  // actionable message instead of a raw `Command failed: sh -c command -v aws`.
+  let awsBin = '';
+  try {
+    awsBin = execFileSync('sh', ['-c', 'command -v aws'], { encoding: 'utf8' }).trim();
+  } catch {
+    awsBin = '';
+  }
   if (!awsBin) throw new Error('aws CLI not found on PATH — pass --aws-root <awscli package dir>');
   // `aws` is usually a symlink into the versioned install; resolve it, then walk
   // up looking for the packaged `awscli` directory.
@@ -116,9 +124,24 @@ function pythonSitePackages(dir: string): string[] {
 export function buildFixture(pkgRoot: string): RemovedCommandsFixture {
   const source = readFileSync(join(pkgRoot, 'customizations', 'removals.py'), 'utf8');
   const parsed = parseRemovalsSource(source);
+  // Reconcile against an INDEPENDENT count of the events in the file. The
+  // regex requires `on_event` to be immediately followed by `remove_commands`,
+  // so a reordered or interleaved `remove()` call would drop that service
+  // SILENTLY; a bare `size === 0` guard only catches total failure, which is
+  // the vacuous-pass shape `.claude/rules/testing.md` warns about.
+  const declaredEvents = new Set(
+    [...source.matchAll(/building-command-table\.([a-z0-9-]+)/g)].map((m) => m[1]!)
+  );
   if (parsed.size === 0) {
     throw new Error(`parsed 0 removals from ${pkgRoot}/customizations/removals.py — parser is stale`);
   }
+  const missed = [...declaredEvents].filter((s) => !parsed.has(s));
+  if (missed.length > 0) {
+    throw new Error(
+      `parser missed ${missed.length} command-table removal event(s) (${missed.join(', ')}) in ` +
+        `${pkgRoot}/customizations/removals.py — the AWS CLI changed the call shape, update parseRemovalsSource`
+    );
+  }
   let version = 'unknown';
   try {
     version = execFileSync('aws', ['--version'], { encoding: 'utf8' }).trim();
@@ -143,19 +166,33 @@ function main(): void {
   const argv = process.argv.slice(2);
   const check = argv.includes('--check');
   const rootIdx = argv.indexOf('--aws-root');
-  const explicitRoot = rootIdx >= 0 ? argv[rootIdx + 1] : undefined;
+  const rootValue = rootIdx >= 0 ? argv[rootIdx + 1] : undefined;
+  // `--aws-root` with no value (or followed by another flag) must ERROR, not
+  // silently fall back to PATH — or worse, take `--check` as the root path.
+  if (rootIdx >= 0 && (rootValue === undefined || rootValue.startsWith('-'))) {
+    console.error('--aws-root requires a path argument (the awscli package directory)');
+    process.exit(1);
+  }
+  const explicitRoot = rootValue;
 
   const fixture = buildFixture(findAwsCliPackageRoot(explicitRoot));
   const serialized = `${JSON.stringify(fixture, null, 2)}\n`;
 
   if (check) {
     const current = existsSync(FIXTURE_PATH) ? readFileSync(FIXTURE_PATH, 'utf8') : '';
     // Compare the removal DATA only: the captured `aws --version` differs per
-    // machine and must not make a same-data capture read as drift.
+    // machine and must not make a same-data capture read as drift. A corrupt
+    // or missing fixture reports STALE (the actionable verdict) rather than
+    // throwing a parse error at the user.
+    let currentRemoved: unknown;
+    try {
+      currentRemoved = current === '' ? undefined : (JSON.parse(current) as RemovedCommandsFixture).removed;
+    } catch {
+      currentRemoved = undefined;
+    }
     const sameData =
-      current !== '' &&
-      JSON.stringify((JSON.parse(current) as RemovedCommandsFixture).removed) ===
-        JSON.stringify(fixture.removed);
+      currentRemoved !== undefined &&
+      JSON.stringify(currentRemoved) === JSON.stringify(fixture.removed);
     if (!sameData) {
       console.error(
         `aws-cli-removed-commands.json is stale vs ${fixture.$awsCliVersion} — re-run: node scripts/refresh-aws-cli-removals.ts`

tests/unit/scripts/integ-aws-commands.test.ts

@@ -167,6 +167,46 @@ describe('extractAwsInvocations', () => {
     expect(extractAwsInvocations('aws --version\n')).toEqual([]);
   });
 
+  // False positives found by review against the REAL tree. None of these is an
+  // invocation, and before the quoted-span blanking + `:`-in-token change all
+  // three parsed as one — inflating the coverage floors with non-invocations
+  // and, worse, making a fixture's own `echo "... aws emr list-instance-groups
+  // ..."` explanation a CI-blocking violation curable only by deleting the
+  // explanation.
+  it.each([
+    ['prose in a double-quoted echo', 'echo "==> Verifying deploy via aws cli (sanity)"'],
+    ['an ARN literal', 'ARN="arn:aws:s3:::my-bucket"'],
+    ['an unquoted ARN literal', 'aws s3 rm arn:aws:s3:::my-bucket'],
+    ['a JMESPath backtick literal', "QUERY=\"Tags[?Key==\\`aws:cdk:path\\`]\""],
+    ['a single-quoted explanation', "echo 'do not call aws emr list-instance-groups'"],
+  ])('does not parse %s as an invocation', (_label, body) => {
+    const found = extractAwsInvocations(`${body}\n`).map((i) => `${i.service} ${i.verb}`);
+    // `aws s3 rm ...` IS a real invocation in one case — assert only that the
+    // ARN did not additionally yield a bogus `s3 my-bucket`.
+    expect(found.filter((f) => f !== 's3 rm')).toEqual([]);
+  });
+
+  // The capture form must SURVIVE quoted-span blanking — it is the single
+  // largest shape in the tree (~1000 invocations) and lives inside double
+  // quotes, so blanking without preserving `$( )` would delete it wholesale.
+  it('still sees an invocation inside a double-quoted command substitution', () => {
+    const inv = extractAwsInvocations('IDS="$(aws emr list-clusters --active)"\n');
+    expect(inv.map((i) => `${i.service} ${i.verb}`)).toEqual(['emr list-clusters']);
+  });
+
+  // Without the global-option skip this invocation VANISHED silently — the
+  // service slot held `--region`, `NAME` rejected it, and the call went
+  // unchecked with no diagnostic.
+  it.each([
+    ['--region', 'aws --region us-east-1 emr list-instance-groups --cluster-id j-A'],
+    ['--region=value', 'aws --region=us-east-1 emr list-instance-groups --cluster-id j-A'],
+    ['a boolean global', 'aws --no-cli-pager emr list-instance-groups --cluster-id j-A'],
+    ['two globals', 'aws --profile p --debug emr list-instance-groups --cluster-id j-A'],
+  ])('sees the invocation behind a leading global option: %s', (_label, body) => {
+    const inv = extractAwsInvocations(`${body}\n`);
+    expect(inv.map((i) => `${i.service} ${i.verb}`)).toEqual(['emr list-instance-groups']);
+  });
+
   it('marks the escape hatch on the same line and the line above', () => {
     const same = extractAwsInvocations(
       `aws emr list-instance-groups --cluster-id x # ${ALLOW_MARKER} proven to work here\n`
@@ -177,6 +217,22 @@ describe('extractAwsInvocations', () => {
       `# ${ALLOW_MARKER} proven to work here\naws emr list-instance-groups --cluster-id x\n`
     );
     expect(above[0]!.allowed).toBe(true);
+
+    // The marker must be a COMMENT, not any occurrence of the string. A
+    // fixture that merely PRINTS the marker text must not silently disarm the
+    // check for the command on that line.
+    const inString = extractAwsInvocations(
+      `echo "${ALLOW_MARKER} nope"; aws emr list-instance-groups --cluster-id x\n`
+    );
+    expect(inString.some((i) => i.allowed)).toBe(false);
+
+    // "The line above" means a WHOLE-line comment. A marker trailing the
+    // PREVIOUS command is about that command, not the next one.
+    const trailingAbove = extractAwsInvocations(
+      `aws emr list-clusters # ${ALLOW_MARKER} about this line only\naws emr list-instance-groups --cluster-id x\n`
+    );
+    const leaked = trailingAbove.find((i) => i.verb === 'list-instance-groups');
+    expect(leaked?.allowed).toBe(false);
   });
 });
 
@@ -232,7 +288,7 @@ describe('integ fixture aws invocations (#1402)', () => {
   it('parses a substantial share of the fixture tree', () => {
     const stats = collectStats();
     expect(stats.fixtures).toBeGreaterThan(150);
-    // Current: 2687 invocations across 221 fixtures, 70 services, 362 verbs.
+    // Current: 2640 invocations across 221 fixtures, 68 services, 355 verbs.
     expect(stats.total).toBeGreaterThan(2200);
     expect(stats.services.size).toBeGreaterThan(55);
     expect(stats.verbs.size).toBeGreaterThan(290);
@@ -249,7 +305,7 @@ describe('integ fixture aws invocations (#1402)', () => {
   it('parses every invocation shape it claims to support', () => {
     const stats = collectStats();
     const floors: Record<keyof ReturnType<typeof collectStats>['shapes'], number> = {
-      // Current: plain 1012, substitution 1030, helperArgument 414,
+      // Current: plain 1008, substitution 1028, helperArgument 414,
       // condition 211. Floors sit ~20% under, low enough for fixture churn and
       // high enough that losing a shape outright cannot slip through.
       // `aws ...` at the start of a segment — the common form.