diff --git a/src/app/actions/log-work.ts b/src/app/actions/log-work.ts index 7e94893..39a6a29 100644 --- a/src/app/actions/log-work.ts +++ b/src/app/actions/log-work.ts @@ -15,21 +15,35 @@ const MAX_MESSAGE_LENGTH = 2000; const MAX_HOURS = 24; const ISO_DATE = /^\d{4}-\d{2}-\d{2}$/; -/** See the security note at the top of src/app/actions/cards.ts. */ +/** See the security note at the top of src/app/actions/cards.ts for why a + * role check like this matters — unlike cards.ts, this file writes through + * the superuser connection (see requireOwnerPb below), so this check is not + * backstopped by PocketBase's own rules the way a cards.ts write is. */ async function requireOwner() { const session = await requireSession(); if (session.role !== "owner") throw new Error("Not found"); return session; } +/** + * The owner's superuser connection — never obtainable without passing the + * owner check above first. getAdminPb() bypasses every collection rule and + * checks nothing itself ("Every call site MUST have already checked + * authorization itself" per its own doc comment); this is the only place in + * this file that imports it, so there is no code path here that can reach it + * without going through requireOwner() first. + */ +async function requireOwnerPb() { + await requireOwner(); + return getAdminPb(); +} + /** * Log a block of off-repo work as a commits row. * - * Writes through the superuser connection, not the owner's own token — - * `commits.createRule` is `null` (superuser-only) because every OTHER row in - * this collection comes from the ingestion pipeline, which already writes - * this way. The owner-only check above is what stands in for a collection - * rule here, exactly as it does for every client-manager write in this app. + * Never throws — every failure, including "not signed in" and "wrong role", + * comes back as { ok: false, message }, so the calling form only ever has + * one shape to handle. */ export async function logWorkAction(input: { projectId: string; @@ -40,62 +54,110 @@ export async function logWorkAction(input: { message: string; cardId?: string; }): Promise<{ ok: boolean; message?: string }> { - await requireOwner(); + let pb; + try { + pb = await requireOwnerPb(); + } catch { + return { ok: false, message: "Not found" }; + } const message = input.message.trim().slice(0, MAX_MESSAGE_LENGTH); if (!message) return { ok: false, message: "A message is required" }; if (!Number.isFinite(input.hours) || input.hours <= 0 || input.hours > MAX_HOURS) { - return { ok: false, message: `Hours must be between 0 and ${MAX_HOURS}` }; + return { + ok: false, + message: `Hours must be greater than 0 and at most ${MAX_HOURS}`, + }; } if (!ISO_DATE.test(input.date)) { return { ok: false, message: "Invalid date" }; } - const pb = await getAdminPb(); const cardId = input.cardId ?? ""; - await pb.collection("commits").create({ - project: input.projectId, - // Guaranteed not to collide with a real 40-character git sha, and unique - // enough to satisfy the (project, sha) index every other commit relies on. - sha: `manual-${randomUUID()}`, - short_sha: "manual", - message, - subject: message, - type: "", - scope: "", - author_name: "", - author_email: "", - authored_at: `${input.date} 12:00:00.000Z`, - is_merge: false, - additions: 0, - deletions: 0, - card: cardId, - // Reuses the existing meaning of "manual" on link_source: a card link the - // owner chose by hand, not one the linking rules derived. - link_source: cardId ? "manual" : "none", - url: "", - summary: message, - summary_version: 0, - summary_attempts: 0, - source: "manual", - manual_minutes: Math.round(input.hours * 60), - }); + if (cardId) { + // A card id from a different project would count toward THIS project's + // metrics while appearing in another project's card history — the same + // mismatch src/lib/data/card.ts guards against for the same reason. + const card = await pb + .collection("cards") + .getOne(cardId) + .catch(() => null); + if (!card || card.project !== input.projectId) { + return { ok: false, message: "That card does not belong to this project" }; + } + } - // So the dashboard's KPIs reflect it on the very next load, not only after - // the owner separately clicks Recompute. - await recomputeMetrics(input.projectId); + let created: CommitRecord; + try { + created = await pb.collection("commits").create({ + project: input.projectId, + // Guaranteed not to collide with a real 40-character git sha, and + // unique enough to satisfy the (project, sha) index every other + // commit relies on. + sha: `manual-${randomUUID()}`, + short_sha: "manual", + message, + subject: message, + type: "", + scope: "", + author_name: "", + author_email: "", + authored_at: `${input.date} 12:00:00.000Z`, + is_merge: false, + additions: 0, + deletions: 0, + card: cardId, + // Always "manual", whether or not a card is linked — this is a human + // deliberately deciding the card association (including deciding not + // to have one), and must be inert to the scope/reference auto-linking + // rules the way a real commit's link is inert once it is "manual". + // Leaving an unlinked entry as "none" would let incidental text in + // the owner's note (e.g. a stray "#12") silently attach it later. + link_source: "manual", + url: "", + summary: message, + summary_version: 0, + summary_attempts: 0, + source: "manual", + manual_minutes: Math.round(input.hours * 60), + }); + } catch { + return { ok: false, message: "Could not save that entry" }; + } + + try { + // So the dashboard's KPIs reflect it on the very next load, not only + // after the owner separately clicks Recompute. + await recomputeMetrics(input.projectId); + } catch { + // The entry is already saved — only the cached KPIs are stale. Do NOT + // report this as a failed log: the synthetic sha is fresh on every call, + // so a retry after a false "failed" would create a duplicate entry + // rather than being caught by the (project, sha) unique index. The + // Recompute button exists for exactly this case. + } revalidatePath(`/p/${input.clientSlug}/${input.projectSlug}/dashboard`); + if (cardId) { + revalidatePath(`/p/${input.clientSlug}/${input.projectSlug}/c/${cardId}`); + } else { + revalidatePath(`/p/${input.clientSlug}/${input.projectSlug}/unassigned`); + } + return { ok: true }; } export async function polishNoteAction(input: { text: string; }): Promise<{ ok: boolean; text?: string; message?: string }> { - await requireOwner(); + try { + await requireOwner(); + } catch { + return { ok: false, message: "Not found" }; + } const text = input.text.trim().slice(0, MAX_MESSAGE_LENGTH); if (!text) return { ok: false, message: "Nothing to polish yet" };