fix(secrets): keep rollup metadata with its run, and stop shadowed bindings faking usage

Review round 1.

- record.ts: last_execution_id/last_trigger were assigned unconditionally while
  last_used_at was chosen by greatest(), so two runs completing out of order split
  one row between them — the newer run's timestamp beside the older run's execution
  id, making "View log" open a run the row does not describe. Both are now guarded
  on the timestamp actually advancing, so the row's metadata always belongs to the
  run that owns its timestamp.
- javascript.ts: a local binding named environmentVariables (declaration, parameter,
  destructured binding, or bare reassignment) made reads off the user's own object
  look like mounted-secret reads. Any such binding now disables detection for the
  file; the AST already had parent pointers, so this is a kind check during the
  existing walk.
- python.ts: same class of bug with no parser available, so the rule is an allowlist
  — every mention of the binding must be a literal subscript or .get(), otherwise
  detection is off for the file. This also subsumes the cross-line attribute case
  (other.\n environmentVariables['K']), which the previous space-and-tab look-behind
  missed.

Under-reporting is the safe direction here: a trail that claims a use that never
happened is worse than one that misses a use.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: Vikhyath Mondreti

apps/sim/lib/execution/code-placeholders/compiler.test.ts

@@ -1318,6 +1318,86 @@ describe('direct environment read edge cases', () => {
   })
 })
 
+describe('a shadowed environment binding disables direct-read detection', () => {
+  /**
+   * A local object that merely shares the runtime binding's name is not the mounted
+   * environment, so reading a same-named key off it is not a use of the secret. The trail
+   * must not claim uses that never happened.
+   */
+  it.each([
+    [
+      'const declaration',
+      "const environmentVariables = { API_KEY: 'x' }\nreturn environmentVariables.API_KEY",
+    ],
+    ['let declaration', "let environmentVariables = {}\nreturn environmentVariables['API_KEY']"],
+    [
+      'function parameter',
+      'function read(environmentVariables) { return environmentVariables.API_KEY }\nreturn read({})',
+    ],
+    [
+      'arrow parameter',
+      'const read = (environmentVariables) => environmentVariables.API_KEY\nreturn read({})',
+    ],
+    [
+      'destructured binding',
+      'const { environmentVariables } = payload\nreturn environmentVariables.API_KEY',
+    ],
+    [
+      'bare reassignment',
+      "environmentVariables = { API_KEY: 'x' }\nreturn environmentVariables.API_KEY",
+    ],
+  ])('javascript: %s', async (_label, code) => {
+    expect(await directReadNames(code, CodeLanguage.JavaScript)).toEqual([])
+  })
+
+  it('javascript still reports an unshadowed read', async () => {
+    expect(
+      await directReadNames('return environmentVariables.API_KEY', CodeLanguage.JavaScript)
+    ).toEqual(['API_KEY'])
+  })
+
+  it.each([
+    ['assignment', "environmentVariables = {'API_KEY': 'x'}\nk = environmentVariables['API_KEY']"],
+    [
+      'def parameter',
+      "def read(environmentVariables):\n    return environmentVariables['API_KEY']",
+    ],
+    ['passed to a function', "log(environmentVariables)\nk = environmentVariables['API_KEY']"],
+    ['for target', "for environmentVariables in rows:\n    k = environmentVariables['API_KEY']"],
+    [
+      'with-as target',
+      "with open(p) as environmentVariables:\n    k = environmentVariables['API_KEY']",
+    ],
+  ])('python: %s', async (_label, code) => {
+    expect(await directReadNames(code, CodeLanguage.Python)).toEqual([])
+  })
+
+  /** Cursor's cross-line case: the `.` sits on the previous line inside parentheses. */
+  it('python: attribute access split across a line', async () => {
+    expect(
+      await directReadNames(
+        "k = (other.\n     environmentVariables['API_KEY'])",
+        CodeLanguage.Python
+      )
+    ).toEqual([])
+  })
+
+  it('python: attribute access after a line continuation', async () => {
+    expect(
+      await directReadNames(
+        "k = other.\\\n    environmentVariables['API_KEY']",
+        CodeLanguage.Python
+      )
+    ).toEqual([])
+  })
+
+  it('python still reports an unshadowed read', async () => {
+    expect(
+      await directReadNames("k = environmentVariables['API_KEY']", CodeLanguage.Python)
+    ).toEqual(['API_KEY'])
+  })
+})
+
 describe('shell true positives survive the fail-closed rule', () => {
   it.each([
     ['bare unquoted', 'echo $API_KEY'],

apps/sim/lib/execution/code-placeholders/javascript.ts

@@ -34,6 +34,48 @@ export interface DirectEnvironmentRead {
 /** The runtime identifier the sandbox prologue binds the environment to. */
 const ENVIRONMENT_VARIABLES_IDENTIFIER = 'environmentVariables'
 
+/**
+ * Node kinds whose `name` binds the identifier it holds. A `catch (e)` clause is absent
+ * because its binding is itself a `VariableDeclaration`.
+ */
+const BINDING_NODE_KINDS = new Set<ts.SyntaxKind>([
+  ts.SyntaxKind.VariableDeclaration,
+  ts.SyntaxKind.Parameter,
+  ts.SyntaxKind.BindingElement,
+  ts.SyntaxKind.FunctionDeclaration,
+  ts.SyntaxKind.FunctionExpression,
+  ts.SyntaxKind.ClassDeclaration,
+  ts.SyntaxKind.ClassExpression,
+  ts.SyntaxKind.ImportClause,
+  ts.SyntaxKind.ImportSpecifier,
+  ts.SyntaxKind.NamespaceImport,
+])
+
+/**
+ * Whether this node declares or reassigns the runtime environment identifier, so reads off
+ * it can no longer be attributed to the mounted object.
+ *
+ * `const environmentVariables = { API_KEY: 'x' }` — or a parameter, a destructured binding,
+ * or a bare reassignment — makes `environmentVariables.API_KEY` a read of the user's own
+ * object. Recording that would put a use in the trail that never happened, and a trail that
+ * claims uses is worse than one that misses them, so any such binding disables detection for
+ * the whole file rather than attempting scope resolution.
+ */
+function bindsEnvironmentVariables(node: ts.Node): boolean {
+  if (ts.isBinaryExpression(node)) {
+    return (
+      node.operatorToken.kind === ts.SyntaxKind.EqualsToken &&
+      ts.isIdentifier(node.left) &&
+      node.left.text === ENVIRONMENT_VARIABLES_IDENTIFIER
+    )
+  }
+  if (!BINDING_NODE_KINDS.has(node.kind)) return false
+  const name = Reflect.get(node, 'name') as ts.Node | undefined
+  return (
+    name !== undefined && ts.isIdentifier(name) && name.text === ENVIRONMENT_VARIABLES_IDENTIFIER
+  )
+}
+
 /**
  * Names a statically visible read off the runtime environment object, covering
  * `environmentVariables.NAME`, `environmentVariables['NAME']`, and their optional-chained
@@ -73,6 +115,7 @@ function collectDecodedSyntax(code: string): DecodedJavaScriptSyntax {
   const identifierNames: string[] = []
   const values: string[] = []
   const environmentReads: DirectEnvironmentRead[] = []
+  let environmentVariablesShadowed = false
   const visit = (node: ts.Node): void => {
     const isTemplateToken =
       node.kind === ts.SyntaxKind.TemplateHead ||
@@ -93,11 +136,18 @@ function collectDecodedSyntax(code: string): DecodedJavaScriptSyntax {
     ) {
       const environmentRead = directEnvironmentRead(node)
       if (environmentRead) environmentReads.push(environmentRead)
+    } else if (!environmentVariablesShadowed && bindsEnvironmentVariables(node)) {
+      environmentVariablesShadowed = true
     }
     ts.forEachChild(node, visit)
   }
   visit(sourceFile)
-  return { identifierNames, values, environmentReads }
+  return {
+    identifierNames,
+    values,
+    /** A shadowed binding makes every read ambiguous, so none of them are reported. */
+    environmentReads: environmentVariablesShadowed ? [] : environmentReads,
+  }
 }
 
 function collectForbiddenSentinels(

apps/sim/lib/execution/code-placeholders/python.ts

@@ -572,6 +572,21 @@ function classifyPythonBarePlaceholder(
 const PYTHON_DIRECT_ENVIRONMENT_READ =
   /environmentVariables\s*(?:\[\s*(['"])([A-Za-z0-9_]+)\1\s*\]|\.\s*get\s*\(\s*(['"])([A-Za-z0-9_]+)\3)/g
 
+/** Every mention of the runtime binding, whatever it is being used for. */
+const PYTHON_ENVIRONMENT_IDENTIFIER = /environmentVariables/g
+
+/**
+ * The only two shapes this detector can attribute: a literal subscript or `.get()`.
+ *
+ * Anything else — `environmentVariables = {...}`, a `def` parameter, `for … in`, `as`, or
+ * simply passing it to a function — either rebinds the name or aliases the object somewhere
+ * this scanner cannot follow. There is no Python parser here to resolve scopes with, so the
+ * rule is an allowlist: if a mention is not one of these two reads, detection is off for the
+ * whole file. Under-reporting is the safe direction — a trail that claims a use that never
+ * happened is worse than one that misses a use.
+ */
+const PYTHON_ATTRIBUTABLE_ENVIRONMENT_USE = /^environmentVariables\s*(?:\[|\.\s*get\s*\()/
+
 /**
  * Reports environment reads that bypass `{{NAME}}`, skipping any match that the lexer places
  * inside a string or comment — the same authority the placeholder rewriter uses to decide
@@ -592,14 +607,6 @@ function recordPythonDirectEnvironmentReads(
   let match: RegExpExecArray | null
   while ((match = PYTHON_DIRECT_ENVIRONMENT_READ.exec(code)) !== null) {
     if (isIdentifierCharacter(code[match.index - 1])) continue
-    /**
-     * `other.environmentVariables['NAME']` reads some unrelated object that merely shares the
-     * name, so the binding must be a bare identifier rather than an attribute. The JavaScript
-     * side gets this from the AST; here it is a look-behind past whitespace for a dot.
-     */
-    let previous = match.index - 1
-    while (previous >= 0 && /[ \t]/.test(code[previous])) previous -= 1
-    if (code[previous] === '.') continue
     if (!context.tracksDirectEnvironmentRead(match[2] ?? match[4] ?? '')) continue
     matches.push(match)
   }
@@ -610,6 +617,26 @@ function recordPythonDirectEnvironmentReads(
     ...lexed.comments,
     ...lexed.strings.map((token): [number, number] => [token.start, token.end]),
   ]
+
+  /**
+   * Every real mention has to be an attributable read before any of them is recorded.
+   *
+   * This also settles attribute access: `other.environmentVariables['K']` reads an unrelated
+   * object that merely shares the name, and it is caught here by the preceding `.` rather
+   * than by a look-behind, so a `.` separated by a newline inside parentheses or after a
+   * line continuation is handled the same as one separated by a space.
+   */
+  PYTHON_ENVIRONMENT_IDENTIFIER.lastIndex = 0
+  let mention: RegExpExecArray | null
+  while ((mention = PYTHON_ENVIRONMENT_IDENTIFIER.exec(code)) !== null) {
+    if (isOffsetInRanges(mention.index, ignoredRanges)) continue
+    if (isIdentifierCharacter(code[mention.index - 1])) continue
+    let previous = mention.index - 1
+    while (previous >= 0 && /[\s\\]/.test(code[previous])) previous -= 1
+    if (code[previous] === '.') return
+    if (!PYTHON_ATTRIBUTABLE_ENVIRONMENT_USE.test(code.slice(mention.index))) return
+  }
+
   for (const candidate of matches) {
     if (isOffsetInRanges(candidate.index, ignoredRanges)) continue
     const name = candidate[2] ?? candidate[4]

apps/sim/lib/secrets/usage/record.ts

@@ -115,8 +115,18 @@ async function upsertSecretUsage(
          */
         lastUsedAt: sql`greatest(${secretUsage.lastUsedAt}, excluded.last_used_at)`,
         firstUsedAt: sql`least(${secretUsage.firstUsedAt}, excluded.first_used_at)`,
-        lastExecutionId: sql`excluded.last_execution_id`,
-        lastTrigger: sql`excluded.last_trigger`,
+        /**
+         * The run that owns `last_used_at` has to own the metadata beside it. Assigning
+         * these unconditionally while the timestamp is chosen by `greatest` lets two runs
+         * completing out of order split one row between them — the newer run's timestamp
+         * next to the older run's execution id, so "View log" opens a run that is not the
+         * one the row says it last happened at. The guard keeps both from the same run.
+         *
+         * Postgres evaluates every SET expression against the pre-update row, so
+         * `secret_usage.last_used_at` here is the stored value, not the one being written.
+         */
+        lastExecutionId: sql`case when excluded.last_used_at >= ${secretUsage.lastUsedAt} then excluded.last_execution_id else ${secretUsage.lastExecutionId} end`,
+        lastTrigger: sql`case when excluded.last_used_at >= ${secretUsage.lastUsedAt} then excluded.last_trigger else ${secretUsage.lastTrigger} end`,
         updatedAt: now,
       },
     })