Fix all formal review-set findings: REQTEST traceability, colour safety, doc accuracy, PathHelpers test, review-set consolidation

[Copilot]

Addresses all findings from the formal reviewmark review-sets (CLI, SARIF, Validation, Utilities, Integration). The SARIF REQDOC-01 gap and the unsupported-extension test were fixed in earlier commits; this PR completes the remaining issues and consolidates the review-set configuration.

Description

docs/reqstream/validation.yaml — REQTEST failures (Validation-Review)

SarifMark-Val-ResultFiles, SarifMark-Val-TrxFormat, and SarifMark-Val-JUnitFormat all cited IntegrationTest_ValidateFlag_RunsSelfValidation, which never passes --results and therefore never exercises WriteResultsFile. Replaced with the unit tests that actually prove each format:

• SarifMark-Val-ResultFiles → Validation_Run_WithTrxResultsFile_WritesResultsFile + Validation_Run_WithXmlResultsFile_WritesResultsFile
• SarifMark-Val-TrxFormat → Validation_Run_WithTrxResultsFile_WritesResultsFile
• SarifMark-Val-JUnitFormat → Validation_Run_WithXmlResultsFile_WritesResultsFile

src/DemaConsulting.SarifMark/Context.cs — colour safety (CLI O-3)

WriteError changed the console foreground colour without a try/finally, leaving the terminal red if Console.Error.WriteLine threw. Wrapped in try/finally to guarantee restoration.

docs/design/command-line.md — doc accuracy (CLI O-1)

WriteError description said it unconditionally "writes to Console.Error in red"; silent mode suppresses this. Updated to reflect actual behaviour.

test/.../PathHelpersTests.cs — explicit design-intent test (Utilities OBS-01)

Added PathHelpers_SafePathCombine_FilenameWithEmbeddedDots_ThrowsArgumentException to document that the strict Contains("..") check intentionally rejects names like "v1..0.sarif" — making the over-rejection trade-off visible and tested.

docs/design/sarif.md — traceability gap (SARIF OBS-1)

Added explicit SarifMark-Sarif-Processing citation after the Read Method step list to close the implicit-only coverage noted in the review.

.reviewmark.yaml — review-set consolidation

Merged SarifMark-Integration-Review into SarifMark-CLI-Review (renamed to "Review of SarifMark Command Line and Integration Tests"). The integration tests directly test the command-line layer, and the former integration set contained no source files, making it sparse. The merged set now covers 11 files: source (Program.cs, Context.cs), documentation, CLI/platform/OTS requirements, unit tests, integration tests, runner helper, and assembly info.

Formal review results (all four review sets)

All four reviewmark review sets were executed and pass with zero failures:

• SarifMark-CLI-Review — 28 checks, all pass (merged CLI + Integration set)
• SarifMark-SARIF-Review — 22 checks, all pass
• SarifMark-Validation-Review — 26 checks, all pass
• SarifMark-Utilities-Review — 8 applicable checks, all pass (REQ/REQDOC/REQIMP/REQTEST sections N/A — no requirements files in set)

Type of Change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation update
• Code quality improvement

Pre-Submission Checklist

Build and Test

• Code builds successfully: dotnet build --configuration Release
• All unit tests pass: dotnet test --configuration Release
• Self-validation tests pass:
  dotnet run --project src/DemaConsulting.SarifMark --configuration Release --framework net10.0
--no-build -- --validate
• Code produces zero warnings

Code Quality

• Code formatting is correct: dotnet format --verify-no-changes
• New code has appropriate XML documentation comments
• Static analyzer warnings have been addressed

Quality Checks

• Spell checker passes: cspell "**/*.{md,cs}"
• Markdown linter passes: markdownlint "**/*.md"
• YAML linter passes: yamllint .
• Self-validation passes:
  dotnet run --project src/DemaConsulting.SarifMark --configuration Release --framework net10.0 --no-build -- --validate

Testing

• Added unit tests for new functionality
• Updated existing tests if behavior changed
• All tests follow the AAA (Arrange, Act, Assert) pattern
• Test coverage is maintained or improved

Documentation

• Updated README.md (if applicable)
• Updated ARCHITECTURE.md (if applicable)
• Added code examples for new features (if applicable)
• Updated requirements.yaml (if applicable)

Additional Notes

95/95 tests pass across net8.0 / net9.0 / net10.0. CodeQL: 0 alerts. All four formal reviewmark review sets pass with zero failures (CLI: 28 checks, SARIF: 22 checks, Validation: 26 checks, Utilities: 8 applicable checks).

---

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

[Malcolmnixon]

@copilot Could you investigate merging the SarifMark-Integration-Review files into the SarifMark-CLI-Review. Integration testing really tests the command-line; but more than that there are no source files in the SarifMark-Integration-Review set, so the review is a little sparse.

If it's a good idea then please merge and then use the code-review agent to execute the updated SarifMark-CLI-Review

[Copilot]

@copilot Could you investigate merging the SarifMark-Integration-Review files into the SarifMark-CLI-Review. Integration testing really tests the command-line; but more than that the...

Done in commit 145082a. The SarifMark-Integration-Review set has been removed and its files merged into SarifMark-CLI-Review (title updated to "Review of SarifMark Command Line and Integration Tests"). The merged set now has 11 files covering source, docs, unit tests, integration tests, and the platform/OTS requirement files.

Ran the formal code-review-agent on the merged set — all 22 applicable checks (REQ, DOC, CODE, TEST, REQDOC, REQIMP, REQTEST, CODEDOC) pass.

[Copilot]

Pull request overview

This PR addresses findings from formal reviewmark review-sets by fixing REQTEST traceability (pointing requirements at the correct unit tests), adding colour safety to WriteError, improving documentation accuracy, adding an explicit design-intent test for PathHelpers, and consolidating review-set configuration.

Changes:

• Replaced incorrect test citations in validation.yaml, added try/finally for console colour restoration in Context.WriteError, and updated command-line.md to reflect silent-mode behaviour.
• Changed Program.Main from private to internal to allow direct invocation from tests (removing reflection-based InvokeMain helper), and added tests for unsupported results file extensions and embedded-dot filenames.
• Merged SarifMark-Integration-Review into SarifMark-CLI-Review in .reviewmark.yaml and added a SarifMark-Sarif-Processing citation in sarif.md.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
src/DemaConsulting.SarifMark/Context.cs	Wrapped console colour change in try/finally for safety
src/DemaConsulting.SarifMark/Program.cs	Changed Main from private to internal for testability
test/.../ProgramTests.cs	Replaced reflection-based InvokeMain with direct Program.Main calls
test/.../ValidationTests.cs	Added test for unsupported results file extension
test/.../PathHelpersTests.cs	Added design-intent test for embedded-dots rejection
docs/design/command-line.md	Corrected WriteError description to note silent-mode suppression
docs/design/sarif.md	Added SarifMark-Sarif-Processing citation and CLI integration section
docs/reqstream/validation.yaml	Fixed REQTEST traceability to reference correct unit tests
.reviewmark.yaml	Consolidated integration review-set into CLI review-set

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

[Malcolmnixon]

@copilot Please run the code-review agent to review all code-review sets one after the other and show the results. The revies to run are [SarifMark-CLI-Review, SarifMark-SARIF-Review, SarifMark-Validation-Review, SarifMark-Utilities-Review]

[Copilot]

@copilot Please run the code-review agent to review all code-review sets one after the other and show the results. The revies to run are [SarifMark-CLI-Review, SarifMark-SARIF-Review, SarifMark-Valida...

All four review sets have been executed. Results summary:

---

SarifMark-CLI-Review — ✅ Pass (28 checks)

All REQ, DOC, CODE, TEST, REQDOC, REQIMP, REQTEST, CODEDOC checks pass across 11 files (source, docs, unit tests, integration tests, platform/OTS requirements). Three non-blocking observations:

• OBS-01 (TEST-02): ContextTests has no tests for string-valued arguments missing a following value — low risk, same helper covered by --report-depth / --heading tests.
• OBS-02 (TEST-02): Context_ExitCode_* test redirects Console.Out but WriteError writes to Console.Error — redundant setup, no correctness impact.
• OBS-03 (TEST-01): Comment placement inside Context_Create_LogFile_* is cosmetically misleading.

---

SarifMark-SARIF-Review — ✅ Pass (22 checks)

All checks pass across sarif.yaml, report.yaml, sarif.md, SarifResult.cs, SarifResults.cs, and SarifResultsTests.cs. Two non-blocking observations:

• OBS-1 (CODE-04): ArrayEnumerator struct copy pattern in ValidateSarifStructure is subtle but correct.
• OBS-2 (TEST-01): SarifResults_InternalConstructor_* is scaffolding rather than behavioural — acceptable.

---

SarifMark-Validation-Review — ✅ Pass (26 checks)

All checks pass. Three non-blocking observations:

• OBS-01 (CODE-03): Redundant null-guard inside WriteResultsFile — harmless defensive programming.
• OBS-02 (REQTEST-01): SarifMark-Enf-* requirements also reference integration tests outside this review set — confirmed to exist.
• OBS-03 (TEST-02): Assert.IsFalse(File.Exists(jsonFile)) in the unsupported-extension test is trivially true since the file was never created — harmless redundancy.

---

SarifMark-Utilities-Review — ✅ Pass (8 applicable checks, 12 N/A — no requirements files in set)

All DOC, CODE, TEST, and CODEDOC checks pass. One non-blocking observation:

• OBS-01 (DOC-01/CODEDOC-02): utilities.md correctly states the ".." substring check but doesn't explicitly note the false-positive trade-off for names like v1..0.sarif — the test file documents this; the design doc could optionally be updated for full traceability.

---

Overall: all four review sets pass with zero failures. The OBS items are documented in the individual report files (AGENT_REPORT_review-*.md, excluded from git).