Correctness bugs that look fine in review

Some bugs do not announce themselves. The diff is small, the names look intentional, and nothing throws at runtime in the happy path. A reviewer skimming for security or API shape can easily approve code that is simply wrong, duplicated keys that silently override each other, an assignment where a comparison was meant, or an async Promise executor that drops errors on the floor.

Critiq ships a first wave of ts.correctness.* rules for exactly this class of defect. They are deterministic, tied to inspectable YAML, and designed to fire on patterns humans miss when they are tired or focused on the wrong layer of the change. This post walks through the rules, shows examples that pass casual review, and explains how to run them locally.

Why review misses them

Code review is a attention budget problem. Reviewers optimize for the risk they expect: auth gaps, injection, breaking public APIs, missing tests. Correctness slips that compile cleanly and behave correctly in the demo path often sit below the threshold.

• Duplicate object keys are valid JavaScript, the later property wins with no warning.
• Assignment in a condition (`if (x = y)`) is a classic typo that TypeScript accepts when types align.
• Async Promise executors look like modern async/await style; the rejection semantics are wrong, not the syntax.
• Duplicate imports from the same module are harmless at runtime but signal a merge mistake or half-finished refactor.

Static analysis does not get tired. It reads the AST the same way on line forty as on line four. The first shipped core-language wave maps nine high-signal checks from the public JavaScript analyzer directory into the OSS catalog.

The first wave of ts.correctness rules

These rules landed in the OSS catalog in May 2026. Each one emits a medium-severity finding with a stable rule id you can filter, explain, and gate on in CI.

• ts.correctness.assignment-in-condition
• ts.correctness.duplicate-function-parameter
• ts.correctness.duplicate-object-key
• ts.correctness.duplicate-switch-case
• ts.correctness.async-promise-executor
• ts.correctness.assignment-to-import-binding
• ts.correctness.self-assignment
• ts.correctness.identical-comparison-operands
• ts.correctness.duplicate-import-source

A second wave added ts.correctness.empty-block-statement, ts.correctness.reassign-catch-binding, and ts.correctness.regexp-pattern-unusual-control-character. This post focuses on the first nine because they show up constantly in real diffs, especially in configuration objects, promise wrappers, and files touched by multiple authors during a merge window.

How the rules are implemented

Each rule in the first wave is a catalog entry backed by AST facts from the TypeScript adapter in critiq-core. The adapter emits language.* facts, duplicate-object-key, assignment-in-condition, async-promise-executor, and the rest, and the rules engine matches them with YAML you can read without spelunking the analyzer. That separation matters for review culture: when a finding fires, you can open the rule file, read the rationale field, and decide whether to fix the code, adjust the preset, or suppress with an documented exception.

Severity is medium across the wave. These are not exploit primitives; they are logic defects that become incidents when configuration is wrong, retries behave unexpectedly, or a branch never runs. Confidence is high (0.9 and above on most rules) because the patterns are syntactic, little heuristic guesswork.

Duplicate keys that read like a feature flag merge

Rule: ts.correctness.duplicate-object-key. Object literals with repeated static keys compile and run. The last value wins. In review, two similarly named flags can look like intentional layering when one was meant to replace the other.

function buildRolloutConfig(env: Record<string, string>) {
  return {
    enabled: env.FEATURE_ENABLED === 'true',
    enabled: env.FEATURE_ROLLOUT === 'full', // duplicate key, first value is dead
    sampleRate: Number(env.SAMPLE_RATE ?? '0'),
  };
}

Critiq flags the second enabled entry. The remediation is straightforward: merge the conditions, rename one key, or delete the redundant property. Without the rule, the bug ships as silent configuration drift, staging might enable the flag while production reads the rollout string, and neither environment throws.

Spread syntax does not save you. `{ ...base, enabled: true, enabled: false }` still ends with a single enabled key. Computed keys and symbol keys are out of scope for this rule; it targets the static string and number keys that dominate config objects in application code.

Assignment where the reviewer expected a comparison

Rule: ts.correctness.assignment-in-condition. Using `=` inside an `if`, `while`, `do`, or `for` test is almost always a mistake. It is also easy to miss when the assigned variable name matches what the author meant to compare.

async function drainQueue(queue: AsyncQueue<Message>) {
  let message: Message | undefined;
  while (message = await queue.pop()) {
    await handle(message);
  }
}

A reviewer might read this as idiomatic loop-and-assign. Critiq reports ts.correctness.assignment-in-condition because the while test is an AssignmentExpression, not a boolean comparison. The fix is to separate assignment from the predicate, for example, assign inside the loop body and compare explicitly.

Note: `while ((x = y) != z)` stays clean because the test is a BinaryExpression wrapping the assignment. The rule targets top-level assignment in the condition, which is where the typo class lives.

Async Promise executors that hide thrown errors

Rule: ts.correctness.async-promise-executor. Wrapping async work in `new Promise(async (resolve, reject) => { ... })` is a persistent antipattern. If the async executor throws before the first await, the rejection path does not behave the way callers expect.

export function loadUserProfile(userId: string): Promise<Profile> {
  return new Promise(async (resolve, reject) => {
    const response = await fetch(`/api/users/${userId}`);
    if (!response.ok) {
      throw new Error(`profile fetch failed: ${response.status}`);
    }
    resolve(await response.json());
  });
}

In review this reads like a reasonable adapter around fetch. Critiq flags the async keyword on the executor. Prefer returning the async chain directly, `return fetch(...).then(...)` or an async function that awaits fetch, or use a synchronous executor that calls resolve and reject without marking the callback async.

The failure mode is subtle: thrown errors inside an async executor become unhandled promise rejections instead of rejecting the outer Promise you just constructed. Callers wrapping loadUserProfile in try/catch around the constructor will not catch those failures. That is why linters and analyzers treat this pattern as correctness, not style.

Structural duplicates: parameters, switch cases, imports

Three rules catch duplicates that TypeScript allows but humans should not merge:

• ts.correctness.duplicate-function-parameter, the same identifier twice in a parameter list, often after a bad rename.
• ts.correctness.duplicate-switch-case, two case labels with the same discriminant; the second is unreachable.
• ts.correctness.duplicate-import-source, the same module string imported twice in one file, usually leftover from a conflict resolution.

import type { Finding } from '@critiq/core';
import { normalizeSeverity } from '@critiq/core';
import { renderSummary } from '@critiq/core';

export function formatReport(findings: Finding[], findings: Finding[]) {
  switch (findings.length) {
    case 0:
      return 'clean';
    case 0:
      return 'empty'; // unreachable, duplicate case
    default:
      return renderSummary(findings.map(normalizeSeverity));
  }
}

Each issue is local and obvious once pointed out. None of them fail the type checker. All three produce separate findings so you can prioritize the one that matches your team’s merge hygiene.

Self-assignment and identical comparisons

Two more rules catch copy-paste slips that look harmless:

• ts.correctness.self-assignment, `count = count` or equivalent where both sides normalize to the same source text.
• ts.correctness.identical-comparison-operands, `status === status` or `offset < offset` where both operands match after normalization.

function reconcileTotals(local: Totals, remote: Totals): Totals {
  if (local.hash === local.hash) {
    return remote;
  }
  local.count = local.count;
  return { ...local, ...remote };
}

The hash check looks like defensive coding until you notice both operands are the same binding. The self-assignment on count might have been `local.count = remote.count`. These are low-drama bugs with high embarrassment potential in production.

Assignment to import bindings

Rule: ts.correctness.assignment-to-import-binding. Reassigning a named, namespace, or default import breaks the mental model of module boundaries. It is rare in greenfield code and common after a hurried fix.

import config from './config';

export function applyOverride(key: string, value: string) {
  if (key === 'apiUrl') {
    config = { ...config, apiUrl: value };
  }
}

Critiq flags assignment to the imported config binding. The rule skips `import type` bindings. The fix is to mutate a property, return a new object from a factory, or thread immutable updates through your state layer.

Run the rules locally

You do not need to memorize the list. Run Critiq against the tree and read the rule ids in the output. Each finding links back to YAML you can inspect in the catalog.

npx critiq check .
critiq check --format json . | jq '.findings[] | select(.ruleId | startswith("ts.correctness."))'

In CI, critiq-action can comment on the diff with the same rule ids. Severity gating is optional, many teams start by surfacing medium correctness findings without blocking merges, then tighten once noise is understood. Filter to ts.correctness.* during adoption so security findings and correctness findings do not compete for attention in the same channel.

What to do in review

Automated rules complement review; they do not replace judgment about product intent. Use this checklist when you want to catch the bugs above without running the CLI on every file by hand:

• Scan object literals in config and feature-flag changes for repeated keys.
• Treat every `=` inside loop and branch conditions as suspicious until proven idiomatic.
• Reject `new Promise(async` in new code; ask for a direct async function or a sync executor.
• When imports from the same path appear twice, assume an incomplete merge until explained.
• Flag comparisons where both sides are the same identifier, they are rarely intentional guards.

The point is not to automate away review. The point is to spend review time on decisions that need humans, API shape, threat model, naming, while deterministic rules handle the pattern class that humans consistently under-weight.

Browse the full rule catalog at https://docs.critiq.dev/rules and run the OSS CLI against your repo. If a finding surprises you, open the rule YAML and decide from the text whether it belongs in your preset. That inspectability is the product.