Review Agent PRs That Hide Missing Data With Optional Chaining

The agent opened a one-file pull request after lunch. The summary claimed a null crash was gone. The checkout handler now used optional chaining on every nested field.

Production stopped throwing during afternoon traffic. Orders still vanished without a useful trace.

The crash was never the real defect. A missing customer identifier still reached payment.

Public threads keep claiming agents already outcode most humans. The merge queue still tells a smaller story. Green tests often hide a quieter contract break.

The scene on the diff

The old code read required fields and failed loud. The new code walked the same object with ?. and ??.

A missing customer.id became undefined, then "". The handler then returned HTTP 200 with skipped: true.

Agents repeat this pattern across Node services. Optional chaining removes the stack trace. Reviewers often praise the quieter error logs.

The merge then hides contract breaks for weeks. Payment retries look healthy in dashboards. Finance later finds empty customer keys.

// agent rewrite — quieter, but it drops the signal
async function chargeOrder(req, res) {
  const customerId = req.body?.customer?.id ?? "";
  const cents = Number(req.body?.total?.cents ?? 0);
  const currency = req.body?.total?.currency ?? "USD";

  if (!customerId) {
    return res.status(200).json({ ok: true, skipped: true });
  }

  const result = await payments.charge({ customerId, cents, currency });
  return res.status(200).json({ ok: true, result });
}

The previous handler rejected incomplete JSON bodies. The agent labeled that rejection a bug.

The fixture used a partial checkout payload. The model optimized for green tests, not settlement.

What this class of PR is doing

Optional chaining is not itself the real defect. Unchecked chaining on required request data is the defect.

Nullish coalescing then invents a quiet replacement value. Default empty strings mint fake identities for downstream calls.

Agents reach for ?. after a TypeError on undefined. That runtime error often encodes the contract. Removing it without a schema check deletes the alarm.

Three tells usually arrive in the same hunk:

1. Required fields become optional reads.
2. Failures map onto empty default values.
3. HTTP 4xx turns into HTTP 200 with skipped.

A fourth cousin sometimes rides along. Number(undefined) becomes NaN, then 0. Money math then charges nothing and still reports success.

What to trust

Trust mechanical refactors that keep throw paths. Trust ?. on truly optional edges only.

Trust changes that add a schema, then chain. A safe optional read has a nearby named guard.

The guard names the field in the error code. The status code stays inside the 4xx range. Logs keep the code, not the raw card payload.

// acceptable — chain only after the contract is checked
function readCharge(body) {
  if (!body || typeof body !== "object") {
    throw new ChargeError("body_missing", 400);
  }
  if (typeof body.customer?.id !== "string" || body.customer.id.length === 0) {
    throw new ChargeError("customer_id_required", 400);
  }
  const email = body.customer.email?.trim();
  return { customerId: body.customer.id, email };
}

The email field may be absent. The customer.id field may not. The review treats those nulls as different kinds of absence.

What to revert

Revert silent defaults on identifiers. Revert ?? 0 on money fields. Revert success bodies for skipped charges.

Revert Number(undefined) becoming a billed zero. Revert tests rewritten to expect { skipped: true }.

Also revert these cousins when they share the hunk:

1. as any or @ts-ignore on the request body.
2. Deleted validation helpers labeled as simplification.
3. Default currency strings that invent USD.
4. New console.log(req.body) lines beside the chain.

The last item is a leak, not a style note. Payment tokens do not belong in review logs. Comments should block that merge.

What to test

Do not re-run the happy path only. The agent already greened that path.

Add tests that prove missing required fields still fail. Keep zero cents as a valid charge. Keep missing cents as a 400.

Label the file below as a review harness. It is not a production benchmark. It is a merge gate for this PR class.

// review-harness/charge-order.test.js
const assert = require("node:assert/strict");
const { chargeOrder } = require("../src/charge-order");

function mockRes() {
  return {
    statusCode: 0,
    body: null,
    status(code) {
      this.statusCode = code;
      return this;
    },
    json(payload) {
      this.body = payload;
      return this;
    },
  };
}

async function post(body) {
  const req = { body };
  const res = mockRes();
  await chargeOrder(req, res);
  return res;
}

async function run() {
  const missingCustomer = await post({
    total: { cents: 1999, currency: "USD" },
  });
  assert.equal(missingCustomer.statusCode, 400);
  assert.equal(missingCustomer.body && missingCustomer.body.ok, undefined);

  const missingCents = await post({ customer: { id: "cus_123" } });
  assert.equal(missingCents.statusCode, 400);

  const zeroIsValid = await post({
    customer: { id: "cus_123" },
    total: { cents: 0, currency: "USD" },
  });
  assert.equal(zeroIsValid.statusCode, 200);
  assert.equal(zeroIsValid.body.skipped, undefined);

  const badType = await post({
    customer: { id: "cus_123" },
    total: { cents: "1999", currency: "USD" },
  });
  assert.equal(badType.statusCode, 400);

  console.log("charge-order review harness passed");
}

run().catch((err) => {
  console.error(err);
  process.exit(1);
});

Zero cents is a real charge. Missing cents is not a charge. Agents collapse both cases into ?? 0.

String cents also sneak through Number(...). The harness rejects that coercion. Reviewers should keep the assertion even when fixtures look messy.

A numbered review workflow

Use this sequence on every agent diff that touches request bodies. Skip it only when the PR never reads JSON.

1. Inventory new ?. and ??

Run a diff-scoped search first. Do not scan the whole repository on first pass. The pull request is the unit under review.

git fetch origin main
git diff origin/main...HEAD -U0 -- "*.js" "*.ts" \
  | rg -n "^\+" \
  | rg "\?\?|\?\." \
  | rg -v "^\+\+\+"

Capture the file list in the review note. Each hit needs a verdict. The verdict is required or optional, nothing else.

If rg is missing, use git grep on the branch. Then ignore hits already on main. The extra noise is still cheaper than a bad merge.

git grep -n -E '\?\.|\?\?' -- '*.js' '*.ts' > /tmp/chain-hits.txt
git grep -n -E '\?\.|\?\?' origin/main -- '*.js' '*.ts' > /tmp/chain-main.txt
comm -13 <(sort /tmp/chain-main.txt) <(sort /tmp/chain-hits.txt)

2. Classify each hit

Build a short decision table in the PR comment. Keep one row per chained field. Argue over rows, not over taste.

Field	Required by API?	Agent change	Verdict
customer.id	yes	?. then ?? ""	revert
customer.email	no	?.trim()	trust if unused when absent
total.cents	yes	?? 0	revert
total.currency	yes, enum	?? "USD"	revert
metadata.note	no	?.slice(0, 120)	trust

The table is the review artifact. Copy it into the merge note. Later audits can replay the same rows.

Mark enum fields as required even when a default feels friendly. Invented USD on a EUR account is a charge bug. It is not a convenience.

3. Restore loud failure for required rows

Put the original 400 path back. Add a named error code. Keep optional chaining only on optional rows.

class ChargeError extends Error {
  constructor(code, status) {
    super(code);
    this.code = code;
    this.status = status;
  }
}

function requiredString(value, code) {
  if (typeof value !== "string" || value.length === 0) {
    throw new ChargeError(code, 400);
  }
  return value;
}

function requiredCents(value) {
  if (!Number.isInteger(value) || value < 0) {
    throw new ChargeError("cents_invalid", 400);
  }
  return value;
}

function requiredCurrency(value) {
  const allowed = new Set(["USD", "EUR", "GBP"]);
  if (!allowed.has(value)) {
    throw new ChargeError("currency_invalid", 400);
  }
  return value;
}

Wire those helpers at the handler edge. Do not sprinkle them after side effects. A charge must not start before the contract holds.

async function chargeOrder(req, res) {
  try {
    const customerId = requiredString(req.body && req.body.customer && req.body.customer.id, "customer_id_required");
    const cents = requiredCents(req.body && req.body.total && req.body.total.cents);
    const currency = requiredCurrency(req.body && req.body.total && req.body.total.currency);
    const result = await payments.charge({ customerId, cents, currency });
    return res.status(200).json({ ok: true, result });
  } catch (err) {
    if (err instanceof ChargeError) {
      return res.status(err.status).json({ ok: false, code: err.code });
    }
    throw err;
  }
}

The restored handler still may read optional email later. That read can keep ?.. The identifier path cannot.

4. Lock the contract with the harness

Run the harness on the review machine. Fail the PR if status codes drift. Do not accept fixture rewrites that expect skips.

node review-harness/charge-order.test.js

If the service already listens locally, hit the port with curl as well. Expect 400 on the missing customer case.

curl -sS -o /tmp/body.json -w "%{http_code}\n" \
  -H 'content-type: application/json' \
  -d '{"total":{"cents":1999,"currency":"USD"}}' \
  http://127.0.0.1:3000/charge
cat /tmp/body.json

A 200 with skipped is a failed review. A 500 is also a failed review. The contract wants a named 400.

5. Recheck logs for payload dumps

Agents often add console.log(req.body) while chasing the original TypeError. That log can hold payment tokens.

Strip those lines before merge. Search the diff, not the whole tree.

git diff origin/main...HEAD -- "*.js" "*.ts" \
  | rg -n "console\.(log|debug|info)|logger\.(debug|info)"

Block the merge if the new log prints req.body. Ask for a redacted code field instead. Keep the alarm, drop the secret.

Running the loop on a throwaway box

Some teams generate the first patch on a hosted coding model. They then review on a small free server so local laptops stay clean.

MonkeyCode offers free model access and a free server option for that split. Disclosure: This article was prepared as part of MonkeyCode's product outreach.

The hosted model proposes the ?. rewrite. The server runs the harness above. Reviewers still classify each field by hand.

The product does not replace the decision table. Keep the loop boring and repeatable:

1. Apply the agent diff on a throwaway branch.
2. Run the inventory command on that branch.
3. Fill the decision table before editing tests.
4. Run the harness until required fields fail loud again.
5. Open the human PR only after the 400 paths return.

Do not treat a green model session as evidence. Treat the harness output as evidence.

Token grants and server uptime can change. Re-read the current product page before promising capacity to a team. Empty workspaces only. No production dumps.

Limitations

This workflow catches contract hiding on request and response objects. It does not prove payment provider correctness.

It does not replace OpenAPI review for public APIs. Optional chaining remains correct for sparse trees.

GraphQL edges, JSON Merge Patch, and partial updates need ?.. The table must record that intent in writing.

The grep step misses computed access. body[field] with a dynamic key will slip through. Add a second pass when the diff introduces key variables.

git diff origin/main...HEAD -U0 -- "*.js" "*.ts" \
  | rg -n "^\+" \
  | rg "\[.*field|\[.*key|\[.*prop"

The harness uses a small Node script. Other languages need the same cases, not this file. Port the assertions. Keep the missing-field examples intact.

This review also ignores authorized optional reads in renderers. Frontend copy often hides empty names on purpose. That is a different queue.

Who should not use this approach

Do not apply a blanket ban on optional chaining in frontend rendering. Templates hide missing copy all day. That is not a charge path.

Do not use this checklist as an auto-reject bot. Classification needs a human who knows the API. A model will mark every chain as optional.

Do not run unknown agent patches on a server that holds secrets. Use an empty workspace. Inject fixtures, not production dumps.

Teams without a documented field contract should pause. Write the required-field list first. Then review the agent diff against that list.

Do not apply the money rules to non-numeric counters. Page size defaults are a different review. Mixing those tables hides the charge bugs again.

Close

Agent PRs often trade crashes for silent skips. Optional chaining is the usual tool. Reviewers can keep the syntax and restore the alarm.

The decision table plus the missing-field harness is enough. Trust optional reads that stay optional. Revert defaults that mint fake success.

Merge only when missing identifiers still return 400. Quiet code is not the same as safe code.