Over-Building

Asked for a 12-hour refund window, the agent also delivered a way for each gym to configure its own. Unrequested features, abstractions with one caller and defensive code around things that cannot fail are the commonest shape in agent diffs. Each one costs review time now and maintenance for as long as the code lives.

Over-building rarely breaks anything on the day it merges, which is why it survives review. The cost arrives later, as surface nobody owns and decisions nobody took. This page is about recognising the three forms, pricing them honestly, and stopping them before they are written.

Three Forms

The first form is the extra feature: per-gym refund windows, when the task asked for one window. The second is the abstraction for a single case: a policy object that holds one value, an interface with one implementation, a factory that makes one kind of thing. The third is defensive code: a check for a missing value that the schema guarantees, a catch block that logs and carries on, the same validation repeated in every layer the data passes through.

Each addition looks helpful on its own. Put together, they turn a change that should take ten minutes to review into one that takes an hour, and most of that hour goes on code that does nothing the task required.

The Refund Case

The refund diff is about 140 lines. The approved plan named two files and fewer than 20 lines. About 60 of the 140 lines are a new policy object, so that each gym "can configure its own window". It has two callers, and both pass the same default. It holds one value. It reads a new gym setting that no gym sets, and when the setting is missing it falls back to the 12 hours the task asked for. Eight or nine new tests for it, depending on the language, and the wiring in booking and the gym settings take its share of the diff past 100 lines.

"""Refund policy, configurable per gym.

Each gym can set its own refund window with ``refund_window_hours`` in its
settings. A gym that sets nothing uses the default window passed in by the
caller, so behaviour only changes for a gym that opts in.

Build one with ``Policy.for_gym`` rather than by hand.
"""

from __future__ import annotations

from dataclasses import dataclass
from datetime import timedelta

from cragmoor.gyms import Gym

# The longest window a gym may configure. Anything above it is a typo.
MAX_WINDOW_HOURS = 72


class PolicyError(ValueError):
    """A gym's refund settings are not valid."""


@dataclass(frozen=True)
class Policy:
    """How refunds work at one gym."""

    window_hours: int

    def __post_init__(self) -> None:
        if not isinstance(self.window_hours, int):
            raise PolicyError(f"window must be whole hours, got {self.window_hours!r}")
        if self.window_hours < 0:
            raise PolicyError("window cannot be negative")
        if self.window_hours > MAX_WINDOW_HOURS:
            raise PolicyError(f"window cannot exceed {MAX_WINDOW_HOURS} hours")

    @classmethod
    def for_gym(cls, gym: Gym, default: timedelta) -> Policy:
        """The gym's own window if it sets one, otherwise the default."""
        configured = gym.refund_window_hours
        if configured is None:
            return cls(window_hours=int(default.total_seconds() // 3600))
        return cls(window_hours=configured)

    @property
    def window(self) -> timedelta:
        """The refund window as a duration."""
        return timedelta(hours=self.window_hours)

    def allows_refunds(self) -> bool:
        """Whether cancellations at this gym are refunded at all."""
        return self.window_hours > 0

    def with_window(self, hours: int) -> Policy:
        """A copy of this policy with a different window."""
        return Policy(window_hours=hours)

    def describe(self) -> str:
        """A short description for logs and support tools."""
        if not self.allows_refunds():
            return "no refunds"
        return f"full refund up to {self.window_hours} hours before class"

// Refund policy, configurable per gym.
//
// Each gym can set its own refund window with `refundWindowHours` in its
// settings. A gym that sets nothing uses the default window passed in by the
// caller, so behaviour only changes for a gym that opts in.
//
// Build one with Policy.forGym rather than by hand.
import type { Gym } from "./gyms.ts";

const HOUR_MS = 60 * 60 * 1000;

// The longest window a gym may configure. Anything above it is a typo.
export const MAX_WINDOW_HOURS = 72;

/** A gym's refund settings are not valid. */
export class PolicyError extends Error {}

/** How refunds work at one gym. */
export class Policy {
  readonly windowHours: number;

  constructor(windowHours: number) {
    if (!Number.isInteger(windowHours)) {
      throw new PolicyError(`window must be whole hours, got ${windowHours}`);
    }
    if (windowHours < 0) {
      throw new PolicyError("window cannot be negative");
    }
    if (windowHours > MAX_WINDOW_HOURS) {
      throw new PolicyError(`window cannot exceed ${MAX_WINDOW_HOURS} hours`);
    }
    this.windowHours = windowHours;
  }

  /** The gym's own window if it sets one, otherwise the default. */
  static forGym(gym: Gym, defaultMs: number): Policy {
    const configured = gym.refundWindowHours;
    if (configured === undefined) {
      return new Policy(Math.floor(defaultMs / HOUR_MS));
    }
    return new Policy(configured);
  }

  /** The refund window in milliseconds. */
  get windowMs(): number {
    return this.windowHours * HOUR_MS;
  }

  /** Whether cancellations at this gym are refunded at all. */
  allowsRefunds(): boolean {
    return this.windowHours > 0;
  }

  /** A copy of this policy with a different window. */
  withWindow(hours: number): Policy {
    return new Policy(hours);
  }

  /** A short description for logs and support tools. */
  describe(): string {
    if (!this.allowsRefunds()) {
      return "no refunds";
    }
    return `full refund up to ${this.windowHours} hours before class`;
  }
}

package org.cragmoor;

import java.time.Duration;

/** Refund policy, configurable per gym. Build one with {@link #forGym} rather than by hand. */
public final class Policy {
  /** The longest window a gym may configure. Anything above it is a typo. */
  public static final int MAX_WINDOW_HOURS = 72;

  public static final class PolicyException extends IllegalArgumentException {
    PolicyException(String message) {
      super(message);
    }
  }

  private final int windowHours;

  public Policy(int windowHours) {
    if (windowHours < 0) {
      throw new PolicyException("window cannot be negative");
    }
    if (windowHours > MAX_WINDOW_HOURS) {
      throw new PolicyException("window cannot exceed " + MAX_WINDOW_HOURS + " hours");
    }
    this.windowHours = windowHours;
  }

  /** The gym's own window if it sets one, otherwise the default. */
  public static Policy forGym(Gym gym, Duration defaultWindow) {
    Integer configured = gym.refundWindowHours();
    if (configured == null) {
      return new Policy((int) defaultWindow.toHours());
    }
    return new Policy(configured);
  }

  public int windowHours() {
    return windowHours;
  }

  /** The refund window as a duration. */
  public Duration window() {
    return Duration.ofHours(windowHours);
  }

  /** Whether cancellations at this gym are refunded at all. */
  public boolean allowsRefunds() {
    return windowHours > 0;
  }

  /** A copy of this policy with a different window. */
  public Policy withWindow(int hours) {
    return new Policy(hours);
  }

  /** A short description for logs and support tools. */
  public String describe() {
    if (!allowsRefunds()) {
      return "no refunds";
    }
    return "full refund up to " + windowHours + " hours before class";
  }
}

Read it as a reviewer would. A lookup for a setting nobody sets. A fallback to the only value anyone uses. Validation against a 72-hour maximum nobody chose. A method that copies the policy with a different window, which nothing calls. A description "for logs and support tools" that no log prints. And a zero-hour window that switches refunds off entirely at a gym. None of it is wrong, and none of it was asked for.

The deeper problem is not the lines. Per-gym refund windows are a product decision Cragmoor never made. Merging the diff would have made that decision by default: the moment someone sets the key for one gym, members at that gym live under a different refund rule, and nobody in operations ever agreed to it.

Why It Happens

Public code rewards generality. Libraries, frameworks and tutorials, which dominate what a model learns from, are written for many callers, so the model writes for the general case even when there is one. It also tends towards answers that look complete, and a configurable window looks more complete than a constant.

And nothing in the task said what not to build. "Change the refund window to 12 hours" leaves every addition open, and closing them is the job of the out-of-bounds list from Chapter 5. An agent that is told what to build and not what to leave alone will fill the gap with whatever looks most helpful.

There is also a pull from the agent's own loop. A task that ends in "and update the tests" gives it room to write tests for whatever it built, and new tests make new code look finished. The refund diff's policy object arrived with its own tests, all green, under a summary whose "updated the tests accordingly" covered them along with everything else. Each piece vouched for the others, and none of them had been asked for.

What It Costs

The first cost is reading time. The object alone is about 60 lines read for no requested behaviour: 43% of the review spent on 0% of the request, before its tests are counted. The second is surface: a configuration key someone will one day set wrongly, or set for one gym without telling anyone. The third is tests nobody asked for and nobody reviewed with care: the agent wrote eight or nine for its own object, and they check that the object does what the object does, which Chapter 8 shows is not the same as checking a rule anyone agreed. The fourth is a design decision made by nobody.

Defensive code has a cost of its own: it hides failures. A catch that logs and returns a default turns a crash into wrong data, and a check that quietly skips a bad record keeps the bad record forever. The next page, on symptom fixes, is about defensive code placed exactly where something already failed.

The Response

Narrow it out, as Chapter 6 showed: keep the hunks that change the window and drop the policy object, its tests and the new gym setting. If the idea has merit, and per-gym windows might one day, file it as its own task with an owner and a product decision behind it. That task goes on its own branch, as Chapter 9 sets out.

For each defensive check, ask what condition it guards against and whether that condition can occur. Keep guards where data enters from outside the system: a request body, a file upload, a third-party response. Inside the codebase, trust the types and the schema, and let an impossible state fail loudly where it can be seen.

At Cragmoor the per-gym idea went to Ines as a single question in the review: does any gym want a different refund window? None did, and operations had no plans for one. The object was dropped in the narrowing, the gym setting with it, and the question was recorded on the team's board in case the answer ever changes. If it does, the feature will be specified, tested and decided as a feature, instead of arriving as a side effect of a change to one number.

Steering It Up Front

The cheapest place to stop an abstraction is before it exists. Say in the task, and in the project instructions file, "no new abstractions, configuration or dependencies without asking". Then ask for a plan that lists every new file, type and configuration key, as Chapter 5 described. A policy object that appears in a plan costs one line to strike out. The same object in a diff costs an hour to read and a decision to remove.

The line in Cragmoor's project instructions file reads: "Do not add new types, settings, configuration keys or dependencies unless the task asks for them; if you think one is needed, say so in the plan and stop." It is one of the shortest lines in the file and it saves more review time than any other, because it moves the decision from the diff, where it is expensive, to the plan, where it is a sentence.