Close the DocDown.Office artifact gap and run the full 125-set review campaign - #3
Conversation
The Office system and the Com subsystem were added to the SysML2 model pointing at requirements, design and verification documents that did not exist. Nothing validates model document references, so lint stayed clean while the newest system in the repository had no requirements of its own. Five documents now exist. The system requirements state the two things that belong to the system rather than to any one format: that every backend is registered through one explicit call without reflection, and that each format carries a probe document authored in the application that produces it. The Com subsystem requirements state that availability is answered without launching an application, and that a delegated managed run's "does not render pages" statement is suppressed when the COM run has rendered them. All ten cited tests were checked against both the source and a TRX before being cited, because requirements citing tests that do not exist has been this repository's most repeated defect. Requirements are 593, satisfied 543 - four more of each than before, which is the four added here. Two stale claims are corrected while in the area. Forty-one documents still named test projects that were merged into DemaConsulting.DocDown.Office.Tests. And six verification documents were headed "Fixtures are generated, never committed", which stopped being true when the self-test probes were committed; they now distinguish the suite's generated fixtures, which must cover structures one document cannot, from the one committed probe per backend. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved traceability, document-structure, and verification-description issues remain.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds requirements, design, and verification artifacts for DocDown.Office and its Com subsystem.
Changes:
- Registers new Office and COM documentation.
- Updates traceability and review-set definitions.
- Corrects test-project references and fixture descriptions.
File summaries
| File | Summary |
|---|---|
requirements.yaml |
Adds Office requirements. |
docs/verification/docdown-pdf.md |
Updates PDF fixture guidance. |
docs/verification/docdown-pdf-rendering.md |
Updates PDF rendering fixture guidance. |
docs/verification/docdown-office/word/word-doc-down-builder-extensions.md |
Corrects test-project reference. |
docs/verification/docdown-office/word/open-xml/word-open-xml-reader.md |
Corrects test-project reference. |
docs/verification/docdown-office/word/open-xml/word-open-xml-image-reader.md |
Corrects test-project reference. |
docs/verification/docdown-office/word/open-xml/word-open-xml-extractor.md |
Corrects test-project reference. |
docs/verification/docdown-office/word/open-xml.md |
Corrects test-project reference. |
docs/verification/docdown-office/word/markdown/word-table-writer.md |
Corrects test-project reference. |
docs/verification/docdown-office/word/markdown/word-markdown-writer.md |
Corrects test-project reference. |
docs/verification/docdown-office/word.md |
Updates test-project and fixture documentation. |
docs/verification/docdown-office/visio/visio-doc-down-builder-extensions.md |
Corrects test-project reference. |
docs/verification/docdown-office/visio/open-xml/visio-package-reader.md |
Corrects test-project reference. |
docs/verification/docdown-office/visio/open-xml/visio-image-reader.md |
Corrects test-project reference. |
docs/verification/docdown-office/visio/markdown/visio-shape-labeler.md |
Corrects test-project reference. |
docs/verification/docdown-office/visio/markdown/visio-content-emitter.md |
Corrects test-project reference. |
docs/verification/docdown-office/visio/markdown.md |
Corrects test-project reference. |
docs/verification/docdown-office/visio/com/visio-com-extractor.md |
Corrects test-project reference. |
docs/verification/docdown-office/visio/com/visio-com-availability.md |
Corrects test-project reference. |
docs/verification/docdown-office/visio/com.md |
Corrects test-project reference. |
docs/verification/docdown-office/visio.md |
Updates test-project and fixture documentation. |
docs/verification/docdown-office/powerpoint/powerpoint-doc-down-builder-extensions.md |
Corrects test-project reference. |
docs/verification/docdown-office/powerpoint/open-xml/powerpoint-open-xml-reader.md |
Corrects test-project reference. |
docs/verification/docdown-office/powerpoint/open-xml/powerpoint-open-xml-image-reader.md |
Corrects test-project reference. |
docs/verification/docdown-office/powerpoint/open-xml/powerpoint-open-xml-extractor.md |
Corrects test-project reference. |
docs/verification/docdown-office/powerpoint/open-xml.md |
Corrects test-project reference. |
docs/verification/docdown-office/powerpoint/markdown/powerpoint-content-emitter.md |
Corrects test-project reference. |
docs/verification/docdown-office/powerpoint/markdown.md |
Corrects test-project reference. |
docs/verification/docdown-office/powerpoint/com/powerpoint-com-extractor.md |
Corrects test-project reference. |
docs/verification/docdown-office/powerpoint/com/powerpoint-com-availability.md |
Corrects test-project reference. |
docs/verification/docdown-office/powerpoint/com.md |
Corrects test-project reference. |
docs/verification/docdown-office/powerpoint.md |
Updates test-project and fixture documentation. |
docs/verification/docdown-office/excel/open-xml/excel-open-xml-reader.md |
Corrects test-project reference. |
docs/verification/docdown-office/excel/open-xml/excel-open-xml-image-reader.md |
Corrects test-project reference. |
docs/verification/docdown-office/excel/open-xml/excel-open-xml-extractor.md |
Corrects test-project reference. |
docs/verification/docdown-office/excel/open-xml/excel-drawing-text-reader.md |
Corrects test-project reference. |
docs/verification/docdown-office/excel/open-xml/excel-chart-reader.md |
Corrects test-project reference. |
docs/verification/docdown-office/excel/open-xml.md |
Corrects test-project reference. |
docs/verification/docdown-office/excel/markdown/excel-content-emitter.md |
Corrects test-project reference. |
docs/verification/docdown-office/excel/markdown/excel-chart-writer.md |
Corrects test-project reference. |
docs/verification/docdown-office/excel/markdown.md |
Corrects test-project reference. |
docs/verification/docdown-office/excel/excel-doc-down-builder-extensions.md |
Corrects test-project reference. |
docs/verification/docdown-office/excel.md |
Updates test-project and fixture documentation. |
docs/verification/docdown-office/com.md |
Adds COM subsystem verification design. |
docs/verification/docdown-office.md |
Adds Office system verification design. |
docs/verification/definition.yaml |
Registers verification documents. |
docs/reqstream/docdown-office/powerpoint.yaml |
Updates test-project documentation. |
docs/reqstream/docdown-office/com.yaml |
Adds COM requirements. |
docs/reqstream/docdown-office.yaml |
Adds Office requirements. |
docs/design/docdown-office/com.md |
Adds COM subsystem design. |
docs/design/definition.yaml |
Registers the design document. |
.reviewmark.yaml |
Adds artifacts to review sets. |
Review details
Suppressed comments (7)
.reviewmark.yaml:734
- This new subsystem requirement is added to a review-set whose context contains only the parent design. The reviewmark standard requires subsystem reviews to include both the parent system design and parent system requirements; add
docs/reqstream/docdown-office.yamlto the context so this decomposition is reviewed against its parent.
- "docs/reqstream/docdown-office/com.yaml" # subsystem requirements
docs/design/docdown-office/com.md:3
- This new design document links to
OfficeComView.svg, butdocs/sysml2/views/design-views.sysmldefines noOfficeComViewand no such SVG is present. The rendered design therefore has a broken diagram link; add the corresponding view and generated artifact, or remove this image reference.

docs/reqstream/docdown-office.yaml:16
- The
without reflection or assembly scanningclause is an implementation constraint, not observable requirement behavior, and the listed tests can only verify the registered set and chaining. Keep the requirement focused on the explicit registration contract and document the reflection-free mechanism in the design/risk controls instead.
The system shall register every Office backend it ships through one explicit call, without
reflection or assembly scanning.
docs/reqstream/docdown-office.yaml:43
- The requirement promises probes authored by the producing applications, but its only cited test,
Probe_EmbeddedResource_LoadsAsAnOpenPackage, loads bytes and checks only non-empty content plus thePKheader. That evidence cannot distinguish an application-authored readable probe from a synthesized or malformed package; add an extraction/authorship-level check or narrow the requirement to embedded package presence.
- id: DocDownOffice-SelfTestProbes
title: >-
The system shall carry, for each format it reads, a probe document authored in the
application that produces that format.
tags: [system]
justification: |
A self-test exists to answer whether this deployment can read what the real application
emits. A backend that synthesized its own document could only ever prove that a library
agreed with itself, and would keep reporting success while failing on every genuine
document. Carrying the probe inside the package also means the check does not depend on
the file system, the working directory, or anything the host happens to have installed.
tests:
- Probe_EmbeddedResource_LoadsAsAnOpenPackage
docs/verification/docdown-office.md:10
- The additional root-level tests are
OfficeDocDownBuilderExtensionsTestsandOfficeProbeResourceTests, which are package/unit assertions rather than aDocDownOfficesystem-integration test suite; the format system tests are already named in the preceding clause. Calling these tests system-level overstates the verification level. Describe them as package-level tests or add a true system-level test.
`DocDown.Office` is verified through the per-format system tests described in the Word, Excel,
PowerPoint, and Visio subsystem verification documents, plus the system-level tests in
`DemaConsulting.DocDown.Office.Tests`, running on xUnit v3 across net8.0, net9.0, and net10.0.
docs/verification/docdown-office.md:17
- The final clause is grammatically incorrect and does not identify what the four calls register. This should say that
AddOffice()must register the same backends as those four calls.
`AddOffice()` stands in for four separate calls, so it is only safe if it registers what those four
registered. The test asserts the **whole set** of extractor identifiers, not that particular ones are
docs/verification/docdown-office/word.md:40
No fixture is committed.immediately contradicts the next sentence's committed self-test binary and makes the new heading misleading. Say that no generated test fixture is committed, then keep the explicit exception for the embedded probe.
- Files reviewed: 52/52 changed files
- Comments generated: 12
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Twenty review-sets are now reviewed and twenty-five defects fixed. Every defect in this wave was the same shape: prose that stopped matching the code when the code changed. The removed detection confidence score left residue in six places - detection.md, format-sniffer.md, the FormatSniffer verification document, a code comment, and a requirement whose title still mandated "a confidence" that nothing implemented, nothing justified and no test asserted. That requirement and its test are renamed to state what is actually reported: the basis. Three documents stated the artifact write order and all three had manifest and metadata the wrong way round. The code writes content, manifest, metadata, summary; docdown-core.md, manifest-writer.md and doc-down-engine.md each said metadata came first, one of them as a flat statement of fact about what the engine calls. Four documents described a source hash. There is no source hash: the three SHA-256 sites in the codebase all deduplicate image bytes, and the digest the engine once computed was removed as having no consumer. manifest-writer.md listed BuildEnvironment, BuildOptions and ScratchModeString among its methods - the three projections deleted with the manifest's environment and requestedOptions blocks - and omitted several helpers that do exist. Every method the document now names resolves. Two defects were mine, introduced during last night's edits: a doc comment reading "an extension is weaker evidence than 1.0", left when the confidence phrase was cut, and a duplicated summary line in DocumentSource.cs left by the surgery that removed BackendHeuristic. Also corrected: the ContentPartKind doc listed the four kinds removed for having no producer and omitted Chart, which has one, and the sniffer design's extension table omitted .vsdm, which the code maps and a test exercises. ImageTextConfidence is deliberately kept. Two earlier audits called it a verdict reducible to a boolean; it is neither. Production reads two distinct gates - Descriptive for alt text, and not-Fallback for the manifest description - so three states are observable, and what it encodes is which slot the document offered the text in, not a judgement about the document. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved requirement, design, and verification inconsistencies remain.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (16)
Previously missed (2) — in code that hasn't changed since the last review.
docs/reqstream/docdown-office/com.yaml:35
- This requirement says the contradictory fact is suppressed only when the COM backend has rendered pages, but
ComposingDelegatedSinkfilters the matching unavailable fact during the delegated run, beforeRenderPagesAsyncruns and without receiving any render-success state. If every page render fails, the fact is still suppressed (and the extractor still records its renderer fact); either gate suppression and the authoritative fact on actual rendered pages or weaken this requirement and add the failure case to verification.
docs/verification/docdown-office/word.md:43 - This sentence says no fixture is committed and is immediately followed by the statement that one committed binary is the self-test probe. That is internally contradictory; the generated test fixtures are not committed, but the embedded probe is. Please qualify the sentence as applying to generated fixtures.
docs/design/docdown-office/com.md:1
- This is a subsystem companion under
docs/design/docdown-office/; sibling subsystem designs use a##top heading (for exampledocs/design/docdown-office/powerpoint/com.md:1). Using#makes this artifact look like a second system document and breaks the established heading hierarchy.
# DocDown.Office Com Subsystem Design
docs/reqstream/docdown-office.yaml:25
DocDownOffice-Com-Availabilityis unrelated to registration, so making it a child ofDocDownOffice-Registrationmisrepresents the requirement decomposition. System registration requirements in the existing Office requirement files decompose into registration-unit requirements; remove this cross-behavior child link or add a dedicated system requirement for availability.
children:
- DocDownOffice-Com-Availability
docs/reqstream/docdown-office.yaml:43
- The requirement says each probe is authored in its producing application, but
Probe_EmbeddedResource_LoadsAsAnOpenPackageonly checks that bytes are non-empty and begin withPK\x03\x04(the verification text explicitly calls this a packaging assertion). That test cannot establish the authoring/application-specific content; cite the four format self-test cases or weaken the requirement to embedded-package presence.
tests:
- Probe_EmbeddedResource_LoadsAsAnOpenPackage
docs/reqstream/docdown-office/com.yaml:25
DocDownOffice-Com-DelegatedCompositionis an independent behavior, not a decomposition of availability; nesting it underDocDownOffice-Com-Availabilitygives the ReqStream hierarchy the wrong meaning. Keep the two requirements as siblings underCom Requirements, as the existing PowerPoint and Visio Com requirement files do.
children:
- DocDownOffice-Com-DelegatedComposition
docs/verification/docdown-office.md:62
- The test sorts
engine.Extractorsbefore comparing it, so it proves an exact set but cannot prove an ordered set or registration order. Either describe this as an exact set or stop sorting if order is part of the verification claim.
Builds an engine through `AddOffice()` and asserts the ordered set of extractor identifiers is
exactly `excel-openxml`, `powerpoint-com`, `powerpoint-openxml`, `visio-com`, `visio-openxml`, and
`word-openxml`. Evidence for `DocDownOffice-Registration`.
docs/verification/docdown-office.md:83
Probe_EmbeddedResource_LoadsAsAnOpenPackageonly checks non-empty bytes and thePK\x03\x04prefix; an arbitrary or truncated blob with that prefix would pass. That does not establish the acceptance criterion that all four probes are Open XML packages, so validate the archive/package structure or weaken the criterion and wording.
**Test**: `Probe_EmbeddedResource_LoadsAsAnOpenPackage`
Loads each of the four embedded probes from the shipped assembly and asserts it is non-empty and
begins with the Zip signature. Evidence for `DocDownOffice-SelfTestProbes`.
docs/verification/docdown-office.md:17
- The sentence is grammatically incomplete:
what those four registeredshould identify the calls and use the conditional verb.
registered. The test asserts the **whole set** of extractor identifiers, not that particular ones are
docs/verification/docdown-office/com.md:53
- The acceptance criterion repeats a condition the implementation does not enforce: the sink suppresses the delegate's unavailable page-rendering fact before rendering is attempted, and the tests cover only the all-pages-rendered path. Add a failed-render scenario and make suppression/
pages.rendererreflect whether at least one page actually rendered, or change the criterion to match the current behavior.
- A delegated managed run's "rendering not provided" fact is suppressed when the COM run rendered,
and nothing else the delegate wrote is altered.
docs/verification/docdown-office/com.md:1
- This is a subsystem verification companion under
docs/verification/docdown-office/; the neighboring subsystem documents use a##top heading (for exampledocs/verification/docdown-office/powerpoint/com.md:1). Using#breaks the established document hierarchy.
# DocDown.Office Com Verification Design
docs/verification/docdown-office/excel.md:41
- The new heading says a probe is committed, but the following paragraph still says no binary
.xlsxor.xlsis committed.src/DemaConsulting.DocDown.Office/Resources/probe.xlsxis an embedded committed binary; distinguish generated fixtures from that probe.
### Test fixtures are generated; the self-test probe is committed
docs/verification/docdown-office/powerpoint.md:52
- The new heading says a probe is committed, but the following paragraph still says no binary
.pptxor.pptis committed.src/DemaConsulting.DocDown.Office/Resources/probe.pptxis an embedded committed binary; distinguish generated fixtures from that probe.
### Test fixtures are generated; the self-test probe is committed
docs/verification/docdown-office/visio.md:52
- The new heading says a probe is committed, but the following paragraph still says no binary
.vsdxor.vsdis committed.src/DemaConsulting.DocDown.Office/Resources/probe.vsdxis an embedded committed binary; distinguish generated fixtures from that probe.
docs/verification/docdown-pdf-rendering.md:44 - The new heading claims a committed probe, but this section still says every PDF used by the suite is generated and never explains the embedded probe. The rendering package does ship
Resources/probe.pdf; state that only generated test fixtures are excluded from the repository so the document does not contradict the package contents.
docs/verification/docdown-pdf.md:27 - This new heading says a self-test probe is committed, but the paragraph below still says no binary PDF is committed.
src/DemaConsulting.DocDown.Pdf/Resources/probe.pdfis embedded and loaded byPdfDocumentExtractor, so the verification design is contradictory; distinguish generated test fixtures from the committed probe.
- Files reviewed: 66/66 changed files
- Comments generated: 3
- Review effort level: Lite
Fifty-eight Office review-sets produced the same defect over and over: prose describing the document synthesis that was deleted yesterday. Seven documents named methods that no longer exist - BuildSelfTestDeck, BuildProbeDeck, BuildSelfTestDrawing, BuildProbeDocument, BuildProbeWorkbook, IsPowerPointRegistered and IsVisioRegistered. Each was removed with the code it described: the first five when the backends stopped synthesizing their own probe documents, the last two when the two availability probes became one shared helper. Nine source doc-comments and eleven design passages still said a backend "builds a one-sheet workbook in memory", "reads a document it built itself", or depended on SpreadsheetDocument.Create for its self-test. None of that is true of any backend now: each reads an embedded document authored in the application whose format it reads. That distinction is the whole reason the probes exist, so a document describing the old behavior described a self-test that could not detect the failure it is there to detect. The PowerPoint and Visio com.md files still listed ComposingDelegatedSink and DelegatedExtractionContext as their own, with the suppressed fact key hard-coded. Both moved to the shared DocDown.Office.Com, and the key is a constructor argument - which was the only real difference between what were two byte-equivalent copies. Four verification documents contradicted themselves within a few lines, carrying both the corrected heading about the committed probe and the original claim that no binary is committed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Four requirement justifications still described a self-test that renders a document the backend builds itself. None does: the Visio, PowerPoint and PDF rendering cases all rasterize an embedded document authored in the application that produces the format. A justification is the record of why a requirement exists, so one describing a check the product no longer performs justifies nothing. Three design documents still listed VisioPackageBuilder among the Visio package's own types. It moved to the test project yesterday: writing Visio packages is work for a test harness, not for a library whose job is reading them. The verification documents legitimately still name it, because the suite really does use it, and now say where it lives. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved documentation and traceability inconsistencies remain across the model, designs, requirements, and verification artifacts.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (29)
Previously missed (2) — in code that hasn't changed since the last review.
docs/verification/docdown-office.md:18
- The sentence says
AddOffice()must register “what those four registered,” which is grammatically unclear and leaves the referent implicit. State explicitly that the method must register the same backends as the four per-format calls.
test/DemaConsulting.DocDown.Core.Tests/Detection/FormatSnifferTests.cs:213 - The renamed test now checks only
Basis, but this file still describes “basis-and-confidence reporting” at the class level and repeatedly mentions 0.9 or zero confidence in assertion comments. Update those narratives so the test documentation matches the current record, which has no confidence value.
docs/design/docdown-core/detection.md:56
- This design now documents
FormatDetectionas format plus basis, but the SysML companion atdocs/sysml2/model/docdown-core/detection.sysml:31-37still says it carries a confidence score. Update the model artifact in the same change so the architecture model and design contract do not disagree.
- **`DetectionBasis`** — `Extension` or `ContentSignature`.
- **`FormatDetection`** — the identified format and the basis it was identified by.
docs/design/docdown-core/detection/format-sniffer.md:28
- This changed detection design still describes confidence scores in its algorithm bullets at lines 39-43 (
0.9,1.0, and0.0), even though the currentFormatDetectionno longer has that field and the verification/requirements artifacts now remove it. Update those bullets as part of this document change so the design is internally consistent.
The extension table maps `.pdf`, `.docx`/`.doc`, `.xlsx`/`.xls`, `.pptx`/`.ppt`, `.vsdx`/`.vsdm`/`.vsd`,
`.html`/`.htm`, and `.txt`/`.text`/`.log` to their formats — the `.doc`, `.xls`, `.ppt`, and `.vsd`
entries are the legacy binary Office formats, detectable but not extractable. Recognizing an
extension here does not imply an extractor is registered for it — that is `ExtractorSelector`'s
docs/design/docdown-office/com.md:1
- This is a subsystem design document, but its heading hierarchy is one level too high: existing subsystem designs use
## Com Subsystemfollowed by###sections (for exampledocs/design/docdown-office/powerpoint/com.md:1-5), as required by.github/standards/design-documentation.md:59-66. Change the root and section headings so the generated documentation structure remains consistent.
# DocDown.Office Com Subsystem Design
docs/design/docdown-office/excel/open-xml/excel-open-xml-extractor.md:70
- The replacement of
SpreadsheetDocument.Createleft the wrapped noun on the next line, so this now reads “the managed reader this backend is built on workbook.” Complete the sentence with the embedded probe instead.
- **DocumentFormat.OpenXml** (OTS) — the managed reader this backend is built on
workbook.
docs/design/docdown-office/powerpoint/com/powerpoint-com-availability.md:24
- “Use in common” is not idiomatic US English here; the shared helper is something PowerPoint and Visio
share.
The probe itself lives in the shared `OfficeComAvailability`, which PowerPoint and Visio use in
common: the two probes differed only in the application name and the ProgID, so those are arguments
docs/design/docdown-office/powerpoint/com/powerpoint-com-extractor.md:56
- The new
ProbeResourceNamebullet says the render case uses an embedded.pptx, but the precedingRunRenderdescription still says it builds a synthetic deck in a work folder and deletes that deck. After this change the case should render the embedded probe (and must not describe a synthesized temporary input). The companion SysML extractor description also still says synthetic.
- **`ProbeResourceName`** (private const) — names the embedded deck the render case rasterizes: a
real `.pptx` authored in Microsoft PowerPoint, read through `SelfTestProbe.Load`. The synthesizer
that once stood here built a theme, color scheme, font scheme, format scheme, slide master and
layout, because PowerPoint refuses to open a deck without them.
docs/design/docdown-office/powerpoint/open-xml/powerpoint-open-xml-extractor.md:69
- The replacement of
PresentationDocument.Createleft the wrapped noun on the next line, so this now reads “the managed reader this backend is built on deck.” Complete the sentence with the embedded probe instead.
- **DocumentFormat.OpenXml** (OTS) — the managed reader this backend is built on
deck.
docs/design/docdown-office/visio/com/visio-com-availability.md:23
- “Use in common” is not idiomatic US English here; the shared helper is something Visio and PowerPoint
share.
The probe itself lives in the shared `OfficeComAvailability`, which Visio and PowerPoint use in
common: the two probes differed only in the application name and the ProgID, so those are arguments
docs/design/docdown-office/visio/com/visio-com-extractor.md:54
- The new
ProbeResourceNamebullet says the render case uses an embedded.vsdx, but the precedingvisio.com.renderdescription still says it builds a synthetic drawing in a work folder and deletes it. After this change the case should render the embedded probe (and must not describe a synthesized temporary input). The companion SysML extractor description also still says synthetic.
- **`ProbeResourceName`** (private const) — names the embedded drawing the render case rasterizes: a
real `.vsdx` authored in Microsoft Visio, read through `SelfTestProbe.Load`. The synthesizer that
once stood here had to write a window part and real shape geometry, because Visio refuses to open a
package without the first and draws nothing for a shape without the second.
docs/design/docdown-office/word/open-xml.md:44
- Replacing the writer call left the sentence as “builds a embedded document.” The self-test now reads the probe, and the article should be “an” if this sentence is retained.
embedded document authored in Microsoft Word, reads it with the reader, and passes
docs/reqstream/docdown-office.yaml:25
DocDownOffice-Com-Availabilityis not a refinement ofDocDownOffice-Registration: its title and justification govern environment probing, not explicit backend registration. Thischildrenlink makes the requirement tree report availability under registration while leaving the format-registration decomposition unrepresented; move it under an appropriate parent or make it a separate root requirement, following the focused registration children used indocs/reqstream/docdown-pdf.yaml:67-74.
children:
- DocDownOffice-Com-Availability
docs/reqstream/docdown-office.yaml:29
AddOffice_NullBuilder_Throwsis an acceptance criterion and test indocs/verification/docdown-office.md, but this registration requirement cites only the other three tests. Without a citation here, the new system traceability omits one of its stated acceptance tests; add the test or model a child requirement for null rejection.
- AddOffice_OnBuilder_RegistersEveryOfficeBackend
- AddOffice_ReturnsSameBuilderForChaining
- AddExcel_Alone_RegistersOnlyTheExcelBackend
docs/reqstream/docdown-office/com.yaml:23
- This justification first says the reader can act on the probe's reason and then immediately says the reader cannot act on it. Rephrase the sentence so it consistently describes an environmental fact rather than an installation instruction.
registry, never throws, and answers with a declarative reason the reader can act on -
or rather, one the reader cannot act on, which is why the reasons state a fact about
the environment instead of instructing an installation.
docs/verification/docdown-office/com.md:1
- This subsystem verification document uses a
#root and##sections, unlike the repository's subsystem verification layout, which uses a##root and###sections (seedocs/verification/docdown-office/powerpoint/com.md:1-6and.github/standards/design-documentation.md:59-66). Adjust the heading levels throughout this new document so verification sections are nested correctly.
docs/verification/docdown-office/excel.md:52 - After acknowledging the embedded probe, this paragraph still says the repository stays text-only.
probe.xlsxis a committed binary, so limit this statement to the generated test fixtures; otherwise the verification claim is false.
docs/verification/docdown-office/powerpoint.md:64 - The new paragraph says no
.pptxis committed, but this package now commitsResources/probe.pptxfor the self-test. Restrict the statement to test fixtures; as written, the verification document contradicts its preceding probe description.
docs/verification/docdown-pdf-rendering.md:44 - The new heading says a self-test probe is committed, but the following paragraph still says the repository stays text-only and never identifies the committed probe.
PdfPageRenderingExtractorloads the embeddedResources/probe.pdf, so this verification document contradicts the packaging it is meant to describe; distinguish generated test fixtures from the committed self-test probe.
docs/verification/docdown-pdf.md:34 - The new paragraph says a probe binary is committed, but this document's Test Environment still says “no committed binaries” at line 47. Update that input description to distinguish generated test fixtures from the embedded self-test probe; otherwise the verification design contradicts itself.
src/DemaConsulting.DocDown.Core/Output/ContentWriter.cs:214 ContentPartcurrently exposes onlySheetandChartkinds, so the unchanged summary's example of a slide is inaccurate. Update the summary with this parameter description; otherwise the public API docs describe a kind the enum does not provide.
src/DemaConsulting.DocDown.Office/Excel/OpenXml/ExcelOpenXmlExtractor.cs:147- Although this lower remark now says the case reads an embedded Excel workbook, the
GetSelfTestCasesremark above still says it reads a workbook “it builds itself” (lines 112-114). Update that earlier contract comment too, or the self-test documentation remains contradictory.
src/DemaConsulting.DocDown.Office/PowerPoint/OpenXml/PowerPointOpenXmlExtractor.cs:136 - Although this lower remark now says the case reads an embedded PowerPoint deck, the
GetSelfTestCasesremark above still says it reads a deck “it builds itself” (lines 101-103). Update that earlier contract comment too, or the self-test documentation remains contradictory.
src/DemaConsulting.DocDown.Office/Visio/OpenXml/VisioOpenXmlExtractor.cs:135 - This implementation now loads
SelfTestProbe, butdocs/design/docdown-office/visio/open-xml/visio-open-xml-extractor.md:32-35and:51still describe aVisioPackageBuilder-generated self-test drawing. Update that companion design artifact so the new application-authored probe is documented consistently.
src/DemaConsulting.DocDown.Office/Visio/OpenXml/VisioOpenXmlExtractor.cs:135 - Although this lower remark now says the case reads an embedded Visio drawing, the
GetSelfTestCasesremark above still says it reads a drawing “it builds itself” (lines 101-102). Update that earlier contract comment too, or the self-test documentation remains contradictory.
src/DemaConsulting.DocDown.Office/Word/OpenXml/WordOpenXmlExtractor.cs:141 - The source now reads the embedded Word probe, but the shared OTS design still says
DocumentFormat.OpenXmlprovides the writer used to build the parse-round-trip probe (docs/design/ots/openxml.md:12-14). Update that companion description so it no longer attributes the removed writer path to the self-test.
src/DemaConsulting.DocDown.Office/Word/OpenXml/WordOpenXmlExtractor.cs:141 - Although this lower remark now says the case reads an embedded Word document, the
GetSelfTestCasesremark above still says it reads a document “it builds itself” (lines 103-105). Update that earlier contract comment too, or the public self-test documentation remains contradictory.
src/DemaConsulting.DocDown.Pdf/PdfDocumentExtractor.cs:241 - The implementation now reads the embedded PDF probe, but its companion design still says
GetSelfTestCasesbuilds and rereads a one-page PDF in memory (docs/design/docdown-pdf/pdf-document-extractor.md:42-44). Update that design artifact in the same change so the published self-test contract matches the code.
src/DemaConsulting.DocDown.Pdf/PdfDocumentExtractor.cs:241 - Although this lower remark now says the case reads an embedded PDF, the
GetSelfTestCasesremark above still says it parses a document “it builds itself” (lines 120-122). Update that earlier contract comment too, or the self-test documentation remains contradictory.
- Files reviewed: 94/94 changed files
- Comments generated: 5
- Review effort level: Lite
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Final review comments identify unresolved documentation and requirement-structure inconsistencies.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (28)
Previously missed (3) — in code that hasn't changed since the last review.
docs/design/docdown-office/excel/open-xml/excel-open-xml-extractor.md:70
- The edit leaves the old wrapped noun on the next line, so the dependency now reads “the managed reader this backend is built on workbook.” Remove the orphaned
workbook.and end the dependency description after “built on.”
docs/design/docdown-office/powerpoint/open-xml/powerpoint-open-xml-extractor.md:69 - The edit leaves the old wrapped noun on the next line, so the dependency now reads “the managed reader this backend is built on deck.” Remove the orphaned
deck.and end the dependency description after “built on.”
docs/design/docdown-office/word/open-xml.md:44 - The preceding text still says the case “builds a” document, so this edit produces “builds a embedded document” and continues to describe the removed in-memory construction. Reword the two-line sentence to say that the case reads the embedded document.
docs/design/docdown-core/detection/format-sniffer.md:26
- Although this hunk updates the extension table, this design document still says the sniffer reports confidence and still documents extension results as
Extension / 0.9elsewhere.FormatDetectionnow contains only the format and basis, so this artifact remains inconsistent with the updated source and verification document; remove the confidence claims throughout the file.
The extension table maps `.pdf`, `.docx`/`.doc`, `.xlsx`/`.xls`, `.pptx`/`.ppt`, `.vsdx`/`.vsdm`/`.vsd`,
`.html`/`.htm`, and `.txt`/`.text`/`.log` to their formats — the `.doc`, `.xls`, `.ppt`, and `.vsd`
docs/design/docdown-office/com.md:1
- This new subsystem design document is nested under
docs/design/docdown-office, but its top-level heading is level 1. Existing nested Office subsystem documents use a##title, so this renders as a second system-level document instead of a child ofDocDown.Office.
# DocDown.Office Com Subsystem Design
docs/design/docdown-office/com.md:5
- This is a subsystem design companion, but it uses a system-level
#title and##sections, and it does not provide the mandatory subsystemOverview,Interfaces, andDesignsections. Sibling subsystem designs such asdocs/design/docdown-office/powerpoint/com.mduse a##title with###sections; align this new artifact with that hierarchy.
# DocDown.Office Com Subsystem Design

The Com subsystem holds what the PowerPoint and Visio COM automation backends share. It exists
docs/design/docdown-office/excel/open-xml.md:38
- The preceding text still says the case “builds a”, so this edit produces “builds a embedded workbook” and leaves the self-test described as constructing a fixture. Reword the two-line sentence to say that it reads the embedded workbook.
embedded workbook authored in Microsoft Excel, reads it with the reader, and
docs/design/docdown-office/excel/open-xml/excel-open-xml-extractor.md:46
- These lines now contradict the embedded-probe description above: they still claim that building rather than embedding keeps the case free of a shipped payload, even though the self-test loads the embedded workbook. Replace this with the reason the application-authored probe is intentionally embedded.
worksheet. Building rather than embedding a fixture keeps the case free of a shipped binary
payload.
docs/design/docdown-office/powerpoint/open-xml.md:40
- The preceding text still says the case “builds a”, so this edit produces “builds a embedded deck” and leaves the self-test described as constructing a fixture. Reword the two-line sentence to say that it reads the embedded deck.
embedded deck authored in Microsoft PowerPoint, reads it with the reader, and passes
docs/design/docdown-office/powerpoint/open-xml/powerpoint-open-xml-extractor.md:44
- This sentence still claims that building rather than embedding keeps the case free of a shipped payload, contradicting the new embedded-deck description and
ProbeResourceNameimplementation. Replace it with the reason the application-authored probe is intentionally embedded.
slide. Building rather than embedding a fixture keeps the case free of a shipped binary payload.
docs/design/docdown-office/visio/open-xml/visio-package-reader.md:57
- The changed sentence leaves an ungrammatical construction: it says the extractor self-test reads the probe, then uses “and both ... and” to join the fixture clause. Split the extractor self-test from the generated-fixture statement.
`VisioOpenXmlExtractor.ExtractAsync` calls `Read()` on the buffered drawing stream, the
extractor's self-test reads the embedded probe drawing, and the test fixtures build packages
with `VisioPackageBuilder` in the test project for it to read back.
docs/reqstream/docdown-office.yaml:29
- The verification design lists
AddOffice_NullBuilder_Throwsas evidence for the registration criterion and the acceptance criteria explicitly require null rejection, but this requirement's test list omits it. Add the test here so that behavior remains traceable in ReqStream like the per-format registration requirements.
- AddExcel_Alone_RegistersOnlyTheExcelBackend
docs/reqstream/docdown-office.yaml:25
DocDownOffice-Registrationis a registration requirement, but it decomposes intoDocDownOffice-Com-Availability, which is a separate subsystem responsibility rather than a child of registration. Existing Office requirement files keep registration and COM availability as separate requirement branches (for example,docs/reqstream/docdown-office/powerpoint/com.yaml:15-52). Remove this child edge and keep the two requirements as siblings.
children:
- DocDownOffice-Com-Availability
docs/reqstream/docdown-office/com.yaml:25
- Availability and delegated-output composition are independent COM responsibilities, but
DocDownOffice-Com-Availabilitycurrently makesDocDownOffice-Com-DelegatedCompositionits child. The existing PowerPoint and Visio COM requirement files model availability, rendering, and automation as sibling branches; remove this false decomposition and keep both requirements at the subsystem level.
children:
- DocDownOffice-Com-DelegatedComposition
docs/verification/docdown-office.md:17
- This sentence says
AddOffice()registers “what those four registered”, which is grammatically incomplete and obscures that the four calls are the subject. Use “what those calls register” before describing the exact-set assertion.
docs/verification/docdown-office/com.md:1 - This new subsystem verification document is nested under
docs/verification/docdown-office, but its top-level heading is level 1. Existing nested Office verification documents use a##title, so this breaks the document hierarchy.
docs/verification/docdown-office/excel.md:52 - The new paragraph still says the repository stays text-only, but
probe.xlsxis a committed binary embedded by the Office project. Limit this claim to the generated test fixtures; otherwise the verification document contradicts the committed self-test probe described immediately above.
docs/verification/docdown-office/powerpoint.md:64 - This new paragraph says no
.pptxis committed and the repository stays text-only, butprobe.pptxis a committed embedded resource. Limit the statement to the suite's generated fixtures so it does not contradict the self-test probe paragraph above.
docs/verification/docdown-office/powerpoint/com/powerpoint-com-extractor.md:9 - This document is being updated for the consolidated test project, but its release-time paragraph still says
docdown --validatebuilds a synthetic single-slide deck. The implementation now writes the embeddedprobe.pptxto the work folder, so the verification description remains false after this update.
docs/verification/docdown-office/visio/com/visio-com-extractor.md:9 - This document is being updated for the consolidated test project, but its release-time paragraph still says
docdown --validatebuilds a synthetic single-page drawing. The implementation now writes the embeddedprobe.vsdxto the work folder, so the verification description remains false after this update.
docs/verification/docdown-pdf-rendering.md:44 - The heading now acknowledges a committed self-test probe, but the following paragraph still says every PDF is generated at test time and that the repository stays text-only. The backend now reads a committed embedded PDF probe, so distinguish that probe from the generated test fixtures here.
src/DemaConsulting.DocDown.Office/Excel/OpenXml/ExcelOpenXmlExtractor.cs:147 - This updated remark correctly says the self-test reads an embedded Excel workbook, but the enclosing
GetSelfTestCasesremarks above still say it reads a workbook the backend builds itself. Update that outer description so the public XML documentation matches the shipped probe.
src/DemaConsulting.DocDown.Office/PowerPoint/OpenXml/PowerPointOpenXmlExtractor.cs:136 - This updated remark correctly says the self-test reads an embedded PowerPoint deck, but the enclosing
GetSelfTestCasesremarks above still say it reads a deck the backend builds itself. Update that outer description so the public XML documentation matches the shipped probe.
src/DemaConsulting.DocDown.Office/Visio/OpenXml/VisioOpenXmlExtractor.cs:134 - This new remark is grammatically broken: it ends with “joined by a glued” and the next line starts “it back”, so it reads as “glued it back” instead of describing the glued connector and the read operation. Please reword the two lines to describe the embedded drawing and the resolved connection.
src/DemaConsulting.DocDown.Office/Visio/OpenXml/VisioOpenXmlExtractor.cs:135 - As with the other extractors, this new embedded-probe remark conflicts with the enclosing
GetSelfTestCasesremarks above, which still say the backend reads a drawing it builds itself. Update the outer description to match the embedded probe.
src/DemaConsulting.DocDown.Office/Word/OpenXml/WordOpenXmlExtractor.cs:141 - This updated remark correctly says the self-test reads an embedded Word document, but the enclosing
GetSelfTestCasesremarks above still say it reads a document the backend builds itself. Update that outer description so the public XML documentation does not describe the old probe implementation.
src/DemaConsulting.DocDown.Pdf/PdfDocumentExtractor.cs:241 - This updated remark correctly says the self-test reads an embedded PDF, but the enclosing
GetSelfTestCasesremarks above still say it parses a document the backend builds itself. The two descriptions now contradict each other; update the outer remark to describe the embedded probe as well.
test/DemaConsulting.DocDown.Core.Tests/Detection/FormatSnifferTests.cs:213 - The renamed test no longer claims to verify confidence, but this file still describes confidence throughout, including the class summary and assertion comments (for example, the old
0.9contract below).FormatDetectionhas no confidence member; remove the stale confidence claims so the tests do not document a deleted contract.
- Files reviewed: 94/94 changed files
- Comments generated: 4
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved moderate documentation and requirement-traceability inconsistencies remain.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (34)
Previously missed (8) — in code that hasn't changed since the last review.
docs/design/docdown-office/excel/open-xml/excel-open-xml-extractor.md:70
- The dependency bullet is now truncated to
built on workbook, so the design document no longer states a valid dependency. End the sentence afterbuilt on(or explicitly say it is used by the self-test) instead of leaving the oldworkbookfragment behind.
docs/design/docdown-office/powerpoint/open-xml/powerpoint-open-xml-extractor.md:69 - The dependency bullet is now truncated to
built on deck, so the design document no longer states a valid dependency. End the sentence afterbuilt on(or explicitly say it is used by the self-test) instead of leaving the olddeckfragment behind.
docs/design/docdown-office/word/open-xml.md:44 - The updated sentence leaves the old
builds awording in place, producingbuilds a embeddedand redundantly saying the document is read afterward. Describe the operation as reading the embedded Word probe instead.
docs/verification/docdown-office.md:17 - The registration explanation has a grammatical error:
what those four registeredshould use the present-tense verbregister.
src/DemaConsulting.DocDown.Office/Excel/OpenXml/ExcelOpenXmlExtractor.cs:145 - The lower remark now says this case reads an embedded Excel probe, but the earlier
GetSelfTestCasessummary still says it builds the workbook itself. Update that summary as well so the API documentation describes the actual self-test input.
src/DemaConsulting.DocDown.Office/PowerPoint/OpenXml/PowerPointOpenXmlExtractor.cs:135 - The lower remark now says this case reads an embedded PowerPoint probe, but the earlier
GetSelfTestCasessummary still says it builds the deck itself. Update that summary too; otherwise the same public documentation gives two different accounts of the self-test.
src/DemaConsulting.DocDown.Office/Word/OpenXml/WordOpenXmlExtractor.cs:139 - The lower remark now says this case reads an embedded Word probe, but this earlier
GetSelfTestCasesdocumentation still says it builds the document itself. That leaves the public self-test description internally contradictory; update the earlier summary to describe the embedded probe too.
src/DemaConsulting.DocDown.Pdf/PdfDocumentExtractor.cs:239 - The lower remark now says the PDF case reads an embedded Word-exported probe, but the earlier
GetSelfTestCasessummary still says it builds a document itself. Update that earlier summary too, or the self-test documentation remains inconsistent with the implementation.
docs/design/docdown-core/detection/format-sniffer.md:25
- This file's algorithm list still reports
0.9,1.0, and0.0confidence values at lines 39–43, contradicting the API change documented elsewhere in the PR thatFormatDetectioncarries no confidence. Remove those score fields from the algorithm description too.
The extension table maps `.pdf`, `.docx`/`.doc`, `.xlsx`/`.xls`, `.pptx`/`.ppt`, `.vsdx`/`.vsdm`/`.vsd`,
docs/design/docdown-core/output/manifest-writer.md:74
- This PR updates the manifest documentation to say
ManifestWriterruns afterContentWriterand beforeMetadataWriter, but theContentWritercompanion document still says the old order withManifestWriterafterMetadataWriter. Update the companion statement as part of the ordering change so the two design artifacts do not contradict each other.
`DocDownEngine` calls `ManifestWriter` after `ContentWriter` and before `MetadataWriter`.
docs/design/docdown-office/com.md:1
- This is a subsystem document nested under the DocDown.Office system, but it uses a level-one heading. Existing nested subsystem design documents use a level-two top heading (for example
docs/design/docdown-office/visio/com.md:1); use##here so the document hierarchy is valid.
# DocDown.Office Com Subsystem Design
docs/design/docdown-office/com.md:63
- The sentence ends with
because it does, which has no grammatical antecedent and does not explain the cost claim. Remove the fragment or replace it with the intended rationale so this new design constraint is readable.
- The availability probe is cheap enough to run on every extraction, because it does.
docs/design/docdown-office/excel/open-xml.md:38
- The changed self-test description still says it builds a fixture, and the next sentence says that building avoids a shipped payload. The implementation now reads an embedded probe, so this paragraph both has the
a embeddedgrammar error and contradicts the shipped-resource design.
embedded workbook authored in Microsoft Excel, reads it with the reader, and
docs/design/docdown-office/excel/open-xml/excel-open-xml-extractor.md:46
- The new probe wording is followed by the stale claim that building rather than embedding keeps the case free of a shipped binary. This self-test reads
ProbeResourceName, so remove that claim and keep the description consistent with the embedded workbook.
- `excel.openxml.parseRoundTrip`, which reads the embedded workbook authored in Microsoft Excel
and passes when the model carries at least one
worksheet. Building rather than embedding a fixture keeps the case free of a shipped binary
payload.
docs/design/docdown-office/powerpoint/com/powerpoint-com-extractor.md:54
- The new
ProbeResourceNameentry is contradicted by the precedingRunRenderdescription, which still says the case builds a synthetic deck and ships no fixture. The implementation writes the embedded probe to the work folder; update that earlier paragraph as well.
- **`ProbeResourceName`** (private const) — names the embedded deck the render case rasterizes: a
real `.pptx` authored in Microsoft PowerPoint, read through `SelfTestProbe.Load`. The synthesizer
docs/design/docdown-office/powerpoint/open-xml.md:40
- The changed self-test description still says it builds a fixture, and the next sentence says that building avoids a shipped payload. The implementation now reads an embedded probe, so this paragraph both has the
a embeddedgrammar error and contradicts the shipped-resource design.
embedded deck authored in Microsoft PowerPoint, reads it with the reader, and passes
docs/design/docdown-office/powerpoint/open-xml/powerpoint-open-xml-extractor.md:44
- The new probe wording is followed by the stale claim that building rather than embedding keeps the case free of a shipped binary. This self-test reads
ProbeResourceName, so remove that claim and keep the description consistent with the embedded deck.
- `powerpoint.openxml.parseRoundTrip`, which reads the embedded deck authored in Microsoft PowerPoint
and passes when the model carries at least one
slide. Building rather than embedding a fixture keeps the case free of a shipped binary payload.
docs/design/docdown-office/visio/com/visio-com-extractor.md:52
- The new
ProbeResourceNameentry is contradicted by the precedingRunRenderdescription, which still says the case builds a synthetic drawing and ships no fixture. The implementation writes the embedded probe to the work folder; update that earlier paragraph as well.
- **`ProbeResourceName`** (private const) — names the embedded drawing the render case rasterizes: a
real `.vsdx` authored in Microsoft Visio, read through `SelfTestProbe.Load`. The synthesizer that
docs/design/docdown-office/visio/open-xml/visio-open-xml-extractor.md:51
- The dependency list names
SelfTestProbe, but the self-test description still says it builds a drawing at runtime. The implementation now reads the embedded Visio probe, so change this description to match the actual case.
- **`SelfTestProbe`** (Core) — loads the embedded drawing the self-test reads.
docs/reqstream/docdown-office.yaml:25
- The COM availability requirement is not a decomposition of the system registration requirement: backend registration and runtime availability are independent behaviors and are tested separately. Existing package-level requirement files keep registration and availability as sibling requirements; remove this child link rather than making availability appear to implement registration.
children:
- DocDownOffice-Com-Availability
docs/reqstream/docdown-office/com.yaml:25
DocDownOffice-Com-DelegatedCompositionis an independent COM behavior, not a decomposition of availability: the availability probe can succeed while composition is still wrong, and the two requirements have separate test sets. The existing PowerPoint and Visio COM requirement files model rendering, availability, and automation as sibling requirements; remove thischildrenlink so the new shared requirements have the same traceability.
children:
- DocDownOffice-Com-DelegatedComposition
docs/verification/docdown-office/com.md:1
- This is a subsystem verification document nested under the DocDown.Office system, but it uses a level-one heading. Existing nested subsystem verification documents use a level-two top heading (for example
docs/verification/docdown-office/visio/com.md:1); use##here.
docs/verification/docdown-office/excel.md:49 - This section acknowledges a committed workbook probe, but it still says every workbook is generated and later says the repository stays text-only. The probe is a shipped
.xlsx, so scope the generated statements to test fixtures and remove the text-only claim.
docs/verification/docdown-office/powerpoint.md:61 - This section now says a probe binary is committed, but it also says every deck is generated and explicitly claims no
.pptxis committed and the repository is text-only. The shipped probe makes those statements false; distinguish test fixtures from the probe throughout this paragraph.
docs/verification/docdown-office/powerpoint/com/powerpoint-com-extractor.md:9 - The updated test-project reference leaves the release-time description at lines 100–102 saying
docdown --validatebuilds a synthetic deck and ships no fixture. The COM self-test now writes the embedded PowerPoint probe; update that scenario so the verification document does not describe the old implementation.
docs/verification/docdown-office/visio.md:52 - The new heading distinguishes fixtures from the probe, but this unchanged sentence still claims every drawing used by the suite is generated. The COM/Open XML self-tests read the embedded application-authored probe, so limit this claim to test fixtures.
docs/verification/docdown-office/visio/com/visio-com-extractor.md:9 - The updated test-project reference leaves the release-time description at lines 96–98 saying
docdown --validatebuilds a synthetic drawing and ships no fixture. The COM self-test now writes the embedded Visio probe; update that scenario so the verification document does not describe the old implementation.
docs/verification/docdown-office/visio/open-xml/visio-open-xml-extractor.md:18 - This test-environment entry still says the self-test round-trip uses a drawing built at test time, although the implementation and the updated design now read the embedded Visio probe. Distinguish the embedded self-test input from the generated reader/integration fixtures.
docs/verification/docdown-office/word.md:35 - The new heading distinguishes fixtures from the probe, but this unchanged sentence still claims every document used by the suite is generated. The self-test reads the embedded application-authored document, so this should say
test fixturerather thandocument.
docs/verification/docdown-pdf-rendering.md:44 - The heading now acknowledges a committed probe, but the following paragraph still says every PDF is generated and that the repository stays text-only. That makes the verification design contradict the embedded rendering probe; distinguish generated test fixtures from the committed self-test input here.
docs/verification/docdown-pdf.md:27 - This sentence now says what is true of generated fixtures, but not of the newly committed self-test probe: the probe is also a document used by the self-test. Limit the claim to test fixtures so it does not contradict the next sentence.
src/DemaConsulting.DocDown.Core/Output/ContentWriter.cs:214 - This update narrows the parameter description to the two current enum values, but the preceding summary still says a part can be a slide even though
ContentPartKindonly containsSheetandChart. Please update the summary at the same time so the public XML documentation is internally consistent.
src/DemaConsulting.DocDown.Office/Visio/OpenXml/VisioOpenXmlExtractor.cs:134 - The sentence is malformed after the probe change:
gluedis left hanging andit backis the remainder of the removed "reads it back" phrase. This XML comment should describe the embedded drawing and the topology assertion without the orphaned text.
test/DemaConsulting.DocDown.Core.Tests/Detection/FormatSnifferTests.cs:213 - This test was renamed to assert only the detection basis, but the file still documents and comments a confidence contract in the class remarks and several assertions, including the new assertion comment below.
FormatDetectionno longer has confidence, so remove those remaining claims throughout this test file; otherwise its documentation describes an API that no longer exists.
- Files reviewed: 94/94 changed files
- Comments generated: 1
- Review effort level: Lite
The probe-fixture swap left prose across source doc comments, design, verification and requirements still describing self-tests that build their own fixtures. They read embedded probes now. Also drops the last confidence references left by the FormatDetection cut, restructures the two Com documents onto the subsystem heading template, narrows three over-broad verification claims to what the tests actually assert, and gives the two Com requirements a real system parent instead of hanging them off registration. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The review flagged four files; the same half-applied edit had left 'builds a embedded workbook' and 'Building rather than embedding a fixture' in nine more design and requirements documents describing self-tests that now read an embedded probe. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved documentation and traceability inconsistencies remain across the reviewed design, requirements, model, and source comments.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (23)
Previously missed (4) — in code that hasn't changed since the last review.
docs/design/docdown-core/detection.md:56
- This lead-in says the supporting value types remain unchanged, but the following bullets document the changed
FormatDetectioncontract after confidence was removed. The section now contradicts itself; change the lead-in to introduce the current type definitions.
docs/design/docdown-office/excel/open-xml/excel-open-xml-extractor.md:70 - The replacement leaves the old
workbook.fragment on the next line, so the dependency entry now reads “the managed reader this backend is built on workbook.” Remove the orphaned fragment and finish this bullet as a complete dependency description.
docs/design/docdown-office/powerpoint/open-xml/powerpoint-open-xml-extractor.md:69 - The replacement leaves the old
deck.fragment on the next line, so the dependency entry now reads “the managed reader this backend is built on deck.” Remove the orphaned fragment and finish this bullet as a complete dependency description.
docs/design/docdown-office/word/open-xml.md:44 - This sentence still says the parse case “builds a embedded” document. The case now reads the shipped probe, so the article is grammatically wrong and the verb contradicts the implementation.
docs/design/docdown-office/excel/open-xml.md:38
- This sentence still says the parse case “builds a embedded” workbook. The case now reads the shipped probe, so the article is grammatically wrong and the verb contradicts the implementation.
**The self-test set.** Two cases are exposed: an `excel.openxml.parseRoundTrip` case that reads the
embedded workbook authored in Microsoft Excel, reads it with the reader, and
docs/design/docdown-office/excel/open-xml/excel-open-xml-extractor.md:46
- The new probe description is immediately followed by the old claim that building rather than embedding keeps the case free of a shipped binary. That is now false: this self-test reads an embedded
.xlsx; remove the obsolete sentence.
- `excel.openxml.parseRoundTrip`, which reads the embedded workbook authored in Microsoft Excel
and passes when the model carries at least one
worksheet. Embedding a probe Excel itself authored is what makes the case prove this deployment
can read what the real application emits, rather than that a library agrees with itself.
docs/design/docdown-office/excel/open-xml/excel-open-xml-extractor.md:44
- The design now says this case reads the embedded Excel probe, but its linked requirement still justifies the self-test as one that "builds and reads a workbook" (
docs/reqstream/docdown-office/excel/open-xml/excel-open-xml-extractor.yaml:82-85). Update that requirement too so the source, design, and ReqStream statement describe the same probe.
- `excel.openxml.parseRoundTrip`, which reads the embedded workbook authored in Microsoft Excel
and passes when the model carries at least one
docs/design/docdown-office/powerpoint/com/powerpoint-com-availability.md:25
- “PowerPoint and Visio use in common” is not grammatical English. Replace it with “PowerPoint and Visio share” so the explanation reads naturally.
The probe itself lives in the shared `OfficeComAvailability`, which PowerPoint and Visio use in
common: the two probes differed only in the application name and the ProgID, so those are arguments
rather than two copies of the logic. This type names PowerPoint's own entry point and supplies them.
docs/design/docdown-office/powerpoint/com/powerpoint-com-extractor.md:48
- The embedded probe carries two slides, and
PowerPointComExtractorrequiresslides.Count == ProbeSlideCountwithProbeSlideCount = 2; this design text still says the self-test passes when exactly one slide returns. That documents a false pass criterion. Describe the expected two slides instead.
writes the embedded probe deck into the self-test work folder, renders it through the real
`PowerPointAutomation` at 96 DPI, and passes only when exactly one slide came back carrying
docs/design/docdown-office/powerpoint/com/powerpoint-com-extractor.md:48
- This new description is not synchronized with the SysML2 unit model:
docs/sysml2/model/docdown-office/powerpoint/com/powerpoint-com-extractor.sysml:3still sayspowerpoint.com.renderbuilds a synthetic single-slide deck, while the source and requirement now use an embedded two-slide probe. Update the model in the same change so the source, design, and model do not disagree.
writes the embedded probe deck into the self-test work folder, renders it through the real
`PowerPointAutomation` at 96 DPI, and passes only when exactly one slide came back carrying
docs/design/docdown-office/powerpoint/open-xml.md:40
- This sentence still says the parse case “builds a embedded” deck. The case now reads the shipped probe, so the article is grammatically wrong and the verb contradicts the implementation.
**The self-test set.** Two cases are exposed: a `powerpoint.openxml.parseRoundTrip` case that reads
the embedded deck authored in Microsoft PowerPoint with the reader, and passes
docs/design/docdown-office/powerpoint/open-xml/powerpoint-open-xml-extractor.md:44
- The new probe description is immediately followed by the old claim that building rather than embedding keeps the case free of a shipped binary. That is now false: this self-test reads an embedded
.pptx; remove the obsolete sentence.
- `powerpoint.openxml.parseRoundTrip`, which reads the embedded deck authored in Microsoft PowerPoint
and passes when the model carries at least one
slide. Embedding a probe PowerPoint itself authored is what makes the case prove this deployment
docs/design/docdown-office/powerpoint/open-xml/powerpoint-open-xml-extractor.md:43
- The design now says this case reads the embedded PowerPoint probe, but its linked requirement still justifies the self-test as one that "builds and reads a deck" (
docs/reqstream/docdown-office/powerpoint/open-xml/powerpoint-open-xml-extractor.yaml:80-83). Update that requirement too so the source, design, and ReqStream statement describe the same probe.
- `powerpoint.openxml.parseRoundTrip`, which reads the embedded deck authored in Microsoft PowerPoint
and passes when the model carries at least one
docs/design/docdown-office/visio/com/visio-com-availability.md:24
- “Visio and PowerPoint use in common” is not grammatical English. Replace it with “Visio and PowerPoint share” so the explanation reads naturally.
The probe itself lives in the shared `OfficeComAvailability`, which Visio and PowerPoint use in
common: the two probes differed only in the application name and the ProgID, so those are arguments
rather than two copies of the logic. This type names Visio's own entry point and supplies them.
docs/design/docdown-office/visio/com/visio-com-extractor.md:45
- This new description is not synchronized with the SysML2 unit model:
docs/sysml2/model/docdown-office/visio/com/visio-com-extractor.sysml:3still saysvisio.com.renderbuilds a synthetic single-page drawing, while the source and requirement now use an embedded probe. Update the model in the same change so the source, design, and model do not disagree.
`visio.com.render` walks through the door the availability case only knocks on: it writes the
embedded probe drawing into the self-test work folder, renders it through the real
docs/design/docdown-office/word/open-xml/word-open-xml-extractor.md:40
- The design now says this case reads the embedded Word probe, but its linked requirement still justifies the self-test as one that "builds and reads a document" (
docs/reqstream/docdown-office/word/open-xml/word-open-xml-extractor.yaml:77-79). Update that requirement too so the source, design, and ReqStream statement describe the same probe.
- `word.openxml.parseRoundTrip`, which reads the embedded document authored in Microsoft Word
and passes when the body carries content.
docs/reqstream/docdown-office.yaml:27
- The verification design treats
AddOffice_NullBuilder_Throwsas part ofDocDownOffice-Registration, but this ReqStream requirement lists only the other three scenarios. That leaves the null-builder acceptance behavior unlinked in the requirements trace; add this test here or introduce a dedicated child requirement for the null-input contract.
tests:
- AddOffice_OnBuilder_RegistersEveryOfficeBackend
- AddOffice_ReturnsSameBuilderForChaining
- AddExcel_Alone_RegistersOnlyTheExcelBackend
docs/reqstream/docdown-office/visio/open-xml/visio-open-xml-extractor.yaml:78
- Updating this nested requirement to say the managed Visio self-test reads the embedded probe exposes a stale parent statement at
docs/reqstream/docdown-office/visio.yaml:251-254, which still says the round-trip builds a drawing. Update the parent requirement too so both ReqStream levels describe the same self-test.
docs/reqstream/ots/openxml.yaml:50 - The
testslist still usesWordOpenXmlExtractor_SelfValidation_ReportsCasesas evidence for the SDK's document-creation requirement, but that self-test now reads the embedded Word probe and no longer exercisesDocumentFormat.OpenXml's writer. The linked verification text also still says it builds a document. Remove that self-test from this OTS evidence or replace the trace with a writer-backed fixture test.
docs/reqstream/ots/pdfpig.yaml:86 - The
testslist still usesPdfDocumentExtractor_GetSelfTestCases_DeployedBackend_ReturnsParseAndRenderingCasesas evidence for PdfPig document creation, but that self-test now reads an embedded PDF and no longer exercises PdfPig's writer. The linked verification text still says the self-test builds a document. Remove that self-test from this OTS evidence or replace the trace with a writer-backed fixture test.
src/DemaConsulting.DocDown.Core/Output/ContentWriter.cs:214 - The new parameter text narrows
ContentPartto worksheets/charts, but the summary immediately above still says a part can be a sheet or slide, and theContentPartKindremarks below likewise describe decks/slides even though the enum only hasSheetandChart. Please update the surrounding XML documentation to one consistent vocabulary; otherwise the public API docs contradict the actual type.
src/DemaConsulting.DocDown.Office/PowerPoint/Com/PowerPointComExtractor.cs:308 - Although this hunk correctly describes the embedded probe, the class still documents
RenderAndInspect's input as a synthetic deck and retains a probe-size remark describing a single slide, whileProbeSlideCountis 2. The updated XML documentation therefore still contradicts the implementation; update those remaining probe descriptions.
src/DemaConsulting.DocDown.Office/Visio/Com/VisioComExtractor.cs:290 - This hunk switches the case to the embedded probe, but the same class still emits
for a drawing the self-test built with exactly one.fromDescribeRenderShortfall(and its render-helper parameter still calls the input synthetic). Those descriptions are now false and can mislead release-time failure output; update the remaining provenance text.
- Files reviewed: 106/106 changed files
- Comments generated: 2
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved documentation and contract inconsistencies remain in the reviewed changes.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (15)
Previously missed (1) — in code that hasn't changed since the last review.
docs/design/docdown-office/com.md:65
- The added ending “because it does” makes this bullet grammatically and logically incomplete: running the probe on every extraction is not the reason it is cheap. Remove the dangling clause or state the intended reason explicitly.
docs/design/docdown-office/powerpoint/com/powerpoint-com-availability.md:25
- "Which PowerPoint and Visio use in common" is not grammatical English in this newly added paragraph. Use "which PowerPoint and Visio share" so the description of the shared helper is clear.
The probe itself lives in the shared `OfficeComAvailability`, which PowerPoint and Visio use in
common: the two probes differed only in the application name and the ProgID, so those are arguments
rather than two copies of the logic. This type names PowerPoint's own entry point and supplies them.
docs/design/docdown-office/powerpoint/com/powerpoint-com-extractor.md:54
- This design update now says the COM self-test renders an embedded PowerPoint deck, but the companion SysML unit model still documents
powerpoint.com.renderas rendering a synthetic single-slide deck (docs/sysml2/model/docdown-office/powerpoint/com/powerpoint-com-extractor.sysml:3). Update that model description in the same change so the architecture model does not contradict its linked design and source.
- **`ProbeResourceName`** (private const) — names the embedded deck the render case rasterizes: a
real `.pptx` authored in Microsoft PowerPoint, read through `SelfTestProbe.Load`. The synthesizer
docs/design/docdown-office/visio/com/visio-com-availability.md:24
- "Which Visio and PowerPoint use in common" is not grammatical English in this newly added paragraph. Use "which Visio and PowerPoint share" so the description of the shared helper is clear.
The probe itself lives in the shared `OfficeComAvailability`, which Visio and PowerPoint use in
common: the two probes differed only in the application name and the ProgID, so those are arguments
rather than two copies of the logic. This type names Visio's own entry point and supplies them.
docs/design/docdown-office/visio/com/visio-com-extractor.md:52
- This design update now says the COM self-test renders an embedded Visio drawing, but the companion SysML unit model still documents
visio.com.renderas rendering a synthetic single-page drawing (docs/sysml2/model/docdown-office/visio/com/visio-com-extractor.sysml:3). Update that model description in the same change so the architecture model does not contradict its linked design and source.
- **`ProbeResourceName`** (private const) — names the embedded drawing the render case rasterizes: a
real `.vsdx` authored in Microsoft Visio, read through `SelfTestProbe.Load`. The synthesizer that
docs/design/docdown-pdf/pdf-document-extractor.md:44
- This now says the self-test reads the embedded probe but then "rereads it," which is still the old build-and-read behavior. The implementation only loads and parses the embedded bytes once; please describe this as a parse case rather than a round trip.
parse round trip that reads the embedded probe document and rereads it in memory, and a
page-rendering case that
docs/verification/docdown-office.md:17
- This new sentence is grammatically incorrect: “what those four registered” has no noun for
registeredto modify. Say thatAddOffice()must register the same backends as the four calls.
docs/verification/docdown-office.md:32 - The probe set includes Visio, which is read through the Open Packaging/OPC path rather than the Open XML SDK. Calling the ZIP header an invariant of "every Open XML package" is too narrow for the four-probe assertion; use OPC/package wording here.
src/DemaConsulting.DocDown.Core/Output/ContentWriter.cs:214 - The parameter text now describes worksheets/charts, but the summary immediately above still says “sheet or slide,” while
ContentPartKindexposesSheetandChartonly. Update the summary with this correction so the public XML documentation is internally consistent.
src/DemaConsulting.DocDown.Office/Excel/OpenXml/ExcelOpenXmlExtractor.cs:148 - The preceding sentence was updated from a generated workbook to the embedded probe, but this line still claims the operation is a round trip. The code only loads the probe into memory, so this remark is now inaccurate.
src/DemaConsulting.DocDown.Office/PowerPoint/Com/PowerPointComExtractor.cs:308 - The outer self-test documentation now says the embedded probe is written to the work folder, but the
RenderAndInspectparameter documentation below still calls that input a synthetic deck. Update the helper documentation too so the source no longer describes a document that is not synthesized.
src/DemaConsulting.DocDown.Office/Visio/Com/VisioComExtractor.cs:290 - The outer self-test documentation now says the embedded probe is written to the work folder, but the
RenderAndInspectparameter documentation below still calls that input a synthetic drawing. Update the helper documentation too so the source no longer describes a document that is not synthesized.
src/DemaConsulting.DocDown.Office/Word/OpenXml/WordOpenXmlExtractor.cs:142 - The preceding sentence was updated from a generated document to the embedded probe, but this line still claims the operation is a round trip. The code only loads the probe into memory, so this remark is now inaccurate.
src/DemaConsulting.DocDown.Pdf/PdfDocumentExtractor.cs:242 - The preceding sentence was updated from building a PDF to reading the embedded probe, but this line still claims the operation is a round trip. The code only loads and parses the embedded bytes, so this remark is now inaccurate.
test/DemaConsulting.DocDown.Core.Tests/Detection/FormatSnifferTests.cs:213 - This hunk removes confidence from one test's contract, but the same test class still documents confidence throughout (for example the class remarks and the PDF, HTML, VSDX, VSDM, legacy-extension, and unknown-format assertions). Leaving those claims in place makes the test documentation contradict the current
FormatDetectionAPI; update the remaining references as part of this contract change.
- Files reviewed: 106/106 changed files
- Comments generated: 2
- Review effort level: Lite
The COM param docs still called the render input a synthetic deck/drawing, and the model documented both COM self-tests as rendering synthetic documents. Also replaces the copy-pasted 'authored in the application that produces the format' with the actual application per backend, drops a duplicated probe sentence in the Visio document, and corrects two OTS verification claims that still described the self-tests as writing their own document. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Outstanding documentation and evidence corrections remain in the final review comments.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (21)
Previously missed (2) — in code that hasn't changed since the last review.
docs/design/docdown-pdf/pdf-document-extractor.md:45
- This design bullet still says the parse case reads and then rereads the probe, but the implementation only loads the embedded PDF and parses it once. Calling it a round trip with a reread preserves the old generated-fixture behavior that this PR is removing; describe it as a single in-memory probe read.
src/DemaConsulting.DocDown.Core/Output/ContentWriter.cs:214 - The
ContentPartKindenum currently exposes onlySheetandChart(seeContentWriter.cs:236-249), but the summary immediately above still advertises a slide. This new parameter text narrows the description without fixing that stale summary, leaving the public XML documentation inconsistent; update the summary and parameter together.
docs/design/docdown-office/com.md:65
ExtractorRegistry.GetCandidates()caches availability per registry and re-probes only afterRefreshAvailability(src/DemaConsulting.DocDown.Core/Extraction/ExtractorRegistry.cs:118-125), so this constraint is inaccurate: the probe does not run on every extraction. Please document the lazy first probe/cache and explicit refresh behavior instead, otherwise the design overstates registry and OS/registry I/O.
- The availability probe is cheap enough to run on every extraction, because it does.
docs/design/docdown-office/com.md:51
- The same design section still says an Office probe would run on every extraction, but
ExtractorRegistrycaches availability per registry and only re-probes afterRefreshAvailability(src/DemaConsulting.DocDown.Core/Extraction/ExtractorRegistry.cs:118-125). Please describe the cached result and explicit refresh boundary so the operational behavior is accurate.
- **The probe never launches an application.** The engine probes every registered backend before
selecting one, so a probe that started Microsoft Office would do so on every extraction, including
on machines that cannot run it.
docs/reqstream/docdown-office.yaml:33
DocDownOffice-ComAutomationis a requirement with a combined behavior but has notests:entry; only its child requirements carry tests. That leaves the parent without direct ReqStream evidence and differs from the neighboring parent requirements, which link a test. Add a direct system-level test link or restructure this as a grouping node rather than a separately verified requirement.
- id: DocDownOffice-ComAutomation
title: >-
The system shall report whether its COM automation backends can run in the current
environment, and shall keep a delegated run's output consistent with what it produced.
tags: [system]
docs/reqstream/docdown-office.yaml:50
- The sole test linked to
DocDownOffice-SelfTestProbeschecks only that four resources are non-empty ZIPs. That cannot establish the requirement's stronger condition that each probe was authored by the corresponding Office application; a DocDown-generated package would pass the same test. Either weaken the requirement to an embedded valid probe or add traceable provenance evidence for the authorship claim.
- id: DocDownOffice-SelfTestProbes
title: >-
The system shall carry, for each format it reads, a probe document authored in the
application that produces that format.
docs/reqstream/docdown-office/com.yaml:33
- This new requirement makes suppression conditional on the COM run having rendered pages, but the current extractors wrap the sink before rendering and report
pages.rendereravailable beforeRenderSlidesAsync/RenderPagesAsync. If every page fails, the delegatedpageRenderingfact is still suppressed even though no page was rendered. Make the suppression depend on at least one successful render (with an all-failed test), or define this requirement as a capability statement rather than a rendered-output statement.
- id: DocDownOffice-Com-DelegatedComposition
title: >-
The subsystem shall suppress a delegated managed backend's statement that it does not
render pages, when the COM backend has rendered them.
docs/reqstream/docdown-office/com.yaml:20
ExtractorRegistrycaches each backend's availability after the first candidate query and re-probes only afterRefreshAvailability; it is not probed “on every extraction.” This makes the justification factually inaccurate and overstates the side effect being prevented. Please describe the cached probe pass instead.
The engine probes every registered backend before it selects one, so a probe that
launched Microsoft Office would start an application on a machine that may not even be
able to run it, on every extraction. A probe reads the operating system and the
docs/reqstream/docdown-office/excel/open-xml/excel-open-xml-extractor.yaml:86
- This justification still says the embedded workbook is read back in a round trip, but the self-test only loads the committed Excel-authored probe and parses it. Replace the round-trip claim with the actual probe-parse behavior so the requirement evidence remains truthful.
docs/reqstream/docdown-office/powerpoint/open-xml/powerpoint-open-xml-extractor.yaml:84 - The requirement justification says the embedded probe is read back, which implies a writer/read round trip that this implementation no longer performs. It only loads and parses the application-authored deck; revise this justification so the requirement does not claim evidence from a nonexistent write step.
docs/reqstream/docdown-office/visio/open-xml/visio-open-xml-extractor.yaml:81 - This justification says the embedded Visio drawing is read back in a round trip, but the self-test only loads and parses the committed application-authored probe. That overstates what the test proves; revise it to describe the direct topology parse.
docs/reqstream/docdown-office/word/open-xml/word-open-xml-extractor.yaml:80 - The requirement justification still describes reading the embedded Word probe back after a write, but the implementation only loads and parses the application-authored document. Update this evidence statement to describe the actual embedded-probe parse.
docs/verification/docdown-office/visio/open-xml.md:30 - This subsystem input description says there are no committed binary fixtures, but the managed self-test reads the committed application-authored
probe.vsdx. Distinguish generated suite packages from the embedded probe; otherwise the verification setup omits a required input and contradicts the implementation.
src/DemaConsulting.DocDown.Core/Detection/FormatSniffer.cs:162 - The implementation treats a recognized extension as the primary signal and returns before reading content, so calling the extension "weaker evidence" than a content signature is misleading now that confidence scores are gone. Please describe the extension as the primary/short-circuiting signal and the signature as the fallback instead.
src/DemaConsulting.DocDown.Office/Excel/OpenXml/ExcelOpenXmlExtractor.cs:148 - The updated description says the case reads an embedded Excel workbook, but the last sentence still says the whole round trip fits in memory. This case no longer creates a workbook or performs a round trip; describe the embedded probe as being loaded and parsed in memory instead.
src/DemaConsulting.DocDown.Office/PowerPoint/OpenXml/PowerPointOpenXmlExtractor.cs:137 - This hunk now says the self-test only reads the embedded deck, but the immediately preceding summary and case ID still call it a “parse round-trip,” and the exception path below still reports a round trip. Since this code never writes a deck, update those remaining labels/messages so the self-test documentation and failure output describe the operation that actually runs.
src/DemaConsulting.DocDown.Office/Visio/OpenXml/VisioOpenXmlExtractor.cs:136 - Although this hunk now loads the embedded drawing, it still says the drawing is parsed “back” and leaves the method summary/case name and exception text as a parse round-trip. Nothing is written before the reader runs, so this describes a different operation and makes failures misleading; update the remaining round-trip wording consistently.
src/DemaConsulting.DocDown.Office/Word/OpenXml/WordOpenXmlExtractor.cs:142 - The updated description says the case reads an embedded Word document, but the last sentence still says the whole round trip fits in memory. This case no longer creates a document or performs a round trip; describe the embedded probe as being loaded and parsed in memory instead.
src/DemaConsulting.DocDown.Pdf.Rendering/PdfPageRenderingExtractor.cs:295 - The implementation now rasterizes a page from an embedded PDF, not a PDF it creates and rereads, but the method summary and catch message still call this a render round trip. That leaves the self-test's description and reported failures factually wrong; replace the remaining round-trip wording with render-specific wording.
src/DemaConsulting.DocDown.Pdf/PdfDocumentExtractor.cs:242 - The first sentence now correctly describes a shipped probe, but the final sentence still calls this a round trip. The self-test no longer builds and rereads a document; it loads one embedded PDF and parses it, so this remark misstates what the case proves.
test/DemaConsulting.DocDown.Office.Tests/Visio/OpenXml/VisioOpenXmlExtractorTests.cs:47 - The updated test description still says the self-test “reads back” the embedded drawing, which implies a prior write that no longer occurs. The test only loads the committed Visio-authored probe and parses it; remove “back” so the evidence describes the actual check.
- Files reviewed: 111/111 changed files
- Comments generated: 2
- Review effort level: Lite
probe.pdf reports Producer 'Microsoft Word for Microsoft 365' and the Office probes were authored in their own applications, so neither self-validation case exercises the SDK or PdfPig writer. Both are dropped from the DocumentCreation requirements and the verification text now says why they are excluded. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved moderate traceability and documentation issues remain.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (21)
Previously missed (1) — in code that hasn't changed since the last review.
src/DemaConsulting.DocDown.Office/Visio/Com/VisioComExtractor.cs:282
- These updated remarks now say the render writes the embedded probe, but
DescribeRenderShortfallstill emits “for a drawing the self-test built with exactly one.” That failure message is false for the current implementation; update it to identify the embedded probe drawing instead.
docs/design/docdown-core/output/manifest-writer.md:74
- This caller note is unconditional, but unreadable runs go through
WriteFailureAsyncand callProduceResultAsyncwith noContentWriterresult; only produced runs invokeContentWriterbeforeManifestWriter. Clarify the failure-path exception so this design sequence matches the implementation.
`DocDownEngine` calls `ManifestWriter` after `ContentWriter` and before `MetadataWriter`.
docs/design/docdown-office/excel/open-xml/excel-open-xml-extractor.md:51
- The self-test now only loads and parses the embedded workbook; it does not build or read back a workbook, so calling this the “round-trip case” is inaccurate and conflicts with the preceding probe description.
- **`RunParseRoundTrip`** (private) — the round-trip case body. It reads the embedded workbook named
docs/design/docdown-office/powerpoint/com.md:65
- This replacement drops the
PowerPointComDispatchsupporting-type bullet and leaves its continuation as an orphaned sentence, so the section no longer documents the dispatch helper and appears to attribute watchdog/COM details to the shared sink. Keep the shared-type bullet and re-addPowerPointComDispatchas its own bullet before the section ends.
- **`ComposingDelegatedSink`** and **`DelegatedExtractionContext`** — not owned by this
subsystem. They live in the shared `DocDown.Office.Com` namespace, because Visio and PowerPoint
needed byte-equivalent copies of both. The sink suppresses the delegated managed backend's
`powerpoint.pageRendering` unavailable fact so it does not contradict the rendering this run performed; the fact
key is a constructor argument, which was the only real difference between the two former copies.
docs/design/docdown-office/powerpoint/open-xml/powerpoint-open-xml-extractor.md:51
- The self-test now only loads and parses the embedded deck; it does not build or read back a deck, so calling this the “round-trip case” is inaccurate and conflicts with the preceding probe description.
- **`RunParseRoundTrip`** (private) — the round-trip case body. It reads the embedded deck named by
docs/design/docdown-office/visio/com.md:52
- The shared-type replacement removes
VisioComDispatchfrom the subsystem's supporting-type list, even though this design doc is the model's documented design location for that unit and the adapter still depends on it. Add the low-level dispatch helper as its own bullet after the shared types.
- **`ComposingDelegatedSink`** and **`DelegatedExtractionContext`** — not owned by this
subsystem. They live in the shared `DocDown.Office.Com` namespace, because Visio and PowerPoint
needed byte-equivalent copies of both. The sink suppresses the delegated managed backend's
`visio.pageRendering` unavailable fact so it does not contradict the rendering this run performed; the fact
key is a constructor argument, which was the only real difference between the two former copies.
docs/design/docdown-office/word/open-xml/word-open-xml-extractor.md:49
- The self-test now only loads and parses the embedded document; it no longer performs a write/read round trip, so describing this resource as what the “round-trip case” reads is inaccurate.
- **`ProbeResourceName`** (private const) — names the embedded document the round-trip case reads: a
docs/reqstream/docdown-office.yaml:45
- This new system requirement has child requirements but no
tests:entry, so ReqStream has no direct system-level evidence for the stated COM behavior. The repository's requirements standard requires every requirement to link a passing test; add a system-level scenario (or otherwise restructure this as a non-requirement grouping) so the requirement is traceable.
- id: DocDownOffice-ComAutomation
title: >-
The system shall report whether its COM automation backends can run in the current
environment, and shall keep a delegated run's output consistent with what it produced.
tags: [system]
justification: |
Two of the six backends this package ships drive a Microsoft Office application over COM,
and neither can run where that application is absent - which is every CI runner and most
Linux and macOS hosts. The system must therefore answer whether such a backend is usable
before it is selected, and must do so without launching the application. Where a COM
backend does run, it delegates content extraction to its managed counterpart and adds
rendering on top, so the composed report must not carry the delegate's statement that no
rendering was performed. Both behaviors are shared by the COM backends and decompose into
the Com subsystem.
children:
- DocDownOffice-Com-Availability
- DocDownOffice-Com-DelegatedComposition
docs/reqstream/docdown-office.yaml:59
- The linked
Probe_EmbeddedResource_LoadsAsAnOpenPackagetest only checks that each resource is a non-empty ZIP; a DocDown-synthesized package would pass it too. Nothing in the automated evidence proves the stronger requirement that the probes were authored by Word, Excel, PowerPoint, and Visio, so either add verifiable provenance evidence or narrow the requirement to carrying embedded probes.
- id: DocDownOffice-SelfTestProbes
title: >-
The system shall carry, for each format it reads, a probe document authored in the
application that produces that format.
tags: [system]
justification: |
A self-test exists to answer whether this deployment can read what the real application
emits. A backend that synthesized its own document could only ever prove that a library
agreed with itself, and would keep reporting success while failing on every genuine
document. Carrying the probe inside the package also means the check does not depend on
the file system, the working directory, or anything the host happens to have installed.
tests:
- Probe_EmbeddedResource_LoadsAsAnOpenPackage
docs/verification/docdown-office/excel.md:44
- This sentence says every workbook used by the suite is generated, but the extractor self-test exercised by the suite reads the committed embedded Excel probe. Qualify the sentence as referring to test fixtures so it does not contradict the probe distinction immediately below.
docs/verification/docdown-office/powerpoint.md:55 - This sentence says every deck used by the suite is generated, but
PowerPointOpenXmlExtractor_SelfTestCases_RoundTripPassesAndRenderingSkippedruns against the committed embedded PowerPoint probe. Qualify it as referring to test fixtures to preserve the distinction this section is intended to document.
docs/verification/docdown-office/visio.md:55 - This sentence says every drawing used by the suite is generated, but
VisioOpenXmlExtractor_SelfTestCases_RoundTripPassesAndRenderingSkippedreads the committed embedded Visio probe. Qualify it as referring to test fixtures so it matches the probe distinction stated in the next paragraphs.
docs/verification/docdown-office/word.md:38 - This sentence says every document used by the suite is generated, but
WordOpenXmlExtractor_SelfValidation_ReportsCasesexecutes the self-test that reads the committed embedded Word probe. Since the section is distinguishing fixtures from probes, qualify this sentence as referring to test fixtures.
docs/verification/docdown-pdf-rendering.md:49 - The paragraph calls the embedded probe an exception to the rule that every PDF used by the suite is generated, but the probe is not a generated test fixture and is consumed by the validation test. Phrase the rule around test fixtures and describe the probe as a separate self-test input instead.
docs/verification/docdown-pdf.md:31 - This sentence says every PDF used by the suite is generated, but
PdfDocumentExtractor_GetSelfTestCases_DeployedBackend_ReturnsParseAndRenderingCasesruns the self-test against the committed embedded probe. Qualify this as referring to PDF fixtures so the paragraph does not contradict its own probe exception.
src/DemaConsulting.DocDown.Office/Excel/OpenXml/ExcelOpenXmlExtractor.cs:148 - This self-test now only loads and parses the embedded Excel workbook; it no longer creates a workbook, so the remaining “whole round trip” wording is stale. Describe the embedded probe being loaded and parsed in memory instead.
src/DemaConsulting.DocDown.Office/PowerPoint/Com/PowerPointComExtractor.cs:300 - These updated remarks correctly identify the embedded probe, but the adjacent
MaximumPlausiblePixelsdocumentation still says the deck is a single slide even thoughProbeSlideCountis 2. Please update that bound's explanation to describe the per-slide limit without the contradictory single-slide claim.
src/DemaConsulting.DocDown.Office/Word/OpenXml/WordOpenXmlExtractor.cs:142 - This self-test now only loads and parses the embedded Word document; it no longer creates a document, so the remaining “whole round trip” wording is stale. Describe the embedded probe being loaded and parsed in memory instead.
src/DemaConsulting.DocDown.Pdf.Rendering/PdfPageRenderingExtractor.cs:316 - The changed failure message now identifies the embedded document, but the exception path below still reports that a round trip failed. This render self-test only loads and rasterizes probe bytes, so the operator-facing error should describe failure to rasterize the embedded document.
src/DemaConsulting.DocDown.Pdf/PdfDocumentExtractor.cs:242 - This self-test now only loads and parses an embedded PDF; it no longer creates a document, so describing the operation as a round trip is inaccurate. Replace the final sentence with the in-memory embedded-probe behavior so the API documentation matches the implementation.
src/DemaConsulting.DocDown.Pdf/PdfDocumentExtractor.cs:257 - The changed success-path message now identifies the embedded probe, but the exception path below still tells the operator that a round trip failed. Since this case no longer builds anything, the failure message should describe inability to read the embedded document rather than a nonexistent round trip.
- Files reviewed: 111/111 changed files
- Comments generated: 1
- Review effort level: Lite
DocDown has zero document-creation capability: every WordprocessingDocument.Create, SpreadsheetDocument.Create, PresentationDocument.Create and PdfDocumentBuilder call in the repository is in a test project. The two requirements claimed the OTS writers as a DocDown capability and cited reader and extraction tests as evidence -- tests whose every assertion is about DocDown's readers, with the writer appearing only as fixture scaffolding in the arrange step. Nothing could have falsified them. Both came from c387ea7, the same generated batch as the withdrawn 'without requiring native binaries' requirement. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Unresolved documentation and requirement inconsistencies remain, including one moderate finding.
Review details
Suppressed comments (13)
Previously missed (1) — in code that hasn't changed since the last review.
docs/reqstream/docdown-office/com.yaml:33
- The shared sink suppresses the delegate's unavailable fact when it wraps the managed extraction, before the COM render result is known; even an all-pages-failed render follows that path. This requirement therefore promises conditional behavior the implementation does not provide and conflicts with the design's description of filtering the unavailable fact. Phrase the requirement around the COM backend owning the rendering path, or change the implementation to defer suppression.
docs/design/docdown-pdf/pdf-document-extractor.md:43
RunParseRoundTripnow loads the embedded probe once; it does not reread it. This sentence still describes the removed writer-based round trip, so the design document contradicts the implementation it was just updated to describe.
parse round trip that reads the embedded probe document and rereads it in memory, and a
docs/reqstream/docdown-office.yaml:13
- The PR description's
589 → 593requirement count is not supported by the checked-in ReqStream: this tree contains 592 requirement IDs. This change adds five Office IDs, removes two OTS IDs, and renames one Core ID, for a net increase of three from 589; please reconcile the campaign summary with the actual count.
- id: DocDownOffice-Registration
docs/reqstream/ots/openxml.yaml:42
- Removing
DocDown-OTS-OpenXml-DocumentCreationleaves the companion verification document's Required Functionality section still stating that the SDK must build documents suitable for test fixtures, while the suite still generates those fixtures at runtime. Either retain this requirement and its evidence, or remove/update that claim in the companion documents so the requirements and verification artifacts do not disagree.
docs/reqstream/ots/pdfpig.yaml:78 - Removing
DocDown-OTS-PdfPig-DocumentCreationleaves the companion verification document's Required Functionality section still stating that PdfPig must build documents suitable for test fixtures, while the suite still generates those fixtures at runtime. Either retain this requirement and its evidence, or remove/update that claim in the companion documents so the requirements and verification artifacts do not disagree.
docs/verification/docdown-office/com.md:53 - This acceptance criterion has the same mismatch as the requirement:
ComposingDelegatedSinkfilters the managed unavailable fact before rendering and does so even when the adapter returns only per-page failures. It should describe suppression while the COM backend owns the rendering path, or the implementation and tests need to be changed to make suppression conditional on successful rendering.
docs/verification/ots/openxml.md:70 - Removing the
DocDown-OTS-OpenXml-DocumentCreationrequirement leaves this verification document's Required Functionality section still claiming that the SDK must build test-fixture documents. Fixture generation is test infrastructure rather than a remaining OTS requirement, so this section now overstates the contract; remove the writer clause there while retaining the Test Environment description of how fixtures are generated.
docs/verification/ots/pdfpig.md:113 - Removing the
DocDown-OTS-PdfPig-DocumentCreationrequirement leaves this verification document's Required Functionality section still claiming that PdfPig must build test-fixture documents. The writer is test infrastructure, not a capability required by the parser contract, so remove that clause from the required functionality statement while retaining the Test Environment description of generated fixtures.
src/DemaConsulting.DocDown.Core/Detection/FormatSniffer.cs:162 - This still says an extension is weaker evidence and that only the decoder can prove the bytes agree with it, but
Detectreturns a recognized extension result before checking content (FormatSniffer.cs:106-110), and the tests explicitly assert that an extension wins over contradictory bytes. Update this remark to describe the actual contract: extension is primary and content signature is fallback.
src/DemaConsulting.DocDown.Office/Excel/OpenXml/ExcelOpenXmlExtractor.cs:148 - The updated description correctly says the self-test reads an embedded Excel workbook, but the final sentence still calls it a "round trip." No workbook is created or reread here; the probe is loaded and parsed once.
src/DemaConsulting.DocDown.Office/Visio/Com/VisioComExtractor.cs:290 - These new remarks say the self-test writes and renders the embedded probe, but
DescribeRenderShortfallstill reports a wrong page count as coming from a drawing “the self-test built.” That failure message now contradicts the actual input and the updated self-test description; update it to refer to the embedded probe drawing.
src/DemaConsulting.DocDown.Office/Word/OpenXml/WordOpenXmlExtractor.cs:142 - The updated description correctly says the self-test reads an embedded Word document, but the final sentence still calls it a "round trip." No document is created or reread here; the probe is loaded and parsed once.
src/DemaConsulting.DocDown.Pdf/PdfDocumentExtractor.cs:242 - This remark now describes loading the embedded PDF, but it still says that the "whole round trip" fits in memory. The self-test no longer builds or rereads a document; it loads the probe and parses it once, so this leaves the new behavior inaccurately documented.
- Files reviewed: 113/113 changed files
- Comments generated: 0 new
- Review effort level: Lite
PDFium and SkiaSharp were listed as OTS items with their own requirements, design and verification documents, but DocDown referenced neither: both arrive transitively under PDFtoImage. The one place SkiaSharp was named in DocDown source -- PageRenderer's SKBitmap.Encode -- is replaced by PDFtoImage's own Conversion.SavePng, so no DocDown type names either component now. PdfPig was likewise being called directly from Pdf.Rendering, solely to count pages, which would have made it a second direct dependency of that package. PDFtoImage.GetPageCount answers the same question, removes a redundant full parse of every document, and makes the page count come from the component that actually rasterizes the pages. Pdf.Rendering's runtime reference set is now exactly PDFtoImage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
One or more issues must be addressed before approval.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (7)
docs/design/ots/pdftoimage.md:22
- Adding
GetPageCounthere makes PDFtoImage usage no longer confined toPageRenderer: the implementation calls it directly fromPdfPageRenderingExtractor.SelectPageNumbers. That contradicts the single-file native-interop/containment statements in this design and the PageRenderer requirement; route page counting throughPageRendererunder its gate, or update the containment and synchronization contract together.
docs/design/ots/pdftoimage.md:21 PDFtoImage.GetPageCountis not the API used by the implementation; the call isPDFtoImage.Conversion.GetPageCount(...). As written, this feature list points readers to a nonexistent type/member combination.
docs/reqstream/docdown-office.yaml:27- The verification acceptance criteria include
AddOffice_NullBuilder_Throws(and list it as a scenario), but this requirement's test list omits it. The null-builder behavior is therefore tested and documented but not traceable toDocDownOffice-Registration; please add it to this list.
docs/verification/docdown-pdf-rendering.md:48 - This section now identifies the committed probe as the self-test input, but the same document's self-test scenario at lines 186-187 still says the backend “builds itself”; line 85 also still counts PDFium and SkiaSharp as separate OTS items. Leaving those statements unchanged makes the verification document contradict the new probe and OTS structure.
docs/verification/ots.md:45 - Removing PDFium and SkiaSharp as standalone OTS items here leaves contradictory companion evidence:
docs/verification/introduction.mdstill lists both as OTS items anddocs/verification/docdown-pdf-rendering.mdpluspage-renderer.mdstill call them separate OTS items. Update those artifacts as well so the canonical inventory and the cited verification sets agree.
src/DemaConsulting.DocDown.Office/PowerPoint/Com/PowerPointComExtractor.cs:300 - The embedded PowerPoint probe is now a two-slide deck, but the existing
MaximumPlausiblePixelsremark still describes a single 13.3-by-7.5-inch slide. Update that explanation to match the two-slide probe (or explain the bound per slide), otherwise the source documents contradictory probe geometry.
src/DemaConsulting.DocDown.Office/Visio/Com/VisioComExtractor.cs:282 - The changed self-test now renders the embedded Visio probe, but the shortfall text in
DescribeRenderShortfallstill says the drawing was "built with exactly one." A page-count mismatch will report false provenance; change that message to identify the embedded probe drawing.
- Files reviewed: 128/128 changed files
- Comments generated: 3
- Review effort level: Lite
The GetPageCount call sat in the extractor, which created a second PDFtoImage seam against the documented single-seam invariant and, worse, ran before the per-page catch: a count fault on a document the managed delegate had already read would have collapsed the whole extraction to Unreadable instead of costing only the page images. Counting now goes through PageRenderer under the same lock, its failure is isolated into a plain note, and a test injects a faulting counter to prove it. Also corrects the System.IO.Packaging item, which described itself as reached only transitively and cited only Word tests. DocDown.Office declares it and VisioPackageReader calls Package.Open directly, because the Open XML SDK does not read Visio drawings, so the Visio package-reader tests are its direct evidence. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved moderate issues remain in PDF rendering test coverage and OTS/design documentation.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (6)
docs/design/docdown-pdf-rendering/page-renderer.md:45
PageRenderernow also callsConversion.GetPageCountto choose pages, but this dependency entry still lists onlySavePngandRenderOptions. Update the design dependency list so the new native page-count call is documented alongside the rendering path.
- **PDFtoImage** (OTS) — `Conversion.SavePng` and `RenderOptions`, which rasterize the page and write
its PNG bytes in one call. No type from the transitive native stack is named here. See *PDFtoImage*
under the OTS integration design.
docs/design/introduction.md:133
- This rationale uses public type naming as the OTS boundary, but the repository standard classifies external third-party components, not only directly named types, as OTS (
.github/standards/software-items.md:87-100). PDFium and SkiaSharp still ship transitively here and provide the native raster/PNG functionality; this claim therefore leaves the design inconsistent with the repository's OTS classification and remaining verification references.
- **PDFtoImage** — managed page-rasterization API, the runtime dependency of DocDown.Pdf.Rendering;
it delivers a native rasterizer and 2D backend (PDFium and SkiaSharp) transitively, and those are
the only native binaries DocDown ships. No DocDown type names either of them, so neither is a
listed OTS item
docs/design/ots.md:24
- The OTS inventory now drops PDFium and SkiaSharp solely because DocDown does not name their types, but both remain third-party components delivered in this package's runtime dependency graph and provide the native raster/PNG functionality. The repository standard defines external third-party components as OTS (
.github/standards/software-items.md:87-100), anddocs/verification/introduction.mdstill lists both as OTS items. Keep their individual traceability artifacts, or update the categorization standard/model and every remaining cross-reference consistently.
**PdfPig, PDFtoImage, the Open XML SDK, and TestResults are the exceptions, and deliberately so.**
PdfPig is a runtime library that `DocDown.Pdf` depends on and that therefore flows to consumers of
that package; PDFtoImage (with its transitive PDFium and SkiaSharp native stack) is the runtime
library the optional `DocDown.Pdf.Rendering` package depends on to rasterize pages; the Open XML SDK
docs/verification/ots.md:41
- Removing PDFium and SkiaSharp from this OTS inventory leaves the verification documentation inconsistent:
docs/verification/docdown-pdf-rendering.md:85-86,docs/verification/docdown-pdf-rendering/page-renderer.md:12-13, anddocs/verification/introduction.md:165-173still list them as separate OTS items. Update those references in the same change so the declared OTS inventory and its evidence agree.
test/DemaConsulting.DocDown.Pdf.Rendering.Tests/PdfPageRenderingExtractorTests.cs:129 SkipWhenRendererUnavailable()probes the native stack and skips this test when it is absent, but the injectedpageCountdelegate throws beforePageRenderer.Renderis ever called. The count-fault isolation path therefore is not exercised on hosts without the native renderer; remove this skip so the new behavior is covered everywhere.
test/DemaConsulting.DocDown.Pdf.Rendering.Tests/PdfPageRenderingExtractorTests.cs:152- The new test is named and documented as keeping the delegated content, but
ContractAssert.LayoutPresentonly checkssummary.txtandmanifest.json; it never checkscontent.md. A regression that loses the delegated text while preserving the top-level layout would still pass, so assert the content file (and a fixture phrase) here.
- Files reviewed: 133/133 changed files
- Comments generated: 1
- Review effort level: Lite
Written as PDFtoImage.GetPageCount, which is not a callable name; the member is Conversion.GetPageCount, as the SavePng entry above it already shows. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Outstanding documentation and traceability inconsistencies require updates before approval.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (12)
docs/design/ots.md:50
- This inventory now says the Open XML SDK reads Office documents, but
docs/design/ots/openxml.mdstill describes onlyDocDown.Word/WordprocessingML, still listsWordprocessingDocument.Createas a used feature, and points at the removedDemaConsulting.DocDown.Word.csproj. Update that companion design (and its requirements/verification companions) so the consolidated OTS entry is internally consistent.
| Open XML SDK | Reads Office documents for the DocDown.Office extraction package |
docs/design/ots.md:53
- This inventory now lists PDFtoImage as the sole PDF-rendering OTS item, but the changed PDF rendering verification document still enumerates PDFium and SkiaSharp as separate OTS items at
docs/verification/docdown-pdf-rendering.md:85. The OTS inventory and evidence set are therefore inconsistent; update that narrative in this consolidation.
| PDFtoImage | Managed page-rasterization API for the DocDown.Pdf.Rendering package |
docs/reqstream/docdown-office.yaml:51
DocDownOffice-SelfTestProbesrequires each probe to be authored by the application that produces its format, but its only cited test checks that each resource is non-empty and starts withPK; the verification document explicitly calls that a packaging assertion, not a content or provenance assertion. Link the four backend self-test cases that read the embedded probes, or narrow this requirement to the property the cited test actually proves.
docs/verification/docdown-office.md:35- The only cited probe test checks that each resource is a non-empty ZIP, and this section explicitly calls that a packaging—not content—assertion. That cannot evidence the requirement's distinguishing claim that each probe was authored by the producing application; either add format-specific checks that reject synthetic probes or narrow the requirement and its justification.
docs/verification/ots.md:38 - The broadened inventory entry is not reflected in
docs/verification/ots/openxml.md, which still describes only the Word test suite andDocDown.Wordeven though the SDK now serves the consolidated Office package. Update the companion verification document before treating the OTS campaign as complete.
src/DemaConsulting.DocDown.Office/Excel/OpenXml/ExcelOpenXmlExtractor.cs:148 - This self-test now reads the embedded Excel probe rather than building a workbook, so it is no longer a round trip. The final sentence still describes a round trip that fits in memory; update it to explain that the probe is loaded directly from the assembly.
src/DemaConsulting.DocDown.Office/Word/OpenXml/WordOpenXmlExtractor.cs:142 - This self-test now reads the embedded Word probe rather than building a document, so it is no longer a round trip. The final sentence still describes a round trip that fits in memory; update it to explain that the probe is loaded directly from the assembly.
src/DemaConsulting.DocDown.Pdf.Rendering/PageRenderer.cs:126 - This new
GetPageCountseam is not described indocs/design/docdown-pdf-rendering/page-renderer.md's Key Methods section, which still lists onlyRenderandProbeAvailability. Because page counting now drives selection and sharesRenderGate, the design and its verification evidence should document this behavior too.
src/DemaConsulting.DocDown.Pdf.Rendering/PdfPageRenderingExtractor.cs:250 - The new page-count exception path is not reflected in the companion ReqStream/verification artifacts: they still cover only per-page render faults. This leaves the new
Produced/note behavior untraced. Add a count-failure requirement/scenario, or explicitly extend the existing failure requirement to cover it.
src/DemaConsulting.DocDown.Pdf.Rendering/PdfPageRenderingExtractor.cs:61 _pageCountis now a second unit field and the source of page selection, butdocs/design/docdown-pdf-rendering/pdf-page-rendering-extractor.mdstill says_renderis the only field and that PdfPig supplies the page count. Please update the companion design to describe the PDFtoImage count delegate and the injected count-fault constructor; otherwise the unit's architecture and dependency documentation are false.
src/DemaConsulting.DocDown.Pdf/PdfDocumentExtractor.cs:242- After switching this self-test from building a document to loading the embedded probe, it no longer performs a round trip. The updated remark still claims that the whole round trip fits in memory; describe the assembly-loaded probe instead so the documentation matches the implementation.
test/DemaConsulting.DocDown.Pdf.Rendering.Tests/PageRendererTests.cs:13 - This updated test remark correctly attributes the evidence to PDFtoImage, but the companion verification documents still describe standalone PDFium and SkiaSharp OTS items, and
docs/verification/introduction.mdstill lists both. Update those artifacts so the OTS verification inventory agrees with the consolidated PDFtoImage item.
- Files reviewed: 133/133 changed files
- Comments generated: 2
- Review effort level: Lite
The two items were deleted from the register and requirements but four places still described them as listed OTS items: the verification introduction's inventory, the Pdf.Rendering verification overview's 'three OTS items', the PageRenderer verification note, and the PDFtoImage design's own containment paragraph, which asserted the opposite of the statement added above it. Register rows, reqstream files and review sets now agree at 17 each. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Four moderate findings remain unresolved, along with five documentation nits.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (8)
Previously missed (1) — in code that hasn't changed since the last review.
test/DemaConsulting.DocDown.Pdf.Rendering.Tests/PdfPageRenderingExtractorTests.cs:152
- The test name and assertion comment promise to keep delegated content, but
ContractAssert.LayoutPresentonly checks summary/manifest existence and mandatory blocks; it never inspectscontent.md. A regression that drops the delegated text would still pass. Assert thatcontent.mdcontains text fromRenderingFixtures.SimpleText()(for example, the quick-brown-fox line).
docs/design/docdown-office/com.md:65
- This sentence is incomplete: “because it does” never states what makes the probe cheap, leaving the design constraint unreadable. Finish the rationale, such as the fact that it only checks the OS and ProgID registration.
- The availability probe is cheap enough to run on every extraction, because it does.
docs/design/introduction.md:133
- These lines remove PDFium and SkiaSharp from the OTS inventory solely because DocDown does not name their types. The repository's OTS standard covers third-party components that provide functionality (
.github/standards/software-items.md:22-27), and the PDFtoImage design identifies these as independently delivered native components (docs/design/ots/pdftoimage.md:12-14); transitive delivery and public-surface containment do not remove their traceability obligations. Restore separate OTS requirements/design/verification/review entries, or introduce and apply a documented bundling rule consistently.
the only native binaries DocDown ships. No DocDown type names either of them, so neither is a
listed OTS item
docs/reqstream/docdown-office.yaml:50
- The requirement requires each probe to be authored by the originating application, but its only linked test checks that the embedded bytes are non-empty and start with a ZIP header. A synthetic package would pass those assertions, so the distinguishing requirement is not verified; add content/provenance evidence or narrow the requirement to an embedded valid package.
docs/reqstream/docdown-office.yaml:28 - The new system verification document lists
AddOffice_NullBuilder_Throwsas evidence forDocDownOffice-Registration, but the requirement'stestslist omits it. Add the named test here (or stop attributing that scenario to this requirement) so the ReqStream trace and verification document agree.
docs/verification/docdown-pdf-rendering.md:42 - The system verification still documents only the injected per-page render fault, while the PR adds
PdfPageRenderingExtractor_ExtractAsync_PageCountFaults_ReportsNoteAndKeepsDelegatedContent. Add a system scenario and acceptance criterion for the count-failure note and preserved delegated output.
docs/verification/docdown-pdf-rendering.md:87 - The sentence is grammatically incomplete:
eitherhas no object. Complete it aseither componentso this verification document is readable.
docs/verification/introduction.md:174 - This entry attributes the verification of System.IO.Packaging only to DocDown.Word, but VisioPackageReader calls the API directly and the consolidated Office tests cover that path. Update the evidence description to include the direct Visio usage rather than implying Word-only evidence.
- Files reviewed: 135/135 changed files
- Comments generated: 1
- Review effort level: Lite
What this fixes
The
DocDownOfficesystem and itsComsubsystem were added to the SysML2 model pointingat requirements, design and verification documents that were never written. Nothing validates model
document references —
sysml2tools lintchecks structure, not whether a cited path exists — so lintstayed clean while the newest system in the repository had no requirements of its own.
That gap is closed, and the full formal review campaign across all 125 review-sets has now been
run on top of it.
What is here
Five new documents:
docs/reqstream/docdown-office.yamldocs/reqstream/docdown-office/com.yamldocs/design/docdown-office/com.mdComDispatchis deliberately not shareddocs/verification/docdown-office.mddocs/verification/docdown-office/com.mdReview campaign
125 of 125 review-sets reviewed. 68 defects found and fixed.
Essentially every defect was the same species: prose that had stopped matching the code. Counts that
no longer counted the right things, cited tests that had been renamed or merged, option lists that
still named options which had been removed, and layout diagrams describing a folder tree that had
since been restructured. None were code defects — which is the point of running the campaign after
the consolidation rather than before it.
Verification
.fileassert.yamlentry, and thedocdown --validateself-validation case. Citing tests that donot exist has been this repository's most repeated defect, so this was checked rather than assumed.
build.ps1— 1937 tests, 0 failed, none skipped.lint.ps1— clean.reviewmark --plan— every file covered by a review-set.directions.
Also corrected
Stale claims found while in the area:
DemaConsulting.DocDown.Office.Tests.stopped being true when the self-test probes were committed. They now distinguish the suite's
generated fixtures — which must cover structures one document cannot — from the one committed probe
per backend.
definition.yamlfiles, which were failing the docsbuild with
withBinaryFile: does not exist. Nothing validates those paths either.