Repository navigation
Conversation
Bring ISMS documents onto the Policies versioning model. Approving a document freezes an immutable published version (v1). Editing afterwards creates a draft without touching the published version, which stays live and exportable. Re-approving publishes the next version and moves the old one into history. Rendered DOCX/PDF are retained per published version so an auditor sampling v1 later downloads exactly what was approved. - schema: IsmsDocument gains currentVersion/draftNarrative/draftSnapshot; IsmsDocumentVersion gains contentSnapshot/changelog/publishedBy. Backfill migration (pre-GA, flag-gated) moves the old single-version model over. - api: new IsmsVersionService freezes a version at approval and renders + uploads PDF/DOCX to S3 after the txn; lists history; serves a version's export (stored file -> snapshot re-render -> live fallback). Editing an approved document reverts status to draft but preserves the published version. Drift baseline + draft narrative moved onto the document. New GET /v1/isms/documents/:id/versions; optional versionId on export. - app: version-history section on every document detail page with per-version PDF/DOCX download, and a "Published vN / editing creates a new draft" signal. - tests: 312 api + 119 app tests passing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012CweXoSSEP89mX93u3Bdcp
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
cubic analysis
10 issues found across 48 files
Confidence score: 2/5
- The highest risk is export integrity in
apps/api/src/isms/isms-context.service.tsandapps/api/src/isms/isms-version.service.ts: whenversionIdis omitted or stored exports are absent, the code can return a working draft instead of the approved historical artifact, which can break audit/compliance expectations. Before merging, lock these paths to published-version data and prevent draft fallbacks for historical requests. - In
apps/api/src/isms/isms.service.ts, approval can commit even if DOCX/PDF rendering fails, andapps/api/src/isms/documents/generate.tscan mutate approved content without clearly transitioning it back to draft state; this risks “approved” versions that are not reproducible or internally inconsistent. Require successful artifact persistence before publish and enforce explicit draft-state transitions when regenerating approved narratives. apps/app/src/app/(app)/[orgId]/documents/isms/components/IsmsVersionHistory.tsxalways shows download actions, andapps/api/src/isms/isms-version.service.tsmay fail hard on missing S3 objects even whencontentSnapshotexists, so users can hit avoidable download errors. Hide/disable unavailable formats and add a resilient fallback to snapshot parsing before merging.apps/app/src/app/(app)/[orgId]/documents/isms/hooks/useIsmsDocumentVersions.tscan query version history under the wrong organization context (missingorganizationIdheader), creating cross-org correctness risk. Pass the route org header consistently and verify with a targeted integration test before merge.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/db/prisma/schema/isms.prisma">
<violation number="1" location="packages/db/prisma/schema/isms.prisma:130">
P2: According to linked Linear issue CS-701, version history must retain the approver for each published ISMS version; this relation nulls `publishedById` if the Member is deleted, leaving historical rows without an approver. Consider storing immutable publisher display metadata on the version or using deletion semantics that preserve the audit record.</violation>
</file>
<file name="apps/api/src/isms/documents/generate.ts">
<violation number="1" location="apps/api/src/isms/documents/generate.ts:185">
P2: Regenerating an already approved narrative document can change the working draft without marking the document as draft, so the UI/API may still report the existing approval while draft content has changed. Consider invalidating approval in the regenerate path when `draftNarrative` is written, matching `IsmsNarrativeService.save`.</violation>
</file>
<file name="apps/api/src/isms/isms.service.ts">
<violation number="1" location="apps/api/src/isms/isms.service.ts:261">
P1: According to linked Linear issue CS-701, each approved ISMS version must retain its DOCX/PDF exports so a later auditor gets exactly the approved artifact. This approval path commits the version even when `publishRenders` fails, because that method swallows render/upload errors; consider making export persistence required or surfacing a failed publish state/retry before treating approval as complete.</violation>
</file>
Linked issue analysis
Linked issue: CS-701: [Feature] Document versioning and history for ISMS documents
| Status | Acceptance criteria | Notes |
|---|---|---|
| ✅ | Approving a draft creates an immutable published version and stores a content snapshot / rendered exports | Schema, service and export pipeline added to freeze a snapshot and persist per-version metadata and files; migration and tests present. |
| ✅ | Editing an approved document creates a draft without touching the published version; published version stays live and exportable | Draft fields moved onto IsmsDocument and narrative writes update draft fields; approval flow preserves currentVersion and only the draft is edited. |
| ✅ | Version history is visible on the document detail page with date, approver and per-version download actions | New hook + UI component expose the version feed and per-version download; controller/service wiring and tests added. |
| ✅ | Exporting a historical version returns the same stored DOCX/PDF (with snapshot re-render fallback) | Export DTO accepts versionId; export-payload and version service support rendering stored snapshot and fallback rendering; export tests present. |
| ✅ | Behaviour applies across all six ISMS documents (Context, Scope, Interested Parties Register, Interested Parties Requirements, Leadership Commitment, Objectives Plan) | Multiple per-document components were updated to use the new draft/currentVersion shape and the history UI is a shared choke-point used by all documents; tests updated for document clients. |
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
|
|
||
| // Render + upload the frozen exports outside the transaction (Policies | ||
| // pattern) so an S3 hiccup never orphans the published version. | ||
| await this.versionService.publishRenders({ |
Contributor
There was a problem hiding this comment.
P1: According to linked Linear issue CS-701, each approved ISMS version must retain its DOCX/PDF exports so a later auditor gets exactly the approved artifact. This approval path commits the version even when publishRenders fails, because that method swallows render/upload errors; consider making export persistence required or surfacing a failed publish state/retry before treating approval as complete.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/isms/isms.service.ts, line 261:
<comment>According to linked Linear issue CS-701, each approved ISMS version must retain its DOCX/PDF exports so a later auditor gets exactly the approved artifact. This approval path commits the version even when `publishRenders` fails, because that method swallows render/upload errors; consider making export persistence required or surfacing a failed publish state/retry before treating approval as complete.</comment>
<file context>
@@ -216,11 +230,40 @@ export class IsmsService {
+
+ // Render + upload the frozen exports outside the transaction (Policies
+ // pattern) so an S3 hiccup never orphans the published version.
+ await this.versionService.publishRenders({
+ organizationId,
+ documentId,
</file context>
Contributor
Author
There was a problem hiding this comment.
Intentional — not a defect (won't-fix by design). createPublishedVersion persists an immutable contentSnapshot inside the approve transaction, so if publishRenders (the post-commit S3 upload) fails, the version still re-renders on demand from that snapshot — the approved export is never lost. The AWS SDK already retries transient errors. Forcing a hard failure here would strand a document 'approved but unretryable' (the approve guard rejects re-approval once status has flipped). The download path was hardened instead (resilient S3 read + snapshot fallback).
Contributor
There was a problem hiding this comment.
The parent comment was wrong for this PR: createPublishedVersion keeps the approved export in an immutable contentSnapshot, and publishRenders is intentionally best-effort after commit. If S3 upload fails, the version can still be re-rendered on demand, and the hardened download path covers the fallback.
| // The member who published (approved) this version, and when. `publishedAt != null` | ||
| // is the marker of a published version (drafts are not materialized as rows). | ||
| publishedById String? | ||
| publishedBy Member? @relation("IsmsDocumentVersionPublisher", fields: [publishedById], references: [id], onDelete: SetNull) |
Contributor
There was a problem hiding this comment.
P2: According to linked Linear issue CS-701, version history must retain the approver for each published ISMS version; this relation nulls publishedById if the Member is deleted, leaving historical rows without an approver. Consider storing immutable publisher display metadata on the version or using deletion semantics that preserve the audit record.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/db/prisma/schema/isms.prisma, line 130:
<comment>According to linked Linear issue CS-701, version history must retain the approver for each published ISMS version; this relation nulls `publishedById` if the Member is deleted, leaving historical rows without an approver. Consider storing immutable publisher display metadata on the version or using deletion semantics that preserve the audit record.</comment>
<file context>
@@ -82,32 +99,46 @@ model IsmsDocument {
+ // The member who published (approved) this version, and when. `publishedAt != null`
+ // is the marker of a published version (drafts are not materialized as rows).
publishedById String?
+ publishedBy Member? @relation("IsmsDocumentVersionPublisher", fields: [publishedById], references: [id], onDelete: SetNull)
publishedAt DateTime?
+ // Optional free-text note describing what changed in this version.
</file context>
| await tx.ismsDocumentVersion.create({ | ||
| data: { documentId, version: 1, isLatest: true, narrative }, | ||
| if (document && hasNarrativeContent(document.draftNarrative)) return; | ||
| await tx.ismsDocument.update({ |
Contributor
There was a problem hiding this comment.
P2: Regenerating an already approved narrative document can change the working draft without marking the document as draft, so the UI/API may still report the existing approval while draft content has changed. Consider invalidating approval in the regenerate path when draftNarrative is written, matching IsmsNarrativeService.save.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/isms/documents/generate.ts, line 185:
<comment>Regenerating an already approved narrative document can change the working draft without marking the document as draft, so the UI/API may still report the existing approval while draft content has changed. Consider invalidating approval in the regenerate path when `draftNarrative` is written, matching `IsmsNarrativeService.save`.</comment>
<file context>
@@ -174,22 +174,17 @@ async function generateNarrative({
- await tx.ismsDocumentVersion.create({
- data: { documentId, version: 1, isLatest: true, narrative },
+ if (document && hasNarrativeContent(document.draftNarrative)) return;
+ await tx.ismsDocument.update({
+ where: { id: documentId },
+ data: { draftNarrative: narrative },
</file context>
…org scoping
Fixes the real issues from the automated PR review:
- Versioned export never serves live/draft data (audit integrity): getVersionExport
now falls back to the immutable snapshot if a stored S3 object is missing, and
throws for a version with neither a file nor a snapshot instead of rendering
current live data.
- Default export of a clean approved document serves its retained published
artifact (byte-identical to approval); only in-progress drafts export the draft.
- Regenerating ("Generate from platform data") now invalidates approval, matching
every other edit path, so an approved doc can't silently diverge from its
published version.
- Version-history download buttons are gated on real availability (file or
snapshot); unavailable formats are hidden with a clear note.
- Version-history fetch passes the route org (X-Organization-Id), matching the
per-version download path.
- Approval success is separated from the best-effort post-approval refresh so a
revalidation hiccup no longer reports "Failed to approve".
- Version history retains the approver via the frozen snapshot metadata if the
approving member is later deleted.
Not changed (deliberate): approval stays committed if export rendering fails —
the immutable contentSnapshot makes every version reproducible and the AWS SDK
retries transient errors; throwing would strand a doc "approved but unretryable".
Tests: 316 api + 122 app ISMS tests passing; typecheck clean.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012CweXoSSEP89mX93u3Bdcp
Contributor
Author
|
@cubic-dev-ai review it |
Contributor
@tofikwest I have started the AI code review. It will take a few minutes to complete. |
Contributor
There was a problem hiding this comment.
cubic analysis
4 issues found across 48 files
Confidence score: 3/5
- In
apps/api/src/isms/isms-context.service.ts, the approved-document edit path can skip the retainedcurrentVersionId, which conflicts with CS-701’s requirement that v1 remain live/exportable while v2 is drafted; merging as-is risks making the live/exportable version unavailable in that state—fix the condition so retained current versions are preserved after edits before merging. - In
apps/app/src/app/(app)/[orgId]/documents/isms/hooks/useIsmsDocumentVersions.ts, failed historical exports are not caught, so users get silent failures and possible unhandled promise rejections instead of clear feedback; this can look like broken downloads in production—catchexportIsmsDocumenterrors and surface the existingtoast.errormessage path. - In
apps/api/src/isms/isms-version.service.ts(createPublishedVersion/resolveOrgProfile), reading org profile via globaldbinstead of the passedtxcan bypass transaction isolation during approve flows; this introduces inconsistent reads under concurrency—route org-profile reads through the transaction client before merge. - In
apps/app/src/app/(app)/[orgId]/documents/isms/components/IsmsVersionHistory.tsx, per-version export buttons share generic accessible names (PDF/DOCX), making voice-control and screen-reader actions ambiguous; merging now risks usability/accessibility regressions for version-specific downloads—include version-specific labeling in each button name.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/db/prisma/schema/isms.prisma">
<violation number="1" location="packages/db/prisma/schema/isms.prisma:130">
P2: According to linked Linear issue CS-701, version history must retain the approver for each published ISMS version; this relation nulls `publishedById` if the Member is deleted, leaving historical rows without an approver. Consider storing immutable publisher display metadata on the version or using deletion semantics that preserve the audit record.</violation>
</file>
<file name="apps/api/src/isms/documents/generate.ts">
<violation number="1" location="apps/api/src/isms/documents/generate.ts:185">
P2: Regenerating an already approved narrative document can change the working draft without marking the document as draft, so the UI/API may still report the existing approval while draft content has changed. Consider invalidating approval in the regenerate path when `draftNarrative` is written, matching `IsmsNarrativeService.save`.</violation>
</file>
<file name="apps/api/src/isms/isms.service.ts">
<violation number="1" location="apps/api/src/isms/isms.service.ts:261">
P1: According to linked Linear issue CS-701, each approved ISMS version must retain its DOCX/PDF exports so a later auditor gets exactly the approved artifact. This approval path commits the version even when `publishRenders` fails, because that method swallows render/upload errors; consider making export persistence required or surfacing a failed publish state/retry before treating approval as complete.</violation>
</file>
Linked issue analysis
Linked issue: CS-701: [Feature] Document versioning and history for ISMS documents
| Status | Acceptance criteria | Notes |
|---|---|---|
| ✅ | Approving a draft creates an immutable published version (content snapshot + publish metadata) with a version number. | PR adds an IsmsDocumentVersion model snapshot flow, publish logic, migration and tests demonstrating the freeze-on-approve lifecycle. |
| ✅ | Editing an approved document creates a working draft without touching the published version; the published version remains live and exportable while the draft accumulates changes. | Draft fields moved to IsmsDocument (draftNarrative/draftSnapshot); narrative writes update the document draft; code avoids mutating currentVersion on edits. UI components read draftNarrative/currentVersionId. |
| ✅ | Approving the draft promotes it to the next published version (vN -> vN+1) and the previous published version moves into history. | Version service and approval flow implement creation of a new version row and updating currentVersionId; specs cover the promotion lifecycle. |
| ✅ | Version history is visible on the document detail page: list of previous published versions with date, approver, and view/download actions. | New version-history UI component, hook and API wiring present; tests for the history component and SWR hook exist. |
| ✅ | Rendered outputs are versioned and retained; export endpoint supports requesting a specific published version and returns stored artifact (with snapshot re-render fallback). | DTO/API accepts versionId, export-payload and export-generator implement snapshot storage/fallback, version service handles render/upload, and tests cover export snapshot/render behaviour. |
| Behaviour applies uniformly across all six ISMS document types (Context, Scope, Interested Parties Register, Interested Parties Requirements, Leadership Commitment, Objectives Plan). | The PR centralises versioning (choke-point) and updates multiple per-document clients and tests so the pattern is applied across components, but the change is feature-flagged and not yet exercised against a live seeded org per the PR description. |
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
…solation, a11y
Addresses the follow-up automated review on the fix commit:
- Default export (no versionId) now serves the current PUBLISHED version whenever
one exists — so v1 stays the live/exportable default while v2 is being drafted
(CS-701). Only a never-published document renders the live draft. (P1)
- Per-version downloads now catch export failures and surface a toast instead of
failing silently / as an unhandled rejection. (P2)
- The approve snapshot reads the org profile through the same transaction client
(threaded through resolveOrgProfile/loadOrgProfile) so profile + register rows
share one point-in-time. (P2)
- Version-history download buttons get version-specific accessible labels
("Download v2 as PDF") for screen-reader / voice-control users. (P3)
Tests: 318 api + 123 app ISMS tests passing; typecheck clean.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012CweXoSSEP89mX93u3Bdcp
Contributor
Author
|
@cubic-dev-ai review it |
Contributor
@tofikwest I have started the AI code review. It will take a few minutes to complete. |
Contributor
There was a problem hiding this comment.
cubic analysis
2 issues found across 49 files
Confidence score: 3/5
- In
apps/api/src/isms/isms-version.service.ts, migrated approved ISMS versions without a stored artifact can now 404 because the fallback export path throws whencontentSnapshotis missing, which conflicts with the CS-701 requirement that approved versions stay live/exportable; this could break historical exports for existing approved records—restore a non-throwing fallback (or equivalent legacy handling) before merging. - In
apps/app/src/app/(app)/[orgId]/documents/isms/hooks/useIsmsDocumentVersions.ts, overlapping historical exports can cleardownloadingVersionIdfrom an earlier request’sfinally, making a newer export appear complete too soon and causing misleading UI state—guard the cleanup so only the active request can reset that ID.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/db/prisma/schema/isms.prisma">
<violation number="1" location="packages/db/prisma/schema/isms.prisma:130">
P2: According to linked Linear issue CS-701, version history must retain the approver for each published ISMS version; this relation nulls `publishedById` if the Member is deleted, leaving historical rows without an approver. Consider storing immutable publisher display metadata on the version or using deletion semantics that preserve the audit record.</violation>
</file>
<file name="apps/api/src/isms/documents/generate.ts">
<violation number="1" location="apps/api/src/isms/documents/generate.ts:185">
P2: Regenerating an already approved narrative document can change the working draft without marking the document as draft, so the UI/API may still report the existing approval while draft content has changed. Consider invalidating approval in the regenerate path when `draftNarrative` is written, matching `IsmsNarrativeService.save`.</violation>
</file>
<file name="apps/api/src/isms/isms.service.ts">
<violation number="1" location="apps/api/src/isms/isms.service.ts:261">
P1: According to linked Linear issue CS-701, each approved ISMS version must retain its DOCX/PDF exports so a later auditor gets exactly the approved artifact. This approval path commits the version even when `publishRenders` fails, because that method swallows render/upload errors; consider making export persistence required or surfacing a failed publish state/retry before treating approval as complete.</violation>
</file>
<file name="apps/api/src/isms/isms-version.service.ts">
<violation number="1" location="apps/api/src/isms/isms-version.service.ts:278">
P1: According to linked Linear issue CS-701, approved versions should remain live and exportable, but migrated approved ISMS versions with no stored artifact now return 404 because the fallback path throws when contentSnapshot is null. Consider backfilling contentSnapshot/artifacts or handling those legacy current versions before exposing them as currentVersionId.</violation>
</file>
Linked issue analysis
Linked issue: CS-701: [Feature] Document versioning and history for ISMS documents
| Status | Acceptance criteria | Notes |
|---|---|---|
| ✅ | Approving a draft creates an immutable published version (content snapshot) and stores rendered artifacts (PDF/DOCX). | New IsmsVersionService implements the publish/freeze flow; migration adds per-version snapshot columns; publish tests and export tests exercise the render+upload path. |
| ✅ | Editing an approved document creates a working draft without modifying the published version; published version remains live/exportable. | Draft fields moved to IsmsDocument (draftNarrative/draftSnapshot/currentVersionId); narrative save and derivation now write to the document draft and never clobber currentVersion; app components read draftNarrative for editing. |
| ✅ | Approving the draft promotes it to the new published version and previous published version moves into history. | Version service creates new IsmsDocumentVersion rows and updates currentVersionId on approve; approve flow tests cover lifecycle and promotion. |
| ✅ | Version history is visible on the document detail page with date, approver, and per-version view/download actions. | New API route & controller wiring plus a UI section and hook expose version history and per-version download buttons; component tests added for the history UI. |
| ✅ | Exporting a historical version returns the same DOCX/PDF as when that version was approved (stored file + snapshot re-render fallback). | Export DTO gained versionId, export-payload includes immutable snapshot parsing and render fallback, versioned export logic and tests included; client hook supports versionId downloads. |
| ✅ | Behaviour applies uniformly across all six ISMS documents (single choke-point / shared components). | UI uses a shared IsmsVersionHistory section and shared hooks; multiple per-document client components were updated to use the draft/currentVersion fields, and tests for several document clients were modified/added. |
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| // Neither a stored file nor a snapshot exists (only possible for a legacy / | ||
| // migrated version). Never serve current live data for a specific historical | ||
| // version — an auditor must get exactly what was approved, or a clear failure. | ||
| throw new NotFoundException( |
Contributor
There was a problem hiding this comment.
P1: According to linked Linear issue CS-701, approved versions should remain live and exportable, but migrated approved ISMS versions with no stored artifact now return 404 because the fallback path throws when contentSnapshot is null. Consider backfilling contentSnapshot/artifacts or handling those legacy current versions before exposing them as currentVersionId.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/isms/isms-version.service.ts, line 278:
<comment>According to linked Linear issue CS-701, approved versions should remain live and exportable, but migrated approved ISMS versions with no stored artifact now return 404 because the fallback path throws when contentSnapshot is null. Consider backfilling contentSnapshot/artifacts or handling those legacy current versions before exposing them as currentVersionId.</comment>
<file context>
@@ -0,0 +1,294 @@
+ // Neither a stored file nor a snapshot exists (only possible for a legacy /
+ // migrated version). Never serve current live data for a specific historical
+ // version — an auditor must get exactly what was approved, or a clear failure.
+ throw new NotFoundException(
+ 'No retained export exists for this document version',
+ );
</file context>
…ownload races - Migration no longer promotes legacy approved docs to a published version. The pre-CS-701 version row had no contentSnapshot, so serving it as currentVersion would 404 on export (regression from the default-export + no-live-fallback fixes). Legacy approved docs revert to draft and re-approve once to establish a real v1; all legacy version rows are dropped. currentVersionId is now only ever set by the approve flow, which always writes a contentSnapshot — so it can never point at a snapshot-less version. (P1) - Per-version download indicator clears only if the finishing request still owns it, so starting a second download no longer clears the first's spinner early. (P3) Not changed (deliberate, re-confirmed): approval stays committed if publishRenders fails — createPublishedVersion always persists an immutable contentSnapshot, so the version re-renders on demand (no lost export); forcing a throw would strand a doc "approved but unretryable". Tests: 318 api + 123 app ISMS tests passing; typecheck clean; migration SQL validated. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012CweXoSSEP89mX93u3Bdcp
Contributor
Author
|
@cubic-dev-ai please re-review — pushed Latest review (run against
|
Contributor
@tofikwest I have started the AI code review. It will take a few minutes to complete. |
Contributor
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/db/prisma/migrations/20260707120000_isms_document_versioning/migration.sql">
<violation number="1" location="packages/db/prisma/migrations/20260707120000_isms_document_versioning/migration.sql:49">
P2: Existing approved ISMS documents become unapproved after this migration: approved rows are reset to `draft` and every version row is deleted, leaving `currentVersionId` null and version history/export rows empty. Consider backfilling a real v1 snapshot for approved docs, or making this destructive reset explicitly limited to environments with no existing approvals.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
Contributor
There was a problem hiding this comment.
cubic analysis
1 issue found across 49 files
Confidence score: 2/5
packages/db/prisma/migrations/20260707120000_isms_document_versioning/migration.sqlmay fail mid-migration ifIsmsDocumentVersion.publishedByIdcontains orphaned member IDs, which can block deployment before the script reaches its own cleanup step; reorder the steps to null invalidpublishedByIdvalues (or pre-clean those rows) before adding/enforcing the FK changes, then rerun migration validation before merging.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/db/prisma/schema/isms.prisma">
<violation number="1" location="packages/db/prisma/schema/isms.prisma:130">
P2: According to linked Linear issue CS-701, version history must retain the approver for each published ISMS version; this relation nulls `publishedById` if the Member is deleted, leaving historical rows without an approver. Consider storing immutable publisher display metadata on the version or using deletion semantics that preserve the audit record.</violation>
</file>
<file name="apps/api/src/isms/documents/generate.ts">
<violation number="1" location="apps/api/src/isms/documents/generate.ts:185">
P2: Regenerating an already approved narrative document can change the working draft without marking the document as draft, so the UI/API may still report the existing approval while draft content has changed. Consider invalidating approval in the regenerate path when `draftNarrative` is written, matching `IsmsNarrativeService.save`.</violation>
</file>
<file name="apps/api/src/isms/isms.service.ts">
<violation number="1" location="apps/api/src/isms/isms.service.ts:261">
P1: According to linked Linear issue CS-701, each approved ISMS version must retain its DOCX/PDF exports so a later auditor gets exactly the approved artifact. This approval path commits the version even when `publishRenders` fails, because that method swallows render/upload errors; consider making export persistence required or surfacing a failed publish state/retry before treating approval as complete.</violation>
</file>
<file name="apps/api/src/isms/isms-version.service.ts">
<violation number="1" location="apps/api/src/isms/isms-version.service.ts:278">
P1: According to linked Linear issue CS-701, approved versions should remain live and exportable, but migrated approved ISMS versions with no stored artifact now return 404 because the fallback path throws when contentSnapshot is null. Consider backfilling contentSnapshot/artifacts or handling those legacy current versions before exposing them as currentVersionId.</violation>
</file>
Linked issue analysis
Linked issue: CS-701: [Feature] Document versioning and history for ISMS documents
| Status | Acceptance criteria | Notes |
|---|---|---|
| ✅ | Approving a draft creates an immutable published version (vN) stored as a frozen snapshot | Schema and service changes implement a publish/create-version flow and tests cover the approval lifecycle. |
| ✅ | Editing an approved document creates/updates the working draft without touching the published version (published stays intact) | Draft narrative and snapshot moved onto IsmsDocument (not the version row); narrative saves write to draftNarrative instead of mutating versions. |
| ✅ | Published version remains live and is exportable while a draft is edited (default export serves published artifact) | Export payload logic and API support returning the current published artifact by default; export route/dto and client hook support versioned/default exports. |
| ✅ | Approving a draft promotes it to the next published version and moves the previous version into history | Version service and approve flow create a new IsmsDocumentVersion and mark it current; tests exercise the publish/promotion behavior. |
| ✅ | Version history is exposed on the document detail page with date, approver, and per-version view/download actions | New API surface and frontend hook/component provide version-history feed and per-version download UI; components wired into the document shell used by all ISMS docs. |
| ✅ | Rendered outputs (PDF/DOCX) are retained/versioned so historical exports return the same approved artifact | Per-version content snapshot + attachments integration and export-payload support stored-file + render fallback; migration adds contentSnapshot and code uploads renders after transaction. |
| ✅ | Behavior applies across all six ISMS documents (Context, Scope, Interested Parties, Requirements, Leadership, Objectives) | The UI uses a single IsmsDocumentShell and the versioning changes are applied at the document/service layer so all document clients inherit the behavior; multiple client tests updated to the new draft/currentVersion shape. |
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
Addresses the latest review on the CS-701 migration: - Non-destructive: existing approved documents keep their approved status. The migration no longer resets them to draft — it only drops the legacy (pre-CS-701, snapshot-less) version rows and leaves currentVersionId null, so an approved doc exports its current content on demand and captures a real versioned artifact on its next approval. currentVersionId is only ever set by the approve flow, which always writes a contentSnapshot. (P2) - Defensive ordering: the legacy-row DELETE now runs BEFORE the foreign keys are added, so FK validation can never trip on a stale publishedById. (In practice publishedById was a dead/always-null column pre-migration, so it could not have failed — but the ordering makes it robust regardless.) (P2) Verified: migration applies cleanly from scratch (fresh DB, all migrations deploy); 318 api + 123 app ISMS tests passing; typecheck clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012CweXoSSEP89mX93u3Bdcp
Contributor
Author
|
@cubic-dev-ai please re-review — pushed Latest review (against
Also re-confirmed one intentional non-change (reply on that thread): approval staying committed if the post-commit S3 render fails is by design — the immutable Verified: migration applies cleanly from scratch (fresh DB, full |
Contributor
@tofikwest I have started the AI code review. It will take a few minutes to complete. |
Contributor
There was a problem hiding this comment.
cubic analysis
No issues found across 49 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Linked issue analysis
Linked issue: CS-701: [Feature] Document versioning and history for ISMS documents
| Status | Acceptance criteria | Notes |
|---|---|---|
| ✅ | Approving a draft creates an immutable published version (v1) and records publish metadata | PR adds IsmsDocumentVersion publish flow, stores contentSnapshot/changelog/publishedBy and sets IsmsDocument.currentVersionId inside the approve txn; covered by unit tests for the version lifecycle and approve flow. |
| ✅ | Editing an approved document creates a working draft without mutating the published version; published version remains live/exportable | Draft fields moved onto IsmsDocument (draftNarrative/draftSnapshot); narrative saves write draftNarrative and do not touch published version; UI now reads draft vs currentVersion and shows editing signal. |
| ✅ | Re-approving a draft promotes it to the next published version and previous published version moves to history | Publish flow creates a new immutable version row and updates currentVersionId; version lifecycle and publish behavior have dedicated service and tests covering promotion. |
| ✅ | Version history is exposed on the document detail page with date, approver, and per-version view/download actions | API provides versions endpoint and controller wiring; client exposes useIsmsDocumentVersions hook and IsmsVersionHistory UI component with tests for rendering and download actions. |
| ✅ | Historical exports (DOCX/PDF) are retained and downloadable; export API supports requesting a specific published version with a fallback re-render | Export DTO and export-payload implement versionId parameter, stored contentSnapshot + attachment handling, and snapshot re-render fallback; tests cover export payload parsing and version export flow. |
| ✅ | Behavior applies uniformly across the six ISMS document types | The change centralizes versioning on IsmsDocument and IsmsVersionService and updates the shared document shell/hooks and each document client to use draftNarrative/currentVersion, with component tests updated for multiple doc types. |
| ✅ | Migration/backfill preserves existing approvals and is defensively backfilled | Migration SQL includes backfill steps and the PR describes defensive ordering to avoid FK issues; service behavior keeps currentVersionId set only via approve flow. |
You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.
Re-trigger cubic
tofikwest
deleted the
tofik/cs-701-feature-document-versioning-and-history-for-isms-documents
branch
July 7, 2026 17:27
This was referenced Jul 7, 2026
claudfuen
pushed a commit
that referenced
this pull request
Jul 7, 2026
# [3.99.0](v3.98.1...v3.99.0) (2026-07-07) ### Bug Fixes * **integrations:** don't persist unverified GitHub installation_id ([#3370](#3370)) ([9c6f6fa](9c6f6fa)) * **isms:** lock before writing control links so edits serialize with approve ([#3369](#3369)) ([a21f866](a21f866)) * **isms:** production-deploy review follow-ups (CS-701) ([#3365](#3365)) ([d756d4a](d756d4a)), closes [#3364](#3364) * **isms:** serialize approve against ALL register/content edits (central lock) ([#3367](#3367)) ([5c88a3d](5c88a3d)) * **isms:** serialize approve/decline to prevent concurrent double-publish (CS-701) ([#3366](#3366)) ([b158bc9](b158bc9)), closes [#3364](#3364) ### Features * **integrations:** add read-only GitHub App integration (CS-710) ([#3363](#3363)) ([a5a9243](a5a9243)) * **isms:** document versioning and history (CS-701) ([#3361](#3361)) ([d95dc2e](d95dc2e))
Contributor
|
🎉 This PR is included in version 3.99.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
This branch was successfully deployed
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
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.
Why
Approving an ISMS document made it published, but any subsequent edit reverted the whole document to unpublished and lost the approved version — no history, no way to draft an update without losing what's live. That's an audit exposure (auditors expect the exact version that was live when they sampled) and it discourages customers from keeping documents current.
What this delivers
Brings the six ISMS documents (Context 4.1, Interested Parties Register 4.2a, Requirements 4.2b/c, Scope 4.3, Leadership 5.1, Objectives 6.2) onto the same versioning model the Policies module already uses — published version + working draft + version history.
How it works (design)
The schema was already shaped for this but only ever kept one mutable version. Mirroring Policies'
draftContent+currentVersionIdpattern:IsmsDocument.draftNarrativeare the editable draft;IsmsDocumentVersionrows are published-only immutable snapshots, pointed to byIsmsDocument.currentVersionId.IsmsVersionServicefreezes a full content snapshot into a new version inside the txn, then renders + uploads PDF/DOCX to S3 after the txn (so an S3 hiccup never orphans a row) — the Policies publish pattern.statusto draft but never touchescurrentVersion, so the published version stays live.Scope of changes
IsmsDocument.{currentVersion,draftNarrative,draftSnapshot},IsmsDocumentVersion.{contentSnapshot,changelog,publishedBy}; defensive backfill (feature is pre-GA / flag-gated).IsmsVersionService, sharedexport-payloadutil, reworkedapprove(),AttachmentsModulewired in,GET /v1/isms/documents/:id/versions, optionalversionIdon export.useIsmsDocumentVersionshook,IsmsVersionHistorysection (one choke-point → all six documents inherit it), per-version download, "Published vN / editing creates a new draft" signal.Testing
npx jest src/isms); new specs for the version lifecycle, snapshot, export routing, and best-effort renders.npx vitest run …/documents/isms); new component + hook tests, permission-gated coverage retained.is-isms-enabled); covered by unit/integration tests.Closes CS-701.
🤖 Generated with Claude Code
https://claude.ai/code/session_012CweXoSSEP89mX93u3Bdcp
Summary by cubic
Adds versioning and history to ISMS documents (CS-701) so approved versions stay live while edits happen in a draft. Migration preserves existing approvals; exports default to the current published artifact with reliable, org‑scoped per‑version downloads.
New Features
versionIdsupported on export.GET /v1/isms/documents/:id/versionsand a new UI section with per-version downloads.IsmsDocumentgainscurrentVersionId,draftNarrative,draftSnapshot;IsmsDocumentVersiongainscontentSnapshot,changelog, and publisher linkage. NewIsmsVersionServicefreezes versions and renders/uploads exports after the DB transaction.Bug Fixes
currentVersionIdis only set by the approve flow; foreign keys are added after legacy cleanup.Written for commit 71cefa7. Summary will update on new commits.