ERRORS.md

AI Error Log

Use this file as a compact memory of recurring AI mistakes.

Rules

• One error per line
• Keep each line actionable and specific
• Use format: [YYYY-MM-DD] <scope>: <wrong> -> <right>
• Add only confirmed/recurrent mistakes and avoid duplicates

Entries

• [2026-02-22] functions: new or modified functions without JSDoc header -> always add JSDoc (description + @param for each arg + @returns for non-void return values and all async functions)
• [2026-02-22] tests: never patch code to pass a test -> if a test is wrong, fix the test; if logic needs refactoring, refactor it
• [2026-02-23] pr skill: stopping after gh pr ready -> always enter the monitor loop (wait CI → 3min grace → read feedback → iterate) until stop condition is met
• [2026-02-23] pr skill: skipping issue creation when none found -> always create a GitHub issue before opening a PR (gh issue create --web or via CLI)
• [2026-02-23] zod v4: z.string({ error: 'msg' }) does NOT override invalid_type message -> Zod v4 produces 'Invalid input: expected string, received number'; update test expectations accordingly and remove the no-op error option
• [2026-03-11] architecture: importing mongoose in controllers/services/helpers -> mongoose must only be imported in repositories and models
• [2026-03-11] architecture: controller importing a Repository directly -> controllers call Services only; cross-module access goes through the other module's Service
• [2026-03-11] services: wrapping return values in Promise.resolve() -> unnecessary when calling async/promise-returning functions, just return directly
• [2026-03-13] architecture: service importing another module's Repository -> use target module's Service
• [2026-03-13] architecture: middleware using mongoose.model() directly -> use module's Service or Repository
• [2026-03-13] tests: test file placed outside its module (e.g. lib/middlewares/tests/) -> tests belong in modules/{module}/tests/
• [2026-03-13] docs: duplicate doc files covering the same topic (e.g. MIGRATION.md + MIGRATIONS.md) -> single file, no duplication
• [2026-03-14] middleware: assuming config section exists in all environments (e.g. config.rateLimit) -> always handle missing config gracefully (passthrough/no-op); dev config often omits sections that only prod defines
• [2026-03-15] cross-stack: changing a Node API without checking Vue E2E tests -> when modifying an endpoint Vue consumes, run Vue E2E tests before pushing
• [2026-03-15] pr scope: batching multiple unrelated fixes in one PR -> one fix = one PR to isolate blast radius and reduce iteration loops
• [2026-05-05] repository: Repository.update(doc) doing new Model(doc).save() rewrites the full document from in-memory state, silently clobbering any concurrent partial update that landed after the read -> always use Model.updateOne({ _id }, { $set: ... }) or findOneAndUpdate({ _id }, { $set: ... }) for partial updates to avoid race conditions; see #3605
• [2026-05-31] billing/stripe: reading price.metadata.planId in customer.subscription.updated webhook handler -> field is EMPTY in real Stripe webhook payloads (planId lives on the Product, not the Price); use a priceId → plan map built at boot from config.stripe.prices instead; see #3742
• [2026-06-04] repository: top-level const Foo = mongoose.model('Foo') in a repository file -> this is evaluated at import time; safe in an HTTP server (loadModels() runs first) but silently crashes standalone scripts (crons, migrations) with MissingSchemaError when import order differs; tests miss it because jest mocks intercept the module entirely; fix = lazy getter const Foo = () => mongoose.model('Foo') (call sites: Foo().find(...)) or dynamic import after loadModels() in the entrypoint; see #3789
• [2026-06-15] deps/audit: leaving npm audit advisories unaddressed on the assumption they need a major bump -> run npm audit fix (never --force) first; the runtime-tree DoS/ReDoS items (qs, path-to-regexp, brace-expansion) all fixed via in-range bumps, no residual. These are DoS-class but NOT attacker-reachable in this stack: Express route patterns are static (no user-controlled path-to-regexp input) and qs/brace-expansion only parse server-side query strings under fixed code paths — still bump them to keep the tree clean and avoid scanner noise.
• [2026-07-16] security: users.repository.js findByIdAndUpdatePopulated() does .populate() with no .select(), so organizations.controller.js switchOrganization serialized the raw doc (password hash + OAuth tokens + reset/verification tokens) straight to the client; users.account.controller.js me() separately forwarded providerData (OAuth tokens) verbatim -> any endpoint returning a Mongoose user doc must go through UserService.removeSensitive() (whitelist, modules/users/utils/sanitizeUser.js) at the response boundary, never serialize req.user/a populated doc directly; a local-signup fixture's providerData defaults to {} so a naive falsy check won't catch this — seed a fake OAuth token in tests to prove the leak is actually closed; see #3963
• [2026-07-16] billing/stripe: the #3742 priceId-map fix (resolvePlan/buildPriceIdToPlanMap) was applied only to the webhook handler; billing.admin.service.js resolveStripePlan (admin force-sync, a DB WRITE path) and billing.reconcile.service.js resolveStripePlan (LOG-ONLY divergence check) kept their own copies reading only metadata.planId -> a paid org's Stripe subscription (which never carries metadata.planId) resolved to 'free' on admin sync (silently downgrading a paying org) and produced a false planMismatch alert on every reconcile run; fix = one shared resolver (modules/billing/lib/billing.planResolver.js) used by all three call sites, and a null-unresolved sentinel (never guess 'free') so the admin write path ABORTS instead of downgrading and the reconcile log path skips the comparison instead of alerting; see #3964
• [2026-07-16] architecture: users.service.js remove()'s sole-owner cascade called OrganizationsRepository.remove() directly instead of organizations.crud.service.js#remove(), silently skipping runOrganizationRemovedHandlers (the onOrganizationRemoved seam modules/tasks/tasks.init.js registers for org-scoped task cleanup) -> deleting an org must always route through the owning module's SERVICE method, never straight to its Repository, even for a "bare" cascade delete from another module — a direct-repository shortcut silently bypasses any registered removal hook; a stale cross-test fixture in tasks.integration.tests.js (an orphaned-but-still-existing task reused across unrelated test cases) was inadvertently relying on this bug and needed a properly isolated fixture once the cascade actually cleaned up; see #3965
• [2026-07-17] error handling: shared helpers (lib/helpers/mailer/index.js#sendMail, modules/audit/services/audit.service.js#log) wrapped the failing call in their own try/catch, logged a single generic line, and resolved null -> every caller's own context-rich .catch() (action/userId/orgId) became dead code on a real outage, since the promise never rejected; a central helper must let the underlying call's rejection propagate and let each CALL SITE own the "never break the main flow" .catch() with its own context — never swallow centrally just because most callers currently attach a catch; see #3966
• [2026-07-17] billing/stripe: #3964 fixed the admin (409 abort) and reconcile (skip-comparison) call sites of the shared plan resolver but left billing.webhook.service.js resolvePlan's ?? 'free' fallback in place -> a webhook for an existing paid org whose Stripe price is unresolvable (unmapped priceId, no valid metadata.planId — e.g. a manually-sold enterprise price) silently downgraded the org to 'free' on the next customer.subscription.updated/.created/invoice.payment_succeeded event; a webhook cannot 409-abort mid-flight like the admin path, so the fix instead retains the org's/subscription's already-loaded .plan (never a hardcoded 'free') and emits billing.webhook.plan_unresolved for manual review — only a brand-new subscription with zero prior plan reference (no DB row anywhere for that org) still defaults to 'free'; see #3970
• [2026-07-25] billing: forceRotateForPlanChange (week-quota snapshot rotation) was wired only into handleSubscriptionUpdated's plan-change block -> handleCheckoutCompleted and both branches of handleSubscriptionCreated never rotated the current-week quota snapshot after a mid-week plan activation, so an upgraded org kept the previous plan's weekly quota (often 0 on free→paid) until the next weekly reset; fixed by calling the same non-fatal, logged forceRotateForPlanChange(organizationId, { preserveUsage: true }) from all three activation call sites, and by making billing.usage.service.js incrementMeter read the live plan quota (already fetched for the snapshot write) instead of the potentially-stale updatedDoc.meterQuota for overflow decisions — mirrors the existing live-quota display fix in billing.controller.js; see #3988
• [2026-09-04] deps/engines: engines.node declared ">=22.0.0" while the committed lock was already npm-11-shaped, so npm ci under npm 10 (bundled by every Node 22.x) failed mid-install with a confusing Missing: conventional-commits-filter@6.0.1 from lock file error instead of a clean engines rejection -> a public package's engines.node/engines.npm must bound the exact npm major that wrote the lock, not just "a Node version that happens to work today"; pin the toolchain with .nvmrc + CI's setup-node reading it (node-version-file, not a floating lts/*) and add engine-strict=true to .npmrc so an unsupported toolchain fails fast at the engines check instead of at dependency resolution; see #4053
• [2026-09-04] docker: #4053 pinned the toolchain for CI and local installs, but the Dockerfile still copied package*.json, ran npm ci, and only then COPY . . — so .npmrc (engine-strict=true) landed AFTER the install, and FROM node:lts-slim floated independently of .nvmrc, so an unsupported base image inside Docker still reproduced the confusing lockfile error instead of a clean EBADENGINE, and the image could silently drift onto a newer Node major once it enters LTS -> a toolchain fail-fast fix must be verified in every install path, not just the one exercised by CI; copy .npmrc alongside package*.json before npm ci in every installing stage, and pin FROM to the .nvmrc major with a unit test checking the two against each other instead of a floating tag; see #4060
• [2026-09-04] error handling: eight call sites (auth.controller.js OAuth catch blocks, uploads.repository.js GridFS catch blocks) built AppError with details: err / details: err.details || err — a raw caught exception handed wholesale to the response layer — and getDescription (lib/helpers/responses.js) read details.message into the client-facing description field in EVERY environment, including production, so whatever a dependency's error object happened to carry (stack fragments, internal hostnames, driver taxonomy) decided what leaked -> curate details at each throw site to an explicit, deliberately-chosen field set (here: { message: err.message } — a short reason, nothing else) instead of forwarding the exception, AND separately production-gate every details-derived read (getDescription's string/array/.message resolution, plus auth.controller.js's oauthErrorRedirect — a SECOND, independent consumer of details.message that bypasses getDescription entirely by building its own redirect envelope by hand) — NOTE the "same way" wording below turned out to be only partially true, see the 2026-09-05 follow-up entry; curating the call site alone is not sufficient — the curated message still flows through the same details.message slot every consumer reads, so both the source (call sites) and every sink (each place that reads details.message) must be fixed together; see #4059
• [2026-09-05] error handling / review follow-up: #4059's review surfaced two gaps in the 2026-09-04 fix -> (1) "eight call sites" undercounted: users.images.controller.js updateAvatar forwarded a raw Multer error wholesale via details: req.multerErr (code/field/etc. included), the identical anti-pattern, curated the same way (details: { message: req.multerErr.message }); (2) oauthErrorRedirect was NOT gated "the same way" as getDescription as claimed — only its details.message read was production-gated, its title (err?.message || fallbackTitle) was not, so a future non-AppError with a dynamic .message could still leak in production (today's real producers are safe, but nothing enforced it) -> when a fix claims parity with an existing safeguard ("gated the same way"), verify EVERY read the safeguard covers has a matching gate, not just the one that prompted the fix; and re-grep the whole pattern (not just the files named in the originating issue) before trusting a count like "eight call sites" — see the two new tests added for oauthErrorRedirect's title gate and users.images.controller.js's curation; see #4059
• [2026-09-05] billing/error handling: billing.requireQuota.js's catch block extracted const details = err.details (for branching on the AppError sub-type) and then passed that extracted sub-object — not err itself — into responses.error(res, status, ...)(details) at all five call sites (402/429/503) -> responses.error reads error.details off whatever it is handed, so it read details.details, always undefined, silently dropping the whitelisted type/upgradeUrl payload from every response this middleware ever sent, in every environment; fix = pass err (keep the extracted details var only for the ?.type === branching). This also reshaped the dev-only payload.error blob: it now serializes the real AppError, so its curated fields moved from the blob's top level to nested under .details — eight existing tests across two files (billing.quota.unit.tests.js, billing.webhook.hardening.unit.tests.js) were asserting the old (buggy) flat shape and needed the same nested-path update; see #4062
• [2026-09-05] error handling: #4062's sweep of the responses.error(res, ...)(x) misuse (x not error-shaped, so x.details is undefined and payload.details never emits) covered billing.requireQuota.js only -> two more sites outside billing: analytics.requireFeatureFlag.js's 403 branch passed { type, flag } flat (no .details wrapper) — real signal loss, a client blocked by a feature flag got no machine-readable type in production; home.controller.js's degraded-health 503 branch passed the raw health payload directly — same wrong shape, harmless today only because nothing on that payload matches the whitelist. Fixed both by wrapping the payload in a real AppError's .details. Deliberately did NOT whitelist flag (an internal PostHog feature-toggle key) alongside type — publishing it would let a client enumerate which flags gate which routes; a grep of every remaining responses.error(res, ...)(x) call site in the repo found no further non-error-shaped x; see #4064
• [2026-09-19] billing/stripe: two of three terminal-cancel write sites left cancelAtPeriodEnd/cancelAt stale — handleCustomerDeleted wrote { plan: 'free', status: 'canceled' } only, and admin cancelSubscription never touched the fields at all -> handleCustomerDeleted detaches the doc from Stripe entirely (stripeSubscriptionId: null), so once detached getSubscription serves the stored doc as-is with no live Stripe re-read to mask the drift — a subscription canceled while cancelAtPeriodEnd was already true (a scheduled period-end cancel) kept that stale true forever. Fix: handleCustomerDeleted now explicitly resets both fields to false/null (a customer payload carries no subscription object, so there is nothing to mirror); admin cancelSubscription now mirrors the post-cancel Stripe retrieve, guarded so a failed retrieve writes neither key rather than fabricating one; handleSubscriptionDeleted now re-syncs both fields from the deleted event's Stripe object instead of leaving them untouched, so an out-of-band drift (e.g. an immediate cancel superseding an earlier scheduled one) no longer survives with a stale value. Never hardcode false on a mirror path — only the customer-deleted reset does that, and only because no subscription object exists there; see #4100
• [2026-09-24] billing/meter: overflow units (usage past meterQuota) are debited from extras, which may go negative, but the weekly reset only zeroed meterUsed and never touched that debt -> on a plan with a weekly quota the debt cut every later week by the same amount and locked the org out for good once it reached the quota; resetWeek now settles overflow debt once per week from the target week's REMAINING quota. Four traps on the way: (1) writing meterUsed = settle into the inserted week doc lost the settlement whenever the doc already existed (concurrent reset, incrementMeter first) -> fixed by charging the STORED credit (idempotent adjustment, refId settle:<weekKey>) with one guarded $inc keyed settle:<weekKey> in consumedAttributionKeys, on every call; (2) counting every refund entry as refund debt excluded overflow debt forever after any refund -> only the part of a refund that takes the replayed balance below zero counts, and a pack purchase repays it; (3) the below-zero part of a pack expiration is treated like refund debt: never settled from quota, only a new pack repays it (known limitation, unchanged: the expiry sweep removes a pack's full amount even when part was consumed), and the replay follows ledger array order (the $push commit order), not at; (4) capping settle at the plan quota assumed a fresh week, but the cron anchors on now, so the target week is usually already partly used -> the part of the credit pushing meterUsed past the quota was forgiven debt; now the week doc is read (or created) FIRST and settle = min(quota − meterUsed, overflowDebt), the rest stays as debt for the next reset; see #3914
• [2026-09-24] billing: billing.meter.service.js unitsFromCosts documented ratios.default as the per-key fallback (billing.config.zod.js JSDoc) but the code never read it, hardcoding 1 for any cost key absent from the plan's ratio map -> a plan configuring ratios: { default: 2 } silently billed every unlisted feature at 1, not 2; latent because the shipped example used default: 1, which happened to match the hardcoded fallback. Fix: compute the fallback once as ratios.default when it is a number >= 0, else 1, and use it in place of the literal 1; see #4025
• [2026-09-25] billing/extras: the expiry sweep (addExpirationEntries) removed a pack's FULL amount even when part or all of it was already consumed or refunded -> phantom debt the next pack paid twice. Now the ledger is replayed in array order, debits (and a pack's own refunds, matched by stripeSessionId) are attributed to live credits earliest-expiry-first (no-expiry credits last, uncovered debt repaid by the next credit), and an expiring pack removes only its own remainder at its expiresAt; a fully spent pack gets a zero-amount expiration marker (schemas allow 0 for that kind only, hidden from listLedgerPage) so the expire-<topupId> guard still holds. The write is one findOneAndUpdate guarded by the snapshot's ledger $size (append-only ⇒ unchanged length = unchanged ledger), retried on a concurrent write. Legacy full-amount entries are left as is; see #4120
• [2026-09-25] billing/stripe: billing.plans.service.js fetchPlansFromStripe fell back to the raw Stripe product id (product.metadata?.planId || product.id) when a product carried no planId metadata -> ANY active Stripe product (a one-time pack, a recurring product sold outside the plans catalogue via a Payment Link) advertised itself as a public plan via GET /api/billing/plans, often with a null price id; fix = filter to product.metadata?.planId truthy BEFORE mapping, drop the id fallback entirely — a product now needs metadata.planId to be listed. Left every OTHER metadata?.planId || fallback untouched (billing.planResolver.js, billing.webhook.service.js, billing.admin.service.js) since those resolve an EXISTING subscription's plan, not the public catalogue, and must keep working for a recurring product sold outside it; see #4113
• [2026-09-25] auth: oauthCallback never provisioned an org, and review of the first fix caught that gating on !user.currentOrganization would also fire for an EXISTING org-less user (removed from org, pending join) on every login -> gate on info.created instead (set by checkOAuthUserProfile's create branch, relayed via passport's verify-callback info), so only a genuine new signup provisions. oauthCallback also needed an outer try/catch (passport invokes it fire-and-forget) and a headersSent guard before any fallback redirect; see #4115
• [2026-09-25] billing: computing a percent-of-grant level as (1 - threshold/100) * signupGrant (e.g. 500 * (1 - 80/100)) lands on 99.99999999999997, not 100, due to float imprecision -> silently misses an exact post === level boundary crossing; use signupGrant * (100 - threshold) / 100 instead, which is exact at common values; see #4117
• [2026-09-28] billing/stripe: checkout.session.completed's subscription-retrieve catch returned silently on failure -> the event was recorded as processed by the idempotency wrapper while the subscription row stayed unlinked on the free plan, with no retry; fix = throw instead of return, so the claim persists (attempts increments, the doc is never deleted on failure) and Stripe redelivers; see #4151
• [2026-09-28] billing/meter: incrementMeter always billed against the subscription's own plan quota, while the admission gate (billing.quota.service.js) already treated a fail-closed status (paused/unpaid/incomplete/incomplete_expired/canceled) as the free plan -> a fail-closed org kept consuming its stale paid-plan quota on the meter even though the gate let it through on free-plan grounds; the fail-closed status list is now one shared constant imported by both the gate and the meter — a literal duplicated in two enforcement paths drifts by design, not by mistake; see #4151
• [2026-09-28] auth: verifyEmail read the user by token (getBrut) then wrote it in a separate step (update) -> two concurrent requests for the same token both passed the read check before either write landed, so both provisioned an organization/grant for the same signup; fix = one atomic findOneAndUpdate (UserService.consumeEmailVerificationToken) that verifies and clears the token together, so only the first concurrent caller can match the still-unexpired token; see #4151
• [2026-10-03] billing/mailer: the 3 billing email builders (quota, credit, payment-failed) built their link from config.app?.url ? ... : '', but no Devkit default config sets app.url -> every billing email rendered an empty href unless a downstream happened to set it itself (undetected because no default-config test asserted a non-empty link). Fix = config.app?.url || getBaseUrl(), the same fallback getBrand()'s url field already uses; app.url still wins when a project sets it. Also corrected billing.email.js's JSDoc, which described a template "glob-merge" override mechanism that was never implemented — the real resolver is config.mailer.templates; see #4128