Promote staging to main: access-checked case images, archived Discover copies, canvas and docs updates - #1003
Merged
Merged
Conversation
…hema ADR 0004 D1: the JSON Schema file becomes a generated artefact (Zod -> JSON Schema, draft-07 for the JSON editor's json-schema-library resolver) instead of a hand-maintained one that had drifted to 6 element types against the Zod schema's 10. Adds pnpm schema:generate and a comment on the export envelope naming version as the format-migration key.
…abularies Fails if the committed json-schema-v1.0.json and a fresh generation disagree, or if the Prisma ElementType/AssertionStatus enums and their Zod counterparts (ElementTypeSchema, AssertionStatusSchema) drift apart in either direction (ADR 0004 D1's build-time consistency check).
…portComment ADR 0004 D1: types/curriculum.ts hand-copied these three from lib/schemas/case-export.ts and had drifted (a `title` field on TreeNode no real export ever set). Re-export the Zod-derived types instead; drop the one dead consumer read of that field in case-data-transformer.ts.
…re-exports fallow flagged these as unused type exports and duplicate exports (vs lib/schemas/case-export.ts, which already exports all four) once the hand-copied TreeNode that consumed them internally was removed.
The __schema<n> -> TreeNode rename assumed exactly one recursive definition reachable from CaseExportNestedSchema; a blanket replaceAll would have silently merged two distinct recursive shapes into one $ref if that assumption ever broke. Extracted into renameRecursiveDef(), which counts distinct placeholder ids and throws, naming the assumption, if the count isn't exactly one. Covered directly with synthetic two-lazy/zero-lazy/one-lazy schemas, since CaseExportNestedSchema only ever exercises the passing case. Also reworded a comment that called the drift test's comparison "byte-for-byte" — it's a structural toEqual, not a byte comparison.
The JSON editor's hover test expects TreeNode.id's description ("Unique
identifier for the element"), which the hand-written schema carried but
TreeNodeSchema never did (case-export.ts's other schema for the same
shape, ElementV2Schema, already described every one of its fields —
only TreeNodeSchema was short a set of .describe() calls).
Compared every description in the pre-generation json-schema-v1.0.json
(git show origin/staging~1) against TreeNodeSchema and restored the
ones that describe a real, unconditional field: id, name, description,
inSandbox, children, comments, fromPattern, modifiedFromPattern,
isDefeater, defeatsElementId. Left alone: the top-level document
description (the "authoritative specification" framing ADR 0004 D1
demotes), ElementType's stale "Context/Justification/Assumption are now
attributes" note (wrong against the current 10-type vocabulary), and
every description that only existed inside the old file's per-type
allOf/if/then blocks (assumption, justification, context, url, level,
moduleReferenceId, moduleEmbedType, modulePublicSummary) — those
blocks encoded validation rules Zod never had, and their specific
per-type wording isn't separable from the rule ADR 0004 D1 accepts the
loss of.
Regenerated json-schema-v1.0.json.
A node with side attachments (data.attachedTo) now becomes an ELK group laid out perpendicular to the tree direction, with the tree itself laid out over groups via SEPARATE_CHILDREN hierarchy handling. Positions are flattened to absolute coordinates before being handed to React Flow. Verified against the nine-node probe from the ADR: every side attachment lands in its target's row, to the right, and a side element's own subtree sits beneath it rather than beneath the challenged node.
- case-fetch-service.ts admits AWAY_GOAL and MODULE children alongside PROPERTY_CLAIM, and resolves cited case/element names and viewer accessibility once per fetch for the new cards to show without a follow-up request. - convert-case.ts routes a defeater to its target's cell (data.attachedTo) when the target is present and visible, otherwise it keeps its ordinary tree position; createEdgesFromNodes draws the challenges edge, replacing the support edge when the target is also the defeater's real parent. - New away-goal-node.tsx and module-node.tsx cards, a challenges-edge.tsx (dashed, destructive token, open arrowhead at the target), and side handles on BaseNode so the edge is straight and horizontal. - flow.tsx's hard-coded four-entry nodeTypes map is replaced by one resolution function (lib/case/node-type-resolver.ts, framework-agnostic; components/cases/node-type-resolver.ts pairs it with renderers) shared by convert-case.ts and the node-kind tests.
…ADR 0005 D7) - "Add defeater" on the add-child menu of a goal, property claim or strategy creates a PROPERTY_CLAIM child flagged isDefeater with defeatsElementId set to the element the menu was opened on, through the existing element-creation path. - "Add away goal" / "Add module" open a two-step picker (case, then goal for an away goal) backed by two new server actions (actions/cited-element-picker.ts): listCitableCases and listCitableGoals, both delegating to case-fetch-service.ts's existing permission checks. listCaseGoalElements is new there. - The edit dialog shows a read-only "Challenges: <target name>" line for a defeater; changing the target stays out of scope. - lib/case/types.ts's CreateNodePayload gains the fields the write path already accepted; lib/element-types.ts's collection-name map gains awaygoals/modules.
…solver createEdgesFromNodes: replace-vs-both edge rules and the missing-target fallback (ADR 0005 D4/D2). convertAssuranceCase: routes a defeater's data.attachedTo when its target resolves, leaves it unset otherwise. resolveReactFlowNodeType: every element-type spelling this codebase uses, plus the unrecognised-type fallback and the D6 flags seam.
…elements createElementSchema silently had no moduleEmbedType field, but lib/prisma.ts's element-validation extension requires it for MODULE at create time — creating a module via the new "Add Module" flow would have thrown. Threaded through createElementSchema, element-service.ts's create, CreateNodePayload, and the picker form (defaults to COPY, no UI choice of embed type in 1.0). Test factory gains the same field for MODULE fixtures. test(canvas): integration coverage for D3 admission and D7 server actions case-fetch-service-side-attachments.test.ts: AWAY_GOAL/MODULE admitted under goal/strategy/property claim, cited names and viewer accessibility resolved, isDefeater/defeatsElementId pass through. cited-element-picker- actions.test.ts: permission matrix (own case, shared VIEW/EDIT, no access, non-existent case) for listCitableCases/listCitableGoals.
Away-goal and module cards shared an identical 20-line "view case" link block — extracted to cited-case-link.tsx (fallow duplication finding). components/cases/node-type-resolver.ts's resolveNodeRenderer and lib/case/node-type-resolver.ts's REACT_FLOW_NODE_KINDS were unused exports (fallow dead-code finding); the nodeTypes map and resolveReactFlowNodeType cover every real caller. test(e2e): canvas coverage for away goals and defeaters (ADR 0005) Builds its fixtures through the canvas's own "Add away goal"/"Add defeater" flows rather than JSON import — case import currently drops isDefeater/defeatsElementId on both formats (tracked separately; the ADR itself notes defeaters reach the canvas by API/UI until that's fixed).
validateCitedElementId only checked existence and self-reference — an AWAY_GOAL's citedElementId could point at any element system-wide, and buildCitationContext then resolved and displayed that element's name to every viewer of the citing case. Now requires citedElement.caseId to match the effective moduleReferenceId (the update path's own value if changing, otherwise the element's existing one), returning the same "must reference an existing element" message as not-found — anti-enumeration. test(canvas): cross-case citedElementId rejected on create and update, and confirm the D7 picker action (listCitableGoals) can't itself produce one since it only ever lists goals scoped to the requested case.
BaseNode gains an isDefeater prop: a destructive-token border override (buildNodeContainerClasses) and a "Defeater" badge in the header, always visible (not just when expanded) — wired through all four existing card kinds (goal, strategy, property, evidence) from data.isDefeater. Previously nothing rendered this at all; the only isDefeater read in components/ was the edit dialog's read-only "Challenges" line. test(canvas): render tests for the chip/border default-off and on-when- isDefeater states, across all four card kinds. fix(e2e): scope the Defeater assertion to the actual defeater's own card (by its description text, since the auto-generated identifier isn't known ahead of time) rather than an unscoped getByText, which was passing against unrelated page text since no chip existed to fail against it. Scope the Case/Goal comboboxes to the dialog. Assert challenges-edge count rather than visibility, which is unreliable on a zero-height horizontal path.
lib/case/__tests__/layout-helper.test.ts: the single large probe it() is now several focused cases (row+right placement, row order, no overlaps, each under TB and LR). Adds a second target with two side attachments (stacked in one column, target's own tree row unaffected) and sort-order coverage (attachments given in reverse identifier order still sort correctly). Fixes an overclaim: the old comment said CSn1 sits "beneath its own root (CG1)" specifically, distinguishing it from G2's column — empirically (printed positions) it does not; only cell-membership row placement is guaranteed, not which member's column a child aligns to. The assertion now matches that.
AddCitedElementForm (CRAP 182, 0% covered) was one component owning server-action fetching, two-step selection state, and the discriminated create payload inline. Split out: - hooks/use-cited-element-picker.ts — cases/goals fetching and selection state, unit-testable via renderHook without rendering the form. - lib/case/build-cited-element-payload.ts — the away-goal-vs-module discriminated payload (citedElementId vs moduleEmbedType: "COPY"), a pure function. The form is now a thin presentational shell wiring the two together. fix(canvas): away goal / module cards now start expanded — the cited or referenced case is the card's whole point, and it was previously hidden behind BaseNode's expand toggle since it's passed as `children` (only rendered in the expanded state). AwayGoalNode/ModuleNode set initialExpanded so it shows without a click, matching what D3 asks the card to show. fix(canvas): handleDefeaterAdd left `loading` stuck true on a create error (return before setLoading(false), mirroring an existing bug in the handleClaimAdd pattern it was copied from) — fixed in the new function so the "Add" button doesn't stay stuck spinning on failure. test(canvas): render tests for AddCitedElementForm (away-goal vs module payload, description prefill, error path), the picker hook (loading, error toasts, case-change clearing the selected goal), the payload builder (discriminated union, name trimming), and handleDefeaterAdd (payload shape, all three opener kinds, error toast).
lib/element-compatibility.ts's REACTFLOW_TO_CANONICAL and VALID_CHILDREN
didn't know awayGoal/module (ADR 0005 D3) — getCompatibleChildTypes and
canBeChildOf treated them as unknown types, so the attach flow never
offered them as valid children of a goal/strategy/property claim they'd
been detached from. Added as leaves (same three parents as property
claim, no children of their own in 1.0).
refactor(canvas): extract the duplicated awayGoals/modules array building
(buildGoalStructure, buildStrategyStructure, buildPropertyClaimStructure
in case-fetch-service.ts each repeated the same filter-sort-map pair) into
buildCitedChildren — the one clone group fallow flagged in this file.
fix(canvas): humanise "away_goal" in the history feed ("Created away goal
\"AG1\"" instead of the raw snake_case type); "module" already matched the
existing lowercase convention unchanged.
test: unit coverage for both.
…gate layout-helper.ts's flattenPositions exceeded the cognitive-complexity threshold even with the split probe tests covering it (coverage lowers CRAP but doesn't remove a raw complexity finding) — extracted the cell- member flattening into flattenCellMembers. case-fetch-service.ts's buildGoalStructure/buildPropertyClaimStructure still shared one filter-sort-map clone (the STRATEGY-children block) after the earlier buildCitedChildren extraction shifted surrounding lines enough for fallow to flag it as newly introduced — extracted buildNestedStrategies.
…oves
enforceCitedElementIdRules short-circuits when citedElementId is absent
from the request, so PUT { moduleReferenceId: caseB } alone left the
existing citedElementId pointing into the old case — the away-goal card
then showed the new case's name beside the old case's element name.
validateUpdateElementFields now re-validates the EXISTING citedElementId
against the new moduleReferenceId whenever moduleReferenceId changes and
the request doesn't also touch citedElementId — same rule, same
not-found-shaped error as an explicit citedElementId. Requires
citedElementId in updateElement's own Prisma select (existing didn't carry
it — validateUpdateElementFields had no way to see it before).
test(canvas): moduleReferenceId-only change away from the citing case is
rejected; moduleReferenceId + a repointed citedElementId in the same
request is accepted.
validateCitedElementId used `moduleReferenceId && target.caseId !==
moduleReferenceId` — a falsy (null/undefined) moduleReferenceId short-
circuited the `&&` and skipped the case-membership check entirely.
updateElementSchema allows moduleReferenceId: null (clearing it), so
PUT { moduleReferenceId: null, citedElementId: <any element in any case> }
was accepted outright: the round-1 leak through a different route.
A citation now requires a case unconditionally: citedElementId with no
effective moduleReferenceId is rejected (same not-found-shaped message),
and the case-membership comparison always runs once moduleReferenceId is
present. This also closes the "clear the case" variant for free, via the
round-2 re-validation path: PUT { moduleReferenceId: null } alone, with an
existing citation, re-validates that existing citedElementId against the
now-null moduleReferenceId and is rejected; PUT { moduleReferenceId: null,
citedElementId: null } clears both together and is accepted.
The create path has no equivalent gap — moduleReferenceId is required
before citedElementId is ever checked (enforceModuleReferenceIdRules runs
first in validateElementReferences) — confirmed with a test rather than
assumed.
test(canvas): four cases in api-elements-cited-element-id.test.ts — cross-
case citation with moduleReferenceId: null rejected; moduleReferenceId:
null alone with an existing citation rejected; both cleared together
accepted; create-path AWAY_GOAL with citedElementId but no
moduleReferenceId at all rejected by the requiredness check.
fix(import): carry defeaters through import in both formats, flag unresolvable targets
…n-schema feat(schemas): generate json-schema-v1.0.json from the Zod case schema (ADR 0004 D1)
…ttachments feat(canvas): render and create away goals, modules and defeaters as side-attached elements (ADR 0005)
…e-queue ci: queue staging deploys and reseeds so they never overlap
… of removing it Deleting a published case now asks whether to remove its Discover copy or keep it as an archived copy that no longer updates. Archived copies survive permanent deletion of their case, carry an "Archived" label on Discover, and can be removed by their author from the Trash page at any time. Restoring a case makes its archived copy live again; account deletion archives rather than removes; the API removes unless asked to archive; publishing refuses a case that is in Trash. This also fixes permanent deletion and the daily trash-purge cron for any case with a published copy: the published-copy table's foreign key to its case is relaxed to ON DELETE SET NULL (migration 20260928000000_archived_published_cases), so a permanent delete clears the link instead of being refused.
Covers behaviour the implementer's own tests do not exercise: no orphan published row survives a refused publish/republish of a trashed case, an Admin collaborator who archives a copy cannot become its remover, the daily batch purge (purgeExpiredCases) with a mix of archived-copy and ordinary cases, a restore after the archived copy was individually removed, a kept account-deletion case left completely untouched, and the DELETE route's own archive query-param parsing (default, false, junk, true).
… and close the publish/trash races - Extract the published-case delete dialog into a shared components/cases/delete-case-dialog.tsx, used by both the case page toolbar and the dashboard case cards (which previously always removed the Discover copy with no choice). Cards now carry a `published` flag through the case-list service/response types. - Word the "Keep as archived" choice for a non-owner Admin collaborator differently from the owner, using a new `isOwner` flag on the case response. - Lock the case row before the published row in publishAssuranceCase and updatePublishedCase, matching softDeleteCase's own lock order, so the two paths can't deadlock against each other. - removeArchivedCopy re-checks its ownership/archived guard at delete time (a single guarded deleteMany), closing a race where a concurrent restore could otherwise let a live copy be deleted. - Validate the archived-copy route's id with uuidSchema so a malformed one 404s like a missing one, instead of 500ing. - archivePublishedCopies no longer re-stamps a copy that's already archived. - listArchivedCopies narrows archivedAt without a cast, matching its own where clause. - Resolve a duplicated session+id validation block by combining the case id and delete options into one Zod schema. - Regenerate docs/database/schema.dbml and public/openapi.json; drop the ruling/date citations from the added comments and doc strings throughout, and correct a stale FK comment in the e2e cleanup.
…hived-copy edge cases - Add a row-lock test helper that holds a genuine Postgres row lock open inside a transaction until released, so a guard that only fires under real concurrency (an updateMany/deleteMany matching zero rows) can be reached deterministically, instead of racing on Promise.all timing or relying on a case already being fully trashed before the call under test starts. - Cover CaseTrashedDuringTransactionError in both publishAssuranceCase and updatePublishedCase, and CaseAlreadyInTrashError in softDeleteCase, none of which any existing test reached (every one trashed the case first, so the earlier permission/pre-check always won). - Cover the removeArchivedCopy race against a concurrent restore, the already-archived re-stamp guard, and removing an archived copy whose case has already been permanently deleted. - Add route tests for the two new archived-copy endpoints (auth, missing id, malformed id, someone else's copy, success) and for the delete action's archive choice and malformed-options rejection. - Add component tests for the shared delete dialog and the Trash page's archived-copies list.
…archiving accurately - Trash/publishing/retention guide pages: describe the delete dialog's published-copy choice, restoring an archived copy, and the Trash page's "Archived on Discover" section. - Correct two inaccuracies: the dialog for a published case replaces the usual one rather than adding an extra step, and account deletion only archives the copies of cases that move to Trash, not every case the deleted person published. - One sentence per source line, for every sentence this change added or changed.
…ests Replaces the fixed 50ms delays in the trash/publish row-lock races with a poll against pg_stat_activity, so each test proves the racing call is actually blocked on the guarded write before releasing the held lock, rather than assuming its pre-transaction read finished in time. Also makes holdRowLock's release() reject with the holder transaction's own error instead of hanging when that transaction fails. Rewrites the adversarial test file's header comment to describe its coverage without naming an issue title or design note.
…ed-case-choice
…s from live edits Screenshot and feature-image bytes now flow through access-checked private routes (/api/cases/[id]/media/screenshot, .../media/feature) instead of raw storage addresses. A case's feature image is copied to its own key at publish/republish time and served by a version-scoped public route, so editing the live image can no longer affect or break a published or archived Discover page. Local uploads move out of public/ to UPLOADS_DIR, and /uploads/[...path] is removed.
Covers the four access outcomes on both private media routes (no session, no access, VIEW, owner after re-upload), the case-information form re-save keeping the displayed route address treated as unchanged, the published-image copy surviving live edits and republish while a superseded version's address stops resolving, an archived copy staying reachable after its case is permanently deleted, every removal path (unpublish, remove-from-discover, archive-then-purge, remove-archived- copy) deleting the file it copied, path-traversal payloads in a stored key never escaping the uploads root, and the same access and cleanup behaviour against the Azure-backed storage path.
…ng a key it doesn't own A case's feature-image write path now accepts only its own media route address (with or without a cache-busting query), empty, or a genuine external https address — a bare storage key, another case's own address, or a legacy stored-value shape is refused. The upload route persists a newly generated key through an internal setter the write path itself cannot reach. Reading, publish-time copying and deleting a case's feature image now also check that the stored key actually starts with that case's own key prefix, refusing (and, for a skipped copy, logging) anything else. Every storage read, copy, delete and upload — on both the local and Azure backends — validates the key first, rejecting an empty, `.` or `..` segment, a backslash, a NUL byte, or a leading slash before it reaches the filesystem or the Azure SDK; the local-storage fallback routes go through the same path-safety check rather than building paths directly. The publish and republish flows now share one helper for composing a snapshot, copying its feature image, running the caller's transaction and cleaning up the copy on failure.
…nses A published item's summary, its detail response and its embedded snapshot content now carry a version-scoped public route address for an internal feature-image key (including a legacy stored shape from before publish-time copies existed), never the raw storage key. A genuine external address and an empty value still pass through unchanged.
…d support conditional 304 requests The private feature-image and screenshot route addresses now carry a short cache-busting query derived from the current storage key, so a browser sees a fresh address every time the image is replaced rather than reusing bytes cached against the old one. The two private routes and the public Discover image route ignore that query but now answer a request whose If-None-Match header matches the current ETag with an empty 304 response.
…ield change in the changelog
Integration tests that write real media files no longer touch this worktree's own uploads directory: each vitest worker now gets a fresh temporary directory for UPLOADS_DIR, removed again once the worker's tests finish. The uploads directory is also excluded from the Docker build context.
Covers the download, upload and delete branches directly against a mocked @azure/storage-blob client — not configured, success, a not-found blob, and any other SDK error — plus the key-validation refusal on all three, none of which any existing test reached since every other test mocks this module wholesale.
…cess and header details Updates seed fixtures whose key shape the feature-image ownership check now refuses, and updates two assertions to expect that refusal rather than a silent overwrite. Adds coverage for a direct EDIT collaborator and a team-granted viewer on both private media routes, Cache-Control and Content-Length on those routes and the public one, a legacy /uploads/ path and a legacy Azure blob URL served through both the private feature route and the public route, and the publish-time copy being deleted when its transaction fails. Also updates assertions that previously expected a private media address to stay fixed across a re-upload, and one address-source comment.
…media fix: check access before serving case images, isolate published copies from live edits
|
promotion PR — structural quality was gated per feature PR on staging |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Promotion: staging → main
Carries every PR merged to staging since #947 (9 September): #910, #948–#954, #956–#1002. Each was gated on its own PR (tests, lint, coverage-mapped structural quality, and review). The Structural Quality check on this PR audits the whole delta as one change and is expected to fail; it is not a required check on
main.Headline changes
/uploads/*route is removed.json-schema-v1.0.jsongenerated from the export schema.Database migrations (six, applied on deploy)
20260914000000_add_defeats_dangling20260916000000_add_module_reference_dangling20260923000000_reconcile_schema_drift— also clears references to hard-deleted users before adding foreign keys20260924000000_add_session_version20260924010000_hash_password_reset_tokens— password reset links issued before the release stop working20260928000000_archived_published_casesAll six apply on every staging reseed. Staging data is freshly seeded each time, so the data-changing steps have not yet run against production's data.
Configuration
Production and staging App Service settings carry the same 21 names, including
TOKEN_ENCRYPTION_KEY.Release effect
No breaking-change markers since
v0.7.0; 18featcommits → minor bump (v0.8.0).After merge
Once production is on this code, the
mediacontainer onteastorageaccount(shared by staging and production) is set to private access, and direct blob addresses are checked to fail while the app still shows images on both environments.