Skip to content

Review of Codex's 2026-06-26→29 commits — overlooked-bug sweep (WOPC voided-PDF fall-through + coaching sessionDate + footgun guard)

Why

Owner, 2026-06-29 (verbatim): "I needed to have Codex (OpenAI agent) to continue the work as I was out of token here on Claude. Can you look thru the GitHub commits, look at the work done, fix possible bugs that were overlooked, and get yourself updated on the current state of the web app as well."

Reviewed every Codex-authored commit since c0feb79a (T-129 coaching-invoice display tokens, T-130 production Firestore getAll/bundling crash, T-131 Bank-Tx period scoping, and the 06-28/29 wopc/ir56m/signing hardening batch 8825c6a2…df362b14). Three parallel reviewers + direct verification. Baseline health: tsc --noEmit clean, accounting vitest 127/127, Next 15.5.19.

What was verified clean (no action)

  • T-130 bundling fix is correct. serverExternalPackages is the right key for Next 15 (not the Next-14 experimental.serverComponentsExternalPackages), placed top-level, survives the Sentry/bundle-analyzer wrappers, and the Dockerfile ships full node_modules + next start so the externalized packages resolve at runtime. All five docRef.get() → where(documentId()).limit(1) swaps are semantically faithful (empty-case via .empty, correct .docs[0].id/.data(), direct collections so documentId() matches the bare ID). Auth profile read is fail-closed. getAll → Promise.all(refs.map(r => r.get())) preserves order + missing-doc semantics + empty-refs.
  • The signing-authorization broadenings (T-13x) fail closed. "Director-only signing access" / "allow signer access" bind the authenticated caller's email to the assigned director's loginEmails; unauthenticated callers still 401; the signing-only NextAuth session is role: pending + empty subsidiaryIds + profileMissing and is denied every general guarded route. The new ir56m-signing/[requestId]/signed-pdf.ts enforces auth before streaming bytes.
  • T-131 did not revert T-119. The flex Transaction-column width logic, btScrollX, ResizeObserver, aote.colw.bankTx.v1 persistence, and the onCell overflow are all intact; period fallback, single-source pagination (default 100), and React-Query deps are correct.

Findings + fixes

Fixed

  1. Coaching {{sessionDate}} rendered blank when invoice.sessionDate was a string — fixed separately as the T-129 follow-up (commit 0107d3ff). Tolerant per-candidate date parser replaces the ||-then-.toDate pattern. (Cross-linked here for completeness.)
  2. Voided WOPC could download as a "sealed" signed PDF (integrity, LOW but real). In lib/wopc/resolvePdfBytes.server.ts and pages/api/records/wopcs/pdf.ts, a voided WOPC (void.driveFileId set) whose voided Drive fetch throws fell through to the signed branch — which df362b14 made reliably resolve pdf.signedFileId (voidWOPC never clears it) — and served the superseded signed copy labelled "sealed". A voided document presented as validly signed. Fix: void is now terminal — on void-fetch failure, resolveWopcArchivedPdf returns null and wopcs/pdf.ts skips the signed branch (&& !voidedDriveFileId), both falling through to the on-demand render of the current (voided) state. Happy path (void file fetchable) unchanged.
  3. Legacy pdf.fileId decoupled from signature state (latent hardening). Same two files read pdf.signedFileId ?? pdf.fileId. pdf.fileId (legacy migrated shape) only meant the signed copy when actually signed; gated it on signature.at so a future unsigned doc carrying pdf.fileId can't be served sealed.
  4. next.config.ts footgun comment. Added a DO-NOT-REMOVE note: the signing write/transaction paths still depend on the serverExternalPackages externalization (T-130's query rewrites cover reads only), so removing it would re-crash signing actions even though page reads survive.

Flagged, NOT fixed (need owner intent — no behavior change made)

  1. IR56M inner-wizard guard dead line (LOW, fails closed). app/ir56m/sign/[requestId]/guard.server.ts:45 is redundant (line 46 if (!(isAssignedDirector || isAdmin)) notFound() already covers it), and it neutralizes an apparent intent to let subsidiary-RBAC viewers into the inner wizard — page.tsx was opened to viewers but every inner step still 404s them. Net: a viewer opens the IR56M entry page, clicks a step, gets an unexplained 404. WOPC did this correctly via an explicit requireSigner option. Needs owner decision: should plain viewers reach the IR56M signing inner steps at all? If no, the inconsistency is harmless dead code; if yes, it needs the WOPC requireSigner pattern. Left untouched to avoid guessing on an auth surface.
  2. ir56m-signing/[requestId]/signed-pdf.ts has no isAssignedDirector || isAdmin second gate (unlike chop-image/preview-signed), so any subsidiary-RBAC user can view a signed IR56M PDF. Reviewer judged this consistent with records-viewing semantics (the signed IR56M is a record), but it is salary data — flagging for the owner to confirm a plain viewer should see it.

Decision log

2026-06-29 — reviewed + fixed (Accounting (Infrastructure))

  • ✅ Attestation (Accounting (Infrastructure)): read AGENTS.md; owner-requested review of another agent's (Codex) merged work. Fixes touch shared signing/records read paths but no data/schema; the flagged items (5, 6) are left for owner intent rather than edited unprompted. Branch signal main 🟢 · nightly 🔴 → main only; mirrored to taskboard.
  • Source: Accounting (Infrastructure) · https://claude.ai/code/session_015P6KzVYsQCLgEmUjR9bMwM
  • Verification: tsc --noEmit clean; accounting vitest 127/127. Render/read-path only; no Firestore writes, no schema change, no signing state-machine change.
  • Blast radius: WOPC PDF byte resolution (records preview + /api/docs/view + /api/records/wopcs/pdf) now treats a void as terminal; one build comment in next.config.ts.

2026-06-29 — owner ruled on the two flagged items; both now fixed (Accounting (Infrastructure))

  • ✅ Attestation (Accounting (Infrastructure)): read AGENTS.md; the owner answered the two access-surface questions via AskUserQuestion, so finishing items 5 & 6 (no longer guessing).
  • Source: Accounting (Infrastructure) · https://claude.ai/code/session_015P6KzVYsQCLgEmUjR9bMwM
  • Item 6 (signed-pdf viewer access) → LOCK DOWN. Owner: restrict the signed IR56M. Added the sibling second gate to pages/api/ir56m-signing/[requestId]/signed-pdf.ts: isAssignedDirector || isAdminLikeRole(role). Kept isAdmin (not literally director-only) on purpose: signed-pdf is called only from components/records/IR56MFilingsTab.tsx, the Records IR56M tab — a director-exclusive lock would 404 the super-admin owner out of viewing signed filings there, and would be inconsistent with the sign/preview-signed/reject siblings that already allow admin. So a general subsidiary viewer (the actual leak) is now blocked; the assigned director + admins/owner retain access.
  • Item 5 (IR56M wizard entry inconsistency) → DIRECTORS/ADMINS ONLY. Owner: gate the entry to match the inner steps. app/ir56m/sign/[requestId]/page.tsx now 404s anyone who is not the assigned director or an admin (was: any subsidiary-access user could load the entry then 404 on every inner step). Removed the dead redundant pre-check + the now-unused canAccessSubsidiary import in both page.tsx and guard.server.ts. Note: this removes the prior read-only "not authorised" banner path for non-director subsidiary viewers — they now 404 at the entry, per the owner's "directors/admins only" choice.
  • Verified: tsc --noEmit clean. Auth-surface change; no data/schema/state-machine change.
  • Status: all of items 1–6 now closed.