fix(secrets): an update reads before it stores, and a del target may be parenthesized

Review round 5. The first of these is a regression from round 4.

- javascript.ts: reusing isWriteIdentifier to answer "is this a read" was wrong.
  That predicate answers the rewriter's question — is this a target the
  substitution must refuse — so it treats every assignment operator alike, which
  is correct there and wrong here: `+=`, `||=`, `??=`, `++` and `--` all load the
  current value before storing, so they are genuine reads and were silently
  losing their masking. Only a plain `=` stores without reading. Replaced with a
  purpose-named predicate, and isWriteIdentifier's parameter is narrowed back to
  ts.Identifier now that nothing else needs it widened.

  A test committed last round asserted the wrong behaviour for `+=`; it has been
  corrected rather than left to pin the bug.

- python.ts: `del (environmentVariables['K'])` slipped past a check that looked
  only at the characters immediately before the match. It now isolates the
  enclosing logical line and tests whether that is a del statement, which also
  covers `del((x))`, `del(x)`, `del a, x`, and a del after a semicolon.

12 tests added or corrected; 10 fail against the previous code.

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

Author: icecrasher321

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

@@ -1322,7 +1322,6 @@ describe('writing a configured name is not reading it', () => {
   it.each([
     ['javascript property assignment', "environmentVariables.API_KEY = 'x'"],
     ['javascript subscript assignment', "environmentVariables['API_KEY'] = 'x'"],
-    ['javascript compound assignment', "environmentVariables.API_KEY += 'x'"],
     ['javascript delete', 'delete environmentVariables.API_KEY'],
     ['javascript delete subscript', "delete environmentVariables['API_KEY']"],
   ])('%s', async (_label, code) => {
@@ -1336,6 +1335,50 @@ describe('writing a configured name is not reading it', () => {
     expect(await directReadNames(code, CodeLanguage.Python)).toEqual([])
   })
 
+  /**
+   * A compound, logical, or increment update loads the current value before storing, so it is
+   * a read of the mounted secret and has to keep its masking. Only a plain `=` stores without
+   * reading.
+   */
+  it.each([
+    ['compound assignment', "environmentVariables.API_KEY += 'x'"],
+    ['logical assignment', "environmentVariables.API_KEY ||= 'x'"],
+    ['nullish assignment', "environmentVariables.API_KEY ??= 'x'"],
+    ['subscript compound assignment', "environmentVariables['API_KEY'] += 'x'"],
+    ['postfix increment', 'environmentVariables.API_KEY++'],
+    ['prefix increment', '++environmentVariables.API_KEY'],
+  ])('javascript reads through an update: %s', async (_label, code) => {
+    expect(await directReadNames(code, CodeLanguage.JavaScript)).toEqual(['API_KEY'])
+  })
+
+  /** Python's augmented assignment reads first too. */
+  it('python reads through an augmented assignment', async () => {
+    expect(
+      await directReadNames("environmentVariables['API_KEY'] += 'x'", CodeLanguage.Python)
+    ).toEqual(['API_KEY'])
+  })
+
+  /** Greptile's case: `del` targets may be parenthesized or listed. */
+  it.each([
+    ['parenthesized', "del (environmentVariables['API_KEY'])"],
+    ['double parenthesized', "del ((environmentVariables['API_KEY']))"],
+    ['no space before paren', "del(environmentVariables['API_KEY'])"],
+    ['multi-target', "del other, environmentVariables['API_KEY']"],
+    ['after a semicolon', "x = 1; del environmentVariables['API_KEY']"],
+  ])('python del target: %s', async (_label, code) => {
+    expect(await directReadNames(code, CodeLanguage.Python)).toEqual([])
+  })
+
+  /** `delete` on a line of its own must not disable a real read elsewhere. */
+  it('python still reports a read on another line', async () => {
+    expect(
+      await directReadNames(
+        "del environmentVariables['API_KEY']\nk = environmentVariables['API_KEY']",
+        CodeLanguage.Python
+      )
+    ).toEqual(['API_KEY'])
+  })
+
   /** Reading the same key elsewhere is still a use, even if another line writes it. */
   it('javascript still reports a read alongside a write', async () => {
     expect(

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

@@ -41,19 +41,34 @@ const ENVIRONMENT_VARIABLES_IDENTIFIER = 'environmentVariables'
  * {@link CodePlaceholderCompilationContext.recordDirectEnvironmentRead}.
  */
 /**
- * Whether this member access is being written or deleted rather than read.
+ * Whether this member access is written *without* being read.
  *
- * `environmentVariables.API_KEY = 'x'` and `delete environmentVariables.API_KEY` both touch the
- * name without ever reading the mounted value, so recording either would put a use in the trail
- * that never happened. `isWriteIdentifier` already answers the write half for the placeholder
- * rewriter; `delete` is asked here because only a read detector cares about it.
+ * Only a plain `=` and `delete` qualify. A compound assignment (`+=`), a logical assignment
+ * (`||=`, `&&=`, `??=`), and `++`/`--` all load the current value before storing, so they are
+ * genuine reads of the mounted secret and have to keep their masking.
+ *
+ * This deliberately does not reuse `isWriteIdentifier`. That predicate answers the rewriter's
+ * question — is this a target the substitution must refuse — and so treats every assignment
+ * operator alike, which is correct there and wrong here. Two different questions; conflating
+ * them silently dropped `+=` from masking.
+ *
+ * A member reached through a destructuring assignment (`({ k: environmentVariables.K } = o)`)
+ * is not recognized and is reported as a read. That is the mild direction: an extra usage row
+ * rather than an unmasked value.
  */
-function writesEnvironmentMember(node: ts.Node): boolean {
-  return ts.isDeleteExpression(node.parent) || isWriteIdentifier(node)
+function writesWithoutReading(node: ts.Node): boolean {
+  const parent = node.parent
+  if (!parent) return false
+  if (ts.isDeleteExpression(parent)) return true
+  return (
+    ts.isBinaryExpression(parent) &&
+    parent.left === node &&
+    parent.operatorToken.kind === ts.SyntaxKind.EqualsToken
+  )
 }
 
 function directEnvironmentRead(node: ts.Node): DirectEnvironmentRead | undefined {
-  if (writesEnvironmentMember(node)) return undefined
+  if (writesWithoutReading(node)) return undefined
   if (ts.isPropertyAccessExpression(node)) {
     if (!ts.isIdentifier(node.expression)) return undefined
     if (node.expression.text !== ENVIRONMENT_VARIABLES_IDENTIFIER) return undefined
@@ -450,7 +465,7 @@ function isDeclarationIdentifier(node: ts.Identifier): boolean {
   )
 }
 
-function isWriteIdentifier(node: ts.Node): boolean {
+function isWriteIdentifier(node: ts.Identifier): boolean {
   let current: ts.Node = node
   let targetPosition = true
   for (let parent = current.parent; parent; current = parent, parent = parent.parent) {

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

@@ -577,10 +577,28 @@ const PYTHON_ENVIRONMENT_IDENTIFIER = /environmentVariables/g
 
 /**
  * A subscript that is being assigned to rather than read: `environmentVariables['K'] = v`.
- * The trailing `=` must not be `==`, `!=`, `<=`, `>=`, or `:=`, none of which write.
+ * The trailing `=` must not be `==`, `!=`, `<=`, `>=`, or `:=`, none of which write. An
+ * augmented assignment (`+=`) is absent on purpose: it loads the current value first, so it
+ * is a read.
  */
 const PYTHON_SUBSCRIPT_WRITE = /^\s*=(?!=)/
 
+/** A `del` statement, once the enclosing logical line has been isolated. */
+const PYTHON_DEL_STATEMENT = /^del[\s(]/
+
+/**
+ * Whether the mention sits inside a `del` statement, which removes the key without reading it.
+ *
+ * Tested against the whole statement rather than the few characters before the match, so the
+ * parenthesized and multi-target forms — `del (environmentVariables['K'])` and
+ * `del other, environmentVariables['K']` — are recognized alongside the plain one.
+ */
+function isDeleteTarget(code: string, offset: number): boolean {
+  const before = code.slice(0, offset)
+  const statementStart = Math.max(before.lastIndexOf('\n'), before.lastIndexOf(';')) + 1
+  return PYTHON_DEL_STATEMENT.test(before.slice(statementStart).trimStart())
+}
+
 /**
  * The only two shapes this detector can attribute: a literal subscript or `.get()`.
  *
@@ -650,7 +668,7 @@ function recordPythonDirectEnvironmentReads(
      * without reading the mounted value, so neither is a use of the secret.
      */
     if (PYTHON_SUBSCRIPT_WRITE.test(code.slice(candidate.index + candidate[0].length))) continue
-    if (/(^|[\s;:])del\s+$/.test(code.slice(0, candidate.index))) continue
+    if (isDeleteTarget(code, candidate.index)) continue
     const name = candidate[2] ?? candidate[4]
     if (name) context.recordDirectEnvironmentRead(name, candidate.index)
   }