diff --git a/openapi.yaml b/openapi.yaml index ea9335f5..02e5704d 100644 --- a/openapi.yaml +++ b/openapi.yaml @@ -714,6 +714,68 @@ paths: '500': $ref: '#/components/responses/InternalServerError' + /specs/{id}/paragraphs/{nodeId}/acknowledgement: + patch: + operationId: acknowledgeParagraph + summary: Acknowledge or un-acknowledge a note or textBox object node + description: > + Per-node acknowledgement (#545, ADR-079 follow-on): the specifier + affirms they have read and accepted a `note` or a `textBox` `object` + node. `acknowledged: true` clears the issuance-readiness gate's + `specifier_note_present` / `body_object_present` finding for it + WITHOUT removing or hiding the content — it still renders exactly as + before in every output format. `acknowledged: false` reverses it. + Deliberately separate state from `vanish`: only `note` nodes and + `textBox`-kind `object` nodes are acknowledgeable — a `table`-kind + object (structural content, ADR-072) or any other node type is + rejected 422. The toggle is idempotent — re-sending the same value is + a no-op that returns the node unchanged without minting a new content + version. The node must belong to the spec in the path (else 403). The + composed edit gate (ADR-018) rejects an archived spec or one locked + upstream in a DMS (409). + tags: [specs] + parameters: + - $ref: '#/components/parameters/SpecId' + - $ref: '#/components/parameters/NodeId' + requestBody: + required: true + content: + application/json: + schema: + type: object + required: [acknowledged] + properties: + acknowledged: + type: boolean + description: true to acknowledge, false to un-acknowledge. + actorLabel: + $ref: '#/components/schemas/ActorLabel' + responses: + '200': + description: Updated paragraph node (with its subtree) + content: + application/json: + schema: + allOf: + - $ref: '#/components/schemas/SuccessResponse' + - type: object + required: [data] + properties: + data: + $ref: '#/components/schemas/SpecNode' + '400': + $ref: '#/components/responses/BadRequest' + '403': + $ref: '#/components/responses/Forbidden' + '404': + $ref: '#/components/responses/NotFound' + '409': + $ref: '#/components/responses/WriteConflict' + '422': + $ref: '#/components/responses/UnprocessableEntity' + '500': + $ref: '#/components/responses/InternalServerError' + /specs/{id}/paragraphs/{nodeId}/reject: patch: operationId: rejectParagraph @@ -989,6 +1051,75 @@ paths: '500': $ref: '#/components/responses/InternalServerError' + /specs/{id}/paragraphs/{nodeId}/comments/{index}/closure: + patch: + operationId: setCommentClosed + summary: Close or reopen a source-document review comment + description: > + A mutable comment-closure toggle (#545, ADR-079 follow-on) — the only + supported path to clear the issuance-readiness gate's `open_comment` + finding on an existing spec. `closed: true` closes the comment at + `index` (captured in `source_facts.comments`); `false` reopens it. + The node must belong to the spec (else 403). `no-comment` (no comment + exists at `index`) is a lookup miss → 404, not a validation failure. + The toggle is idempotent — re-sending the same value is a no-op that + returns the node unchanged without minting a new content version. + Passes the composed edit gate (ADR-018): archived/upstream-locked → + 409. `POST .../accept-as-note` also closes the originating comment as + part of accepting it (#545) — this endpoint is for closing (or + reopening) a comment independently of accepting it as a note. + tags: [specs] + parameters: + - $ref: '#/components/parameters/SpecId' + - $ref: '#/components/parameters/NodeId' + - name: index + in: path + required: true + schema: + type: integer + minimum: 0 + description: Zero-based index into the anchor's source_facts.comments. + requestBody: + required: true + content: + application/json: + schema: + type: object + required: [closed] + properties: + closed: + type: boolean + description: true to close, false to reopen. + actorLabel: + $ref: '#/components/schemas/ActorLabel' + responses: + '200': + description: Updated paragraph node (with its subtree) + content: + application/json: + schema: + allOf: + - $ref: '#/components/schemas/SuccessResponse' + - type: object + required: [data] + properties: + data: + $ref: '#/components/schemas/SpecNode' + '400': + $ref: '#/components/responses/BadRequest' + '403': + $ref: '#/components/responses/Forbidden' + '404': + description: Paragraph not found, or no comment exists at that index + content: + application/json: + schema: + $ref: '#/components/schemas/ErrorResponse' + '409': + $ref: '#/components/responses/WriteConflict' + '500': + $ref: '#/components/responses/InternalServerError' + /specs/{id}/paragraphs/{nodeId}/associations: get: operationId: listAssociations @@ -8069,6 +8200,15 @@ components: vanish: type: boolean description: Hidden (w:vanish) paragraph — editorial note, not owner-facing + acknowledged: + type: boolean + description: >- + A specifier has read and accepted this `note` or `textBox` + `object` node (#545, ADR-079 follow-on), clearing the + issuance-readiness gate's `specifier_note_present` / + `body_object_present` finding for it WITHOUT removing or + hiding the content — it still renders exactly as before. + Deliberately separate from `vanish`. Present only when true. pageBreakBefore: type: boolean description: >- @@ -8643,7 +8783,26 @@ components: ] op: type: string - enum: [edit, insert, remove, restore, merge, accept-note, restructure] + description: > + Mirrors the `paragraph_versions.op` enum exactly (migrations + 046/055). `acknowledge`/`unacknowledge` and + `close-comment`/`reopen-comment` are the ADR-079 follow-on + readiness-clearance ops (#545) — each writes a history row, so a + real history response can carry them. + enum: + [ + edit, + insert, + remove, + restore, + merge, + accept-note, + restructure, + acknowledge, + unacknowledge, + close-comment, + reopen-comment, + ] contentVersion: type: [integer, 'null'] minimum: 1 diff --git a/src/api/contract.integration.test.ts b/src/api/contract.integration.test.ts index af1ce559..1b2936ed 100644 --- a/src/api/contract.integration.test.ts +++ b/src/api/contract.integration.test.ts @@ -89,6 +89,13 @@ const RESPONSE_COVERED = new Set([ 'patch /specs/{}/paragraphs/{}/removal', 'patch /specs/{}/paragraphs/{}/reject', 'get /revisions/{}', + // #545's two clearance ops return a SpecNode for the same reason and were + // allowlisted on that basis while this branch was open. #649 removed the + // reason, so they are response-verified (and INV-6 exact-match-verified) + // instead — in readiness-clearance.integration.test.ts, next to the + // behavioural assertions, rather than in the block below. + 'patch /specs/{}/paragraphs/{}/acknowledgement', + 'patch /specs/{}/paragraphs/{}/comments/{}/closure', ]); // Documented JSON ops not yet response-verified (burned down in PR2…N). diff --git a/src/api/editability.ts b/src/api/editability.ts index 51ee82a1..380df405 100644 --- a/src/api/editability.ts +++ b/src/api/editability.ts @@ -4,6 +4,7 @@ import { PatchEditabilityBodySchema, ReclassifyBodySchema, AcceptNoteBodySchema, + PatchCommentClosureBodySchema, } from '../ast/index.js'; import type { ConventionRules, Editability } from '../ast/index.js'; import { @@ -11,9 +12,10 @@ import { clearSpecEditabilityOverride, reclassifySpec, acceptCommentAsNote, + setParagraphCommentClosed, ConventionValidationError, } from '../db/index.js'; -import type { OwnershipResult, AcceptNoteOutcome } from '../db/index.js'; +import type { OwnershipResult, AcceptNoteOutcome, SetCommentClosedResult } from '../db/index.js'; import { gateErrorResponse } from './edit-gate-response.js'; import { logger } from '../lib/logger.js'; @@ -192,3 +194,66 @@ export async function acceptAsNoteHandler(req: Request, res: Response): Promise< res.status(500).json({ success: false, error: 'internal server error' }); } } + +// ── comment closure ──────────────────────────────────────────────────────── + +function sendCommentClosedResult(res: Response, result: SetCommentClosedResult): void { + switch (result.status) { + case 'not-found': + res.status(404).json({ success: false, error: 'paragraph not found' }); + return; + case 'wrong-spec': + res.status(403).json({ success: false, error: 'paragraph does not belong to this spec' }); + return; + case 'no-comment': + // A lookup miss ("nothing exists at this index to toggle"), not a + // validation failure — mirrors `not-found` rather than acceptAsNoteHandler's + // 422 for "you tried to create something with no material". + res.status(404).json({ success: false, error: 'no comment at that index' }); + return; + case 'updated': + res.status(200).json({ success: true, data: result.node }); + return; + } +} + +/** + * PATCH /specs/:id/paragraphs/:nodeId/comments/:index/closure — a mutable + * comment-closure toggle (#545, ADR-079 follow-on): the only supported path + * to clear `open_comment` on an existing spec. `{ closed: true }` closes the + * comment at `index`; `false` reopens it. Idempotent — a no-op returns the + * node unchanged without bumping any version. Passes the composed edit gate + * (ADR-018): archived/upstream-locked → 409. + */ +export async function closeCommentHandler(req: Request, res: Response): Promise { + const ids = parseIds(req, res); + if (!ids) return; + const index = INDEX_SCHEMA.safeParse(req.params['index']); + if (!index.success) { + res.status(400).json({ success: false, error: 'invalid comment index' }); + return; + } + const body = PatchCommentClosureBodySchema.safeParse(req.body); + if (!body.success) { + res.status(400).json({ success: false, error: 'closed must be a boolean' }); + return; + } + try { + const result = await setParagraphCommentClosed( + ids.specId, + ids.nodeId, + index.data, + body.data.closed, + body.data.actorLabel + ); + sendCommentClosedResult(res, result); + } catch (err) { + const gate = gateErrorResponse(err); + if (gate) { + res.status(gate.status).json(gate.body); + return; + } + logger.error({ err }, 'close comment failed'); + res.status(500).json({ success: false, error: 'internal server error' }); + } +} diff --git a/src/api/paragraph-acknowledgement.ts b/src/api/paragraph-acknowledgement.ts new file mode 100644 index 00000000..50d1fc51 --- /dev/null +++ b/src/api/paragraph-acknowledgement.ts @@ -0,0 +1,70 @@ +import type { Request, Response } from 'express'; +import { z } from 'zod'; +import { PatchAcknowledgementBodySchema } from '../ast/index.js'; +import { setParagraphAcknowledged } from '../db/index.js'; +import { gateErrorResponse } from './edit-gate-response.js'; +import { logger } from '../lib/logger.js'; + +/** + * PATCH /specs/:id/paragraphs/:nodeId/acknowledgement — per-node + * acknowledgement (#545, ADR-079 follow-on). `{ acknowledged: true }` clears + * the readiness gate's `specifier_note_present` / `body_object_present` + * finding for a `note` or `textBox` `object` node WITHOUT removing or hiding + * the content — it still renders exactly as before. Only `note` nodes and + * `textBox`-kind `object` nodes are acknowledgeable; every other node type + * (including a `table`-kind object, ADR-072) is rejected 422. The toggle is + * idempotent — a no-op returns the node unchanged without bumping any + * version. Passes the composed edit gate (ADR-018): archived/upstream-locked + * → 409. Mirrors removeParagraphHandler's structure exactly. + */ +export async function acknowledgeParagraphHandler(req: Request, res: Response): Promise { + const specId = z.uuid().safeParse(req.params['id']); + if (!specId.success) { + res.status(400).json({ success: false, error: 'invalid spec id' }); + return; + } + const nodeId = z.uuid().safeParse(req.params['nodeId']); + if (!nodeId.success) { + res.status(400).json({ success: false, error: 'invalid node id' }); + return; + } + const body = PatchAcknowledgementBodySchema.safeParse(req.body); + if (!body.success) { + res.status(400).json({ success: false, error: 'acknowledged must be a boolean' }); + return; + } + + try { + const result = await setParagraphAcknowledged( + specId.data, + nodeId.data, + body.data.acknowledged, + body.data.actorLabel + ); + switch (result.status) { + case 'not-found': + res.status(404).json({ success: false, error: 'paragraph not found' }); + return; + case 'wrong-spec': + res.status(403).json({ success: false, error: 'paragraph does not belong to this spec' }); + return; + case 'not-acknowledgeable': + res.status(422).json({ + success: false, + error: `node type "${result.nodeType}" cannot be acknowledged — only note nodes and textBox objects are`, + }); + return; + case 'updated': + res.status(200).json({ success: true, data: result.node }); + return; + } + } catch (err) { + const gate = gateErrorResponse(err); + if (gate) { + res.status(gate.status).json(gate.body); + return; + } + logger.error({ err }, 'acknowledge paragraph failed'); + res.status(500).json({ success: false, error: 'internal server error' }); + } +} diff --git a/src/api/paragraph-clearance-routes.ts b/src/api/paragraph-clearance-routes.ts new file mode 100644 index 00000000..b4de09d2 --- /dev/null +++ b/src/api/paragraph-clearance-routes.ts @@ -0,0 +1,28 @@ +import type { Router as RouterType } from 'express'; +import { acknowledgeParagraphHandler } from './paragraph-acknowledgement.js'; +import { + patchEditabilityHandler, + reclassifyHandler, + acceptAsNoteHandler, + closeCommentHandler, +} from './editability.js'; + +/** + * Wires the editability/comment-resolution and readiness-finding-clearing + * routes onto the shared router instance — extracted out of router.ts to + * keep it under the enforced ESLint `max-lines: 400` (project override, + * CLAUDE.md), mirroring `registerCheckpointRoutes`'s identical extraction. + * The last two routes are new in #545 (ADR-079 follow-on) — acknowledgement + * and comment closure, the two remaining supported paths to clear a + * readiness finding. Registers directly on `router` (never a mounted + * sub-router) so `expressRouteManifest` + * (src/test-utils/contract/validate-response.js), which only walks one level + * of `router.stack`, still sees every route. + */ +export function registerParagraphClearanceRoutes(router: RouterType): void { + router.patch('/specs/:id/paragraphs/:nodeId/editability', patchEditabilityHandler); + router.post('/specs/:id/reclassify', reclassifyHandler); + router.post('/specs/:id/paragraphs/:nodeId/comments/:index/accept-as-note', acceptAsNoteHandler); + router.patch('/specs/:id/paragraphs/:nodeId/acknowledgement', acknowledgeParagraphHandler); + router.patch('/specs/:id/paragraphs/:nodeId/comments/:index/closure', closeCommentHandler); +} diff --git a/src/api/readiness-clearance.integration.test.ts b/src/api/readiness-clearance.integration.test.ts new file mode 100644 index 00000000..7066b70d --- /dev/null +++ b/src/api/readiness-clearance.integration.test.ts @@ -0,0 +1,394 @@ +import { randomUUID } from 'node:crypto'; +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import express from 'express'; +import type { Server } from 'http'; +import { router } from './router.js'; +import { errorHandler } from './middleware/error.js'; +import { pool } from '../db/index.js'; +import type { SourceFacts } from '../ast/index.js'; +import { assertResponse, assertResponseExact } from '../test-utils/contract/validate-response.js'; + +// #545, ADR-079 follow-on — end-to-end proof that every one of the four +// readiness-finding kinds now has a SUPPORTED API path to clear it, driven +// through the real REST endpoints (never the DB layer directly), closing +// the "block, never strip — but there is no path to fix the source and +// retry" gap PR 544 (ADR-079) left open. + +const suffix = randomUUID().slice(0, 8); +const specIds: string[] = []; +let specCounter = 0; +let paraCounter = 0; +let server: Server; +let baseUrl: string; + +async function req( + method: string, + path: string, + body?: unknown +): Promise<{ status: number; body: unknown }> { + const res = await fetch(`${baseUrl}${path}`, { + method, + headers: body === undefined ? {} : { 'Content-Type': 'application/json' }, + ...(body === undefined ? {} : { body: JSON.stringify(body) }), + }); + const text = await res.text(); + return { status: res.status, body: text.length > 0 ? JSON.parse(text) : undefined }; +} + +async function newSpec(section: string, title: string): Promise { + const src = `rc_${suffix}_${String(++specCounter).padStart(2, '0')}`; + const r = await pool.query<{ id: string }>( + `INSERT INTO specs (section, title, source, library_id) + VALUES ($1, $2, $3, (SELECT id FROM libraries WHERE name = 'Default Company Master')) + RETURNING id`, + [section, title, src] + ); + const id = r.rows[0]?.id; + if (id === undefined) throw new Error(`newSpec: no id for ${section}`); + specIds.push(id); + return id; +} + +interface ParaOptions { + readonly nodeType?: string; + readonly facts?: SourceFacts; + readonly objectData?: Record; +} + +async function addParagraph(specId: string, text: string, opts: ParaOptions = {}): Promise { + const id = randomUUID(); + await pool.query( + `INSERT INTO paragraphs + (id, spec_id, parent_id, node_type, text, position, source_facts, object_data) + VALUES ($1, $2, NULL, $3, $4, $5, $6::jsonb, $7::jsonb)`, + [ + id, + specId, + opts.nodeType ?? 'pr1', + text, + ++paraCounter, + JSON.stringify(opts.facts ?? {}), + opts.objectData ? JSON.stringify(opts.objectData) : null, + ] + ); + return id; +} + +async function readinessKinds(specId: string): Promise { + const r = await req('GET', `/specs/${specId}/readiness-report`); + const body = r.body as { data: { findings: readonly { type: string }[] } }; + return body.data.findings.map((f) => f.type); +} + +// Every blob node must carry exactly one element tag (object-block.ts's +// buildImportedXmlComponent) — `blob: [{}]` is valid for readiness-only +// assertions (evaluateSpecReadiness never inspects the blob) but fails a +// real DOCX generation, which the full end-to-end scenario below exercises. +const textBoxObject = { + kind: 'textBox', + floating: false, + generation: 'drawingml', + blob: [ + { + 'w:p': [{ 'w:r': [{ 'w:drawing': [{ 'w:txbxContent': [{ 'w:p': [{ 'w:r': [] }] }] }] }] }], + }, + ], +}; + +beforeAll(async () => { + const app = express(); + app.disable('x-powered-by'); + app.use(express.json()); + app.use(router); + app.use(errorHandler); + await new Promise((resolve) => { + server = app.listen(0, () => resolve()); + }); + const addr = server.address(); + baseUrl = `http://localhost:${typeof addr === 'object' && addr ? addr.port : 3000}`; +}); + +afterAll(async () => { + for (const id of specIds) await pool.query('DELETE FROM specs WHERE id = $1', [id]); + await new Promise((resolve) => server.close(() => resolve())); +}); + +describe('PATCH .../paragraphs/:nodeId — resolving a placeholder clears unresolved_choice_token (#545)', () => { + it('editing the text through the real endpoint clears the finding end to end', async () => { + const specId = await newSpec('09 91 26', 'Choice Token Clearance'); + const nodeId = await addParagraph(specId, 'Provide [insert value] finish.', { + facts: { + choiceTokens: [{ kind: 'bracket', options: ['insert value'], span: [8, 22] }], + banner: '** SPECIAL NOTICE **', + }, + }); + + expect(await readinessKinds(specId)).toEqual(['unresolved_choice_token']); + + const patch = await req('PATCH', `/specs/${specId}/paragraphs/${nodeId}`, { + text: 'Provide latex enamel finish.', + }); + expect(patch.status).toBe(200); + const patchBody = patch.body as { data: { meta: { sourceFacts?: SourceFacts } } }; + // The finding-clearing key is gone entirely (never an empty array)... + expect(patchBody.data.meta.sourceFacts?.choiceTokens).toBeUndefined(); + // ...and an unrelated key survives the same write byte-identical (#545 — + // "silently dropping unrelated source facts would be a far worse bug"). + expect(patchBody.data.meta.sourceFacts?.banner).toBe('** SPECIAL NOTICE **'); + + expect(await readinessKinds(specId)).toEqual([]); + }); +}); + +describe('PATCH .../acknowledgement — clears specifier_note_present / body_object_present (#545)', () => { + it('acknowledging a note clears its finding without changing its text', async () => { + const specId = await newSpec('09 91 27', 'Note Acknowledgement'); + const nodeId = await addParagraph(specId, 'Confirm topcoat sheen with owner.', { + nodeType: 'note', + }); + + expect(await readinessKinds(specId)).toEqual(['specifier_note_present']); + + const ack = await req('PATCH', `/specs/${specId}/paragraphs/${nodeId}/acknowledgement`, { + acknowledged: true, + }); + expect(ack.status).toBe(200); + const ackBody = ack.body as { data: { text: string; meta: { acknowledged?: boolean } } }; + expect(ackBody.data.text).toBe('Confirm topcoat sheen with owner.'); + expect(ackBody.data.meta.acknowledged).toBe(true); + + // This op is RESPONSE_COVERED in contract.integration.test.ts rather than + // allowlisted. It could not have been until #649 (PR 657): its success body + // is a SpecNode, whose self-referential `children: SpecNode[]` blew ajv's + // schema-traversal stack under loadSpec()'s old full $ref dereference, so + // every SpecNode-returning op was exempted for that structural reason. + // Bundling removed the reason, so the exemption goes with it — an endpoint + // that ships without a response check is a documented shape nothing proves. + await assertResponse('patch', '/specs/{id}/paragraphs/{nodeId}/acknowledgement', 200, ackBody); + await assertResponseExact( + 'patch', + '/specs/{id}/paragraphs/{nodeId}/acknowledgement', + 200, + ackBody + ); + // INV-6: the exact-match variant genuinely rejects an undocumented key here, + // rather than passing vacuously — the failure mode #640 found. + await expect( + assertResponseExact('patch', '/specs/{id}/paragraphs/{nodeId}/acknowledgement', 200, { + ...ackBody, + data: { ...ackBody.data, rogueKey: 'nope' }, + }) + ).rejects.toThrow(/does not document/); + + expect(await readinessKinds(specId)).toEqual([]); + }); + + it('acknowledging a textBox object clears its finding without changing its content', async () => { + const specId = await newSpec('09 91 28', 'Object Acknowledgement'); + const nodeId = await addParagraph(specId, '', { + nodeType: 'object', + objectData: textBoxObject, + }); + + expect(await readinessKinds(specId)).toEqual(['body_object_present']); + + const ack = await req('PATCH', `/specs/${specId}/paragraphs/${nodeId}/acknowledgement`, { + acknowledged: true, + }); + expect(ack.status).toBe(200); + + expect(await readinessKinds(specId)).toEqual([]); + }); + + it('un-acknowledging restores the finding — the toggle is a real toggle, not a one-way door', async () => { + const specId = await newSpec('09 91 29', 'Note Un-acknowledgement'); + const nodeId = await addParagraph(specId, 'Verify with owner.', { nodeType: 'note' }); + + await req('PATCH', `/specs/${specId}/paragraphs/${nodeId}/acknowledgement`, { + acknowledged: true, + }); + expect(await readinessKinds(specId)).toEqual([]); + + await req('PATCH', `/specs/${specId}/paragraphs/${nodeId}/acknowledgement`, { + acknowledged: false, + }); + expect(await readinessKinds(specId)).toEqual(['specifier_note_present']); + }); + + it('422s a node type that cannot produce either acknowledgeable finding', async () => { + const specId = await newSpec('09 91 30', 'Non-acknowledgeable Node'); + const nodeId = await addParagraph(specId, 'Ordinary body text.'); + + const ack = await req('PATCH', `/specs/${specId}/paragraphs/${nodeId}/acknowledgement`, { + acknowledged: true, + }); + expect(ack.status).toBe(422); + }); +}); + +describe('PATCH .../comments/:index/closure — the only supported path to clear open_comment (#545)', () => { + it('closing the comment clears the finding; reopening restores it', async () => { + const specId = await newSpec('09 91 31', 'Comment Closure'); + const nodeId = await addParagraph(specId, 'Verify substrate.', { + facts: { comments: [{ author: 'Jane', text: 'still open', anchor: [0, 5], closed: false }] }, + }); + + expect(await readinessKinds(specId)).toEqual(['open_comment']); + + const close = await req('PATCH', `/specs/${specId}/paragraphs/${nodeId}/comments/0/closure`, { + closed: true, + }); + expect(close.status).toBe(200); + // Response-covered for the same reason as the acknowledgement op above: + // #649 made this SpecNode-shaped body validatable, so it is checked rather + // than exempted. + await assertResponse( + 'patch', + '/specs/{id}/paragraphs/{nodeId}/comments/{index}/closure', + 200, + close.body + ); + await assertResponseExact( + 'patch', + '/specs/{id}/paragraphs/{nodeId}/comments/{index}/closure', + 200, + close.body + ); + const closeBody = close.body as { data: Record }; + await expect( + assertResponseExact( + 'patch', + '/specs/{id}/paragraphs/{nodeId}/comments/{index}/closure', + 200, + { ...closeBody, data: { ...closeBody.data, rogueKey: 'nope' } } + ) + ).rejects.toThrow(/does not document/); + expect(await readinessKinds(specId)).toEqual([]); + + const reopen = await req('PATCH', `/specs/${specId}/paragraphs/${nodeId}/comments/0/closure`, { + closed: false, + }); + expect(reopen.status).toBe(200); + expect(await readinessKinds(specId)).toEqual(['open_comment']); + }); + + it('404s a comment index that does not exist (a lookup miss, not a validation failure)', async () => { + const specId = await newSpec('09 91 32', 'Comment Closure Miss'); + const nodeId = await addParagraph(specId, 'No comments here.'); + + const close = await req('PATCH', `/specs/${specId}/paragraphs/${nodeId}/comments/0/closure`, { + closed: true, + }); + expect(close.status).toBe(404); + }); +}); + +describe('accept-as-note also closes the originating comment (#545 regression)', () => { + it('no longer leaves the original comment open — accepting a comment no longer strictly increases blocking findings', async () => { + const specId = await newSpec('09 91 33', 'Accept As Note Closes Comment'); + const nodeId = await addParagraph(specId, 'Verify substrate condition.', { + facts: { + comments: [{ author: 'Jane', text: 'pick a primer', anchor: [0, 5], closed: false }], + }, + }); + + expect(await readinessKinds(specId)).toEqual(['open_comment']); + + const accept = await req( + 'POST', + `/specs/${specId}/paragraphs/${nodeId}/comments/0/accept-as-note` + ); + expect(accept.status).toBe(201); + + // Before #545: open_comment (still open) PLUS the new note's + // specifier_note_present — accepting a comment strictly increased the + // blocking-finding count. After #545: the comment is closed as part of + // accepting it, so only the new note's finding remains. + expect(await readinessKinds(specId)).toEqual(['specifier_note_present']); + + const tree = await req('GET', `/specs/${specId}`); + const treeBody = tree.body as { + data: { parts: readonly { meta: { sourceFacts?: SourceFacts } }[] }; + }; + const anchor = treeBody.data.parts.find((p) => p.meta.sourceFacts?.comments); + expect(anchor?.meta.sourceFacts?.comments?.[0]?.closed).toBe(true); + }); +}); + +describe('end-to-end: a final-mode issuance blocked by all four finding kinds succeeds once every finding is cleared through supported API paths ALONE (#545)', () => { + it('drives the full block → clear → succeed scenario with overrideReadinessGate unset', async () => { + const specId = await newSpec('09 91 34', 'Full Readiness Clearance E2E'); + + const choiceNodeId = await addParagraph(specId, 'Provide [insert value] sealant.', { + facts: { choiceTokens: [{ kind: 'bracket', options: ['insert value'], span: [8, 22] }] }, + }); + const noteNodeId = await addParagraph(specId, 'Confirm color with owner.', { + nodeType: 'note', + }); + const objectNodeId = await addParagraph(specId, '', { + nodeType: 'object', + objectData: textBoxObject, + }); + const commentNodeId = await addParagraph(specId, 'Coordinate flashing detail.', { + facts: { + comments: [{ author: 'Sam', text: 'confirm detail', anchor: [0, 5], closed: false }], + }, + }); + + // 1. Blocked: all four finding kinds outstanding, mode: final, no override. + const blocked = await req('POST', `/specs/${specId}/generate`, { mode: 'final' }); + expect(blocked.status).toBe(422); + const blockedBody = blocked.body as { findings: readonly { type: string }[] }; + expect(new Set(blockedBody.findings.map((f) => f.type))).toEqual( + new Set([ + 'unresolved_choice_token', + 'specifier_note_present', + 'body_object_present', + 'open_comment', + ]) + ); + + // 2. Clear each finding through its supported API path — no overrideReadinessGate. + const editRes = await req('PATCH', `/specs/${specId}/paragraphs/${choiceNodeId}`, { + text: 'Provide silicone sealant.', + }); + expect(editRes.status).toBe(200); + + const ackNoteRes = await req( + 'PATCH', + `/specs/${specId}/paragraphs/${noteNodeId}/acknowledgement`, + { acknowledged: true } + ); + expect(ackNoteRes.status).toBe(200); + + const ackObjectRes = await req( + 'PATCH', + `/specs/${specId}/paragraphs/${objectNodeId}/acknowledgement`, + { acknowledged: true } + ); + expect(ackObjectRes.status).toBe(200); + + const closeRes = await req( + 'PATCH', + `/specs/${specId}/paragraphs/${commentNodeId}/comments/0/closure`, + { closed: true } + ); + expect(closeRes.status).toBe(200); + + // 3. Every finding is gone via the dry-run report too. + expect(await readinessKinds(specId)).toEqual([]); + + // 4. The SAME final-mode issuance now succeeds — overrideReadinessGate + // still unset. This is the whole point of #545. + const finalRes = await fetch(`${baseUrl}/specs/${specId}/generate`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ mode: 'final' }), + }); + expect(finalRes.status).toBe(200); + const buffer = Buffer.from(await finalRes.arrayBuffer()); + expect(buffer.length).toBeGreaterThan(0); + expect(buffer[0]).toBe(0x50); // 'P' + expect(buffer[1]).toBe(0x4b); // 'K' — a real DOCX (zip) payload, not an error body. + }); +}); diff --git a/src/api/router.ts b/src/api/router.ts index 566c707c..14521283 100644 --- a/src/api/router.ts +++ b/src/api/router.ts @@ -138,7 +138,7 @@ import { CompareRequestSchema } from '../reporting/index.js'; import { postSubmittalRegisterHandler } from './submittal-register.js'; import { getSpecOpenCommentsHandler, getProjectOpenCommentsHandler } from './open-comments.js'; import { getSpecReadinessHandler, getPackageReadinessHandler } from './readiness.js'; -import { patchEditabilityHandler, reclassifyHandler, acceptAsNoteHandler } from './editability.js'; +import { registerParagraphClearanceRoutes } from './paragraph-clearance-routes.js'; import { createAssociationHandler, listAssociationsHandler, @@ -211,11 +211,9 @@ router.post('/specs/:id/restore', restoreSpecHandler); router.post('/specs/:id/paragraphs', insertParagraphHandler); router.patch('/specs/:id/paragraphs/:nodeId', updateParagraphHandler); router.patch('/specs/:id/paragraphs/:nodeId/removal', removeParagraphHandler); -router.patch('/specs/:id/paragraphs/:nodeId/editability', patchEditabilityHandler); -router.post('/specs/:id/reclassify', reclassifyHandler); router.post('/specs/:id/finalize', finalizeSpecHandler); router.post('/specs/:id/reopen', reopenSpecHandler); -router.post('/specs/:id/paragraphs/:nodeId/comments/:index/accept-as-note', acceptAsNoteHandler); +registerParagraphClearanceRoutes(router); router.get('/specs/:id/lock', getLockHandler); router.put('/specs/:id/lock', acquireLockHandler); router.delete('/specs/:id/lock', releaseLockHandler); diff --git a/src/ast/index.ts b/src/ast/index.ts index 8eda6521..1f68dc84 100644 --- a/src/ast/index.ts +++ b/src/ast/index.ts @@ -125,11 +125,15 @@ export { PatchEditabilityBodySchema, PatchRemovalBodySchema, ReclassifyBodySchema, + PatchAcknowledgementBodySchema, + PatchCommentClosureBodySchema, } from './spec-tree-schemas.js'; export type { PatchEditabilityBody, PatchRemovalBody, ReclassifyBody, + PatchAcknowledgementBody, + PatchCommentClosureBody, } from './spec-tree-schemas.js'; export { ActorLabelSchema, AcceptNoteBodySchema } from './actor-schemas.js'; export type { AcceptNoteBody } from './actor-schemas.js'; diff --git a/src/ast/schemas.test.ts b/src/ast/schemas.test.ts index 6edfd765..69be6dcc 100644 --- a/src/ast/schemas.test.ts +++ b/src/ast/schemas.test.ts @@ -442,6 +442,17 @@ describe('SpecNodeMetaSchema', () => { expect(result.pageBreakBefore).toBe(true); }); + // #545 adversarial-review finding — the exact same bug class as #497 above. + // `acknowledged` was added to the SpecNodeMeta TS type but not to this + // schema, so z.object silently STRIPPED it on every validation path. + // `validateTree` (revision-snapshot.ts) sits on package issuance and + // revision freeze, so an acknowledged note re-blocked a package issuance + // and frozen snapshots lost the state entirely. + it('preserves acknowledged through validation (#545)', () => { + const result = SpecNodeMetaSchema.parse({ acknowledged: true }); + expect(result.acknowledged).toBe(true); + }); + it('rejects invalid source color coverage', () => { expect(() => SpecNodeMetaSchema.parse({ diff --git a/src/ast/spec-tree-schemas.ts b/src/ast/spec-tree-schemas.ts index 8052af42..84b67454 100644 --- a/src/ast/spec-tree-schemas.ts +++ b/src/ast/spec-tree-schemas.ts @@ -242,6 +242,29 @@ export const PatchRemovalBodySchema = z.object({ export type PatchRemovalBody = z.infer; +// #545, ADR-079 follow-on — per-node acknowledgement. `acknowledged: true` +// clears the readiness gate's specifier_note_present / body_object_present +// finding for a note or textBox object node WITHOUT removing or hiding the +// content (a structural toggle, mirroring PatchRemovalBodySchema's shape — +// no expectedVersion, matching that endpoint's precedent over +// updateParagraphText's optimistic-concurrency shape). +export const PatchAcknowledgementBodySchema = z.object({ + acknowledged: z.boolean(), + actorLabel: ActorLabelSchema.exactOptional(), +}); + +export type PatchAcknowledgementBody = z.infer; + +// #545, ADR-079 follow-on — a mutable comment-closure toggle. `closed: true` +// clears the readiness gate's open_comment finding for the comment at the +// path's `:index`. Same structural-toggle shape as removal/acknowledgement. +export const PatchCommentClosureBodySchema = z.object({ + closed: z.boolean(), + actorLabel: ActorLabelSchema.exactOptional(), +}); + +export type PatchCommentClosureBody = z.infer; + // Reclassify input. `rules` (optional) supplies candidate rules for a preview; // omitted → resolve the spec's library convention profile. `preview: true` // computes the diff without persisting (preview-before-save). The rules schema @@ -265,6 +288,15 @@ export const SpecNodeMetaSchema = z.object({ articleRole: ArticleRoleSchema.exactOptional(), object: ObjectMetaSchema.exactOptional(), pageBreakBefore: z.boolean().exactOptional(), + // #545, ADR-079 follow-on — per-node acknowledgement. MUST be mirrored + // here, not just on the `SpecNodeMeta` TS type: this schema has no + // `.catchall()`, so without this line `SpecTreeSchema.parse` silently + // strips `acknowledged` and every readiness finding it had cleared comes + // back. That is not theoretical — `validateTree` (revision-snapshot.ts) + // sits on the package-issuance and revision-freeze paths (revisions.ts, + // reporting.ts), so an acknowledged note would re-block a package + // issuance and frozen snapshots would lose the state entirely. + acknowledged: z.boolean().exactOptional(), // Origin paragraph UUID captured at revision-freeze time (#392, ADR-078). // Kept in lockstep with the `SpecNodeMeta` TS type (ast/types.ts): this // schema has no `.catchall()`, so a field added to the type but not diff --git a/src/ast/types.ts b/src/ast/types.ts index 9a6470d8..9565d1be 100644 --- a/src/ast/types.ts +++ b/src/ast/types.ts @@ -172,6 +172,19 @@ export interface SpecNodeMeta { * derives `alignedBy: 'origin'` from structural keys alone (ADR-078 D6). */ readonly originParagraphId?: string; + /** + * Per-node acknowledgement (#545, ADR-079 follow-on): the specifier + * affirms they have read and accepted a `note` or a `textBox` `object` + * node, clearing the readiness gate's `specifier_note_present` / + * `body_object_present` finding for it WITHOUT removing or hiding the + * content. Deliberately separate from `vanish` — reusing vanish here would + * silently lie about the removal contract (see the comment above + * `REMOVABLE_NODE_TYPES`, paragraph-vanish.ts, which excludes `note` and + * `object` for exactly this reason). Absent/false === not acknowledged. + * MUST NEVER be consulted by any renderer — it only affects readiness + * evaluation. + */ + readonly acknowledged?: boolean; } export interface SpecNode { diff --git a/src/db/index.ts b/src/db/index.ts index 9c5a83cb..56785f44 100644 --- a/src/db/index.ts +++ b/src/db/index.ts @@ -66,6 +66,26 @@ export { export type { InsertParagraphResult, InsertParagraphInput } from './queries/paragraph-insert.js'; export { setParagraphVanish, setVanishRow } from './queries/paragraph-vanish.js'; export type { SetVanishResult, SetVanishRowResult } from './queries/paragraph-vanish.js'; +// #545, ADR-079 follow-on — the acknowledgement toggle (clears +// specifier_note_present / body_object_present) and the comment-closure +// toggle (clears open_comment), mirroring setParagraphVanish's gate-free/ +// gated core split exactly. +export { + setParagraphAcknowledged, + setAcknowledgedRow, +} from './queries/paragraph-acknowledgement.js'; +export type { + SetAcknowledgedResult, + SetAcknowledgedRowResult, +} from './queries/paragraph-acknowledgement.js'; +export { + setParagraphCommentClosed, + setCommentClosedRow, +} from './queries/paragraph-comment-closure.js'; +export type { + SetCommentClosedResult, + SetCommentClosedRowResult, +} from './queries/paragraph-comment-closure.js'; // ADR-052 D3/D4/D9 (#380) — checkpoints, coalesced paragraph-history sessions, // per-paragraph reject (a restore-to-version write through updateParagraphText // above), and pending-change summaries all barrel through checkpoint-index.ts diff --git a/src/db/migrations/055_paragraph_acknowledgement.ts b/src/db/migrations/055_paragraph_acknowledgement.ts new file mode 100644 index 00000000..e7a91026 --- /dev/null +++ b/src/db/migrations/055_paragraph_acknowledgement.ts @@ -0,0 +1,77 @@ +import type { MigrationBuilder } from 'node-pg-migrate'; + +// #545, ADR-079 follow-on — closes the "no supported API path to clear a +// readiness finding" gap for the two finding kinds that need per-node state +// (specifier_note_present, body_object_present) rather than a text +// re-derivation (unresolved_choice_token, handled by re-deriving +// source_facts.choiceTokens on text edit — no schema change needed there) +// or a facts toggle (open_comment, handled by mutating source_facts.comments +// — also no schema change needed). +// +// `acknowledged` is a per-node boolean, structurally identical to `vanish` +// (migration 003) and `page_break_before` (migration 050): a paragraph-level +// flag with no catch-all `meta` column to ride on. It is deliberately NOT the +// vanish mechanism and NOT added to REMOVABLE_NODE_TYPES +// (paragraph-vanish.ts) — the comment above that set explains why storing +// vanish on `note`/`object` nodes would silently lie about the removal +// contract (both renderers emit/consider those types before ever checking +// vanish). Acknowledgement never suppresses content; it only affirms a human +// has seen it, so it must never be consulted by any renderer. +// +// paragraph_versions_op_check (migration 046) is widened with four new ops: +// `acknowledge`/`unacknowledge` (the new toggle's history rows) and +// `close-comment`/`reopen-comment` (the new comment-closure toggle's history +// rows, including the side effect wired into acceptCommentAsNote). Mirrors +// migration 046's own OPS_SQL_LIST pattern exactly — the literal is +// duplicated in src/db/queries/paragraph-history.ts's PARAGRAPH_HISTORY_OPS +// and must be kept in lockstep by hand (migrations are frozen snapshots, +// never imported into runtime src/). +const OPS = [ + 'edit', + 'insert', + 'remove', + 'restore', + 'merge', + 'accept-note', + 'restructure', + 'acknowledge', + 'unacknowledge', + 'close-comment', + 'reopen-comment', +] as const; +const OPS_SQL_LIST = OPS.map((op) => `'${op}'`).join(', '); + +const PRIOR_OPS = ['edit', 'insert', 'remove', 'restore', 'merge', 'accept-note', 'restructure']; +const PRIOR_OPS_SQL_LIST = PRIOR_OPS.map((op) => `'${op}'`).join(', '); +// The four ops `up` adds. `down` must DELETE their history rows before it +// re-narrows the CHECK, or re-adding the constraint fails outright +// (ATRewriteTable: "check constraint is violated by some row") the moment the +// feature has actually been used — verified against a live DB. Mirrors +// migration 029's down, which deletes the pr6/pr7 style_rules its own up +// widened the node_type CHECK to allow. Losing these rows is correct and not +// extra data loss: they exist only to describe toggles of the `acknowledged` +// column and the comment-closure flag that this same `down` removes/abandons. +const ADDED_OPS_SQL_LIST = OPS.filter((op) => !PRIOR_OPS.includes(op)) + .map((op) => `'${op}'`) + .join(', '); + +const CONSTRAINT_NAME = 'paragraph_versions_op_check'; + +export const up = (pgm: MigrationBuilder): void => { + pgm.addColumns('paragraphs', { + acknowledged: { type: 'boolean', notNull: true, default: false }, + }); + pgm.dropConstraint('paragraph_versions', CONSTRAINT_NAME); + pgm.addConstraint('paragraph_versions', CONSTRAINT_NAME, { + check: `op IN (${OPS_SQL_LIST})`, + }); +}; + +export const down = (pgm: MigrationBuilder): void => { + pgm.sql(`DELETE FROM paragraph_versions WHERE op IN (${ADDED_OPS_SQL_LIST})`); + pgm.dropConstraint('paragraph_versions', CONSTRAINT_NAME); + pgm.addConstraint('paragraph_versions', CONSTRAINT_NAME, { + check: `op IN (${PRIOR_OPS_SQL_LIST})`, + }); + pgm.dropColumns('paragraphs', ['acknowledged']); +}; diff --git a/src/db/queries/paragraph-acknowledgement.ts b/src/db/queries/paragraph-acknowledgement.ts new file mode 100644 index 00000000..b4ce684d --- /dev/null +++ b/src/db/queries/paragraph-acknowledgement.ts @@ -0,0 +1,206 @@ +import { pool, DatabaseError } from '../index.js'; +import { assertSpecWritable } from './edit-gate.js'; +import { bumpSpecContentVersion } from './content-version.js'; +import { recordParagraphHistory, resolveHistoryContext } from './paragraph-history.js'; +import { parseObjectMeta } from './object-meta.js'; +import { NodeTypeSchema } from '../../ast/index.js'; +import type { PoolClient } from 'pg'; +import type { SpecNode } from '../../ast/index.js'; +import { fetchSubtreeNode } from './paragraphs.js'; + +// #545, ADR-079 follow-on: per-node acknowledgement, closing the readiness +// gate's "no supported API path to clear specifier_note_present / +// body_object_present" gap. Deliberately SEPARATE state from `vanish` and +// its REMOVABLE_NODE_TYPES set (paragraph-vanish.ts) — the comment above +// that set explains that the owner-facing renderers emit `note` blockquotes +// and consider `object` nodes before ever checking vanish, so storing vanish +// on those types would silently lie about the removal contract. +// Acknowledgement never hides content: it only affirms a human has read and +// accepted it, and readiness-review.ts is the ONLY consumer — no renderer +// may ever branch on `meta.acknowledged`. + +/** Which node "shapes" can be acknowledged, mirroring exactly the two + * readiness findings acknowledgement exists to clear: a `note` node + * (specifier_note_present), or an `object` node whose captured content is a + * `textBox` (body_object_present — tables are structural content, ADR-072, + * and are never acknowledgeable). Reuses `parseObjectMeta`'s existing + * textBox derivation rather than re-deriving it from raw JSONB. */ +function isAcknowledgeableRow(nodeType: string, objectData: unknown): boolean { + if (nodeType === 'note') return true; + const parsedType = NodeTypeSchema.safeParse(nodeType); + if (!parsedType.success) return false; + const objectMeta = parseObjectMeta(parsedType.data, objectData, 'isAcknowledgeableRow'); + return objectMeta?.kind === 'textBox'; +} + +/** Outcome of {@link setParagraphAcknowledged}: the (specId, nodeId) pairing + * is validated before the write so the API maps `not-found` → 404, + * `wrong-spec` → 403, and `not-acknowledgeable` → 422 for a node type that + * cannot produce either of the two findings acknowledgement clears. */ +export type SetAcknowledgedResult = + | { readonly status: 'updated'; readonly node: SpecNode } + | { readonly status: 'not-found' } + | { readonly status: 'wrong-spec' } + | { readonly status: 'not-acknowledgeable'; readonly nodeType: string }; + +/** Outcome of {@link setAcknowledgedRow}. Widens the public + * {@link SetAcknowledgedResult}'s `updated` branch with the pre-toggle image + * the caller needs to snapshot a `paragraph_versions` row without a second + * `FOR UPDATE` round-trip. {@link applyAcknowledged} strips these extra + * fields before returning the public shape. */ +export type SetAcknowledgedRowResult = + | { + readonly status: 'updated'; + readonly node: SpecNode; + readonly changed: boolean; + readonly previousText: string; + readonly previousNodeType: string; + readonly previousBaseVersion: number; + } + | { readonly status: 'not-found' } + | { readonly status: 'wrong-spec' } + | { readonly status: 'not-acknowledgeable'; readonly nodeType: string }; + +/** + * The reusable DB core behind {@link setParagraphAcknowledged}: lock the + * row, validate ownership and acknowledgeability, and toggle `acknowledged` + * — a no-op when it already matches the requested value. + * + * Deliberately gate-free and bump-free: it never calls `assertSpecWritable` + * and never touches `specs.content_version` — the caller owns both, mirroring + * `setVanishRow`'s (paragraph-vanish.ts) exact same split. + * + * LOCK ORDER (invariant shared with updateParagraphText, setVanishRow, and + * acceptCommentAsNote): the spec row must already be gated/locked by the + * caller BEFORE this runs. + */ +export async function setAcknowledgedRow( + client: PoolClient, + specId: string, + nodeId: string, + acknowledged: boolean +): Promise { + const owner = await client.query<{ + spec_id: string; + node_type: string; + acknowledged: boolean; + text: string; + base_version: number; + object_data: unknown; + }>( + `SELECT spec_id, node_type, acknowledged, text, base_version, object_data + FROM paragraphs WHERE id = $1 FOR UPDATE`, + [nodeId] + ); + const ownerRow = owner.rows[0]; + if (!ownerRow) return { status: 'not-found' }; + // UUIDs compare case-insensitively in PostgreSQL but `pg` returns spec_id + // lowercased, while z.uuid() accepts (and preserves) an uppercase input — + // normalize both sides before comparing (mirrors setVanishRow). + if (ownerRow.spec_id.toLowerCase() !== specId.toLowerCase()) return { status: 'wrong-spec' }; + if (!isAcknowledgeableRow(ownerRow.node_type, ownerRow.object_data)) { + return { status: 'not-acknowledgeable', nodeType: ownerRow.node_type }; + } + + // Idempotent toggle: a no-op (already at the requested value) must NOT + // write — a retried apply must not mint phantom base_version bumps. + const changed = ownerRow.acknowledged !== acknowledged; + if (changed) { + await client.query( + `UPDATE paragraphs SET acknowledged = $2, base_version = base_version + 1, updated_at = now() + WHERE id = $1`, + [nodeId, acknowledged] + ); + } + + const node = await fetchSubtreeNode(client, specId, nodeId); + if (!node) throw new DatabaseError('setAcknowledgedRow: updated node vanished mid-transaction'); + return { + status: 'updated', + node, + changed, + previousText: ownerRow.text, + previousNodeType: ownerRow.node_type, + previousBaseVersion: ownerRow.base_version, + }; +} + +/** In-transaction body of {@link setParagraphAcknowledged}: gate → delegate + * the toggle to {@link setAcknowledgedRow} → snapshot the pre-toggle image + * under op `'acknowledge'`/`'unacknowledge'` → bump `content_version` once, + * only on an effective ('changed') write. Mirrors `applyVanish` + * (paragraph-vanish.ts) structurally. */ +async function applyAcknowledged( + client: PoolClient, + specId: string, + nodeId: string, + acknowledged: boolean, + actorLabel?: string +): Promise { + const gate = await assertSpecWritable(client, specId); + + const result = await setAcknowledgedRow(client, specId, nodeId, acknowledged); + if (result.status !== 'updated') return result; + + if (result.changed) { + const historyContext = await resolveHistoryContext(client, gate.contentVersion, actorLabel); + await recordParagraphHistory(client, { + paragraphId: nodeId, + specId, + // The snapshot records the paragraph's PRE-toggle image: acknowledgement + // changes no text, only readiness-review visibility, so the pre-toggle + // text/node_type IS the state this row must describe (mirrors + // applyVanish's identical reasoning). + version: result.previousBaseVersion + 1, + text: result.previousText, + nodeType: result.previousNodeType, + op: acknowledged ? 'acknowledge' : 'unacknowledge', + contentVersion: historyContext.contentVersion, + userId: historyContext.userId, + payload: null, + }); + await bumpSpecContentVersion(client, specId); + } + + return { status: 'updated', node: result.node }; +} + +/** + * Set or clear a paragraph's `acknowledged` flag by UUID (#545, ADR-079 + * follow-on): the specifier affirms they have read and accepted a `note` or + * a `textBox` `object` node, clearing the readiness gate's + * `specifier_note_present` / `body_object_present` finding for it WITHOUT + * removing or hiding the content — the content still renders exactly as + * before. Only `note` nodes and `textBox`-kind `object` nodes are + * acknowledgeable; every other node type (including a `table`-kind object, + * ADR-072) is rejected `not-acknowledgeable`. Passes the composed edit gate + * (ADR-018) and verifies the (specId, nodeId) pairing under a row lock. The + * toggle is idempotent — a no-op leaves the row untouched; an effective + * change bumps `specs.content_version` and snapshots a `paragraph_versions` + * row under op `'acknowledge'`/`'unacknowledge'`, attributed to `actorLabel` + * (falls back to the SYSTEM_ACTOR_LABEL sentinel when omitted). + */ +export async function setParagraphAcknowledged( + specId: string, + nodeId: string, + acknowledged: boolean, + actorLabel?: string +): Promise { + const client = await pool.connect(); + try { + await client.query('BEGIN'); + const result = await applyAcknowledged(client, specId, nodeId, acknowledged, actorLabel); + await client.query(result.status === 'updated' ? 'COMMIT' : 'ROLLBACK'); + return result; + } catch (err) { + try { + await client.query('ROLLBACK'); + } catch { + /* best-effort */ + } + if (err instanceof DatabaseError) throw err; + throw new DatabaseError('setParagraphAcknowledged failed', { cause: err }); + } finally { + client.release(); + } +} diff --git a/src/db/queries/paragraph-comment-closure.test.ts b/src/db/queries/paragraph-comment-closure.test.ts new file mode 100644 index 00000000..51bf21de --- /dev/null +++ b/src/db/queries/paragraph-comment-closure.test.ts @@ -0,0 +1,43 @@ +import { describe, it, expect } from 'vitest'; +import { deriveCommentClosureFacts } from './paragraph-comment-closure.js'; +import type { SourceFacts } from '../../ast/index.js'; + +describe('deriveCommentClosureFacts', () => { + it('closes the comment at the given index, leaving other keys untouched', () => { + const facts: SourceFacts = { + comments: [ + { author: 'A', text: 'first', anchor: [0, 5], closed: false }, + { author: 'B', text: 'second', anchor: [6, 12], closed: false }, + ], + banner: 'keep me', + }; + const next = deriveCommentClosureFacts(facts, 1, true); + expect(next).toEqual({ + comments: [ + { author: 'A', text: 'first', anchor: [0, 5], closed: false }, + { author: 'B', text: 'second', anchor: [6, 12], closed: true }, + ], + banner: 'keep me', + }); + }); + + it('returns null when there is no comments key', () => { + expect(deriveCommentClosureFacts({}, 0, true)).toBeNull(); + }); + + it('returns null when the index is out of range', () => { + const facts: SourceFacts = { + comments: [{ author: 'A', text: 'x', anchor: [0, 1], closed: false }], + }; + expect(deriveCommentClosureFacts(facts, 5, true)).toBeNull(); + }); + + it('never mutates the input facts object', () => { + const facts: SourceFacts = { + comments: [{ author: 'A', text: 'x', anchor: [0, 1], closed: false }], + }; + const snapshot = structuredClone(facts); + deriveCommentClosureFacts(facts, 0, true); + expect(facts).toEqual(snapshot); + }); +}); diff --git a/src/db/queries/paragraph-comment-closure.ts b/src/db/queries/paragraph-comment-closure.ts new file mode 100644 index 00000000..a062c579 --- /dev/null +++ b/src/db/queries/paragraph-comment-closure.ts @@ -0,0 +1,196 @@ +import { pool, DatabaseError } from '../index.js'; +import { assertSpecWritable } from './edit-gate.js'; +import { bumpSpecContentVersion } from './content-version.js'; +import { recordParagraphHistory, resolveHistoryContext } from './paragraph-history.js'; +import { SourceFactsSchema } from '../../ast/index.js'; +import type { PoolClient } from 'pg'; +import type { SourceFacts, SpecNode } from '../../ast/index.js'; +import { fetchSubtreeNode } from './paragraphs.js'; + +// #545, ADR-079 follow-on: a mutable comment-closure toggle, closing the +// readiness gate's "no supported API path to clear open_comment" gap. +// Deliberately named `paragraph-comment-closure.ts`, NOT `comment-closure.ts` +// — that basename is already owned by two unrelated read-only modules +// (src/parser/docx/comment-closure.ts, src/ast/comment-closure.ts) that +// derive closure at parse time; this file is the write path over the +// persisted fact those modules only ever produce once, at import. + +/** + * Pure: returns a NEW SourceFacts with `comments[index].closed` set, + * every other key/entry untouched. `null` when there is no comment at + * `index` to close/reopen — the caller maps that to a `no-comment` outcome. + * Never mutates `facts`. + */ +export function deriveCommentClosureFacts( + facts: SourceFacts, + index: number, + closed: boolean +): SourceFacts | null { + const comments = facts.comments; + const target = comments?.[index]; + if (!comments || !target) return null; + const nextComments = comments.map((comment, i) => + i === index ? { ...comment, closed } : comment + ); + return { ...facts, comments: nextComments }; +} + +/** Outcome of {@link setParagraphCommentClosed}: the (specId, nodeId) + * pairing is validated before the write so the API maps `not-found` → 404, + * `wrong-spec` → 403, and `no-comment` → 404 (a lookup miss — nothing + * exists at this index to toggle, distinct from a validation failure). */ +export type SetCommentClosedResult = + | { readonly status: 'updated'; readonly node: SpecNode } + | { readonly status: 'not-found' } + | { readonly status: 'wrong-spec' } + | { readonly status: 'no-comment' }; + +/** Outcome of {@link setCommentClosedRow}. Widens the public + * {@link SetCommentClosedResult}'s `updated` branch with the pre-toggle + * image the caller needs to snapshot a history row without a second + * `FOR UPDATE` round-trip. */ +export type SetCommentClosedRowResult = + | { + readonly status: 'updated'; + readonly node: SpecNode; + readonly changed: boolean; + readonly previousText: string; + readonly previousNodeType: string; + readonly previousBaseVersion: number; + } + | { readonly status: 'not-found' } + | { readonly status: 'wrong-spec' } + | { readonly status: 'no-comment' }; + +/** + * The reusable DB core behind {@link setParagraphCommentClosed} AND + * `reclassify.ts`'s `runAccept` (which closes the originating comment as + * part of accepting it as a note — the sharpest symptom in #545): lock the + * row, validate ownership and comment existence, and toggle + * `comments[index].closed` — a no-op when it already matches. + * + * Deliberately gate-free and bump-free: the caller owns both the edit gate + * and the `content_version` bump, mirroring `setVanishRow`/`setAcknowledgedRow`. + */ +export async function setCommentClosedRow( + client: PoolClient, + specId: string, + nodeId: string, + index: number, + closed: boolean +): Promise { + const owner = await client.query<{ + spec_id: string; + node_type: string; + source_facts: unknown; + text: string; + base_version: number; + }>( + `SELECT spec_id, node_type, source_facts, text, base_version + FROM paragraphs WHERE id = $1 FOR UPDATE`, + [nodeId] + ); + const ownerRow = owner.rows[0]; + if (!ownerRow) return { status: 'not-found' }; + if (ownerRow.spec_id.toLowerCase() !== specId.toLowerCase()) return { status: 'wrong-spec' }; + + const facts = SourceFactsSchema.parse(ownerRow.source_facts); + const comment = facts.comments?.[index]; + if (!comment) return { status: 'no-comment' }; + + const changed = comment.closed !== closed; + if (changed) { + const nextFacts = deriveCommentClosureFacts(facts, index, closed); + if (!nextFacts) return { status: 'no-comment' }; + await client.query( + `UPDATE paragraphs SET source_facts = $2::jsonb, base_version = base_version + 1, updated_at = now() + WHERE id = $1`, + [nodeId, JSON.stringify(nextFacts)] + ); + } + + const node = await fetchSubtreeNode(client, specId, nodeId); + if (!node) throw new DatabaseError('setCommentClosedRow: updated node vanished mid-transaction'); + return { + status: 'updated', + node, + changed, + previousText: ownerRow.text, + previousNodeType: ownerRow.node_type, + previousBaseVersion: ownerRow.base_version, + }; +} + +/** In-transaction body of {@link setParagraphCommentClosed}: gate → delegate + * the toggle to {@link setCommentClosedRow} → snapshot the pre-toggle image + * under op `'close-comment'`/`'reopen-comment'` → bump `content_version` + * once, only on an effective ('changed') write. */ +async function applyCommentClosed( + client: PoolClient, + specId: string, + nodeId: string, + index: number, + closed: boolean, + actorLabel?: string +): Promise { + const gate = await assertSpecWritable(client, specId); + + const result = await setCommentClosedRow(client, specId, nodeId, index, closed); + if (result.status !== 'updated') return result; + + if (result.changed) { + const historyContext = await resolveHistoryContext(client, gate.contentVersion, actorLabel); + await recordParagraphHistory(client, { + paragraphId: nodeId, + specId, + version: result.previousBaseVersion + 1, + text: result.previousText, + nodeType: result.previousNodeType, + op: closed ? 'close-comment' : 'reopen-comment', + contentVersion: historyContext.contentVersion, + userId: historyContext.userId, + payload: null, + }); + await bumpSpecContentVersion(client, specId); + } + + return { status: 'updated', node: result.node }; +} + +/** + * Close or reopen a source-document review comment on an existing spec by + * (nodeId, index) (#545, ADR-079 follow-on): the only supported path to + * clear `open_comment` — until this, `comments[*].closed` was a parse-time- + * only fact no write path ever touched. Passes the composed edit gate + * (ADR-018). The toggle is idempotent — a no-op leaves the row untouched; an + * effective change bumps `specs.content_version` and snapshots a + * `paragraph_versions` row under op `'close-comment'`/`'reopen-comment'`, + * attributed to `actorLabel` (falls back to the SYSTEM_ACTOR_LABEL sentinel + * when omitted). `no-comment` when `index` is out of range for the node's + * `source_facts.comments` (→ 404, a lookup miss). + */ +export async function setParagraphCommentClosed( + specId: string, + nodeId: string, + index: number, + closed: boolean, + actorLabel?: string +): Promise { + const client = await pool.connect(); + try { + await client.query('BEGIN'); + const result = await applyCommentClosed(client, specId, nodeId, index, closed, actorLabel); + await client.query(result.status === 'updated' ? 'COMMIT' : 'ROLLBACK'); + return result; + } catch (err) { + try { + await client.query('ROLLBACK'); + } catch { + /* best-effort */ + } + if (err instanceof DatabaseError) throw err; + throw new DatabaseError('setParagraphCommentClosed failed', { cause: err }); + } finally { + client.release(); + } +} diff --git a/src/db/queries/paragraph-history-openapi.test.ts b/src/db/queries/paragraph-history-openapi.test.ts new file mode 100644 index 00000000..f4214956 --- /dev/null +++ b/src/db/queries/paragraph-history-openapi.test.ts @@ -0,0 +1,52 @@ +import { describe, expect, it } from 'vitest'; +import { z } from 'zod'; + +import { loadRawSpec } from '../../test-utils/contract/validate-response.js'; +import { PARAGRAPH_HISTORY_OPS } from './paragraph-history.js'; + +// PARAGRAPH_HISTORY_OPS, migration 055's OPS_SQL_LIST, and openapi.yaml's +// ParagraphHistoryEntry.op enum are three copies of one list, and both source +// comments say to "keep the two in lockstep by hand". Nothing checked the third. +// +// That drift is invisible to the rest of CI: `get /specs/{}/paragraphs/{}/history` +// is in contract.integration.test.ts's RESPONSE_ALLOWLIST, so no response-schema +// validation ever compares a real history payload against this enum. #545 added +// four ops (acknowledge/unacknowledge, close-comment/reopen-comment), every one +// of which writes a history row — and the enum went un-updated, so a perfectly +// valid response documented as impossible. Caught in review, not by a gate. +// +// A unit test, deliberately: it reads openapi.yaml and a frozen literal, needs +// no DB, and so runs in `pnpm test` on every push rather than behind the +// integration gate. +const OpEnumDoc = z.object({ + components: z.object({ + schemas: z.object({ + ParagraphHistoryEntry: z.object({ + properties: z.object({ + op: z.object({ enum: z.array(z.string()) }), + }), + }), + }), + }), +}); + +describe('openapi ParagraphHistoryEntry.op vs PARAGRAPH_HISTORY_OPS (#545)', () => { + it('documents exactly the ops the DB accepts — no more, no fewer', async () => { + const doc = OpEnumDoc.parse(await loadRawSpec()); + const documented = doc.components.schemas.ParagraphHistoryEntry.properties.op.enum; + // Code-unit comparator, spelled as a statement: `sonarjs/no-alphabetical-sort` + // rejects a bare `.sort()` and `sonarjs/no-nested-conditional` rejects the + // one-line ternary form. Ordering only has to be stable for the comparison. + const byCodeUnit = (a: string, b: string): number => { + if (a < b) return -1; + return a > b ? 1 : 0; + }; + const sorted = (values: readonly string[]): string[] => [...values].sort(byCodeUnit); + + // Set equality in BOTH directions on purpose. A missing value documents a + // real response as invalid (the #545 bug); an extra one documents an op the + // DB's CHECK constraint would reject, so a client could branch on a case + // that can never arrive. + expect(sorted(documented)).toEqual(sorted(PARAGRAPH_HISTORY_OPS)); + }); +}); diff --git a/src/db/queries/paragraph-history.test.ts b/src/db/queries/paragraph-history.test.ts index bc371c73..b8d4dfe1 100644 --- a/src/db/queries/paragraph-history.test.ts +++ b/src/db/queries/paragraph-history.test.ts @@ -218,7 +218,7 @@ describe('lazyHistoryContext', () => { }); describe('PARAGRAPH_HISTORY_OPS', () => { - it("mirrors migration 046's paragraph_versions_op_check CHECK constraint exactly", async () => { + it("mirrors migration 055's paragraph_versions_op_check CHECK constraint exactly", async () => { const { PARAGRAPH_HISTORY_OPS } = await import('./paragraph-history.js'); expect(PARAGRAPH_HISTORY_OPS).toEqual([ 'edit', @@ -228,6 +228,10 @@ describe('PARAGRAPH_HISTORY_OPS', () => { 'merge', 'accept-note', 'restructure', + 'acknowledge', + 'unacknowledge', + 'close-comment', + 'reopen-comment', ]); }); }); diff --git a/src/db/queries/paragraph-history.ts b/src/db/queries/paragraph-history.ts index 3ffe9d30..a718454d 100644 --- a/src/db/queries/paragraph-history.ts +++ b/src/db/queries/paragraph-history.ts @@ -20,6 +20,15 @@ export const PARAGRAPH_HISTORY_OPS = [ 'merge', 'accept-note', 'restructure', + // #545, ADR-079 follow-on: acknowledge/unacknowledge (the note/textBox + // acknowledgement toggle) and close-comment/reopen-comment (the mutable + // comment-closure toggle, including the side effect wired into + // acceptCommentAsNote). Mirrors migration 055's identical OPS_SQL_LIST — + // keep the two in lockstep by hand (migrations are frozen snapshots). + 'acknowledge', + 'unacknowledge', + 'close-comment', + 'reopen-comment', ] as const; export type ParagraphHistoryOp = (typeof PARAGRAPH_HISTORY_OPS)[number]; diff --git a/src/db/queries/paragraph-subtree.ts b/src/db/queries/paragraph-subtree.ts new file mode 100644 index 00000000..dcc1877c --- /dev/null +++ b/src/db/queries/paragraph-subtree.ts @@ -0,0 +1,91 @@ +import { parseSourceFacts, deriveArticleRole } from '../../ast/index.js'; +import type { SignalConflict, SourceFacts, SpecNode } from '../../ast/index.js'; +import { parseNodeType } from './node-type.js'; +import { deriveInference } from './inference-meta.js'; +import { parseObjectMeta } from './object-meta.js'; + +// Split out of paragraphs.ts (#545): with buildSubtree/buildSubtreeMeta/ +// SubtreeRow inlined there, that file measured over the repo's enforced 400- +// line max-lines cap. paragraphs.ts already has several such companion +// files (object-text-edit.ts, object-meta.ts, node-type.ts, inference- +// meta.ts, associations.ts, paragraphs-batch.ts, source-facts-rederive.ts) +// — this follows the same convention. + +export interface SubtreeRow { + readonly id: string; + readonly parentId: string | null; + readonly nodeType: string; + readonly text: string; + readonly position: number; + readonly vanish: boolean; + readonly conflicts: readonly SignalConflict[]; + readonly sourceFacts: SourceFacts; + readonly signalProvenance: unknown; + readonly objectData: unknown; + readonly pageBreakBefore: boolean; + readonly acknowledged: boolean; +} + +function hasSourceFacts(sourceFacts: SourceFacts): boolean { + return Object.keys(sourceFacts).length > 0; +} + +/** Assemble one subtree row's `meta`, each field omitted when empty (mirrors + * specs.ts's buildNodeMeta) — split out of `buildSubtree`'s `build` closure + * purely to keep that closure under the repo's enforced complexity cap. */ +function buildSubtreeMeta( + row: SubtreeRow, + derived: { + readonly sourceFacts: SourceFacts; + readonly articleRole: ReturnType; + readonly inference: ReturnType; + readonly objectMeta: ReturnType; + } +): SpecNode['meta'] { + const { sourceFacts, articleRole, inference, objectMeta } = derived; + return { + ...(row.vanish ? { vanish: true } : {}), + ...(row.conflicts.length > 0 ? { conflicts: row.conflicts } : {}), + ...(hasSourceFacts(sourceFacts) ? { sourceFacts } : {}), + ...(articleRole !== undefined ? { articleRole } : {}), + ...(inference ? { inference } : {}), + ...(objectMeta ? { object: objectMeta } : {}), + ...(row.pageBreakBefore ? { pageBreakBefore: true } : {}), + ...(row.acknowledged ? { acknowledged: true } : {}), + }; +} + +/** Assemble subtree rows (a node plus all its descendants) into one SpecNode + * rooted at `rootId`. Mirrors buildNodeTree's meta shaping (specs.ts) but roots + * at a non-null parent rather than the forest roots. Used by + * {@link import('./paragraphs.js').fetchSubtreeNode} — every paragraph write + * path's shared "reconstruct the written node" step. */ +export function buildSubtree(rows: readonly SubtreeRow[], rootId: string): SpecNode | null { + const childrenByParent = new Map(); + for (const row of rows) { + childrenByParent.set(row.parentId, [...(childrenByParent.get(row.parentId) ?? []), row]); + } + const root = rows.find((r) => r.id === rootId); + if (!root) return null; + + const build = (row: SubtreeRow): SpecNode => { + // Normalize through the schema so legacy comment facts gain the backfilled + // `closed` flag before they reach the API response (#262). + const sourceFacts = parseSourceFacts(row.sourceFacts); + const articleRole = row.nodeType === 'article' ? deriveArticleRole(row.text) : undefined; + const nodeType = parseNodeType(row.nodeType, 'buildSubtree'); + const inference = deriveInference(row.signalProvenance, row.conflicts, nodeType); + const objectMeta = parseObjectMeta(nodeType, row.objectData, 'buildSubtree'); + return { + id: row.id, + type: nodeType, + text: row.text, + children: (childrenByParent.get(row.id) ?? []) + .sort((a, b) => a.position - b.position) + .map(build), + meta: buildSubtreeMeta(row, { sourceFacts, articleRole, inference, objectMeta }), + }; + }; + + return build(root); +} diff --git a/src/db/queries/paragraphs-batch.ts b/src/db/queries/paragraphs-batch.ts index 7b676926..a2b6617c 100644 --- a/src/db/queries/paragraphs-batch.ts +++ b/src/db/queries/paragraphs-batch.ts @@ -24,6 +24,8 @@ export interface FlatRow { readonly objectData: ObjectMeta | null; /** Manual page break (#497, ADR-075). True === node begins on a new page. */ readonly pageBreakBefore: boolean; + /** Per-node acknowledgement (#545, ADR-079 follow-on). True === acknowledged. */ + readonly acknowledged: boolean; } /** Column order for a batched `INSERT INTO paragraphs`, matching @@ -42,6 +44,7 @@ export const PARAGRAPH_COLUMNS: readonly ColumnSpec[] = [ { name: 'signal_provenance', cast: 'jsonb' }, { name: 'object_data', cast: 'jsonb' }, { name: 'page_break_before' }, + { name: 'acknowledged' }, ]; /** One row's bind params, in {@link PARAGRAPH_COLUMNS} order. Pure — no I/O. */ @@ -59,5 +62,6 @@ export function paragraphRowToParams(row: FlatRow): readonly unknown[] { row.signalProvenance ? JSON.stringify(row.signalProvenance) : null, row.objectData ? JSON.stringify(row.objectData) : null, row.pageBreakBefore, + row.acknowledged, ]; } diff --git a/src/db/queries/paragraphs.test.ts b/src/db/queries/paragraphs.test.ts index 7017a7f4..a2236764 100644 --- a/src/db/queries/paragraphs.test.ts +++ b/src/db/queries/paragraphs.test.ts @@ -19,11 +19,11 @@ vi.mock('../../lib/logger.js', () => ({ logger: { info: vi.fn(), error: vi.fn(), debug: vi.fn(), warn: vi.fn() }, })); -// PARAGRAPH_COLUMNS carries 12 columns per row (id, spec_id, parent_id, node_type, +// PARAGRAPH_COLUMNS carries 13 columns per row (id, spec_id, parent_id, node_type, // text, position, vanish, conflicts, source_facts, signal_provenance, object_data, -// page_break_before) — the batched INSERT flattens all rows of a chunk into one -// params array, row-major, so row N's fields start at params[N * 12]. -const PARAGRAPH_COLS = 12; +// page_break_before, acknowledged) — the batched INSERT flattens all rows of a +// chunk into one params array, row-major, so row N's fields start at params[N * 13]. +const PARAGRAPH_COLS = 13; function rowParams( params: readonly unknown[] | undefined, rowIndex: number diff --git a/src/db/queries/paragraphs.ts b/src/db/queries/paragraphs.ts index 0cb01c7d..55b84b72 100644 --- a/src/db/queries/paragraphs.ts +++ b/src/db/queries/paragraphs.ts @@ -3,7 +3,7 @@ import { assertSpecWritable } from './edit-gate.js'; import { bumpSpecContentVersion } from './content-version.js'; import { recordParagraphHistory, resolveHistoryContext } from './paragraph-history.js'; import type { Pool, PoolClient } from 'pg'; -import { NodeTypeSchema, parseSourceFacts, deriveArticleRole } from '../../ast/index.js'; +import { NodeTypeSchema, parseSourceFacts } from '../../ast/index.js'; import type { ObjectMeta, ParagraphAssociation, @@ -14,13 +14,14 @@ import type { SpecTree, } from '../../ast/index.js'; import { listAssociationsForParagraph } from './associations.js'; -import { parseNodeType } from './node-type.js'; import { deriveInference } from './inference-meta.js'; import { parseObjectMeta } from './object-meta.js'; import { rewriteObjectTextBlob } from './object-text-edit.js'; import { insertRowsInChunks, formatIdsPreview } from './batch-insert.js'; import type { FlatRow } from './paragraphs-batch.js'; import { PARAGRAPH_COLUMNS, paragraphRowToParams } from './paragraphs-batch.js'; +import { deriveNextSourceFacts } from './source-facts-rederive.js'; +import { buildSubtree, type SubtreeRow } from './paragraph-subtree.js'; export interface Queryable { query: Pool['query']; @@ -53,6 +54,7 @@ function flattenDfs( : null, objectData: node.meta.object ?? null, pageBreakBefore: node.meta.pageBreakBefore ?? false, + acknowledged: node.meta.acknowledged ?? false, }); flattenDfs(node.children, specId, node.id, rows); }); @@ -114,6 +116,8 @@ export interface ParagraphRow { readonly object?: ObjectMeta; /** Manual page break (#497, ADR-075). Present only when true. */ readonly pageBreakBefore?: boolean; + /** Per-node acknowledgement (#545, ADR-079 follow-on). Present only when true. */ + readonly acknowledged?: boolean; } export interface ParagraphWithAncestors { @@ -131,6 +135,7 @@ interface ChainRow { readonly signalProvenance: unknown; readonly objectData: unknown; readonly pageBreakBefore: boolean; + readonly acknowledged: boolean; readonly depth: number; } @@ -158,6 +163,7 @@ function toParagraphRow(r: ChainRow): ParagraphRow { ...(inference ? { inference } : {}), ...(objectMeta ? { object: objectMeta } : {}), ...(r.pageBreakBefore ? { pageBreakBefore: true } : {}), + ...(r.acknowledged ? { acknowledged: true } : {}), }; } @@ -182,17 +188,19 @@ export async function getParagraphWithAncestors( const result = await pool.query( `WITH RECURSIVE chain AS ( SELECT id, node_type, text, vanish, conflicts, source_facts, signal_provenance, - object_data, page_break_before, parent_id, 0 AS depth + object_data, page_break_before, acknowledged, parent_id, 0 AS depth FROM paragraphs WHERE id = $1 UNION ALL SELECT p.id, p.node_type, p.text, p.vanish, p.conflicts, p.source_facts, - p.signal_provenance, p.object_data, p.page_break_before, p.parent_id, c.depth + 1 + p.signal_provenance, p.object_data, p.page_break_before, p.acknowledged, + p.parent_id, c.depth + 1 FROM paragraphs p JOIN chain c ON p.id = c.parent_id WHERE c.depth + 1 < 10 ) SELECT id, node_type AS "nodeType", text, vanish, conflicts, source_facts AS "sourceFacts", signal_provenance AS "signalProvenance", - object_data AS "objectData", page_break_before AS "pageBreakBefore", depth + object_data AS "objectData", page_break_before AS "pageBreakBefore", + acknowledged, depth FROM chain ORDER BY depth DESC`, [id] ); @@ -211,61 +219,6 @@ export async function getParagraphWithAncestors( } } -interface SubtreeRow { - readonly id: string; - readonly parentId: string | null; - readonly nodeType: string; - readonly text: string; - readonly position: number; - readonly vanish: boolean; - readonly conflicts: readonly SignalConflict[]; - readonly sourceFacts: SourceFacts; - readonly signalProvenance: unknown; - readonly objectData: unknown; - readonly pageBreakBefore: boolean; -} - -/** Assemble subtree rows (a node plus all its descendants) into one SpecNode - * rooted at `rootId`. Mirrors buildNodeTree's meta shaping (specs.ts) but roots - * at a non-null parent rather than the forest roots. */ -function buildSubtree(rows: readonly SubtreeRow[], rootId: string): SpecNode | null { - const childrenByParent = new Map(); - for (const row of rows) { - childrenByParent.set(row.parentId, [...(childrenByParent.get(row.parentId) ?? []), row]); - } - const root = rows.find((r) => r.id === rootId); - if (!root) return null; - - const build = (row: SubtreeRow): SpecNode => { - // Normalize through the schema so legacy comment facts gain the backfilled - // `closed` flag before they reach the API response (#262). - const sourceFacts = parseSourceFacts(row.sourceFacts); - const articleRole = row.nodeType === 'article' ? deriveArticleRole(row.text) : undefined; - const nodeType = parseNodeType(row.nodeType, 'buildSubtree'); - const inference = deriveInference(row.signalProvenance, row.conflicts, nodeType); - const objectMeta = parseObjectMeta(nodeType, row.objectData, 'buildSubtree'); - return { - id: row.id, - type: nodeType, - text: row.text, - children: (childrenByParent.get(row.id) ?? []) - .sort((a, b) => a.position - b.position) - .map(build), - meta: { - ...(row.vanish ? { vanish: true } : {}), - ...(row.conflicts.length > 0 ? { conflicts: row.conflicts } : {}), - ...(hasSourceFacts(sourceFacts) ? { sourceFacts } : {}), - ...(articleRole !== undefined ? { articleRole } : {}), - ...(inference ? { inference } : {}), - ...(objectMeta ? { object: objectMeta } : {}), - ...(row.pageBreakBefore ? { pageBreakBefore: true } : {}), - }, - }; - }; - - return build(root); -} - /** Outcome of {@link updateParagraphText}: the spec/node pairing is validated * before any write so the API can map `not-found` → 404, `wrong-spec` → 403, * and `locked-object` → 422 (#519, ADR-072 decision 3): an `object` row's @@ -297,18 +250,19 @@ export async function fetchSubtreeNode( const result = await db.query( `WITH RECURSIVE subtree AS ( SELECT id, parent_id, node_type, text, position, vanish, conflicts, source_facts, - signal_provenance, object_data, page_break_before + signal_provenance, object_data, page_break_before, acknowledged FROM paragraphs WHERE id = $1 AND spec_id = $2 UNION ALL SELECT p.id, p.parent_id, p.node_type, p.text, p.position, p.vanish, - p.conflicts, p.source_facts, p.signal_provenance, p.object_data, p.page_break_before + p.conflicts, p.source_facts, p.signal_provenance, p.object_data, + p.page_break_before, p.acknowledged FROM paragraphs p JOIN subtree s ON p.parent_id = s.id WHERE p.spec_id = $2 ) SELECT id, parent_id AS "parentId", node_type AS "nodeType", text, position, vanish, conflicts, source_facts AS "sourceFacts", signal_provenance AS "signalProvenance", object_data AS "objectData", - page_break_before AS "pageBreakBefore" + page_break_before AS "pageBreakBefore", acknowledged FROM subtree`, [nodeId, specId] ); @@ -336,15 +290,24 @@ async function fetchUpdateOwnerRow( client: PoolClient, nodeId: string ): Promise< - { specId: string; nodeType: string; baseVersion: number; parentId: string | null } | undefined + | { + specId: string; + nodeType: string; + baseVersion: number; + parentId: string | null; + sourceFacts: SourceFacts; + } + | undefined > { const owner = await client.query<{ spec_id: string; node_type: string; base_version: number; parent_id: string | null; + source_facts: unknown; }>( - `SELECT spec_id, node_type, base_version, parent_id FROM paragraphs WHERE id = $1 FOR UPDATE`, + `SELECT spec_id, node_type, base_version, parent_id, source_facts + FROM paragraphs WHERE id = $1 FOR UPDATE`, [nodeId] ); const row = owner.rows[0]; @@ -354,6 +317,9 @@ async function fetchUpdateOwnerRow( nodeType: row.node_type, baseVersion: row.base_version, parentId: row.parent_id, + // Normalize through the schema so legacy comment facts gain the + // backfilled `closed` flag before deriveNextSourceFacts reads them (#262). + sourceFacts: parseSourceFacts(row.source_facts), }; } @@ -427,9 +393,17 @@ async function applyParagraphUpdate( } const nextVersion = ownerRow.baseVersion + 1; + // #545 — re-derive ONLY the choiceTokens portion of source_facts from the + // new text, in the same statement/transaction as the text write, so + // resolving a placeholder (e.g. a one-option bracket token replaced by + // real text) clears its unresolved_choice_token finding immediately. + // Every other source_facts key (notably `comments` — an OOXML-only, + // non-text-derivable artifact) survives byte-identical. + const nextSourceFacts = deriveNextSourceFacts(ownerRow.sourceFacts, text); await client.query( - `UPDATE paragraphs SET text = $2, base_version = $3, updated_at = now() WHERE id = $1`, - [nodeId, text, nextVersion] + `UPDATE paragraphs SET text = $2, base_version = $3, source_facts = $4::jsonb, updated_at = now() + WHERE id = $1`, + [nodeId, text, nextVersion, JSON.stringify(nextSourceFacts)] ); await rewriteObjectTextIfNeeded(client, specId, ownerRow, nodeId, text); diff --git a/src/db/queries/reclassify.ts b/src/db/queries/reclassify.ts index 9b898cc1..21e2173d 100644 --- a/src/db/queries/reclassify.ts +++ b/src/db/queries/reclassify.ts @@ -18,6 +18,7 @@ import { assertSpecWritable } from './edit-gate.js'; import { bumpSpecContentVersion } from './content-version.js'; import { recordParagraphHistory, resolveHistoryContext } from './paragraph-history.js'; import { SourceFactsSchema } from '../../ast/index.js'; +import { deriveCommentClosureFacts } from './paragraph-comment-closure.js'; import type { PoolClient } from 'pg'; import type { ParagraphHistoryContext } from './paragraph-history.js'; import type { ConventionRules, Editability } from '../../ast/index.js'; @@ -253,6 +254,15 @@ interface AnchorRow { readonly parent_id: string | null; readonly position: number; readonly source_facts: unknown; + // #545 — needed to close the anchor's originating comment as part of + // accepting it as a note (below): base_version to compute the anchor's + // own next version, text/node_type because recordParagraphHistory's + // contract is "always the paragraph's POST-write state" and this write's + // post-write text/type ARE the anchor's own, unchanged by this write + // (mirrors setVanishRow/setCommentClosedRow's identical pre-image reuse). + readonly base_version: number; + readonly text: string; + readonly node_type: string; } function commentTextAt(sourceFacts: unknown, index: number): string | null { @@ -341,6 +351,51 @@ async function insertNoteSibling( return row.id; } +/** + * #545 — the sharpest symptom this issue fixes: accepting a comment as a + * note used to leave the originating comment open, strictly increasing the + * number of blocking readiness findings (a new specifier_note_present PLUS + * the still-open open_comment). Closes `anchor`'s own `comments[index]` in + * the SAME transaction as the note insert, sharing the outer write's already- + * resolved `historyContext` (one content_version generation, not a second + * bump) — a no-op if the comment is somehow already closed. Per + * `paragraph-history.ts`'s own contract ("every content-mutating write path + * calls recordParagraphHistory exactly once per paragraph it touches"), the + * anchor gets its own `close-comment` history row: this write DOES mutate + * the anchor's `source_facts` and bump its `base_version`, so a concurrent + * optimistic-concurrency edit must see it. + */ +async function closeAnchorCommentIfOpen( + client: PoolClient, + anchor: AnchorRow, + anchorId: string, + index: number, + historyContext: ParagraphHistoryContext +): Promise { + const facts = SourceFactsSchema.parse(anchor.source_facts); + if (facts.comments?.[index]?.closed === true) return; + const nextFacts = deriveCommentClosureFacts(facts, index, true); + // Defensive only — commentTextAt already confirmed a comment exists at + // `index` before this is ever called, so nextFacts is never null here. + if (!nextFacts) return; + await client.query( + `UPDATE paragraphs SET source_facts = $2::jsonb, base_version = base_version + 1, updated_at = now() + WHERE id = $1`, + [anchorId, JSON.stringify(nextFacts)] + ); + await recordParagraphHistory(client, { + paragraphId: anchorId, + specId: anchor.spec_id, + version: anchor.base_version + 1, + text: anchor.text, + nodeType: anchor.node_type, + op: 'close-comment', + contentVersion: historyContext.contentVersion, + userId: historyContext.userId, + payload: null, + }); +} + async function runAccept( client: PoolClient, specId: string, @@ -380,7 +435,8 @@ async function runAccept( const gate = await assertSpecWritable(client, specId); const anchorRes = await client.query( - `SELECT spec_id, parent_id, position, source_facts FROM paragraphs WHERE id = $1 FOR UPDATE`, + `SELECT spec_id, parent_id, position, source_facts, base_version, text, node_type + FROM paragraphs WHERE id = $1 FOR UPDATE`, [nodeId] ); const anchor = anchorRes.rows[0]; @@ -406,6 +462,9 @@ async function runAccept( text, historyContext ); + // #545 — accepting a comment as a note also closes the originating + // comment, so it no longer counts as a blocking open_comment finding. + await closeAnchorCommentIfOpen(client, anchor, nodeId, index, historyContext); return { status: 'created', noteId }; } @@ -418,6 +477,12 @@ async function runAccept( * omitted). Idempotent by provenance: a repeat accept of the same * (anchor, index) returns the existing note's id and writes nothing — no gate * check, no history row. + * + * #545: also closes the originating comment (`anchor.source_facts.comments + * [index].closed = true`) in the same transaction, under op + * `'close-comment'` — so accepting a comment as a note no longer strictly + * increases the readiness gate's blocking-finding count (a new note plus a + * still-open comment). A no-op if the comment is already closed. */ export async function acceptCommentAsNote( specId: string, diff --git a/src/db/queries/revision-snapshot.ts b/src/db/queries/revision-snapshot.ts index d47f76a4..7eab8602 100644 --- a/src/db/queries/revision-snapshot.ts +++ b/src/db/queries/revision-snapshot.ts @@ -97,7 +97,7 @@ export async function snapshotMemberTrees( const paras = await client.query( `SELECT id, parent_id, node_type, text, position, vanish, conflicts, source_facts, signal_provenance, classification, editability_override, object_data, - page_break_before, + page_break_before, acknowledged, origin_paragraph_id AS "originParagraphId" FROM paragraphs WHERE spec_id = $1`, [member.spec_id] diff --git a/src/db/queries/source-facts-rederive.test.ts b/src/db/queries/source-facts-rederive.test.ts new file mode 100644 index 00000000..795cac3f --- /dev/null +++ b/src/db/queries/source-facts-rederive.test.ts @@ -0,0 +1,56 @@ +import { describe, it, expect } from 'vitest'; +import { deriveNextSourceFacts } from './source-facts-rederive.js'; +import type { SourceFacts } from '../../ast/index.js'; + +describe('deriveNextSourceFacts', () => { + it('resolving a one-option bracket placeholder to plain text clears choiceTokens', () => { + const existing: SourceFacts = { + choiceTokens: [{ kind: 'bracket', options: ['insert value'], span: [0, 15] }], + }; + const next = deriveNextSourceFacts(existing, 'the resolved value'); + expect(next.choiceTokens).toBeUndefined(); + expect('choiceTokens' in next).toBe(false); + }); + + it('re-derives choiceTokens from fresh adjacent-bracket-group text', () => { + const existing: SourceFacts = { + choiceTokens: [{ kind: 'angle', options: ['old'], span: [0, 5] }], + }; + const next = deriveNextSourceFacts(existing, ''); + expect(next.choiceTokens).toEqual([ + { kind: 'angle', options: ['aluminum', 'steel'], span: [0, 17] }, + ]); + }); + + it('unrelated source_facts keys survive a text edit byte-identical', () => { + const existing: SourceFacts = { + choiceTokens: [{ kind: 'bracket', options: ['insert value'], span: [0, 15] }], + comments: [{ author: 'Jane', text: 'why here?', anchor: [0, 9], closed: false }], + colors: [{ color: 'FF0000', coverage: 0.5, spans: [[0, 5]] }], + highlights: [{ color: 'yellow', text: 'note', span: [0, 4] }], + emphasis: [{ property: 'bold', value: true, expected: false, text: 'x', span: [0, 1] }], + banner: '** SPECIAL NOTICE **', + }; + const next = deriveNextSourceFacts(existing, 'resolved plain text'); + expect(next.comments).toEqual(existing.comments); + expect(next.colors).toEqual(existing.colors); + expect(next.highlights).toEqual(existing.highlights); + expect(next.emphasis).toEqual(existing.emphasis); + expect(next.banner).toBe(existing.banner); + }); + + it('never mutates the existing SourceFacts object passed in', () => { + const existing: SourceFacts = { + choiceTokens: [{ kind: 'bracket', options: ['insert value'], span: [0, 15] }], + banner: 'keep me', + }; + const snapshot = structuredClone(existing); + deriveNextSourceFacts(existing, 'resolved'); + expect(existing).toEqual(snapshot); + }); + + it('an empty existing SourceFacts with no choice tokens in new text stays empty', () => { + const next = deriveNextSourceFacts({}, 'plain text, no placeholders here'); + expect(next).toEqual({}); + }); +}); diff --git a/src/db/queries/source-facts-rederive.ts b/src/db/queries/source-facts-rederive.ts new file mode 100644 index 00000000..067dcd7c --- /dev/null +++ b/src/db/queries/source-facts-rederive.ts @@ -0,0 +1,34 @@ +import { scanChoiceTokens } from '../../parser/index.js'; +import type { SourceFacts } from '../../ast/index.js'; + +// #545 — re-derives ONLY the `choiceTokens` portion of a paragraph's +// `source_facts` from its NEW text on a text edit (updateParagraphText, +// paragraphs.ts). Every other key survives byte-identical: `comments` in +// particular is an OOXML-only artifact (comment authorship/closure) that +// does not survive into plain text and must NEVER be re-derived here — doing +// so would silently fabricate or drop comment-closure state a text edit +// cannot possibly know about. `colors`, `highlights`, `emphasis`, `banner`, +// `vanish`, and any future/unknown key (SourceFacts' index signature) are +// likewise parse-time-only facts, copied through untouched. +// +// Reuses the parser's own choice-token detector (src/parser/docx/choice- +// tokens.ts, exported through the parser barrel) rather than re-implementing +// detection here — the same syntax (adjacent bracket/angle groups) must stay +// in exact lockstep with what DOCX import itself would have found, or an +// edit could silently disagree with a fresh re-import of the same text. +export function deriveNextSourceFacts(existing: SourceFacts, text: string): SourceFacts { + const choiceTokens = scanChoiceTokens(text); + // Matches the parser's own present-only-when-non-empty convention + // (mirrors hasSourceFacts/toParagraphRow in paragraphs.ts) — an empty scan + // OMITS the key entirely rather than storing `[]`, so a resolved + // placeholder's finding clears by the key's absence, not an empty array + // readiness-review.ts still has to special-case. Rebuild without the + // single re-derived key (so a stale value is never left behind), mirroring + // part-prefix.ts's own delete-then-reassemble pattern for the same reason. + const rest: Record = { ...existing }; + delete rest.choiceTokens; + return { + ...rest, + ...(choiceTokens.length > 0 ? { choiceTokens } : {}), + }; +} diff --git a/src/db/queries/specs.test.ts b/src/db/queries/specs.test.ts index f5b62f00..85fdf223 100644 --- a/src/db/queries/specs.test.ts +++ b/src/db/queries/specs.test.ts @@ -140,6 +140,7 @@ describe('buildNodeTree editability derivation', () => { editability_override: null, object_data: null, page_break_before: false, + acknowledged: false, } as const; it('editability: corrupt override fails loud even when classification is null', async () => { @@ -170,6 +171,7 @@ function row( editability_override: null, object_data: null, page_break_before: false, + acknowledged: false, ...p, }; } diff --git a/src/db/queries/specs.ts b/src/db/queries/specs.ts index 1042aa88..b1c69f50 100644 --- a/src/db/queries/specs.ts +++ b/src/db/queries/specs.ts @@ -123,6 +123,7 @@ export interface ParagraphTreeRow { readonly editability_override: unknown; readonly object_data: unknown; readonly page_break_before: boolean; + readonly acknowledged: boolean; } function hasSourceFacts(sourceFacts: SourceFacts): boolean { @@ -152,6 +153,7 @@ function buildNodeMeta( ...(articleRole !== undefined ? { articleRole } : {}), ...(objectMeta ? { object: objectMeta } : {}), ...(row.page_break_before ? { pageBreakBefore: true } : {}), + ...(row.acknowledged ? { acknowledged: true } : {}), }; } @@ -209,7 +211,7 @@ export async function getSpecTree( const paraResult = await db.query( `SELECT id, parent_id, node_type, text, position, vanish, conflicts, source_facts, signal_provenance, classification, editability_override, object_data, - page_break_before + page_break_before, acknowledged FROM paragraphs WHERE spec_id = $1`, [id] ); diff --git a/src/generator/acknowledgement-render-parity.test.ts b/src/generator/acknowledgement-render-parity.test.ts new file mode 100644 index 00000000..2da6105c --- /dev/null +++ b/src/generator/acknowledgement-render-parity.test.ts @@ -0,0 +1,95 @@ +import { describe, it, expect } from 'vitest'; +import JSZip from 'jszip'; +import { generateDocx, generateSec, renderMarkdown } from './index.js'; +import type { SpecNode, SpecTree } from '../ast/types.js'; + +// #545, ADR-079 follow-on — the invariant this path most risks (VERIFICATION +// BAR): acknowledgement MUST NOT change rendering in any renderer. This is +// the strongest form of that assertion — the SAME tree, differing only in +// `meta.acknowledged` on its note/textBox nodes, must produce byte-identical +// output across markdown, `.SEC`, and DOCX. +// +// DOCX cannot use a whole-Buffer equality check: generating the identical +// tree twice already yields two different Buffers (dolanmiu/docx embeds a +// fresh zip/docProps timestamp on every Packer.toBuffer() call, confirmed +// experimentally during this issue's design spike). The correct comparison +// unzips both buffers and compares the `word/document.xml` part as a string. + +function buildTree(acknowledged: boolean): SpecTree { + const note: SpecNode = { + id: '00000000-0000-0000-0000-0000000000n1', + type: 'note', + text: 'Coordinate finish selection with owner before submittal.', + children: [], + meta: acknowledged ? { acknowledged: true } : {}, + }; + const textBox: SpecNode = { + id: '00000000-0000-0000-0000-0000000000o1', + type: 'object', + text: '', + children: [], + meta: { + ...(acknowledged ? { acknowledged: true } : {}), + object: { + kind: 'textBox', + floating: false, + generation: 'drawingml', + blob: [ + { 'w:txbxContent': [{ 'w:p': [{ 'w:r': [{ 'w:t': [{ '#text': 'Callout' }] }] }] }] }, + ], + }, + }, + }; + const article: SpecNode = { + id: '00000000-0000-0000-0000-0000000000a1', + type: 'article', + text: 'SUMMARY', + children: [note], + meta: {}, + }; + const part: SpecNode = { + id: '00000000-0000-0000-0000-0000000000p1', + type: 'part', + text: 'GENERAL', + children: [article, textBox], + meta: {}, + }; + return { + id: '00000000-0000-0000-0000-000000000001', + section: '09 91 26', + title: 'Exterior Painting', + parts: [part], + }; +} + +async function docXml(tree: SpecTree): Promise { + const buffer = await generateDocx(tree); + const zip = await JSZip.loadAsync(buffer); + const file = zip.file('word/document.xml'); + if (!file) throw new Error('document.xml missing'); + return file.async('string'); +} + +describe('acknowledgement never changes rendering (#545)', () => { + it('markdown output is byte-identical acknowledged vs unacknowledged', () => { + const unacknowledged = renderMarkdown(buildTree(false)); + const acknowledged = renderMarkdown(buildTree(true)); + expect(acknowledged).toBe(unacknowledged); + // Non-vacuous: the note/object content actually appears in both outputs. + expect(unacknowledged).toContain('Coordinate finish selection with owner before submittal.'); + }); + + it('.SEC output is byte-identical acknowledged vs unacknowledged', () => { + const unacknowledged = generateSec(buildTree(false)); + const acknowledged = generateSec(buildTree(true)); + expect(acknowledged).toBe(unacknowledged); + expect(unacknowledged).toContain('Coordinate finish selection with owner before submittal.'); + }); + + it('DOCX word/document.xml is byte-identical acknowledged vs unacknowledged', async () => { + const unacknowledged = await docXml(buildTree(false)); + const acknowledged = await docXml(buildTree(true)); + expect(acknowledged).toBe(unacknowledged); + expect(unacknowledged).toContain('Coordinate finish selection with owner before submittal.'); + }); +}); diff --git a/src/lib/readiness-review.test.ts b/src/lib/readiness-review.test.ts index db382f4e..195599bb 100644 --- a/src/lib/readiness-review.test.ts +++ b/src/lib/readiness-review.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from 'vitest'; -import type { SpecNode, SpecTree } from '../ast/index.js'; +import { SpecTreeSchema, type SpecNode, type SpecTree } from '../ast/index.js'; import { evaluateSpecReadiness, summarizeReadinessFindings, @@ -162,6 +162,184 @@ describe('evaluateSpecReadiness', () => { expect(result.findings).toEqual([]); expect(result.highlightAdvisory.total).toBe(0); }); + + it('acknowledged note produces no specifier_note_present finding (#545)', () => { + const acknowledged = node({ + id: 'n1', + type: 'note', + text: 'Coordinate with owner.', + meta: { acknowledged: true }, + }); + + const result = evaluateSpecReadiness(treeOf([acknowledged])); + + expect(result.findings).toEqual([]); + }); + + // Regression (#545, adversarial review): acknowledgement was lost at the + // SpecTree validation boundary. `validateTree` (revision-snapshot.ts) + // parses through SpecTreeSchema on the package-issuance and + // revision-freeze paths, and SpecNodeMetaSchema had no `acknowledged` + // key — so z.object stripped it and every cleared finding came back. + // Asserts the CLEARED state survives the round-trip, not just the key. + it('readiness: acknowledgement survives a SpecTreeSchema round-trip (#545)', () => { + const tree = treeOf([ + node({ id: 'n1', type: 'note', text: 'Coordinate.', meta: { acknowledged: true } }), + node({ + id: 'o1', + type: 'object', + text: 'Callout', + meta: { + acknowledged: true, + object: { + kind: 'textBox', + floating: false, + generation: 'drawingml', + blob: [{ 'w:p': [{ 'w:r': [] }] }], + }, + }, + }), + ]); + + expect(evaluateSpecReadiness(tree).findings).toEqual([]); + + const roundTripped = SpecTreeSchema.parse({ + ...tree, + id: '11111111-1111-4111-8111-111111111111', + parts: tree.parts.map((p, i) => ({ + ...p, + id: `2222222${i}-2222-4222-8222-222222222222`, + })), + }); + + expect(roundTripped.parts[0]?.meta.acknowledged).toBe(true); + expect(evaluateSpecReadiness(roundTripped).findings).toEqual([]); + }); + + // Regression (#545, adversarial review): acknowledging a note used to + // return a bare `[]`, so an open comment or unresolved choice token + // carried ON the note became invisible to the gate. Pre-#545 the note + // itself always blocked, so those facts never needed their own guard — + // acknowledgement introduced the bypass. Acknowledgement clears ONLY + // specifier_note_present; every other finding kind keeps its own + // supported clearing path (comment closure / text edit). + it('readiness: acknowledged note still reports its OWN open_comment — ack is not a blanket bypass (#545)', () => { + const acknowledgedWithComment = node({ + id: 'n1', + type: 'note', + text: 'Coordinate with owner.', + meta: { + acknowledged: true, + sourceFacts: { + comments: [{ author: 'Jane', text: 'which primer?', anchor: [0, 5], closed: false }], + }, + }, + }); + + const result = evaluateSpecReadiness(treeOf([acknowledgedWithComment])); + + expect(result.findings).toEqual([ + { type: 'open_comment', nodeId: 'n1', text: 'Coordinate with owner.', author: 'Jane' }, + ]); + }); + + it('readiness: acknowledged note still reports its OWN unresolved_choice_token (#545)', () => { + const acknowledgedWithToken = node({ + id: 'n1', + type: 'note', + text: 'Use [insert product] here.', + meta: { + acknowledged: true, + sourceFacts: { + choiceTokens: [{ kind: 'bracket', options: ['insert product'], span: [4, 20] }], + }, + }, + }); + + const result = evaluateSpecReadiness(treeOf([acknowledgedWithToken])); + + expect(result.findings).toEqual([ + { + type: 'unresolved_choice_token', + nodeId: 'n1', + text: 'Use [insert product] here.', + kind: 'bracket', + options: ['insert product'], + }, + ]); + }); + + it('unacknowledged note still blocks — acknowledgement gate is not vacuous (#545)', () => { + const unacknowledged = node({ + id: 'n1', + type: 'note', + text: 'Coordinate with owner.', + meta: {}, + }); + + const result = evaluateSpecReadiness(treeOf([unacknowledged])); + + expect(result.findings).toEqual([ + { type: 'specifier_note_present', nodeId: 'n1', text: 'Coordinate with owner.' }, + ]); + }); + + it('acknowledged textBox object produces no body_object_present finding (#545)', () => { + const acknowledged = node({ + id: 'o1', + type: 'object', + text: '', + meta: { + acknowledged: true, + object: { kind: 'textBox', floating: false, generation: 'drawingml', blob: [{}] }, + }, + }); + + const result = evaluateSpecReadiness(treeOf([acknowledged])); + + expect(result.findings).toEqual([]); + }); + + it('unacknowledged textBox object still blocks — acknowledgement gate is not vacuous (#545)', () => { + const unacknowledged = node({ + id: 'o1', + type: 'object', + text: '', + meta: { + object: { kind: 'textBox', floating: false, generation: 'drawingml', blob: [{}] }, + }, + }); + + const result = evaluateSpecReadiness(treeOf([unacknowledged])); + + expect(result.findings).toEqual([ + { type: 'body_object_present', nodeId: 'o1', text: '', objectKind: 'textBox' }, + ]); + }); + + it('meta.acknowledged on a non-note/object node is simply irrelevant, never a magic bypass', () => { + const acknowledgedButOrdinary = node({ + id: 'p1', + type: 'pr1', + text: 'Provide .', + meta: { + acknowledged: true, + sourceFacts: { choiceTokens: [{ kind: 'angle', options: ['A', 'B'], span: [8, 22] }] }, + }, + }); + + const result = evaluateSpecReadiness(treeOf([acknowledgedButOrdinary])); + + expect(result.findings).toEqual([ + { + type: 'unresolved_choice_token', + nodeId: 'p1', + text: 'Provide .', + kind: 'angle', + options: ['A', 'B'], + }, + ]); + }); }); describe('summarizeReadinessFindings', () => { diff --git a/src/lib/readiness-review.ts b/src/lib/readiness-review.ts index 7893f167..100469c5 100644 --- a/src/lib/readiness-review.ts +++ b/src/lib/readiness-review.ts @@ -78,8 +78,17 @@ function openCommentFindings(node: SpecNode): readonly ReadinessFinding[] { })); } +// `object.kind === 'textBox'` AND unacknowledged. Acknowledgement (#545, +// ADR-079 follow-on) clears this finding WITHOUT changing rendering — it is +// a separate, additive piece of state, never the vanish mechanism (see the +// comment above REMOVABLE_NODE_TYPES, paragraph-vanish.ts, for why vanish +// cannot be reused here). function bodyObjectFinding(node: SpecNode): readonly ReadinessFinding[] { - if (node.type === 'object' && node.meta.object?.kind === 'textBox') { + if ( + node.type === 'object' && + node.meta.object?.kind === 'textBox' && + node.meta.acknowledged !== true + ) { return [ { type: 'body_object_present', nodeId: node.id, text: node.text, objectKind: 'textBox' }, ]; @@ -87,15 +96,27 @@ function bodyObjectFinding(node: SpecNode): readonly ReadinessFinding[] { return []; } -// A `note` always flags, checked before `meta.vanish` is ever consulted — -// mirrors the generator's own rendering order (generator/index.ts emits -// `note` unconditionally, then gates every other type on `vanish`). Every -// other node type short-circuits to no findings once vanished: nothing -// renders it in any output format, so a hidden choice token, comment, or -// text box cannot block an issuance the reader will never see (ADR-079 -// decision 5, vanish-asymmetry-by-type). +// A `note` always flags UNLESS acknowledged (#545), checked before +// `meta.vanish` is ever consulted — mirrors the generator's own rendering +// order (generator/index.ts emits `note` unconditionally, then gates every +// other type on `vanish`). Acknowledgement clears the finding without +// suppressing the note — it still renders exactly as before; only the +// gate's view of it changes. Every other node type short-circuits to no +// findings once vanished: nothing renders it in any output format, so a +// hidden choice token, comment, or text box cannot block an issuance the +// reader will never see (ADR-079 decision 5, vanish-asymmetry-by-type). function assessNode(node: SpecNode): readonly ReadinessFinding[] { if (node.type === 'note') { + // Acknowledgement clears ONLY `specifier_note_present` — a note's OWN + // source facts (an open comment, an unresolved choice token) are + // different finding kinds with their own supported clearing paths + // (comment closure / text edit), so they must keep blocking. Returning + // a bare `[]` here would let an acknowledged note smuggle unresolved + // review material past a final-mode issuance: pre-#545 the note itself + // always blocked, so those facts never needed their own guard. + if (node.meta.acknowledged === true) { + return [...choiceTokenFindings(node), ...openCommentFindings(node)]; + } return [{ type: 'specifier_note_present', nodeId: node.id, text: node.text }]; } if (node.meta.vanish === true) return []; diff --git a/src/mcp/capabilities.ts b/src/mcp/capabilities.ts index 59f590e0..9d4e84e1 100644 --- a/src/mcp/capabilities.ts +++ b/src/mcp/capabilities.ts @@ -83,6 +83,11 @@ export const TOOL_TIERS: ReadonlyMap = new Map([ ['list_associations', 'read'], ['create_association', 'write'], ['accept_comment_as_note', 'write'], + // #545, ADR-079 follow-on — clear specifier_note_present/body_object_present + // (acknowledge_paragraph) and open_comment (set_comment_closed). Both are + // reversible structural toggles, not destructive, mirroring remove_paragraph. + ['acknowledge_paragraph', 'write'], + ['set_comment_closed', 'write'], // wave 4 — spec lifecycle ['update_spec', 'write'], ['finalize_spec', 'write'], diff --git a/src/mcp/contract-map.ts b/src/mcp/contract-map.ts index 0fe57c47..29fcb984 100644 --- a/src/mcp/contract-map.ts +++ b/src/mcp/contract-map.ts @@ -47,6 +47,9 @@ export const OP_TO_TOOL: ReadonlyMap = new Map([ ['post /specs/{}/paragraphs/{}/associations', 'create_association'], ['delete /specs/{}/paragraphs/{}/associations/{}', 'delete_association'], // destructive tier ['post /specs/{}/paragraphs/{}/comments/{}/accept-as-note', 'accept_comment_as_note'], + // #545, ADR-079 follow-on — the two remaining supported readiness-clearing paths + ['patch /specs/{}/paragraphs/{}/acknowledgement', 'acknowledge_paragraph'], + ['patch /specs/{}/paragraphs/{}/comments/{}/closure', 'set_comment_closed'], // wave 4 — spec lifecycle ['patch /specs/{}', 'update_spec'], ['post /specs/{}/finalize', 'finalize_spec'], diff --git a/src/mcp/contract-write-response-map.ts b/src/mcp/contract-write-response-map.ts index 2520f1f3..a10d3091 100644 --- a/src/mcp/contract-write-response-map.ts +++ b/src/mcp/contract-write-response-map.ts @@ -49,6 +49,9 @@ export const INV6_WRITE_PENDING: ReadonlySet = new Set([ 'patch /specs/{}/paragraphs/{}', 'patch /specs/{}/paragraphs/{}/editability', 'patch /specs/{}/paragraphs/{}/removal', + // #545, ADR-079 follow-on — same SpecNode-mirroring posture as removal above. + 'patch /specs/{}/paragraphs/{}/acknowledgement', + 'patch /specs/{}/paragraphs/{}/comments/{}/closure', 'patch /templates/{}', 'post /libraries/{}/conventions/clone', 'post /libraries/{}/numbering-profiles', @@ -96,11 +99,12 @@ export const INV6_WRITE_PENDING: ReadonlySet = new Set([ /** * INV-6 ratchet baseline (#549). The write-pending burn-down (INV6_WRITE_PENDING.size) must never - * grow past this count, mirroring INV5_READ_PENDING_BASELINE. Verified by direct count on - * 2026-08-03 (#627): the set holds exactly 62 entries (71 write-mapped JSON ops total, minus 8 - * driven — create_project, create_client, resolve_user, create_client_library, - * submittal_register, delete_spec, delete_package, delete_project — minus 1 exempt — - * parse_document). Was 66 as of 2026-08-02, before submittal_register/delete_spec/delete_package/ - * delete_project were promoted out of this set. + * grow past this count, mirroring INV5_READ_PENDING_BASELINE — EXCEPT when new write-mapped JSON + * ops are genuinely added to the API surface (a real scope increase, not a coverage regression), + * in which case the baseline moves up in lockstep with the new pending entries. Verified by direct + * count on 2026-08-04 (#545): the set holds exactly 64 entries — the prior 62 (#627, 2026-08-03) + * plus the two new ops this issue adds (`patch .../acknowledgement`, `patch .../comments/{}/ + * closure`), both landing straight in this pending burn-down for the same SpecNode-mirroring + * reason `patch .../removal` already does. */ -export const INV6_WRITE_PENDING_BASELINE = 62; +export const INV6_WRITE_PENDING_BASELINE = 64; diff --git a/src/mcp/paragraph-handlers.ts b/src/mcp/paragraph-handlers.ts index 386bbc66..92f49f7c 100644 --- a/src/mcp/paragraph-handlers.ts +++ b/src/mcp/paragraph-handlers.ts @@ -6,6 +6,8 @@ import { insertParagraphAfter, lockedObjectMessage, invalidInsertTypeMessage, + setParagraphAcknowledged, + setParagraphCommentClosed, StaleVersionError, SpecWriteForbiddenError, SpecNotFoundError, @@ -14,6 +16,8 @@ import { UpdateParagraphBodySchema, PatchRemovalBodySchema, InsertParagraphBodySchema, + PatchAcknowledgementBodySchema, + PatchCommentClosureBodySchema, ActorLabelSchema, } from '../ast/index.js'; import { logger } from '../lib/logger.js'; @@ -148,6 +152,76 @@ export async function handleRemoveParagraph(args: unknown): Promise } } +export const AcknowledgeParagraphShape = { + specId: z.uuid().describe('Spec UUID (from get_spec / list_sections)'), + nodeId: z.uuid().describe('Paragraph UUID (a node id within the spec tree, from get_spec)'), + ...PatchAcknowledgementBodySchema.shape, +}; +const AcknowledgeParagraphArgs = z.object(AcknowledgeParagraphShape); + +export async function handleAcknowledgeParagraph(args: unknown): Promise { + const parsed = AcknowledgeParagraphArgs.safeParse(args); + if (!parsed.success) { + return toolError( + 'invalid acknowledge_paragraph input: specId, nodeId (UUIDs) and acknowledged (boolean) are required' + ); + } + const { specId, nodeId, acknowledged, actorLabel } = parsed.data; + try { + // Affirms a note/textBox object has been read and accepted (#545) — + // clears its readiness finding WITHOUT hiding the content. + const result = await setParagraphAcknowledged(specId, nodeId, acknowledged, actorLabel); + if (result.status === 'not-found') return toolError(`paragraph not found: id=${nodeId}`); + if (result.status === 'wrong-spec') return toolError('paragraph does not belong to this spec'); + if (result.status === 'not-acknowledgeable') { + return toolError( + `node type "${result.nodeType}" cannot be acknowledged — only note nodes and textBox objects are` + ); + } + return ok(result.node); + } catch (err) { + const gate = gateToolError(err); + if (gate) return gate; + logger.error({ err }, 'mcp tool acknowledge_paragraph failed'); + return toolError('Internal error — paragraph acknowledgement failed'); + } +} + +export const SetCommentClosedShape = { + specId: z.uuid().describe('Spec UUID (from get_spec / list_sections)'), + nodeId: z.uuid().describe('Anchor paragraph UUID whose margin comment is being toggled'), + index: z + .number() + .int() + .min(0) + .describe('Zero-based index into the anchor paragraph’s source_facts.comments'), + ...PatchCommentClosureBodySchema.shape, +}; +const SetCommentClosedArgs = z.object(SetCommentClosedShape); + +export async function handleSetCommentClosed(args: unknown): Promise { + const parsed = SetCommentClosedArgs.safeParse(args); + if (!parsed.success) { + return toolError( + 'invalid set_comment_closed input: specId, nodeId (UUIDs), index (integer ≥ 0), and closed (boolean) are required' + ); + } + const { specId, nodeId, index, closed, actorLabel } = parsed.data; + try { + // The only supported path to clear open_comment (#545). + const result = await setParagraphCommentClosed(specId, nodeId, index, closed, actorLabel); + if (result.status === 'not-found') return toolError(`paragraph not found: id=${nodeId}`); + if (result.status === 'wrong-spec') return toolError('paragraph does not belong to this spec'); + if (result.status === 'no-comment') return toolError(`no comment at index ${index}`); + return ok(result.node); + } catch (err) { + const gate = gateToolError(err); + if (gate) return gate; + logger.error({ err }, 'mcp tool set_comment_closed failed'); + return toolError('Internal error — comment closure toggle failed'); + } +} + export const AcceptCommentShape = { specId: z.uuid().describe('Spec UUID (from get_spec / list_sections)'), nodeId: z.uuid().describe('Anchor paragraph UUID whose margin comment is being accepted'), diff --git a/src/mcp/paragraph-tools.integration.test.ts b/src/mcp/paragraph-tools.integration.test.ts index a432e23d..d8ce0679 100644 --- a/src/mcp/paragraph-tools.integration.test.ts +++ b/src/mcp/paragraph-tools.integration.test.ts @@ -13,6 +13,8 @@ import { handleRemoveParagraph, handleInsertParagraph, handleAcceptCommentAsNote, + handleAcknowledgeParagraph, + handleSetCommentClosed, } from './paragraph-handlers.js'; import { handleListAssociations, @@ -112,6 +114,29 @@ async function insertAnchoredObjectPair( return { objectId, textId }; } +let commentAnchorPosition = 100; + +/** A `pr1` paragraph carrying a single `source_facts.comments[0]` entry + * (#545, ADR-079 follow-on) — the minimal fixture `set_comment_closed` + * needs to exercise the real toggle end to end. Each call claims a fresh + * `position` so parallel-inserted anchors never collide. */ +async function insertCommentAnchor(specForAnchor: string, closed = false): Promise { + const r = await pool.query<{ id: string }>( + `INSERT INTO paragraphs (spec_id, parent_id, node_type, text, position, base_version, source_facts) + VALUES ($1, NULL, 'pr1', 'Comment anchor.', $2, 1, $3::jsonb) RETURNING id`, + [ + specForAnchor, + commentAnchorPosition++, + JSON.stringify({ + comments: [{ author: 'Reviewer', text: 'Verify substrate.', anchor: [0, 5], closed }], + }), + ] + ); + const id = r.rows[0]?.id; + if (!id) throw new Error('failed to insert comment anchor'); + return id; +} + beforeAll(async () => { const lib = await pool.query<{ id: string }>( `SELECT id FROM libraries ORDER BY created_at LIMIT 1` @@ -572,3 +597,144 @@ describe('accept_comment_as_note MCP tool — actorLabel attribution (#377)', () expect(await historyActor(pool, noteId, 1)).toBe(SYSTEM_ACTOR_LABEL); }); }); + +// #545 review finding: acknowledge_paragraph and set_comment_closed (both +// registered in paragraph-tools.ts, ADR-079 follow-on) had no test that ever +// called them through the MCP tool path — only structural contract-map / +// capability-tier checks touched their names. These drive the real handlers +// end-to-end against the DB, mirroring the coverage every other paragraph- +// mutation tool above already has. +describe('acknowledge_paragraph MCP tool', () => { + it('acknowledges a note node and un-acknowledging is a real toggle, not one-way', async () => { + const target = await insertParagraph(specId, 'note', 'Confirm sheen with owner.'); + + const on = await handleAcknowledgeParagraph({ specId, nodeId: target, acknowledged: true }); + expect(isToolError(on)).toBe(false); + const onNode = parse<{ id: string; meta: { acknowledged?: boolean } }>(on); + expect(onNode.id).toBe(target); + expect(onNode.meta.acknowledged).toBe(true); + + const off = await handleAcknowledgeParagraph({ specId, nodeId: target, acknowledged: false }); + expect(isToolError(off)).toBe(false); + // Absent/false === not acknowledged (src/ast/types.ts) — the DB layer omits + // the key entirely rather than round-tripping an explicit `false`. + expect(parse<{ meta: { acknowledged?: boolean } }>(off).meta.acknowledged).toBeUndefined(); + }); + + it('rejects a node type that cannot be acknowledged (ordinary pr1 body text, #545 not-acknowledgeable → 422 in REST)', async () => { + const target = await insertParagraph(specId, 'pr1', 'Ordinary body text.'); + const res = await handleAcknowledgeParagraph({ specId, nodeId: target, acknowledged: true }); + expect(isToolError(res)).toBe(true); + expect(res.content[0]!.text).toContain('cannot be acknowledged'); + }); + + it('rejects a missing node and a node from a different spec', async () => { + expect( + isToolError(await handleAcknowledgeParagraph({ specId, nodeId: MISSING, acknowledged: true })) + ).toBe(true); + const target = await insertParagraph(specId, 'note', 'Cross-spec probe.'); + expect( + isToolError( + await handleAcknowledgeParagraph({ + specId: otherSpecId, + nodeId: target, + acknowledged: true, + }) + ) + ).toBe(true); + }); +}); + +describe('acknowledge_paragraph MCP tool — actorLabel attribution (#377)', () => { + it('a supplied actorLabel attributes the acknowledge history row', async () => { + const target = await insertParagraph(specId, 'note', 'Ack attribution target.'); + const res = await handleAcknowledgeParagraph({ + specId, + nodeId: target, + acknowledged: true, + actorLabel: 'mcp.bot', + }); + expect(isToolError(res)).toBe(false); + expect(await historyActor(pool, target, 2)).toBe('mcp.bot'); // base_version 1 → 2 + }); + + it('omitting actorLabel attributes the acknowledge history row to the SYSTEM_ACTOR_LABEL sentinel', async () => { + const target = await insertParagraph(specId, 'note', 'Ack attribution target 2.'); + const res = await handleAcknowledgeParagraph({ specId, nodeId: target, acknowledged: true }); + expect(isToolError(res)).toBe(false); + expect(await historyActor(pool, target, 2)).toBe(SYSTEM_ACTOR_LABEL); + }); +}); + +describe('set_comment_closed MCP tool', () => { + it('closes then reopens a comment, returning the updated node each time', async () => { + const anchor = await insertCommentAnchor(specId, false); + + const closed = await handleSetCommentClosed({ specId, nodeId: anchor, index: 0, closed: true }); + expect(isToolError(closed)).toBe(false); + const closedNode = parse<{ + id: string; + meta: { sourceFacts?: { comments?: readonly { closed: boolean }[] } }; + }>(closed); + expect(closedNode.id).toBe(anchor); + expect(closedNode.meta.sourceFacts?.comments?.[0]?.closed).toBe(true); + + const reopened = await handleSetCommentClosed({ + specId, + nodeId: anchor, + index: 0, + closed: false, + }); + expect(isToolError(reopened)).toBe(false); + const reopenedNode = parse<{ + meta: { sourceFacts?: { comments?: readonly { closed: boolean }[] } }; + }>(reopened); + expect(reopenedNode.meta.sourceFacts?.comments?.[0]?.closed).toBe(false); + }); + + it('returns a tool error for a comment index that does not exist (a lookup miss, 404 in REST)', async () => { + const anchor = await insertCommentAnchor(specId, false); + const res = await handleSetCommentClosed({ specId, nodeId: anchor, index: 5, closed: true }); + expect(isToolError(res)).toBe(true); + expect(res.content[0]!.text).toContain('no comment'); + }); + + it('rejects a missing anchor and an anchor from a different spec', async () => { + expect( + isToolError(await handleSetCommentClosed({ specId, nodeId: MISSING, index: 0, closed: true })) + ).toBe(true); + const anchor = await insertCommentAnchor(specId, false); + expect( + isToolError( + await handleSetCommentClosed({ + specId: otherSpecId, + nodeId: anchor, + index: 0, + closed: true, + }) + ) + ).toBe(true); + }); +}); + +describe('set_comment_closed MCP tool — actorLabel attribution (#377)', () => { + it('a supplied actorLabel attributes the closure history row', async () => { + const anchor = await insertCommentAnchor(specId, false); + const res = await handleSetCommentClosed({ + specId, + nodeId: anchor, + index: 0, + closed: true, + actorLabel: 'mcp.bot', + }); + expect(isToolError(res)).toBe(false); + expect(await historyActor(pool, anchor, 2)).toBe('mcp.bot'); // base_version 1 → 2 + }); + + it('omitting actorLabel attributes the closure history row to the SYSTEM_ACTOR_LABEL sentinel', async () => { + const anchor = await insertCommentAnchor(specId, false); + const res = await handleSetCommentClosed({ specId, nodeId: anchor, index: 0, closed: true }); + expect(isToolError(res)).toBe(false); + expect(await historyActor(pool, anchor, 2)).toBe(SYSTEM_ACTOR_LABEL); + }); +}); diff --git a/src/mcp/paragraph-tools.ts b/src/mcp/paragraph-tools.ts index b50830a7..f1e66e54 100644 --- a/src/mcp/paragraph-tools.ts +++ b/src/mcp/paragraph-tools.ts @@ -3,10 +3,14 @@ import { handleRemoveParagraph, handleInsertParagraph, handleAcceptCommentAsNote, + handleAcknowledgeParagraph, + handleSetCommentClosed, UpdateParagraphShape, RemoveParagraphShape, InsertParagraphShape, AcceptCommentShape, + AcknowledgeParagraphShape, + SetCommentClosedShape, } from './paragraph-handlers.js'; import { handleListAssociations, @@ -20,6 +24,7 @@ import type { ToolRegistrar } from './tool-registry.js'; export function registerParagraphTools(reg: ToolRegistrar): void { registerParagraphContentTools(reg); + registerAcknowledgementTool(reg); registerCommentResolutionTools(reg); registerAssociationTools(reg); } @@ -71,6 +76,25 @@ function registerParagraphContentTools(reg: ToolRegistrar): void { ); } +// Split out of registerParagraphContentTools purely to keep that function +// under the repo's enforced max-lines-per-function cap (#545 pushed it over). +function registerAcknowledgementTool(reg: ToolRegistrar): void { + reg.register( + 'acknowledge_paragraph', + { + description: + 'Acknowledge or un-acknowledge a note or textBox object node (#545, ADR-079 ' + + 'follow-on): affirms a specifier has read and accepted it, clearing the ' + + 'issuance-readiness gate’s specifier_note_present / body_object_present finding ' + + 'WITHOUT removing or hiding the content — it still renders exactly as before. ' + + 'Only note nodes and textBox-kind object nodes are acknowledgeable (never a ' + + 'table object). Idempotent.', + inputSchema: AcknowledgeParagraphShape, + }, + handleAcknowledgeParagraph + ); +} + function registerCommentResolutionTools(reg: ToolRegistrar): void { reg.register( 'accept_comment_as_note', @@ -84,6 +108,19 @@ function registerCommentResolutionTools(reg: ToolRegistrar): void { }, handleAcceptCommentAsNote ); + + reg.register( + 'set_comment_closed', + { + description: + 'Close or reopen a source-document review comment on an existing spec (#545, ' + + 'ADR-079 follow-on) — the only supported path to clear the readiness gate’s ' + + 'open_comment finding. index is the zero-based position in the anchor’s ' + + 'source_facts.comments. Idempotent.', + inputSchema: SetCommentClosedShape, + }, + handleSetCommentClosed + ); } function registerAssociationTools(reg: ToolRegistrar): void { diff --git a/src/parser/docx/index.ts b/src/parser/docx/index.ts index 18f619b2..f05ede04 100644 --- a/src/parser/docx/index.ts +++ b/src/parser/docx/index.ts @@ -324,6 +324,7 @@ export { stripLeadingTitleBlockRoots } from './heuristics.js'; export { resolveStyleCascade } from './resolver.js'; export { scoreHierarchyConfidence } from './hierarchy-confidence.js'; export { findAnchoredParagraph, replaceAnchoredParagraphText } from './object-blob-edit.js'; +export { scanChoiceTokens } from './choice-tokens.js'; // #648: hasRunVanish is the single ST_OnOff-aware vanish predicate shared by // object capture and object-blob edit rewrite (ADR-092); re-exported so // merge/extract.ts can reuse it through this barrel instead of writing a diff --git a/src/parser/index.ts b/src/parser/index.ts index b7a1dcc3..3816481c 100644 --- a/src/parser/index.ts +++ b/src/parser/index.ts @@ -25,6 +25,7 @@ export { scoreHierarchyConfidence, findAnchoredParagraph, replaceAnchoredParagraphText, + scanChoiceTokens, hasRunVanish, } from './docx/index.js'; export type {