fix: eliminate false positives in v1.51.0 codemods

.changeset/fix-form-decorators-member-access.md

@@ -0,0 +1,5 @@
+---
+'@backstage/experimental-form-decorators-to-stable': patch
+---
+
+Fixed false positive where property accesses like `template.spec.EXPERIMENTAL_formDecorators` inside a nullish coalescing fallback were incorrectly renamed, turning `formDecorators ?? EXPERIMENTAL_formDecorators` into the redundant `formDecorators ?? formDecorators`.

.changeset/fix-policy-query-user-scoping.md

@@ -0,0 +1,7 @@
+---
+'@backstage/migrate-policy-query-user': patch
+---
+
+Fixed false positives where unrelated `token` fields were incorrectly removed from destructuring patterns. The codemod now only processes object patterns that are directly typed as `PolicyQueryUser` or destructure a tracked `PolicyQueryUser` binding, instead of modifying every `{ token }` destructuring in files that import from `@backstage/plugin-permission-node`.
+
+Also fixed empty destructuring cleanup (`const {} = await expr` now simplifies to `await expr`), and variable declarations that define bindings used elsewhere are no longer deleted (a TODO comment is added above instead).

codemods/v1.51.0/experimental-form-decorators-to-stable/scripts/codemod.ts

@@ -28,6 +28,12 @@ const transform: Codemod<TSX> = async (root) => {
   })
 
   for (const key of propertyKeys) {
+    // Skip property accesses (e.g. template.spec.EXPERIMENTAL_formDecorators)
+    // — only rename actual object literal keys, not member-expression properties
+    const parent = key.parent()
+    if (parent?.kind() === 'member_expression') {
+      continue
+    }
     renames.increment()
     edits.push(key.replace(NEW_KEY))
   }

codemods/v1.51.0/experimental-form-decorators-to-stable/tests/nullish-coalescing-fallback/expected.ts

@@ -0,0 +1,10 @@
+// The fallback `?? EXPERIMENTAL_formDecorators` is a property access,
+// not an object key — the codemod must leave it alone so backward-compat
+// fallback patterns are preserved.
+export function getDecorators(template: any) {
+  return {
+    formDecorators:
+      template.spec.formDecorators ??
+      template.spec.EXPERIMENTAL_formDecorators,
+  };
+}

codemods/v1.51.0/experimental-form-decorators-to-stable/tests/nullish-coalescing-fallback/input.ts

@@ -0,0 +1,10 @@
+// The fallback `?? EXPERIMENTAL_formDecorators` is a property access,
+// not an object key — the codemod must leave it alone so backward-compat
+// fallback patterns are preserved.
+export function getDecorators(template: any) {
+  return {
+    formDecorators:
+      template.spec.formDecorators ??
+      template.spec.EXPERIMENTAL_formDecorators,
+  };
+}

codemods/v1.51.0/migrate-policy-query-user/scripts/codemod.ts

@@ -181,12 +181,75 @@ function rebuildObjectPattern(objectPattern: SgNode<TSX>): { text: string; renam
   return { text: `{ ${kept.join(', ')} }`, renamedLocals }
 }
 
-function processObjectPattern(objectPattern: SgNode<TSX>, edits: Edit[]): Map<string, string> {
+function isPolicyQueryUserObjectPattern(objectPattern: SgNode<TSX>, userBindings: Set<string>): boolean {
+  const parent = objectPattern.parent()
+  if (!parent) {
+    return false
+  }
+
+  // Direct parameter annotation: ({ token, credentials }: PolicyQueryUser)
+  if (parent.is('required_parameter') || parent.is('optional_parameter')) {
+    const typeNode = parent.field('type')
+    return isPolicyQueryUserTypeAnnotation(typeNode)
+  }
+
+  // Variable declaration: const { token } = user (where user is a tracked PolicyQueryUser binding)
+  if (parent.is('variable_declarator')) {
+    const initNode = parent.field('value')
+    if (initNode?.is('identifier') && userBindings.has(initNode.text())) {
+      return true
+    }
+    // Explicit type annotation: const { token }: PolicyQueryUser = ...
+    const typeNode = parent.field('type')
+    if (isPolicyQueryUserTypeAnnotation(typeNode)) {
+      return true
+    }
+  }
+
+  return false
+}
+
+function processObjectPattern(objectPattern: SgNode<TSX>, edits: Edit[], source: string): Map<string, string> {
   const rebuilt = rebuildObjectPattern(objectPattern)
   if (!rebuilt) {
     return new Map()
   }
 
+  // When all fields have been removed, the destructuring becomes `const {} = expr`
+  // which is useless.  Clean up the enclosing statement instead.
+  if (rebuilt.text === '{}') {
+    const statement = getEnclosingStatement(objectPattern)
+    if (statement) {
+      const kind = statement.kind()
+      if (kind === 'lexical_declaration' || kind === 'variable_declarator') {
+        const decl = kind === 'variable_declarator' ? statement.parent() : statement
+        if (decl) {
+          // If the RHS contains an await, keep the await for side-effects
+          const awaitExpr = decl.find({ rule: { kind: 'await_expression' } })
+          if (awaitExpr) {
+            const range = decl.range()
+            const indent = getLineIndent(source, range.start.index)
+            const endPos = consumeTrailingNewline(source, range.end.index)
+            const lineStart = source.lastIndexOf('\n', range.start.index - 1) + 1
+            edits.push({
+              startPos: lineStart,
+              endPos,
+              insertedText: `${indent}${awaitExpr.text()};\n`,
+            })
+          } else {
+            // No side-effects — remove the entire statement
+            const range = decl.range()
+            const lineStart = source.lastIndexOf('\n', range.start.index - 1) + 1
+            const endPos = consumeTrailingNewline(source, range.end.index)
+            edits.push({ startPos: lineStart, endPos, insertedText: '' })
+          }
+          migrationMetric.increment({ action: 'empty-destructuring-cleaned' })
+          return rebuilt.renamedLocals
+        }
+      }
+    }
+  }
+
   edits.push(objectPattern.replace(rebuilt.text))
   migrationMetric.increment({ action: 'object-pattern-updated' })
   return rebuilt.renamedLocals
@@ -328,10 +391,60 @@ function getEnclosingStatement(node: SgNode<TSX>): SgNode<TSX> | null {
   return null
 }
 
-function replaceStatementWithTodo(statement: SgNode<TSX>, source: string, edits: Edit[]): void {
+function declaresUsedBinding(statement: SgNode<TSX>, rootNode: SgNode<TSX>): boolean {
+  const kind = statement.kind()
+  const decl = kind === 'lexical_declaration' ? statement : kind === 'variable_declarator' ? statement.parent() : null
+  if (!decl) {
+    return false
+  }
+
+  const declarators = decl.findAll({ rule: { kind: 'variable_declarator' } })
+  for (const d of declarators) {
+    const nameNode = d.field('name')
+    if (!nameNode?.is('identifier')) {
+      continue
+    }
+    const name = nameNode.text()
+    // Check if the declared name is used outside this statement.
+    // Search both regular identifiers and shorthand object properties
+    // (e.g. `{ secrets }` parses as shorthand_property_identifier).
+    const identifierKinds = ['identifier', 'shorthand_property_identifier'] as const
+    for (const identKind of identifierKinds) {
+      const usages = rootNode.findAll({
+        rule: { kind: identKind, regex: exactMatchRegex(name) },
+      })
+      for (const usage of usages) {
+        if (isBindingIdentifier(usage)) {
+          continue
+        }
+        // Usage is outside the declaration statement
+        const usageIdx = usage.range().start.index
+        if (usageIdx < decl.range().start.index || usageIdx >= decl.range().end.index) {
+          return true
+        }
+      }
+    }
+  }
+  return false
+}
+
+function replaceStatementWithTodo(statement: SgNode<TSX>, source: string, edits: Edit[], rootNode: SgNode<TSX>): void {
   const range = statement.range()
   const lineStart = source.lastIndexOf('\n', range.start.index - 1) + 1
   const indent = getLineIndent(source, range.start.index)
+
+  // If the statement defines a variable used elsewhere, add a TODO comment
+  // above instead of deleting it — otherwise downstream references break.
+  if (declaresUsedBinding(statement, rootNode)) {
+    edits.push({
+      startPos: lineStart,
+      endPos: lineStart,
+      insertedText: `${indent}${TODO_TOKEN}\n`,
+    })
+    migrationMetric.increment({ action: 'token-usage-todo-comment' })
+    return
+  }
+
   const endPos = consumeTrailingNewline(source, range.end.index)
 
   edits.push({
@@ -356,13 +469,16 @@ const transform: Codemod<TSX> = async (root) => {
   const statementsWithTodo = new Set<number>()
 
   for (const objectPattern of rootNode.findAll({ rule: { kind: 'object_pattern' } })) {
+    if (!isPolicyQueryUserObjectPattern(objectPattern, userBindings)) {
+      continue
+    }
     const bindings = collectObjectPatternBindings(objectPattern)
     for (const binding of bindings) {
       if (REMOVED_FIELDS.has(binding.fieldName)) {
         tokenLocals.add(binding.localName)
       }
     }
-    const renamed = processObjectPattern(objectPattern, edits)
+    const renamed = processObjectPattern(objectPattern, edits, source)
     for (const [from, to] of renamed) {
       renamedLocals.set(from, to)
     }
@@ -399,15 +515,15 @@ const transform: Codemod<TSX> = async (root) => {
       const statement = getEnclosingStatement(member)
       if (statement && !statementsWithTodo.has(statement.range().start.index)) {
         statementsWithTodo.add(statement.range().start.index)
-        replaceStatementWithTodo(statement, source, edits)
+        replaceStatementWithTodo(statement, source, edits, rootNode)
       }
     }
 
     for (const member of findMemberAccessOnBinding(rootNode, binding, 'expiresInSeconds')) {
       const statement = getEnclosingStatement(member)
       if (statement && !statementsWithTodo.has(statement.range().start.index)) {
         statementsWithTodo.add(statement.range().start.index)
-        replaceStatementWithTodo(statement, source, edits)
+        replaceStatementWithTodo(statement, source, edits, rootNode)
       }
     }
   }
@@ -420,7 +536,7 @@ const transform: Codemod<TSX> = async (root) => {
       const statement = getEnclosingStatement(node)
       if (statement && !statementsWithTodo.has(statement.range().start.index)) {
         statementsWithTodo.add(statement.range().start.index)
-        replaceStatementWithTodo(statement, source, edits)
+        replaceStatementWithTodo(statement, source, edits, rootNode)
       }
     }
   }

codemods/v1.51.0/migrate-policy-query-user/tests/empty-destructuring-await/expected.ts

@@ -0,0 +1,12 @@
+import { PolicyQueryUser } from '@backstage/plugin-permission-node';
+
+async function example() {
+  await fetchUser();
+  // TODO(backstage-codemod): migrate to credentials via coreServices.auth
+}
+
+async function fetchUser(): Promise<PolicyQueryUser> {
+  return {} as PolicyQueryUser;
+}
+
+async function doSomething(_token: string) {}

codemods/v1.51.0/migrate-policy-query-user/tests/empty-destructuring-await/input.ts

@@ -0,0 +1,12 @@
+import { PolicyQueryUser } from '@backstage/plugin-permission-node';
+
+async function example() {
+  const { token }: PolicyQueryUser = await fetchUser();
+  await doSomething(token);
+}
+
+async function fetchUser(): Promise<PolicyQueryUser> {
+  return {} as PolicyQueryUser;
+}
+
+async function doSomething(_token: string) {}

codemods/v1.51.0/migrate-policy-query-user/tests/empty-destructuring-await/metrics.json

@@ -0,0 +1,16 @@
+{
+  "migrate-policy-query-user": [
+    {
+      "cardinality": {
+        "action": "empty-destructuring-cleaned"
+      },
+      "count": 1
+    },
+    {
+      "cardinality": {
+        "action": "token-usage-todo"
+      },
+      "count": 1
+    }
+  ]
+}

codemods/v1.51.0/migrate-policy-query-user/tests/no-op-unrelated-token/expected.ts

@@ -0,0 +1,17 @@
+import { PermissionPolicy, PolicyQuery, PolicyQueryUser } from '@backstage/plugin-permission-node';
+
+// The codemod must NOT touch `token` fields in non-PolicyQueryUser patterns,
+// even when the file imports from @backstage/plugin-permission-node.
+async function handleAutocomplete(req: any) {
+  const { token, context } = req.body;
+  if (!token) throw new Error('Missing token');
+  return { token, context };
+}
+
+type AutocompleteHandler = ({ resource, token, context }: {
+  resource: string;
+  token: string;
+  context: Record<string, string>;
+}) => Promise<{ results: { id: string }[] }>;
+
+export { handleAutocomplete, AutocompleteHandler };

codemods/v1.51.0/migrate-policy-query-user/tests/no-op-unrelated-token/input.ts

@@ -0,0 +1,17 @@
+import { PermissionPolicy, PolicyQuery, PolicyQueryUser } from '@backstage/plugin-permission-node';
+
+// The codemod must NOT touch `token` fields in non-PolicyQueryUser patterns,
+// even when the file imports from @backstage/plugin-permission-node.
+async function handleAutocomplete(req: any) {
+  const { token, context } = req.body;
+  if (!token) throw new Error('Missing token');
+  return { token, context };
+}
+
+type AutocompleteHandler = ({ resource, token, context }: {
+  resource: string;
+  token: string;
+  context: Record<string, string>;
+}) => Promise<{ results: { id: string }[] }>;
+
+export { handleAutocomplete, AutocompleteHandler };

codemods/v1.51.0/migrate-policy-query-user/tests/token-in-used-declaration/expected.ts

@@ -0,0 +1,28 @@
+import {
+  PermissionPolicy,
+  PolicyQuery,
+  PolicyQueryUser,
+} from '@backstage/plugin-permission-node';
+
+interface TaskSecrets {
+  backstageToken: string;
+  otherSecret: string;
+}
+
+class CustomPermissionPolicy implements PermissionPolicy {
+  async handle(
+    request: PolicyQuery,
+    { credentials }: PolicyQueryUser,
+  ) {
+    // TODO(backstage-codemod): migrate to credentials via coreServices.auth
+    const secrets: TaskSecrets = {
+      backstageToken: token,
+      otherSecret: 'foo',
+    };
+
+    await this.retryTask({ secrets, taskId: '123' });
+    return { result: 'ALLOW' };
+  }
+
+  async retryTask(_opts: { secrets: TaskSecrets; taskId: string }) {}
+}

codemods/v1.51.0/migrate-policy-query-user/tests/token-in-used-declaration/input.ts

@@ -0,0 +1,27 @@
+import {
+  PermissionPolicy,
+  PolicyQuery,
+  PolicyQueryUser,
+} from '@backstage/plugin-permission-node';
+
+interface TaskSecrets {
+  backstageToken: string;
+  otherSecret: string;
+}
+
+class CustomPermissionPolicy implements PermissionPolicy {
+  async handle(
+    request: PolicyQuery,
+    { token, credentials }: PolicyQueryUser,
+  ) {
+    const secrets: TaskSecrets = {
+      backstageToken: token,
+      otherSecret: 'foo',
+    };
+
+    await this.retryTask({ secrets, taskId: '123' });
+    return { result: 'ALLOW' };
+  }
+
+  async retryTask(_opts: { secrets: TaskSecrets; taskId: string }) {}
+}

codemods/v1.51.0/migrate-policy-query-user/tests/token-in-used-declaration/metrics.json

@@ -0,0 +1,16 @@
+{
+  "migrate-policy-query-user": [
+    {
+      "cardinality": {
+        "action": "object-pattern-updated"
+      },
+      "count": 1
+    },
+    {
+      "cardinality": {
+        "action": "token-usage-todo-comment"
+      },
+      "count": 1
+    }
+  ]
+}

scripts/generate-readme-codemods.sh

@@ -82,4 +82,8 @@ awk -v start="$START_MARKER" -v end="$END_MARKER" -v content="$section" '
 ' "$README" > "$README.tmp"
 
 mv "$README.tmp" "$README"
+
+# Format so the committed output matches what prettier expects
+yarn format "$README" >/dev/null 2>&1 || true
+
 echo "✅ README.md updated with ${all_versions[0]} and ${all_versions[1]} ($total total on disk)"