Skip to content

Fix duplicate link references and address all reviewmark review-set findings#76

Merged
Malcolmnixon merged 13 commits intomainfrom
copilot/fix-review-sets-issues
Mar 19, 2026
Merged

Fix duplicate link references and address all reviewmark review-set findings#76
Malcolmnixon merged 13 commits intomainfrom
copilot/fix-review-sets-issues

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Mar 17, 2026

Fixes duplicate link references in docs/design/introduction.md, strengthens test assertions using Assert.MatchesRegex, refactors self-validation infrastructure, addresses all findings and observations surfaced by running all 11 .reviewmark.yaml review-sets individually, and removes the redundant docs/design/utilities.md.

Description

  • docs/design/introduction.md: Collapsed duplicate link references ([user-guide] and [requirements-doc]) into a single [sarifmark-repo] reference; updated prose to "Those topics are addressed in other [SarifMark repository][sarifmark-repo] documentation." Removed the utilities-layer bullet and corrected "four" to "three" primary functional layers following the removal of utilities.md.

  • docs/design/utilities.md: Removed — the file was redundant with docs/design/path-helpers.md, which already covers the PathHelpers class in full detail.

  • docs/design/definition.yaml: Removed utilities.md from input-files.

  • .reviewmark.yaml: Added docs/design/program.md and docs/design/context.md to SarifMark-CommandLine-Subsystem-Review paths; added docs/design/sarif-results.md to SarifMark-Report-Subsystem-Review paths; removed docs/design/utilities.md from SarifMark-PathHelpers-SoftwareUnit-Review paths.

  • ProgramTests.cs: Replaced Assert.IsTrue(SemanticVersionRegex().IsMatch(...), ...) with Assert.MatchesRegex(@"^\d+\.\d+\.\d+(?:-[\w.-]+)?(?:\+[\w.-]+)?$", output) in Program_Main_VersionFlag_DisplaysVersionOnly, removing the now-unneeded [GeneratedRegex] method and using System.Text.RegularExpressions. Replaced Assert.Contains("--report", output) with Assert.MatchesRegex(@"--report(?!-)", output) in Program_Main_HelpFlag_DisplaysHelp to prevent a false-positive substring match against --report-depth. Extended Program_Main_HelpFlag_DisplaysHelp to assert all 11 supported flags appear in the help output.

  • IntegrationTests.cs: Replaced vacuous Assert.IsTrue(exitCode is 0 or 1) with Assert.AreEqual(0, exitCode) in IntegrationTest_ValidateFlag_RunsSelfValidation. Replaced Assert.Contains("--report", output) with Assert.MatchesRegex(@"--report(?!-)", output) in IntegrationTest_HelpFlag_DisplaysHelp for the same substring-ambiguity fix.

  • SarifResult.cs / SarifResults.cs: Fixed misleading "Internal constructor for testing purposes" XML doc. Replaced non-idiomatic .Any() + .First() on JsonElement.ArrayEnumerator with direct MoveNext() + Current enumeration to avoid unnecessary allocation.

  • Validation.cs: Refactored RunEnforcementTest to delegate to RunValidationTest (removes ~65 lines of duplicated infrastructure). Extended RunValidationTest with an optional extraArgs parameter and updated validator signature to Func<int, string, string?, string?>.

  • ContextTests.cs: Fixed Context_ExitCode_StartsAtZero_ChangesToOneAfterError to redirect Console.Error instead of the unused Console.Out. Added three distinct log-file tests: Context_WriteLine_WithLogFile_WritesToLog (non-silent case), Context_WriteLine_SilentModeWithLogFile_WritesToLog (silent case), and Context_WriteError_WithLogFile_WritesToLog.

  • ValidationTests.cs: Extended Validation_Run_ValidContext_PrintsValidationHeader to assert all five header fields (OS Version, DotNet Runtime, and Time Stamp, in addition to the existing two).

  • PathHelpersTests.cs: Added ParamName assertions to PathWithDoubleDots, FilenameWithEmbeddedDots, and AbsolutePath tests for consistency with the other argument-exception tests.

  • AssemblyInfo.cs: Added explanatory comment for [assembly: DoNotParallelize] describing why Console redirection requires serial test execution.

  • docs/design/program.md: Fixed missing comma after "absent" in the Version Property section.

  • docs/design/sarif-results.md: Documented the trailing two-space markdown hard line break ( ) appended to each result line in AppendIssuesSection, satisfying requirement SarifMark-Report-LineBreaks.

  • docs/design/validation.md: Added null-guard sentence documenting ArgumentNullException.ThrowIfNull(context) to satisfy SarifMark-Validation-NullCheck. Updated to match the refactored RunValidationTest signature and RunEnforcementTest delegation pattern.

  • docs/reqstream/context.yaml: Added Context_WriteLine_WithLogFile_WritesToLog, Context_WriteLine_SilentModeWithLogFile_WritesToLog, and Context_WriteError_WithLogFile_WritesToLog to the traceability links for SarifMark-Context-WriteLine and SarifMark-Context-WriteError.

  • docs/reqstream/platform.yaml: Changed SarifMark-Plt-Net10 justification from "future .NET versions" to "as adoption grows".

  • docs/reqstream/program.yaml: Added three integration tests to the SarifMark-Program-SarifAnalysis requirement to close the traceability gap.

  • docs/reqstream/sarif-results.yaml: Added SarifResults_ToMarkdown_Depth1_ProducesCorrectOutput as a test link for SarifMark-SarifResults-FormatCount to cover the plural case; added SarifResults_InternalConstructor_CreatesValidInstance to SarifMark-SarifResults-Properties.

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

Related Issues

Pre-Submission Checklist

Before submitting this pull request, ensure you have completed the following:

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

Please run the following checks before submitting:

  • 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

All 11 .reviewmark.yaml review-sets were run individually (no context cross-pollution). All 11 reviews now PASS. Build is zero-warning. All 101 tests pass across .NET 8, 9, and 10.


💡 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.

Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix issues in review sets from .reviewmark.yaml Fix duplicate link references in docs/design/introduction.md Mar 17, 2026
Copilot AI requested a review from Malcolmnixon March 17, 2026 00:35
Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com>
@Malcolmnixon
Copy link
Copy Markdown
Member

@copilot Please use the Code Review agent to run all .reviewmark.yaml review-sets; then fix all findings.

Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Mar 17, 2026

@copilot Please use the Code Review agent to run all .reviewmark.yaml review-sets; then fix all findings.

Done in 7d9e6cb. All 11 review-sets ran; 5 findings were fixed:

  • F1 (ProgramTests.cs): Added Assert.IsFalse(string.IsNullOrWhiteSpace(output)) to Program_Main_VersionFlag_DisplaysVersionOnly so empty output no longer passes.
  • F2 (IntegrationTests.cs): Replaced vacuous Assert.IsTrue(exitCode is 0 or 1) with Assert.AreEqual(0, exitCode).
  • F3 (SarifResult.cs, SarifResults.cs): Fixed internal constructor XML doc from "for testing purposes" to the accurate reason ("to enforce that instances are only created through the validated parsing pipeline").
  • F4+F5 (Validation.cs, docs/design/validation.md): Refactored RunEnforcementTest to delegate to RunValidationTest (removing ~65 lines of duplication). Extended RunValidationTest with an optional extraArgs parameter and updated the validator signature to Func<int, string, string?, string?> (exit code first). Updated validation.md to match.

Copilot AI changed the title Fix duplicate link references in docs/design/introduction.md Fix duplicate link references and address formal review findings Mar 17, 2026
Copilot AI and others added 2 commits March 17, 2026 01:06
Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com>
Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com>
Copilot AI changed the title Fix duplicate link references and address formal review findings Fix duplicate link references in docs/design/introduction.md Mar 17, 2026
@Malcolmnixon Malcolmnixon marked this pull request as ready for review March 17, 2026 01:09
Copilot AI review requested due to automatic review settings March 17, 2026 01:09
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR cleans up documentation link references and tightens self-validation/test infrastructure so that the validate/version behaviors are asserted meaningfully and the validation helper code is de-duplicated.

Changes:

  • Consolidates duplicate reference-style links in docs/design/introduction.md.
  • Strengthens test assertions for --version and --validate behavior in unit/integration tests.
  • Refactors self-validation helper logic in Validation.cs (removes duplicated enforcement test infrastructure) and updates related docs/comments.

Reviewed changes

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

Show a summary per file
File Description
test/DemaConsulting.SarifMark.Tests/ProgramTests.cs Uses a source-generated semver regex to assert --version output is only a version string.
test/DemaConsulting.SarifMark.Tests/IntegrationTests.cs Makes --validate integration test assert a successful (0) exit code.
src/DemaConsulting.SarifMark/Validation.cs Refactors enforcement validation to reuse RunValidationTest; expands helper to accept extra args and pass exit code to validators.
src/DemaConsulting.SarifMark/SarifResults.cs Corrects XML doc to reflect the purpose of the internal constructor.
src/DemaConsulting.SarifMark/SarifResult.cs Corrects XML doc to reflect the purpose of the internal constructor.
docs/design/validation.md Updates design documentation to match the refactored validation helper/validator signature.
docs/design/introduction.md Collapses duplicate link references into a single repository reference.

💡 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
Copy link
Copy Markdown
Member

@copilot Please use the code-review agent to run the .reviewmark.yaml reviews one at a time (so there's no context pollution between reviews) and then gather up all the results.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates SarifMark’s design/requirements documentation and tightens the self-validation + test suite to address findings from running each .reviewmark.yaml review-set independently, while removing redundant design documentation.

Changes:

  • Strengthen unit/integration tests (version output validation, help output flag coverage, exit code assertions, log-file behaviors, argument exception param name checks).
  • Refactor self-validation infrastructure in Validation.cs to reduce duplication and improve failure reporting.
  • Update design + reqstream docs, adjust ReviewMark review-set paths, and remove the redundant docs/design/utilities.md.

Reviewed changes

Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
test/DemaConsulting.SarifMark.Tests/ValidationTests.cs Extends validation header assertions to cover additional fields.
test/DemaConsulting.SarifMark.Tests/ProgramTests.cs Improves version output assertion via source-generated regex; expands help output assertions.
test/DemaConsulting.SarifMark.Tests/PathHelpersTests.cs Adds ParamName assertions for argument exceptions.
test/DemaConsulting.SarifMark.Tests/IntegrationTests.cs Makes validate integration test assert a successful exit code.
test/DemaConsulting.SarifMark.Tests/ContextTests.cs Fixes console redirection target; adds log-file write coverage for WriteLine/WriteError.
test/DemaConsulting.SarifMark.Tests/AssemblyInfo.cs Documents why tests are non-parallel (console redirection).
src/DemaConsulting.SarifMark/Validation.cs Refactors enforcement validation to reuse shared validation helper; enriches validator inputs.
src/DemaConsulting.SarifMark/SarifResults.cs Adjusts SARIF runs enumeration to avoid LINQ over JsonElement enumerators.
src/DemaConsulting.SarifMark/SarifResult.cs Corrects XML doc wording for internal constructor intent.
docs/reqstream/sarif-results.yaml Adds traceability links for additional SarifResults tests.
docs/reqstream/program.yaml Adds integration tests to Program SARIF-analysis requirement links.
docs/reqstream/platform.yaml Updates .NET 10 justification wording.
docs/reqstream/context.yaml Adds traceability links for new log-file-related Context tests.
docs/design/validation.md Updates design notes to reflect null-check and refactored validation helper signature/behavior.
docs/design/utilities.md Removes redundant utilities design doc (covered elsewhere).
docs/design/sarif-results.md Documents required markdown hard line breaks in issue output formatting.
docs/design/program.md Fixes punctuation in version property description.
docs/design/introduction.md Removes duplicate link references; updates layer count after utilities doc removal.
docs/design/definition.yaml Removes deleted utilities.md from design input set.
.reviewmark.yaml Updates review-set paths to include relevant design docs and remove deleted utilities doc.

💡 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.

…eter order

Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com>
@Malcolmnixon
Copy link
Copy Markdown
Member

@copilot Please use the code-review agent to run the .reviewmark.yaml reviews one at a time (so there's no context pollution between reviews) and then gather up all the results - specifically report the actual issues and observations so they can be addressed rather than keeping on popping up. My goal is to try and make the reviews clean.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates SarifMark’s tests and documentation to address ReviewMark findings, tighten CLI output assertions (especially around --report/--report-depth), and reduce duplication in the design docs set.

Changes:

  • Strengthen MSTest assertions (regex for --report, semver output, and additional help/header coverage) and add new Context log-file output tests.
  • Refactor Validation.RunEnforcementTest to delegate to the shared RunValidationTest helper, and enhance validation callbacks to include the program exit code.
  • Remove redundant docs/design/utilities.md, update design/reqstream docs accordingly, and adjust .reviewmark.yaml review-set inputs.

Reviewed changes

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

Show a summary per file
File Description
test/DemaConsulting.SarifMark.Tests/ValidationTests.cs Adds assertions for additional validation header rows (OS/runtime/timestamp).
test/DemaConsulting.SarifMark.Tests/ProgramTests.cs Replaces substring assertions with regex; strengthens version-only output check and expands help option coverage.
test/DemaConsulting.SarifMark.Tests/PathHelpersTests.cs Adds ParamName assertions on thrown ArgumentException for invalid relative paths.
test/DemaConsulting.SarifMark.Tests/IntegrationTests.cs Uses regex for --report to avoid --report-depth false positives; makes validate exit code expectation strict.
test/DemaConsulting.SarifMark.Tests/ContextTests.cs Fixes console stream capture in exit-code test; adds new log-file write coverage for WriteLine/WriteError.
test/DemaConsulting.SarifMark.Tests/AssemblyInfo.cs Disables parallel test execution to avoid Console.Out/Console.Error redirection conflicts.
src/DemaConsulting.SarifMark/Validation.cs Refactors enforcement validation to use shared helper; passes exit code into validators; adds extraArgs support.
src/DemaConsulting.SarifMark/SarifResults.cs Avoids multiple enumeration of runs by using the array enumerator directly.
src/DemaConsulting.SarifMark/SarifResult.cs Updates constructor comment to reflect intent of validated creation pipeline.
docs/reqstream/sarif-results.yaml Adds traceability entries for additional SarifResults tests.
docs/reqstream/program.yaml Adds traceability entries for integration tests covering valid SARIF/report/enforce flows.
docs/reqstream/platform.yaml Edits justification wording for .NET 10 support requirement (formatting/clarity).
docs/reqstream/context.yaml Adds traceability entries for new Context log-file write tests.
docs/design/validation.md Updates design description to match null-check behavior and refactored validation helper signature/flow.
docs/design/utilities.md Removes redundant document (superseded by path-helpers.md).
docs/design/sarif-results.md Clarifies markdown hard line break behavior in generated report output.
docs/design/program.md Minor grammar fix.
docs/design/introduction.md Removes utilities-layer mention and simplifies external doc references.
docs/design/definition.yaml Removes utilities.md from the design document build definition.
.reviewmark.yaml Updates review-set inputs to reflect doc changes and include missing design docs.

💡 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.

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Mar 17, 2026

@copilot Please use the code-review agent to run the .reviewmark.yaml reviews one at a time (so there's no context pollution between reviews) and then gather up all the results - spe...

All 11 review-sets ran one at a time (commit e1eda9e). All 11 PASS — no failures.

Review-set Outcome Observations
SarifMark-CommandLine-Subsystem-Review ✅ PASS OBS-01: broad catch in OpenLogFile intentional; OBS-02: SarifMark_Enforcement test lives in self-validation layer (expected); OBS-03: Program.Run is public static intentionally for in-process testing
SarifMark-Sarif-Subsystem-Review ✅ PASS OBS-01: sarif.md cross-references SarifResult/SarifResults docs (covered by their own review sets); OBS-02: SarifMark-Sarif-Required delegated to CLI layer (correct)
SarifMark-Report-Subsystem-Review ✅ PASS OBS-01: --sarif enforcement is in CLI layer by design; OBS-02: only one integration test for --report flag output, unit coverage comprehensive
SarifMark-Validation-Subsystem-Review ✅ PASS OBS-01: per-method step docs are an acceptable abstraction over the RunValidationTest delegation; OBS-02: only 2 of IntegrationTests.cs tests in scope (expected)
SarifMark-Platform-Subsystem-Review ✅ PASS OBS-01: header comment block defining source-filter prefixes is positive practice; OBS-02: .NET 10 is current/preview, forward-compat intent documented
SarifMark-Program-SoftwareUnit-Review ✅ PASS OBS-01: [assembly: DoNotParallelize] serialises the whole assembly; scoping finer-grained is a future-consideration but not a failure
SarifMark-Context-SoftwareUnit-Review ✅ PASS OBS-01: cross-reference to "Program Class document" in context.md lacks a file path/hyperlink
SarifMark-SarifResult-SoftwareUnit-Review ✅ PASS OBS-01: internal access enforcement is via C# compiler + InternalsVisibleTo (correct mechanism)
SarifMark-SarifResults-SoftwareUnit-Review ✅ PASS OBS-01: sub-heading cap at ###### for depth-6 is correct per min(depth+1,6) — noted for traceability only
SarifMark-Validation-SoftwareUnit-Review ✅ PASS OBS-01: same delegation-abstraction note as Validation Subsystem review
SarifMark-PathHelpers-SoftwareUnit-Review ✅ PASS OBS-01: Contains("..") intentionally rejects filenames like v1..0.sarif; trade-off documented in test XML doc

The only actionable observation across all 11 reviews is OBS-01 in SarifMark-Context-SoftwareUnit-Review: docs/design/context.md cross-references "the Program Class document" without a file path. Everything else is informational.

@Malcolmnixon Malcolmnixon merged commit e19ecbf into main Mar 19, 2026
20 checks passed
@Malcolmnixon Malcolmnixon deleted the copilot/fix-review-sets-issues branch March 19, 2026 15:19
Copilot AI added a commit that referenced this pull request Mar 31, 2026
Agent-Logs-Url: https://github.com/demaconsulting/SarifMark/sessions/8d9bc3e5-e4ae-45e3-b4f6-8f1e5919cf0b

Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com>
Malcolmnixon added a commit that referenced this pull request Mar 31, 2026
…#83)

* Initial plan

* chore: align with TemplateDotNetTool template (PRs #66, #72-#76, #80)

Agent-Logs-Url: https://github.com/demaconsulting/SarifMark/sessions/8d9bc3e5-e4ae-45e3-b4f6-8f1e5919cf0b

Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com>

* Update .github/workflows/build.yaml

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Update .github/agents/code-review.agent.md

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* docs: rename docs/guide to docs/user_guide and guide.md to introduction.md

Agent-Logs-Url: https://github.com/demaconsulting/SarifMark/sessions/2fcc24c8-25e2-4d47-91b3-318e14ffb6d1

Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com>

* chore: remove legacy agents removed from TemplateDotNetTool template

Agent-Logs-Url: https://github.com/demaconsulting/SarifMark/sessions/0162c9bb-1dbc-457e-b5ce-b1b2600688e3

Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com>

* chore: align AGENTS.md structure with TemplateDotNetTool template

Agent-Logs-Url: https://github.com/demaconsulting/SarifMark/sessions/510afeec-654d-468a-9da0-532e2bdfbb21

Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com>

* chore: revert code-review.agent.md step 4 to exact template wording

Agent-Logs-Url: https://github.com/demaconsulting/SarifMark/sessions/3f6495f1-09cd-4ab3-9809-0fba0a28ef78

Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com>
Co-authored-by: Malcolm Nixon <Malcolm.nixon@gmail.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants