Merge pull request #148 from CodeForPhilly/chore/spec-drift-fixes

fix: resolve spec-drift audit findings (code + specs)

Author: themightychris

apps/api/src/routes/people.ts

@@ -122,7 +122,31 @@ export async function peopleRoutes(fastify: FastifyInstance): Promise<void> {
       tags: ['people'],
       summary: 'Update profile',
       params: { type: 'object', properties: { slug: { type: 'string' } }, required: ['slug'] },
-      body: { type: 'object' },
+      // Enumerate the editable fields and reject anything else, so privileged
+      // fields (e.g. accountLevel — see POST /api/people/:slug/account-level)
+      // can't be smuggled through the generic profile PATCH. Per
+      // specs/api/people.md → "PATCH /api/people/:slug".
+      body: {
+        type: 'object',
+        additionalProperties: false,
+        properties: {
+          fullName: { type: 'string' },
+          firstName: { type: ['string', 'null'] },
+          lastName: { type: ['string', 'null'] },
+          bio: { type: ['string', 'null'] },
+          slug: { type: 'string' },
+          email: { type: 'string' },
+          slackHandle: { type: ['string', 'null'] },
+          tags: {
+            type: 'object',
+            additionalProperties: false,
+            properties: {
+              topic: { type: 'array', items: { type: 'string' } },
+              tech: { type: 'array', items: { type: 'string' } },
+            },
+          },
+        },
+      },
     },
   }, async (request) => {
     const { slug } = request.params as { slug: string };
@@ -142,29 +166,6 @@ export async function peopleRoutes(fastify: FastifyInstance): Promise<void> {
     return ok(await fastify.services.people.get(result.value.person.slug, caller));
   });
 
-  // DELETE /api/people/:slug (admin-only soft-delete — legacy, kept for backward compat)
-  fastify.delete('/api/people/:slug', {
-    schema: {
-      tags: ['people'],
-      summary: 'Soft-delete a person (admin only, legacy)',
-      params: { type: 'object', properties: { slug: { type: 'string' } }, required: ['slug'] },
-    },
-  }, async (request, reply) => {
-    const { slug } = request.params as { slug: string };
-    const result = await fastify.store.transact(
-      buildTransactionOptions({
-        request,
-        action: 'person.soft-delete',
-        subjectType: 'person',
-        subjectSlug: slug,
-        responseCode: 204,
-      }),
-      async (tx) => fastify.services.peopleWrite.softDelete(tx, slug, request.session),
-    );
-    result.value.stateApply.apply(fastify.inMemoryState, fastify.fts);
-    return reply.code(204).send();
-  });
-
   // POST /api/people/:slug/deactivate (self | staff)
   // Spec: specs/behaviors/person-lifecycle.md, specs/api/people.md
   fastify.post('/api/people/:slug/deactivate', {

apps/api/src/services/person.write.ts

@@ -166,31 +166,6 @@ export class PersonWriteService {
     return { person: updated, stateApply };
   }
 
-  async softDelete(
-    tx: DualStoreTx,
-    slug: string,
-    session: SessionContext,
-  ): Promise<{ stateApply: StateApply }> {
-    const existing = this.#personOrThrow(slug);
-    requireAuth('administrator', { session });
-
-    if (existing.deletedAt) {
-      return { stateApply: new StateApply() };
-    }
-
-    const now = nowIso();
-    const updated: Person = PersonSchema.parse({
-      ...existing,
-      deletedAt: now,
-      updatedAt: now,
-    });
-
-    await tx.public.people.upsert(updated);
-
-    const stateApply = new StateApply().upsertPerson(updated);
-    return { stateApply };
-  }
-
   /**
    * Deactivate — self-service or staff.
    * Sets `deletedAt = now()`. The person can still sign in and reactivate.

apps/api/src/store/commit-meta.ts

@@ -8,6 +8,7 @@
  * The User-Ip / User-Agent / Authorization / Cookie headers are deliberately
  * NOT included — the public commit log is forever-public.
  */
+import { STATUS_CODES } from 'node:http';
 import type { Author, TransactionOptions } from 'gitsheets';
 import type { FastifyRequest } from 'fastify';
 import type { SessionContext } from '../auth/middleware.js';
@@ -61,6 +62,9 @@ export function buildTransactionOptions(ctx: CommitContext): TransactionOptions
     Host: request.hostname || 'unknown',
     'Content-Type': String(request.headers['content-type'] ?? 'unknown'),
     'Response-Code': String(responseCode),
+    // HTTP reason phrase for the response code, per
+    // specs/behaviors/storage.md#commit-message-shape.
+    'Response-Message': STATUS_CODES[responseCode] ?? 'Unknown',
   };
 
   if (subjectType) trailers['Subject-Type'] = subjectType;

plans/spec-drift-fixes.md

@@ -0,0 +1,76 @@
+---
+status: done
+depends: []
+specs:
+  - specs/behaviors/storage.md
+  - specs/api/people.md
+  - specs/api/conventions.md
+  - specs/behaviors/legacy-id-mapping.md
+issues: []
+pr: 148
+---
+
+# Plan: spec-drift audit fixes
+
+## Scope
+
+A spec-drift audit (two scoped `spec-drift-auditor` runs) compared `specs/`
+against the implementation. This plan records the resolution of the real
+findings — and the false positives that were verified away rather than
+"fixed."
+
+## Real findings fixed
+
+**Code (apps/api):**
+
+- **Removed the legacy `DELETE /api/people/:slug` route + `softDelete` service
+  method.** It was an undocumented admin-only soft-delete running parallel to
+  the canonical self|staff `POST /deactivate` — two soft-delete paths with
+  diverging authz. Superseded by deactivate/reactivate/purge; deleted.
+- **`commit-meta.ts`: added the `Response-Message` trailer** (HTTP reason
+  phrase via `node:http` `STATUS_CODES`) — `storage.md#commit-message-shape`
+  required it but only `Response-Code` was emitted.
+- **`PATCH /api/people/:slug`: tightened the body schema** to enumerate the
+  editable fields + `additionalProperties: false`, so privileged fields (esp.
+  `accountLevel`) are rejected (422) rather than silently ignored — hardening
+  the "account-level only via its dedicated endpoint" rule.
+
+**Specs/docs:**
+
+- **`storage.md` public/private contradiction** — ground-truthed the repo
+  visibility (`codeforphilly-data` is **private** on GitHub today). Reconciled:
+  it's a private repo holding **public-by-design** content; the dangerous table
+  claim that it "contains emails, real names, IPs" (contradicting the whole
+  redaction section) was corrected — PII lives in the private store.
+- `people.md` Person response shape: documented `slackHandle` and `deletedAt`
+  (both returned by the serializer; `deletedAt` gated to self/staff).
+- `people.md`: clarified `?accountLevel=` returns an empty 200 to non-staff
+  (not 403); corrected the PATCH note to point at the dedicated admin endpoint.
+- `legacy-id-mapping.md`: tightened the `byLegacyId` claim — runtime indices
+  exist only for people/projects/blog-posts; tags/buzz/updates carry `legacyId`
+  for import idempotence only.
+- `storage.md`: fixed the `scrub-data.ts` path; clarified that commit bodies
+  carry only the caller `summary` (request-body PII redaction is moot).
+- `conventions.md`: marked Idempotency-Key as built-but-not-yet-wired (status
+  note) — the actual per-route wiring is a follow-up.
+
+## Verified false positives (NOT changed)
+
+- **`reloadInMemoryState` vs `swapPublic`** — the hot-reload path *does* call
+  `store.swapPublic` (`reload.ts:76`); the spec was correct. Auditor only read
+  `internal.ts`.
+- **`perPage` out-of-range "returns 400"** — this app remaps Fastify validation
+  errors to **422** (`errors.ts:179`). Spec is correct.
+- **account-level `Previous-Account-Level: unknown` "race"** — the 404 throws
+  inside the transaction before any write, so no commit (or trailer) persists.
+
+## Validation
+
+- [x] `type-check` + `lint` clean.
+- [x] people-account-level 10/10, people-lifecycle 14/14, write-api 28/28
+      (52 total across affected suites).
+
+## Follow-ups
+
+- Wire `Idempotency-Key` into at-risk mutating endpoints (starting with
+  `POST /api/projects/:slug/updates`) + per-route tests.

specs/api/conventions.md

@@ -170,6 +170,8 @@ Exceeded → `429 rate_limited`, `Retry-After` header in seconds.
 
 Mutating endpoints accept an optional `Idempotency-Key` header (any client-generated string). The API caches the response in-memory keyed by `(personId, idempotencyKey)` for 24 hours; repeat requests with the same key return the same response. In-memory by design — single replica, restart-tolerant: a key that hasn't seen a duplicate within 24h won't see one after a restart either. This matters for cases like "post project update" where a double-tap shouldn't create two updates.
 
+> **Status:** the idempotency plugin (`apps/api/src/plugins/idempotency.ts`) is implemented and registered, but not yet wired into any route handler — mutating endpoints don't honor the header *yet*. Wiring the at-risk endpoints (starting with `POST /api/projects/:slug/updates`) and the per-route tests are tracked as a follow-up; until then, treat this section as the target contract, not current behavior.
+
 ## Logging and trace IDs
 
 Every request has a `traceId` (UUIDv7). It's included in logs and surfaced in error responses' `error.traceId`. If a user reports a problem, the traceId is the link to the server logs.

specs/api/people.md

@@ -26,7 +26,7 @@ See [data-model.md](../data-model.md#person).
 | ----- | ---- | ----- |
 | `q` | string | Full-text on `fullName`, `bio`. |
 | `tag` | string | Repeatable. Tag handle (e.g., `tech.python`). AND across repeats. |
-| `accountLevel` | enum | `user` \| `staff` \| `administrator`. Staff-only filter. |
+| `accountLevel` | enum | `user` \| `staff` \| `administrator`. Staff-only filter — for a non-staff caller it returns an empty list (a 200 with no items), not a `403`, so the filter's existence isn't a signal. |
 | `sort` | sort | Default `-createdAt`. Allowed: `createdAt`, `fullName`. |
 | `page`, `perPage` | int | Default `perPage = 30`. |
 
@@ -80,7 +80,9 @@ See [data-model.md](../data-model.md#person).
   "avatarUrl": "https://...",
   "bio": "Markdown source...",
   "bioHtml": "<p>...</p>",
+  "slackHandle": "janedoe",                 // null if unset
   "accountLevel": "staff",                  // visible to self and staff only; "user" otherwise
+  "deletedAt": null,                        // ISO timestamp if deactivated; null otherwise. Visible to self + staff only (always null to others)
   "tags": { "topic": [Tag, ...], "tech": [Tag, ...] },
   "memberships": [
     {
@@ -102,14 +104,15 @@ Fields visible only to self or staff:
 - `email` (not in the shape above; added only when authorized)
 - `firstName`, `lastName` (visible to all but editable only by self/staff)
 - `accountLevel` value beyond a generic "user" — public callers always see `"user"` regardless of true level
+- `deletedAt` — the real timestamp is shown to self + staff; everyone else always sees `null`
 
 ### Errors
 
 - `404 not_found` — slug doesn't exist, or person is soft-deleted and caller is not staff
 
 ## PATCH /api/people/:slug
 
-Self or staff. Self cannot change their own `accountLevel`; only administrators can change account levels (and only via a staff-only sub-endpoint, not by passing `accountLevel` in a generic PATCH).
+Self or staff. Self cannot change their own `accountLevel`; only administrators can change account levels (and only via the dedicated [`POST /api/people/:slug/account-level`](#post-apipeopleslugaccount-level) endpoint — `accountLevel` passed in a generic PATCH body is rejected by the schema, not silently applied).
 
 ### Request
 

specs/behaviors/legacy-id-mapping.md

@@ -25,7 +25,7 @@ If we ever need to re-import (e.g., catching up on changes made after cutover),
 
 ## Lookup
 
-`legacyId` lookups go through the in-memory `byLegacyId.<entity>` indices documented in [data-model.md](../data-model.md). These indices are built at boot by iterating the sheet and skipping records where `legacyId` is absent.
+`legacyId` lookups that back a live redirect go through in-memory `byLegacyId` indices, built at boot by iterating the sheet and skipping records where `legacyId` is absent. These indices exist only for the entities whose laddr URLs actually referenced numeric IDs — **`people`, `projects`, and `blog-posts`**. The other migrated sheets (`tags`, `project-buzz`, `project-updates`) carry `legacyId` for *import idempotence* but have no runtime `byLegacyId` index, because no live URL resolves them by numeric ID (their legacy URLs were always slug-based). If a future redirect ever needs one of those by numeric ID, add the index then.
 
 Uniqueness of `legacyId` per sheet is enforced by the API's write mutex: a record's `legacyId` is checked against the index before commit, just like `slug` and `email`.
 

specs/behaviors/storage.md

@@ -2,7 +2,7 @@
 
 ## Rule
 
-**Public** persistent data lives in a **gitsheets-backed git repository** — TOML records in templated paths, committed atomically, pushed to a publicly cloneable GitHub remote. There is no relational database. At runtime the API loads all records into typed in-memory structures and serves reads from there; mutations write to the gitsheets repo and update the in-memory state synchronously.
+**Public-by-design** persistent data lives in a **gitsheets-backed git repository** — TOML records in templated paths, committed atomically, pushed to the `codeforphilly-data` GitHub remote. The repo is access-controlled (private) today, but its contents are curated to be publicly shareable (civic transparency), and the commit log is redacted of PII accordingly — treat everything in it as forever-public (see [PII-aware redaction](#pii-aware-redaction)). There is no relational database. At runtime the API loads all records into typed in-memory structures and serves reads from there; mutations write to the gitsheets repo and update the in-memory state synchronously.
 
 **Private** data — emails, password hashes during migration, newsletter subscription state — lives in a separate S3-compatible bucket. See [behaviors/private-storage.md](private-storage.md). This spec covers only the public side.
 
@@ -54,7 +54,7 @@ There are two git repositories:
 | Repo | Purpose | Visibility |
 | ---- | ------- | ---------- |
 | `codeforphilly-rewrite` | The application code (this repo) | Public |
-| `codeforphilly-data` | The live gitsheets data | **Private** — contains emails, real names, IPs |
+| `codeforphilly-data` | The live gitsheets data | **Private repo today**, but **public-by-design** content — member names + projects are public-facing; the commit log is redacted (no emails, IPs, or password hashes — those live in the private store). Treat as forever-public. |
 
 The code repo references the data repo by env (`CFP_DATA_REPO_PATH`). They are not git submodules — too much friction. Sibling clones, locally and in production.
 
@@ -81,7 +81,7 @@ A scrubbed snapshot of the data repo is published as a public tag (e.g., `snapsh
 - `slackHandle` → null
 - `bio`, `overview`, `body` content → unchanged (assumed safe; staff may flag specific records for redaction)
 
-A `scripts/scrub-data.ts` in the code repo produces the snapshot. The contributor bootstrap is:
+An `apps/api/scripts/scrub-data.ts` in the code repo produces the snapshot. The contributor bootstrap is:
 
 ```bash
 git clone https://github.com/CodeForPhilly/codeforphilly-rewrite.git
@@ -264,12 +264,12 @@ Rules:
 
 ### PII-aware redaction
 
-The public data repo is, well, public — redaction is essential, not defense-in-depth:
+The data repo is public-by-design (and may be made public outright) — redaction is essential, not defense-in-depth:
 
 - **No real emails as commit authors.** Author = `<slug>@users.noreply.codeforphilly.org`, never the user's actual email.
 - **No `User-Ip` or `User-Agent` trailers in public commits** (rule above).
 - `Authorization` and `Cookie` request headers are never embedded anywhere.
-- Any request body field matching `/email|password|token|secret/i` is replaced with `[REDACTED]` before embedding in the commit message body.
+- **Commit message bodies carry only the caller-provided `summary` string — never the raw request body** — so request PII can't reach the commit message in the first place. If a `summary` is ever derived from request input, fields matching `/email|password|token|secret/i` must be `[REDACTED]` before embedding.
 - `Set-Cookie` response headers and JWT bodies are not embedded.
 - Trailers for actions on the private store (`PrivateProfile`, `LegacyPasswordCredential`) appear in the public log as semantic action only — `Action: private-profile.update`, with no field-level diff. The actual change details live in the private bucket's object-version history (see [private-storage.md](private-storage.md)).