The first blocking comment

The starter runs, you have seen the /plan surface in the browser, and reviews/chapter 104.md holds a <!-- TODO(L2) --> marker where your first comment goes. That comment is the missing authedAction wrapper on the plan-label mutation, written as a correctly-shaped blocking: finding.

Finding 1 is the worked example. You walk it end to end here so the cadence is set before you write the rest on your own. The deliverable is a single markdown comment under the pass-order header: it pins the defect to a file and line range, names the rule it breaks with a lesson ID, and proposes the fix. You will see the full block in Coding time.

Your mission

Open src/app/(app)/plan/actions.ts and read updatePlanLabel against the canonical wrapper in src/lib/authed-action.ts. The question to ask is: what is the established surface, and where does this bypass it?

The surface is authedAction(role, schema, fn). Every privileged Server Action in this codebase routes through it, so the role gate, input validation, tenant scope, and rate limit all live in one place. updatePlanLabel skips the wrapper. It carries 'use server' and a hand-rolled const session = await getSession(), which means it runs no role check (any signed-in member can rewrite the org’s plan label), reaches past the tenantDb facade to mutate the store directly, and writes with no rate limit. The if (!session) guard is a bonus tell: it is dead code, because getSession() returns a Session or throws, never null.

One bypass drops three guarantees the wrapper enforces, so this is blocking:, not a suggestion: style nit. A write any member can run against the wrong tenant is a security-and-correctness defect, and the severity has to say so.

Two things keep the comment portable. Write it about the code, not the author: “updatePlanLabel hand-rolls the session check” lands cleaner than “you hand-rolled it.” And name the violated rule with its lesson ID, since “this is wrong” alone is the failure mode chapter 103 warned about.

Work the cadence in order: read the file, find the bypass, name the principle and pattern, set the severity, write the comment from the template. Stay on this one finding. The missing audit-log write is in the same file, but that is finding 5, next lesson. And you never touch the source: the fix lives in the comment body, not in a diff.

The review file carries a comment pinned to the file and the line range of the hand-rolled session check in src/app/(app)/plan/actions.ts (L18-21).

untested

The comment carries a blocking: severity label, justified by the security-and-correctness consequence rather than preference.

untested

The observation names the bypass in code terms and the guarantees it drops: the role check and the tenant-facade scope (the dead if (!session) guard and the missing rate limit are fair extra tells).

untested

The comment cites SaaS pattern #2 (lesson 2 of chapter 57) and Principle #5 (chapter 29 / chapter 42) as the violated rule, with the lesson ID.

untested

The comment’s action proposes wrapping the mutation in authedAction('admin', updatePlanLabelSchema, fn) so the role, tenant, and rate-limit gaps close at once, in one sentence.

untested

The comment is phrased in the address-the-code-not-the-author voice.

untested

Coding time

Write comment 1 into reviews/chapter 104.md now — under the shipped pass-order header, in place of the <!-- TODO(L2) --> marker — against the brief above. Use reviews/template.md for the four-part shape. Read the walkthrough below only after your own attempt.

Reference solution and walkthrough

The defect, in source

Read the file the way the review does: find the seam it should lean on, and the three places it doesn’t.

'use server';

import { updateTag } from 'next/cache';
import { orgPlanEntitlementTag } from '@/lib/cache/tags';
import { err, ok, type Result } from '@/lib/result';
import { getSession } from '@/server/session';
import { findOrganization } from '@/server/store';
import type { Organization } from '@/server/types';

// The plan-label mutation. It rolls its own session check by hand, reaches past
// the tenant facade to mutate the org record directly, runs no role check and no
// rate limit, and records nothing to the compliance trail. It compiles, runs, and
// rewrites the label against the in-memory store — working but wrong. The proposed
// fixes live in the review, never in this file.
export const updatePlanLabel = async (
  formData: FormData,
): Promise<Result<Organization>> => {
  const session = await getSession();
  if (!session) {
    throw new Error('Not signed in');
  }

  const planLabel = String(formData.get('planLabel') ?? '');
  if (planLabel.length === 0) {
    return err('validation', 'Plan label is required.');
  }

  const org = findOrganization(session.orgId);
  if (!org) {
    return err('not_found', 'Organization not found.');
  }

  org.planLabel = planLabel;

  updateTag(orgPlanEntitlementTag(session.orgId));
  return ok(org);
};

The hand-rolled gate. getSession() plus if (!session) throw proves the caller is signed in and nothing more. With no role check, any member can rewrite the org’s plan label, not just an admin. And the guard is dead: getSession() returns a Session or throws, so !session is never true.

'use server';

import { updateTag } from 'next/cache';
import { orgPlanEntitlementTag } from '@/lib/cache/tags';
import { err, ok, type Result } from '@/lib/result';
import { getSession } from '@/server/session';
import { findOrganization } from '@/server/store';
import type { Organization } from '@/server/types';

// The plan-label mutation. It rolls its own session check by hand, reaches past
// the tenant facade to mutate the org record directly, runs no role check and no
// rate limit, and records nothing to the compliance trail. It compiles, runs, and
// rewrites the label against the in-memory store — working but wrong. The proposed
// fixes live in the review, never in this file.
export const updatePlanLabel = async (
  formData: FormData,
): Promise<Result<Organization>> => {
  const session = await getSession();
  if (!session) {
    throw new Error('Not signed in');
  }

  const planLabel = String(formData.get('planLabel') ?? '');
  if (planLabel.length === 0) {
    return err('validation', 'Plan label is required.');
  }

  const org = findOrganization(session.orgId);
  if (!org) {
    return err('not_found', 'Organization not found.');
  }

  org.planLabel = planLabel;

  updateTag(orgPlanEntitlementTag(session.orgId));
  return ok(org);
};

The unscoped write. org.planLabel = planLabel mutates the store record directly, reaching past the tenantDb(orgId) facade that enforces the org boundary in one place. The tenant scope is gone.

'use server';

import { updateTag } from 'next/cache';
import { orgPlanEntitlementTag } from '@/lib/cache/tags';
import { err, ok, type Result } from '@/lib/result';
import { getSession } from '@/server/session';
import { findOrganization } from '@/server/store';
import type { Organization } from '@/server/types';

// The plan-label mutation. It rolls its own session check by hand, reaches past
// the tenant facade to mutate the org record directly, runs no role check and no
// rate limit, and records nothing to the compliance trail. It compiles, runs, and
// rewrites the label against the in-memory store — working but wrong. The proposed
// fixes live in the review, never in this file.
export const updatePlanLabel = async (
  formData: FormData,
): Promise<Result<Organization>> => {
  const session = await getSession();
  if (!session) {
    throw new Error('Not signed in');
  }

  const planLabel = String(formData.get('planLabel') ?? '');
  if (planLabel.length === 0) {
    return err('validation', 'Plan label is required.');
  }

  const org = findOrganization(session.orgId);
  if (!org) {
    return err('not_found', 'Organization not found.');
  }

  org.planLabel = planLabel;

  updateTag(orgPlanEntitlementTag(session.orgId));
  return ok(org);
};

The missing shape. There is no authedAction wrap anywhere — no role gate, no parse-and-deny, no rate limit. (The missing audit write is finding 5; leave it.) One bypassed seam, several guarantees gone.

← Previous 1 / 1 Next →

The seam, for contrast

authedAction is the only privileged Server Action shape in this codebase. It resolves the session, checks the role, parses the input, then runs your function — all inside a try/catch that defaults to deny, so an unexpected error becomes a typed refusal instead of a 500.

export const authedAction =
  <TSchema extends z.ZodType, TOut>(
    role: Role,
    schema: TSchema,
    fn: (input: z.infer<TSchema>, ctx: AuthedCtx) => Promise<Result<TOut>>,
  ) =>
  async (
    _prev: Result<TOut> | null,
    formData: FormData,
  ): Promise<Result<TOut>> => {
    try {
      const session = await getSession();

      if (!roleAtLeast(session.role, role)) {
        return err('forbidden', 'You do not have permission to do this.');
      }

      const parsed = schema.safeParse(Object.fromEntries(formData));
      if (!parsed.success) {
        return err(
          'validation',
          'Check the highlighted fields.',
          z.flattenError(parsed.error).fieldErrors as Record<string, string[]>,
        );
      }

      return await fn(parsed.data, {
        session,
        orgId: session.orgId,
        userId: session.userId,
        role: session.role,
      });
    } catch {
      return err('internal', 'Something went wrong. Please try again.');
    }
  };

Every guarantee updatePlanLabel drops is a line you can point at here: the roleAtLeast gate it skips, the typed Result refusals its bare throw skips, the tenantDb-scoped write its direct mutation skips. That is why the fix is to route through this seam, not to bolt a role check onto the hand-rolled version. For the wrapper’s full job, the role taxonomy, and the tenant-scoped facade, lean on the authedAction lesson in chapter 57.

The comment

This is the completed comment block as it lands under the pass-order header in reviews/chapter 104.md.

**blocking:** `src/app/(app)/plan/actions.ts` L18-21 — `updatePlanLabel` hand-rolls `getSession()` with an `if (!session) throw`, so it accepts any signed-in user (no role check), drops the tenant scope on the org update, and runs a write-side mutation with no rate limit.
Principle/pattern: SaaS pattern #2 (lesson 2 of chapter 057 — `authedAction(role, schema, fn)`) and Principle #5 use-framework-conventions (chapter 029 / chapter 042).
Action: replace the manual auth and parse with `authedAction('admin', updatePlanLabelSchema, async (input, ctx) => { ... })`, which closes the role, tenant, and rate-limit gaps in one named seam.

Three decisions worth pausing on.

Wrap in authedAction('admin', ...), don’t add a role check by hand. The wrapper closes the role gate, the parse-and-deny path, the tenant scope, and the rate limit in one seam. Adding only a role check to the hand-rolled version rebuilds a bespoke copy of a seam the codebase already owns, free to drift from it — exactly what Principle #5 warns against. The role is 'admin' because a plan-label change is an administrative mutation, not a member-level one.

Naming the dropped guarantees calibrates the severity. The comment does not say “this looks unsafe.” It names three concrete guarantees the wrapper would enforce: no role check, tenant scope dropped, no rate limit. That list is the difference between blocking: and suggestion:, and the author can verify each claim against the seam. The dead if (!session) guard rides along as the tell that tipped you off, but the missing guarantees are what block.

The Action: proposes the move, not the patch. authedAction('admin', updatePlanLabelSchema, async (input, ctx) => { ... }) names the wrapper, the role, and the schema, and leaves the body to the author. Spelling out every line inside the arrow function writes their code for them. Name the move; the author writes the diff.

For the four-part comment anatomy, the five severity labels, and the address-the-code-not-the-author reflex, see the comment lesson in chapter 103.

Conventional Comments

conventionalcomments.org

The label-plus-decoration format this comment follows — blocking:, suggestion:, and the one-subject shape.

Google eng-practices — How to write code review comments

google.github.io

The address-the-code-not-the-author rule and severity labeling, from Google's reviewer guide.

Moment of truth

This chapter ships no automated checker: lesson-verification/ is empty, there is no pnpm test:lesson, and the deliverable is prose you grade by hand. First check the four-part anatomy. The comment is pinned to a file and a line range, and carries all four parts: a severity label, an observation, a Principle/pattern: line, and an Action: line.

Then hand-check the parts that take judgment, ticking each off as you confirm it:

The severity is blocking:, not suggestion:. A correct defect with the wrong severity still drops the severity half, even when the finding is located perfectly.

untested

The observation names both dropped guarantees, the role gate and the tenant scope, not just one. The dead if (!session) guard and the missing rate limit are fair extra tells, but the role and tenant gaps are load-bearing.

untested

The cited rule is SaaS pattern #2 with its lesson ID, and the action proposes the authedAction('admin', ...) wrap rather than a hand-added role check.

untested

The comment addresses the code, not the author: “updatePlanLabel hand-rolls the session check,” never “you hand-rolled it.”

untested

The reference deliverable lives under solution/reviews/chapter 104.md, but you are on the honor system: don’t open it until you have written both the review and the ADR in the ADR lesson at the end of this chapter. A real PR review has no answer key, and running the pass without peeking is what trains the reflex.