Agent skills I actually use, and why review got expensive

The problem

I run agents constantly: scaffolding, refactors, tests, the boring half of a feature. I'm not anti-AI, and I'm not writing another "AI code is unmaintainable" post.

The failure mode is narrower and more annoying. The output looks done. Types check, tests are green, the PR summary sounds confident. A week later you open the diff and the hard parts are missing - why four different failures collapsed into one false, who owns this state under concurrency, what a comment is doing if the name already says it, whether the test would still catch the bug it claims to guard.

One file is manageable. A week of agent-assisted work across a real project already feels like a tax. I got tired of paying it, so I put together skills for intentional code.

What "review cost" means here

Not "is the code correct in the abstract," but how long it takes a human to trust it: read the names, reconstruct the failure modes, check the concurrency story, decide whether the tests prove anything. That time adds up when the author optimizes for looking finished.

Same instinct as the security-maintenance cost I wrote about for frameworks. The concrete question is simple: if I hand this diff to another developer - or to myself in three months - can they review it without rebuilding intent from scratch?

A few examples that keep showing up

These are shortened from the pack's before/after pages. The full versions are in the repo; here is the shape of the problem.

One false for every failure

A wallet debit path that "works," but hides every outcome behind the same return:

export function process(d: any, flag = true, mode = 0): any {
  try {
    if (mode !== 1) return false;
    if (
      typeof d.uid !== "string" ||
      typeof d.amt !== "number" ||
      d.amt <= 0 ||
      !(d.uid in wallets)
    ) {
      return false;
    }

    const bal = wallets[d.uid] - d.amt;
    wallets[d.uid] = bal;
    if (flag) notifyProvider(d.uid, -d.amt);
    return bal;
  } catch {
    return false;
  }
}

Invalid amount, missing wallet, wrong mode, notify failure after a successful debit - all look the same from outside. A rename to updateWallet does almost nothing if the failure model stays collapsed.

What I want from the agent instead is a clear domain operation for new callers, with distinct failures, and the old process kept as a thin adapter if compatibility matters:

export function debitWallet(
  userId: string,
  amount: number,
  opts: { notify?: boolean } = {},
): number {
  if (amount <= 0) throw new InvalidDebit(amount);
  if (!(userId in wallets)) throw new WalletNotFound(userId);

  const balance = wallets[userId] - amount;
  wallets[userId] = balance;

  if (opts.notify ?? true) {
    try {
      notifyProvider(userId, -amount);
    } catch (cause) {
      // Debit already committed - don't hide that behind a generic false.
      throw new NotifyFailedAfterDebit(userId, balance, cause);
    }
  }

  return balance;
}

That is the kind of change jane-street-style is for: not prettier names alone, but an API a reviewer can actually reason about.

Comments that restate the code

Agents love this pattern:

// Validate the request.
if (!req.productId || req.qty <= 0) return false;

// Get the stock.
const stock = inventory[req.productId] ?? 0;

// Charge the user.
charge(req.userId, req.unitPrice * req.qty);

None of that helps. What does help is the stuff names cannot carry - policy, ordering, external constraints:

// force is the operations override for VIP oversell.
if (stock < req.qty && !opts.force) return false;

// Reserve before charge so concurrent checkouts cannot both pass the
// stock check. Charge failure restores the accepted brief over-hold.
inventory[req.productId] = stock - req.qty;

// Stripe webhook delivery can race the read model; see #1842.
scheduleConfirm(req.orderId, { afterMs: 2000 });

That is essential-comments: keep the why, delete the narration.

Green tests that prove almost nothing

it("processTransfer calls its dependencies", async () => {
  wallets.get.mockResolvedValue({ id: "w1", balance: 100 });
  await processTransfer({ from: "w1", to: "w2", amount: 40 }, { wallets, notify });
  expect(wallets.get).toHaveBeenCalledWith("w1");
  expect(wallets.save).toHaveBeenCalled();
  expect(notify).toHaveBeenCalled();
});

This can stay green while the wrong amount moves, only one side is saved, or notification failure invents a rollback. What I actually want looks more like:

it("keeps committed balances when notification fails", async () => {
  const wallets = memoryWallets({ w1: 100, w2: 10 });
  const notify = vi.fn().mockRejectedValue(new Error("provider down"));

  const result = await processTransfer(
    { from: "w1", to: "w2", amount: 40 },
    { wallets, notify },
  );

  expect(result).toEqual({
    ok: false,
    error: "notify_failed",
    committed: true,
  });
  expect(wallets.snapshot()).toEqual({ w1: 60, w2: 50 });
});

That is tests-that-matter. Green is not confidence.

What's in the pack

The repo is public: github.com/yakoshiq/skills. Install is one command:

npx skills add yakoshiq/skills

Works with Pi, Claude Code, Cursor, Codex, and other agents that support skills. In the picker, Intentional Code is the full set. There are five skills on purpose - short files, guidance that transfers across tasks:

jane-street-style - semantic clarity when that is the goal: domain names, honest failures, useful types. Follow the language and the repo; don't transplant a functional religion.
essential-comments - keep why / invariants / tradeoffs / external constraints. Delete narration and leftover AI comments.
concurrency-invariants - name the guarantee before choosing a mutex, retry, transaction, or idempotency key. Ownership, commit points, replay, interruption. A timeout is usually an unknown outcome unless the API proves otherwise.
surgical-changes - smallest coherent change that fully solves the request. Required and coupled edits only; adjacent cleanup stays out of the diff.
tests-that-matter - prove observable behavior, failure semantics, invariants, boundaries.

Before/after pages live outside the skill folders so installed skills stay model-facing. I also ran the pack across a few current models - DeepSeek, MiMo, GLM, Qwen, MiniMax, Kimi, GPT among them - and trimmed anything that only "worked" on one favorite. That does not make them universal. It just makes them less lab-specific.

How I actually use them

Auto-trigger from the description alone depends on the model. Some pick the skill up, some don't. When the task is exactly one job, I prefer an explicit invoke: /skill:essential-comments, or "use surgical-changes" in the request.

Rough map:

vague domain API, collapsed errors, primitive soup → jane-street-style
noisy comments, AI residue, missing whys → essential-comments
queues, workers, retries, websockets, races → concurrency-invariants
small fix, leave the neighborhood alone → surgical-changes
coverage without confidence, mock-heavy suite → tests-that-matter

I still read the diff. Skills mostly change what I trip over: fewer narration comments, fewer success: boolean APIs, fewer tests that only assert a mock was called. Sometimes the hard question is only named and still wrong - that is already better than silent mush, and cheaper to fix.

A skill will not invent taste or a review culture. Wrong task stays wrong. If nobody will read the code, skip the pack and ship. If the model half-ignores the skill, treat it as a bias, not a contract.

Why I published it

I did not put this out because I'm scared agents will replace craft. Generation is cheap. What stays expensive is review, trust, and the ability to change the code later without archaeology.

If that sounds useful:

skills.yakoshi.dev · github.com/yakoshiq/skills

npx skills add yakoshiq/skills