From 95cf5b55867e03d261fab73c8e26f92ecf982405 Mon Sep 17 00:00:00 2001 From: Malcolm Nixon Date: Tue, 23 Jun 2026 23:04:22 -0400 Subject: [PATCH 1/4] Constrain review-set paths to needs-review file set Review set 'paths' file resolution is now intersected with the needs-review file set. This prevents build artifacts (bin/, obj/) from being included in review sets when broad glob patterns are used, which was causing fingerprints to change on every build and reviews to go spuriously stale. - ReviewSet.GetFiles/GetFingerprint accept an optional constraint set - ReviewMarkConfiguration passes the needs-review set as the constraint in PublishReviewPlan, PublishReviewReport, and ElaborateReviewSet - When needs-review is absent/empty, no constraint is applied - context patterns remain unconstrained (reference material, not files under review) New requirements, design, verification, and user guide documentation added to cover the constraint behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../review-mark-configuration.md | 53 +++-- .../review-mark-configuration.yaml | 44 +++++ docs/user_guide/introduction.md | 17 ++ .../review-mark-configuration.md | 42 ++++ .../Configuration/ReviewMarkConfiguration.cs | 74 +++++-- .../ReviewMarkConfigurationTests.cs | 181 ++++++++++++++++++ 6 files changed, 384 insertions(+), 27 deletions(-) diff --git a/docs/design/review-mark/configuration/review-mark-configuration.md b/docs/design/review-mark/configuration/review-mark-configuration.md index b9553c0..775e870 100644 --- a/docs/design/review-mark/configuration/review-mark-configuration.md +++ b/docs/design/review-mark/configuration/review-mark-configuration.md @@ -13,6 +13,7 @@ compliance documents. | Property | Type | Description | | -------- | ---- | ----------- | +| `NeedsReviewPatterns` | `IReadOnlyList` | Ordered list of glob patterns identifying files that need review (empty when omitted from YAML) | | `EvidenceSource` | `EvidenceSource` | Parsed evidence-source block (`Type`, `Location`, optional credential env-var names) | | `Reviews` | `IReadOnlyList` | Ordered list of review-set definitions | | `GlobalContext` | `IReadOnlyList` | Ordered list of glob patterns identifying global context files (empty when omitted from YAML) | @@ -39,7 +40,7 @@ compliance documents. | Type | Description | | ---- | ----------- | | `EvidenceSource` | Immutable record: `Type`, `Location`, `UsernameEnv`, `PasswordEnv` | -| `ReviewSet` | Class with `Id`, `Title`, `Paths`, `Context`, `GetFingerprint(dir)`, `GetFiles(dir)` | +| `ReviewSet` | Class with `Id`, `Title`, `Paths`, `Context`, `GetFingerprint(dir, constraint?)`, `GetFiles(dir, constraint?)` | | `LintSeverity` | Enum: `Warning`, `Error` | | `LintIssue` | Record: `Location`, `Severity`, `Description`; `ToString()` formats as `{location}: {severity}: {description}` | | `ReviewMarkLoadResult` | Record: `Configuration` (null if error-level issues found), `Issues` | @@ -80,25 +81,53 @@ from YAML strings without touching the file system. **`ReviewMarkConfiguration.PublishReviewPlan(string dir, int depth = 1)`** → `ReviewPlanResult` -Resolves `needs-review` patterns via `GlobMatcher.GetMatchingFiles()` and, for each file, -identifies which review-sets provide coverage. Returns the Markdown document and a boolean -indicating whether any files lack coverage. The `depth` parameter controls heading level. +Computes the needs-review file set once via `GlobMatcher.GetMatchingFiles()` and converts it to a +`HashSet` constraint when `NeedsReviewPatterns` is non-empty (null constraint when empty, for +backward compatibility). For each review set, resolves its file list by calling `ReviewSet.GetFiles(dir, +constraint)` and its fingerprint by calling `ReviewSet.GetFingerprint(dir, constraint)` with the same +constraint. The constraint limits each review set to files in the needs-review set, so build artifacts +excluded from needs-review are also excluded from coverage and fingerprints. Returns the Markdown document +and a boolean indicating whether any files lack coverage. The `depth` parameter controls heading level. **`ReviewMarkConfiguration.PublishReviewReport(ReviewIndex index, string dir, int depth = 1)`** → `ReviewReportResult` -For each review-set: resolves its file list via `GlobMatcher`, computes the SHA-256 -fingerprint, calls `index.GetEvidence(id, fingerprint)` to determine status (Current, -Stale, Missing, or Failed), and generates the report table. Returns the Markdown document -and a boolean indicating whether any review-set is non-current. +Computes the needs-review constraint (same logic as `PublishReviewPlan`) once and reuses it for all +review sets. For each review-set, calls `ReviewSet.GetFingerprint(dir, constraint)` so that build +artifacts excluded from needs-review do not affect the hash. Calls `index.GetEvidence(id, fingerprint)` +to determine status (Current, Stale, Missing, or Failed), and generates the report table. Returns the +Markdown document and a boolean indicating whether any review-set is non-current. **`ReviewMarkConfiguration.ElaborateReviewSet(string id, string dir, int markdownDepth = 1)`** → `ElaborateResult` -Looks up the review-set with the given `id`, resolves its file list and fingerprint, and -returns a Markdown document with a heading at `markdownDepth`, a metadata table (ID, Title, -Fingerprint), an optional Context subsection, and a Files subheading. The Context subsection -lists all resolved context files as plain paths, and is omitted when no context files resolve. +Looks up the review-set with the given `id`. Computes the needs-review constraint (same logic as +`PublishReviewPlan`). Calls `ReviewSet.GetFingerprint(dir, constraint)` and `ReviewSet.GetFiles(dir, +constraint)` with the constraint so that build artifacts excluded from needs-review are also excluded +from the elaborated file list and fingerprint. Returns a Markdown document with a heading at +`markdownDepth`, a metadata table (ID, Title, Fingerprint), an optional Context subsection, and a +Files subheading. The Context subsection lists all resolved context files as plain paths, and is +omitted when no context files resolve. + +**Needs-review constraint:** `PublishReviewPlan`, `PublishReviewReport`, and `ElaborateReviewSet` +each compute a needs-review constraint before iterating review sets. When `NeedsReviewPatterns` +is non-empty, the constraint is a `HashSet` of all files matched by the top-level +`needs-review` patterns. When `NeedsReviewPatterns` is empty (the key is absent or the list +contains only whitespace entries), the constraint is `null` and no filtering is applied — this +preserves backward compatibility for configurations written before `needs-review` was introduced. + +**`ReviewSet.GetFiles(string dir, IReadOnlySet? constraint = null)`** +→ `IReadOnlyList` (sorted, relative paths) + +Calls `GlobMatcher.GetMatchingFiles()` with the review set's ordered `Paths` patterns. When +`constraint` is non-null, filters the result to include only files whose relative paths appear +in the constraint set. When `constraint` is null, returns all glob-matched files without filtering. +Context patterns are never passed here; context is not under review. + +**`ReviewSet.GetFingerprint(string dir, IReadOnlySet? constraint = null)`** → `string` + +Delegates to `GetFiles(dir, constraint)` to obtain the constrained file list, then applies the +fingerprinting algorithm below. **Combined ordered context resolution:** Global and per-review-set context patterns are concatenated into a single ordered pattern list (`GlobalContext` first, then `review.Context`) diff --git a/docs/reqstream/review-mark/configuration/review-mark-configuration.yaml b/docs/reqstream/review-mark/configuration/review-mark-configuration.yaml index 44f391b..6d78587 100644 --- a/docs/reqstream/review-mark/configuration/review-mark-configuration.yaml +++ b/docs/reqstream/review-mark/configuration/review-mark-configuration.yaml @@ -273,3 +273,47 @@ sections: tests: - ReviewMarkConfiguration_ElaborateReviewSet_PerSetContextExcludesGlobalFile - ReviewMarkConfiguration_ElaborateReviewSet_ExclusionDoesNotAffectOtherReviewSets + + - id: ReviewMark-Config-PathsConstrainedToNeedsReview + title: >- + Review set paths resolution shall be constrained to the needs-review file set + when needs-review patterns are present, ensuring review set files are a strict + subset of files declared as needing review. + justification: >- + The needs-review list is the canonical declaration of which files require review. + A review set's paths: list is semantically a partition of that set. Constraining + paths resolution to needs-review ensures that build artifacts, generated files, and + other excluded paths cannot enter a review set even when a broad glob pattern is + used. The same constraint applies to fingerprint computation, so that adding a + build artifact to a directory does not change the fingerprint and does not + invalidate a valid review. + tests: + - ReviewSet_GetFiles_WithConstraint_ExcludesFilesOutsideConstraint + - ReviewSet_GetFingerprint_WithConstraint_ExcludesBuildArtifacts + - ReviewMarkConfiguration_PublishReviewPlan_ReviewSetExcludesBuildArtifacts + - ReviewMarkConfiguration_PublishReviewReport_FingerprintConsistentWithPlan + + - id: ReviewMark-Config-GetFilesUnconstrainedWhenNoConstraint + title: >- + Review set GetFiles shall return all glob-matched files without filtering when + no constraint is provided, preserving backward compatibility. + justification: >- + When called without a constraint (null), GetFiles returns all files matched by the + review set's glob patterns regardless of needs-review. This preserves the + pre-constraint behavior and ensures that callers that do not supply a constraint + receive the complete unconstrained file list. + tests: + - ReviewSet_GetFiles_WithoutConstraint_ReturnsAllMatchedFiles + + - id: ReviewMark-Config-NeedsReviewAbsentUnconstrained + title: >- + When the needs-review patterns list is absent or empty, the tool shall apply no + constraint to review set paths resolution, preserving backward compatibility for + configurations that omit needs-review. + justification: >- + Configurations written before needs-review was introduced have no + needs-review block. Treating an absent or empty needs-review as "no constraint" + ensures those configurations continue to work without modification. Only a + non-empty needs-review list produces a constraint. + tests: + - ReviewSet_GetFiles_WithoutConstraint_ReturnsAllMatchedFiles diff --git a/docs/user_guide/introduction.md b/docs/user_guide/introduction.md index e6b2e44..0e1ca5d 100644 --- a/docs/user_guide/introduction.md +++ b/docs/user_guide/introduction.md @@ -362,6 +362,23 @@ When a reviewer creates evidence, they record the current fingerprint in the PDF ReviewMark matches that recorded fingerprint against the current fingerprint to determine whether the review is still current. +#### Constraint: paths are a subset of needs-review + +When `needs-review` patterns are present, ReviewMark intersects each review set's `paths:` results +with the `needs-review` file set before computing the file count and fingerprint. This means: + +- A file that matches a `paths:` pattern but is excluded from `needs-review` (for example a file + under `obj/` or `bin/`) does **not** appear in the review set and does **not** affect the + fingerprint. +- You do not need to repeat every `!**/obj/**` exclusion in every review set — the `needs-review` + exclusions apply globally. +- Context files are **not** constrained by `needs-review`; they are reference material and are + never subject to coverage or fingerprint computation. + +When `needs-review` is absent or empty, no constraint is applied and all glob-matched files are +included, preserving backward compatibility for configurations written before `needs-review` was +introduced. + ### Design Guidance Good review sets share these properties: diff --git a/docs/verification/review-mark/configuration/review-mark-configuration.md b/docs/verification/review-mark/configuration/review-mark-configuration.md index 559735c..5e04937 100644 --- a/docs/verification/review-mark/configuration/review-mark-configuration.md +++ b/docs/verification/review-mark/configuration/review-mark-configuration.md @@ -200,6 +200,41 @@ entry is dropped. Boundary or error path: individual whitespace paths entry in o list. Requirement coverage: `ReviewMark-Config-Loading`. This scenario is tested by `ReviewMarkConfiguration_Load_WhitespaceOnlyPathsEntry_ReturnsLintWarning`. +**ReviewSet_GetFiles_WithConstraint_ExcludesFilesOutsideConstraint**: +`ReviewSet.GetFiles` is called with a constraint set containing only the source file, while the +directory also contains a generated file in `src/bin/`. Expected outcome: only the file in the +constraint set is returned; the generated file is excluded. Requirement coverage: +`ReviewMark-Config-PathsConstrainedToNeedsReview`. This scenario is tested by +`ReviewSet_GetFiles_WithConstraint_ExcludesFilesOutsideConstraint`. + +**ReviewSet_GetFiles_WithoutConstraint_ReturnsAllMatchedFiles**: +`ReviewSet.GetFiles` is called with no constraint while the directory contains both a source file +and a generated file. Expected outcome: both files are returned. Requirement coverage: +`ReviewMark-Config-GetFilesUnconstrainedWhenNoConstraint`, +`ReviewMark-Config-NeedsReviewAbsentUnconstrained`. This scenario is tested by +`ReviewSet_GetFiles_WithoutConstraint_ReturnsAllMatchedFiles`. + +**ReviewSet_GetFingerprint_WithConstraint_ExcludesBuildArtifacts**: +`ReviewSet.GetFingerprint` is called with a constraint on two directories — one clean (source +only), one built (source plus a generated file in `bin/`). The constraint mirrors `needs-review` +by excluding `bin/`. Expected outcome: both fingerprints are equal because the generated file is +excluded by the constraint. Requirement coverage: `ReviewMark-Config-PathsConstrainedToNeedsReview`. +This scenario is tested by `ReviewSet_GetFingerprint_WithConstraint_ExcludesBuildArtifacts`. + +**ReviewMarkConfiguration_PublishReviewPlan_ReviewSetExcludesBuildArtifacts**: +`PublishReviewPlan` is called on a configuration whose `needs-review` list excludes `bin/`, while +a broad review set pattern matches both the source file and the generated file. Expected outcome: +the review set reports one file and `HasIssues` is false (the single needs-review file is covered). +Requirement coverage: `ReviewMark-Config-PathsConstrainedToNeedsReview`. This scenario is tested +by `ReviewMarkConfiguration_PublishReviewPlan_ReviewSetExcludesBuildArtifacts`. + +**ReviewMarkConfiguration_PublishReviewReport_FingerprintConsistentWithPlan**: +`PublishReviewReport` is called after computing the expected fingerprint via the same +`needs-review` constraint. Expected outcome: `HasIssues` is false and the review is shown as +Current, proving that the fingerprint used in the report is consistent with the one used in the +plan. Requirement coverage: `ReviewMark-Config-PathsConstrainedToNeedsReview`. This scenario is +tested by `ReviewMarkConfiguration_PublishReviewReport_FingerprintConsistentWithPlan`. + #### Requirements Coverage - **ReviewMark-Config-Reading**: @@ -265,3 +300,10 @@ list. Requirement coverage: `ReviewMark-Config-Loading`. This scenario is tested - **ReviewMark-Config-ContextExclusionPatterns**: ReviewMarkConfiguration_ElaborateReviewSet_PerSetContextExcludesGlobalFile, ReviewMarkConfiguration_ElaborateReviewSet_ExclusionDoesNotAffectOtherReviewSets +- **ReviewMark-Config-PathsConstrainedToNeedsReview**: + ReviewSet_GetFiles_WithConstraint_ExcludesFilesOutsideConstraint, + ReviewSet_GetFingerprint_WithConstraint_ExcludesBuildArtifacts, + ReviewMarkConfiguration_PublishReviewPlan_ReviewSetExcludesBuildArtifacts, + ReviewMarkConfiguration_PublishReviewReport_FingerprintConsistentWithPlan +- **ReviewMark-Config-GetFilesUnconstrainedWhenNoConstraint**: ReviewSet_GetFiles_WithoutConstraint_ReturnsAllMatchedFiles +- **ReviewMark-Config-NeedsReviewAbsentUnconstrained**: ReviewSet_GetFiles_WithoutConstraint_ReturnsAllMatchedFiles diff --git a/src/DemaConsulting.ReviewMark/Configuration/ReviewMarkConfiguration.cs b/src/DemaConsulting.ReviewMark/Configuration/ReviewMarkConfiguration.cs index f5251a8..3ee226a 100644 --- a/src/DemaConsulting.ReviewMark/Configuration/ReviewMarkConfiguration.cs +++ b/src/DemaConsulting.ReviewMark/Configuration/ReviewMarkConfiguration.cs @@ -536,18 +536,40 @@ internal ReviewSet(string id, string title, IReadOnlyList paths, IReadOn /// /// Returns all files matched by this review set's glob patterns within - /// . + /// , optionally constrained to a set of allowed paths. /// /// The root directory to search. + /// + /// When non-null, only files whose relative paths appear in this set are + /// returned. Pass the resolved needs-review file set to enforce that review-set + /// files are a strict subset of files declared as needing review (excluding build + /// artifacts and other paths that are excluded from needs-review). + /// When null, all glob-matched files are returned without filtering. + /// /// A sorted list of relative file paths. - internal IReadOnlyList GetFiles(string directory) => - GlobMatcher.GetMatchingFiles(directory, Paths); + internal IReadOnlyList GetFiles(string directory, IReadOnlySet? constraint = null) + { + var files = GlobMatcher.GetMatchingFiles(directory, Paths); + if (constraint == null) + { + return files; + } + + return files.Where(f => constraint.Contains(f)).ToList(); + } /// /// Computes a content-based fingerprint for this review set's matched files /// within . /// /// The root directory to search. + /// + /// When non-null, only files whose relative paths appear in this set + /// contribute to the fingerprint. Pass the resolved needs-review file set so + /// that build artifacts excluded from needs-review do not affect the + /// fingerprint even when the review-set paths are broad. + /// When null, all glob-matched files contribute to the fingerprint. + /// /// /// A lowercase hex SHA-256 string that is stable across file renames but /// changes when any file's content changes. @@ -563,10 +585,11 @@ internal IReadOnlyList GetFiles(string directory) => /// Return as lowercase hex string. /// /// - internal string GetFingerprint(string directory) + internal string GetFingerprint(string directory, IReadOnlySet? constraint = null) { - // Resolve all matching files for this review set - var files = GetFiles(directory); + // Resolve all matching files for this review set, constrained to the needs-review + // set when provided so that build artifacts are not included in the fingerprint. + var files = GetFiles(directory, constraint); // Compute a SHA-256 hash of each file's content and collect as hex strings var contentHashes = files @@ -873,6 +896,14 @@ internal ReviewPlanResult PublishReviewPlan(string directory, int markdownDepth ArgumentOutOfRangeException.ThrowIfNegativeOrZero(markdownDepth); ArgumentOutOfRangeException.ThrowIfGreaterThan(markdownDepth, 5); + // Compute needs-review files once — reused both as the constraint for review-set + // file resolution and for the coverage gap analysis below. When no needs-review + // patterns are defined, no constraint is applied (null means unconstrained). + var needsReviewFiles = GetNeedsReviewFiles(directory); + var needsReviewConstraint = NeedsReviewPatterns.Count > 0 + ? new HashSet(needsReviewFiles, StringComparer.Ordinal) + : null; + // Build the section heading at the requested depth var sb = new StringBuilder(); var heading = new string('#', markdownDepth); @@ -887,9 +918,10 @@ internal ReviewPlanResult PublishReviewPlan(string directory, int markdownDepth var coveredFiles = new HashSet(StringComparer.Ordinal); foreach (var review in Reviews) { - // Resolve matched files and compute the fingerprint for this review set - var files = review.GetFiles(directory); - var fingerprint = review.GetFingerprint(directory); + // Resolve matched files and compute the fingerprint for this review set, + // constrained to the needs-review set so build artifacts are excluded. + var files = review.GetFiles(directory, needsReviewConstraint); + var fingerprint = review.GetFingerprint(directory, needsReviewConstraint); // Abbreviate the fingerprint to first 8 characters followed by an ellipsis var abbreviatedFingerprint = $"`{fingerprint[..8]}\u2026`"; @@ -907,7 +939,6 @@ internal ReviewPlanResult PublishReviewPlan(string directory, int markdownDepth sb.AppendLine(); // Identify files that require review but are not covered by any review set - var needsReviewFiles = GetNeedsReviewFiles(directory); var uncoveredFiles = needsReviewFiles .Where(f => !coveredFiles.Contains(f)) .ToList(); @@ -966,6 +997,12 @@ internal ReviewReportResult PublishReviewReport(ReviewIndex index, string direct ArgumentOutOfRangeException.ThrowIfNegativeOrZero(markdownDepth); ArgumentOutOfRangeException.ThrowIfGreaterThan(markdownDepth, 5); + // Compute the needs-review constraint once so that all fingerprints are computed + // against the same constrained file set, keeping build artifacts out of the hashes. + var needsReviewConstraint = NeedsReviewPatterns.Count > 0 + ? new HashSet(GetNeedsReviewFiles(directory), StringComparer.Ordinal) + : null; + // Build the section heading at the requested depth var sb = new StringBuilder(); var heading = new string('#', markdownDepth); @@ -984,8 +1021,9 @@ internal ReviewReportResult PublishReviewReport(ReviewIndex index, string direct foreach (var review in Reviews) { - // Compute the current content fingerprint for this review set - var fingerprint = review.GetFingerprint(directory); + // Compute the current content fingerprint, constrained to the needs-review set + // so that build artifacts excluded from needs-review do not affect the fingerprint. + var fingerprint = review.GetFingerprint(directory, needsReviewConstraint); // Check if there is evidence with a matching fingerprint for this review set var currentEvidence = index.GetEvidence(review.Id, fingerprint); @@ -1087,14 +1125,20 @@ internal ElaborateResult ElaborateReviewSet(string reviewSetId, string directory throw new ArgumentException($"No review set found with ID '{reviewSetId}'."); } + // Compute the needs-review constraint so that build artifacts excluded from + // needs-review are also excluded from the elaborated file list and fingerprint. + var needsReviewConstraint = NeedsReviewPatterns.Count > 0 + ? new HashSet(GetNeedsReviewFiles(directory), StringComparer.Ordinal) + : null; + // Build the section heading at the requested depth var sb = new StringBuilder(); var heading = new string('#', markdownDepth); sb.AppendLine($"{heading} {review.Id}"); sb.AppendLine(); - // Emit the review metadata table - var fingerprint = review.GetFingerprint(directory); + // Emit the review metadata table, constrained to the needs-review set + var fingerprint = review.GetFingerprint(directory, needsReviewConstraint); sb.AppendLine("| Field | Value |"); sb.AppendLine("| :--- | :--- |"); sb.AppendLine($"| ID | {review.Id} |"); @@ -1103,7 +1147,7 @@ internal ElaborateResult ElaborateReviewSet(string reviewSetId, string directory sb.AppendLine(); // Resolve the files-under-review first so we can suppress them from context - var files = review.GetFiles(directory); + var files = review.GetFiles(directory, needsReviewConstraint); var filesUnderReview = new HashSet(files, StringComparer.Ordinal); // Resolve context as a single ordered pattern list: global patterns first, then diff --git a/test/DemaConsulting.ReviewMark.Tests/Configuration/ReviewMarkConfigurationTests.cs b/test/DemaConsulting.ReviewMark.Tests/Configuration/ReviewMarkConfigurationTests.cs index 8ce4cec..07ee37c 100644 --- a/test/DemaConsulting.ReviewMark.Tests/Configuration/ReviewMarkConfigurationTests.cs +++ b/test/DemaConsulting.ReviewMark.Tests/Configuration/ReviewMarkConfigurationTests.cs @@ -1165,6 +1165,187 @@ public void ReviewSet_GetFingerprint_ContextNotIncluded() Assert.Equal(fp1, fp2); } + /// + /// Test that GetFiles with a constraint excludes files outside the constraint set. + /// + [Fact] + public void ReviewSet_GetFiles_WithConstraint_ExcludesFilesOutsideConstraint() + { + // Arrange — source file in src/ and a generated file in src/bin/; only src/ file is in needs-review + var srcDir = PathHelpers.SafePathCombine(_testDirectory, "src"); + var binDir = PathHelpers.SafePathCombine(srcDir, "bin"); + Directory.CreateDirectory(srcDir); + Directory.CreateDirectory(binDir); + File.WriteAllText(PathHelpers.SafePathCombine(srcDir, "A.cs"), "class A {}"); + File.WriteAllText(PathHelpers.SafePathCombine(binDir, "Generated.cs"), "// generated"); + + var reviewSet = new ReviewSet("Test", "Test Review", ["src/**/*.cs"], []); + var constraint = new HashSet(["src/A.cs"], StringComparer.Ordinal); + + // Act + var files = reviewSet.GetFiles(_testDirectory, constraint); + + // Assert — only the file in the constraint set is returned; the generated file is excluded + Assert.Single(files); + Assert.Contains("src/A.cs", files); + Assert.DoesNotContain("src/bin/Generated.cs", files); + } + + /// + /// Test that GetFiles without a constraint returns all matched files including those outside needs-review. + /// + [Fact] + public void ReviewSet_GetFiles_WithoutConstraint_ReturnsAllMatchedFiles() + { + // Arrange — source file in src/ and a generated file in src/bin/ + var srcDir = PathHelpers.SafePathCombine(_testDirectory, "src"); + var binDir = PathHelpers.SafePathCombine(srcDir, "bin"); + Directory.CreateDirectory(srcDir); + Directory.CreateDirectory(binDir); + File.WriteAllText(PathHelpers.SafePathCombine(srcDir, "A.cs"), "class A {}"); + File.WriteAllText(PathHelpers.SafePathCombine(binDir, "Generated.cs"), "// generated"); + + var reviewSet = new ReviewSet("Test", "Test Review", ["src/**/*.cs"], []); + + // Act + var files = reviewSet.GetFiles(_testDirectory); + + // Assert — both files are returned when no constraint is applied + Assert.Equal(2, files.Count); + Assert.Contains("src/A.cs", files); + Assert.Contains("src/bin/Generated.cs", files); + } + + /// + /// Test that GetFingerprint with a constraint excludes files outside the constraint + /// from the hash so that build artifacts do not affect the fingerprint. + /// + [Fact] + public void ReviewSet_GetFingerprint_WithConstraint_ExcludesBuildArtifacts() + { + // Arrange — two directories: one with source only, one with source + generated file + var dirClean = PathHelpers.SafePathCombine(_testDirectory, "clean"); + var dirBuilt = PathHelpers.SafePathCombine(_testDirectory, "built"); + var dirCleanSrc = PathHelpers.SafePathCombine(dirClean, "src"); + var dirBuiltSrc = PathHelpers.SafePathCombine(dirBuilt, "src"); + var binDir = PathHelpers.SafePathCombine(dirBuiltSrc, "bin"); + Directory.CreateDirectory(dirCleanSrc); + Directory.CreateDirectory(binDir); + File.WriteAllText(PathHelpers.SafePathCombine(dirCleanSrc, "A.cs"), "class A {}"); + File.WriteAllText(PathHelpers.SafePathCombine(dirBuiltSrc, "A.cs"), "class A {}"); + File.WriteAllText(PathHelpers.SafePathCombine(binDir, "Generated.cs"), "// generated"); + + var reviewSet = new ReviewSet("Test", "Test Review", ["src/**/*.cs"], []); + + // The constraint mirrors needs-review: all *.cs files except bin/ + var constraint = new HashSet(["src/A.cs"], StringComparer.Ordinal); + + // Act + var fpClean = reviewSet.GetFingerprint(dirClean, constraint); + var fpBuilt = reviewSet.GetFingerprint(dirBuilt, constraint); + + // Assert — the generated file is excluded by the constraint; fingerprints are equal + Assert.Equal(fpClean, fpBuilt); + } + + /// + /// Test that PublishReviewPlan excludes build-artifact files from review set coverage + /// when needs-review patterns exclude build output directories. + /// + [Fact] + public void ReviewMarkConfiguration_PublishReviewPlan_ReviewSetExcludesBuildArtifacts() + { + // Arrange — source file in src/ and a generated file in src/bin/ + var srcDir = PathHelpers.SafePathCombine(_testDirectory, "src"); + var binDir = PathHelpers.SafePathCombine(srcDir, "bin"); + Directory.CreateDirectory(srcDir); + Directory.CreateDirectory(binDir); + File.WriteAllText(PathHelpers.SafePathCombine(srcDir, "A.cs"), "class A {}"); + File.WriteAllText(PathHelpers.SafePathCombine(binDir, "Generated.cs"), "// generated"); + + // needs-review includes *.cs but excludes bin/; review set uses a broad pattern + var yaml = """ + needs-review: + - "**/*.cs" + - "!**/bin/**" + evidence-source: + type: none + reviews: + - id: Core-Logic + title: Review of core business logic + paths: + - "src/**/*.cs" + """; + var config = ReviewMarkConfiguration.Parse(yaml); + + // Act + var result = config.PublishReviewPlan(_testDirectory); + + // Assert — review set reports 1 file (not 2), and the generated file is not listed as covered + Assert.Contains("| Core-Logic |", result.Markdown); + Assert.Contains("| Core-Logic | Review of core business logic | 1 |", result.Markdown); + Assert.False(result.HasIssues, "The one needs-review file should be covered"); + } + + /// + /// Test that PublishReviewReport computes a fingerprint consistent with PublishReviewPlan + /// when needs-review excludes build output directories. + /// + [Fact] + public void ReviewMarkConfiguration_PublishReviewReport_FingerprintConsistentWithPlan() + { + // Arrange — source file in src/ and a generated file in src/bin/ + var srcDir = PathHelpers.SafePathCombine(_testDirectory, "src"); + var binDir = PathHelpers.SafePathCombine(srcDir, "bin"); + Directory.CreateDirectory(srcDir); + Directory.CreateDirectory(binDir); + File.WriteAllText(PathHelpers.SafePathCombine(srcDir, "A.cs"), "class A {}"); + File.WriteAllText(PathHelpers.SafePathCombine(binDir, "Generated.cs"), "// generated"); + + var yaml = """ + needs-review: + - "**/*.cs" + - "!**/bin/**" + evidence-source: + type: none + reviews: + - id: Core-Logic + title: Review of core business logic + paths: + - "src/**/*.cs" + """; + var config = ReviewMarkConfiguration.Parse(yaml); + + // Compute the fingerprint the same way PublishReviewReport will: via the constraint + var needsReviewConstraint = new HashSet( + config.GetNeedsReviewFiles(_testDirectory), StringComparer.Ordinal); + var expectedFingerprint = config.Reviews[0].GetFingerprint(_testDirectory, needsReviewConstraint); + + // Write an index entry using that fingerprint + var indexPath = PathHelpers.SafePathCombine(_testDirectory, "index.json"); + File.WriteAllText(indexPath, $$""" + { + "reviews": [ + { + "id": "Core-Logic", + "fingerprint": "{{expectedFingerprint}}", + "date": "2026-06-23", + "result": "pass", + "file": "CR-2026-001.pdf" + } + ] + } + """); + var index = ReviewIndex.Load(new EvidenceSource("fileshare", indexPath, null, null)); + + // Act + var result = config.PublishReviewReport(index, _testDirectory); + + // Assert — fingerprint matches (build artifact excluded); review is current + Assert.False(result.HasIssues); + Assert.Contains("\u2705 Current", result.Markdown); + } + /// /// Test that ElaborateReviewSet includes a Context subsection with plain file paths /// when global context files resolve. From a0ec21dcc4af561a3bf23174c047201b06a8dd6c Mon Sep 17 00:00:00 2001 From: Malcolm Nixon Date: Tue, 23 Jun 2026 23:16:22 -0400 Subject: [PATCH 2/4] Fix review findings from formal review of feature branch - Requirement ReviewMark-Config-PathsConstrainedToNeedsReview: change 'strict subset' to 'subset' (review set may equal full needs-review set) - User guide examples: add tests/**/*.cs to needs-review in two examples so all paths entries are covered under the new constraint - Design review-mark-configuration.md: add GetNeedsReviewFiles() to Key Methods section - Design review-mark.md: correct Load() description; glob resolution occurs in the generation/reporting methods, not Load() Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- docs/design/review-mark.md | 7 +++++-- .../configuration/review-mark-configuration.md | 9 +++++++++ .../configuration/review-mark-configuration.yaml | 2 +- docs/user_guide/introduction.md | 2 ++ 4 files changed, 17 insertions(+), 3 deletions(-) diff --git a/docs/design/review-mark.md b/docs/design/review-mark.md index 495a43f..3c6b85a 100644 --- a/docs/design/review-mark.md +++ b/docs/design/review-mark.md @@ -143,8 +143,11 @@ The high-level data flow through the ReviewMark system: 1. **Input**: `string[] args` from the OS shell → parsed by `Context.Create()` into structured flags and paths. -2. **Configuration**: `ReviewMarkConfiguration.Load()` reads `.reviewmark.yaml` from disk - and resolves glob patterns via `GlobMatcher` into sorted file lists. +2. **Configuration**: `ReviewMarkConfiguration.Load()` reads `.reviewmark.yaml` from disk, + parses and validates the YAML structure, and resolves relative fileshare paths. Glob + pattern resolution against the file system is performed later, in `PublishReviewPlan`, + `PublishReviewReport`, and `ElaborateReviewSet`, each of which calls `GlobMatcher` to + enumerate the matching files at generation time. 3. **Evidence**: `ReviewIndex.Load()` reads or downloads the evidence index, populating an in-memory lookup keyed by `(id, fingerprint)`. Three source types are supported: `none` (empty index), `fileshare` (local or network file-path read), and `url` (HTTP or diff --git a/docs/design/review-mark/configuration/review-mark-configuration.md b/docs/design/review-mark/configuration/review-mark-configuration.md index 775e870..b28322e 100644 --- a/docs/design/review-mark/configuration/review-mark-configuration.md +++ b/docs/design/review-mark/configuration/review-mark-configuration.md @@ -50,6 +50,15 @@ compliance documents. #### Key Methods +**`ReviewMarkConfiguration.GetNeedsReviewFiles(string directory)`** → `IReadOnlyList` (sorted, relative paths) + +Resolves `NeedsReviewPatterns` against `directory` using `GlobMatcher` and returns a sorted list +of relative file paths that require review. This method is used by `PublishReviewPlan`, +`PublishReviewReport`, and `ElaborateReviewSet` to compute the constraint passed to +`ReviewSet.GetFiles(dir, constraint)` and `ReviewSet.GetFingerprint(dir, constraint)`. When +`NeedsReviewPatterns` is empty the callers treat the result as an absent constraint (null) rather +than an empty set, preserving backward compatibility. + **`ReviewMarkConfiguration.Load(string filePath)`** → `ReviewMarkLoadResult` - *Parameters*: `string filePath` — path to the `.reviewmark.yaml` file diff --git a/docs/reqstream/review-mark/configuration/review-mark-configuration.yaml b/docs/reqstream/review-mark/configuration/review-mark-configuration.yaml index 6d78587..f3d17c3 100644 --- a/docs/reqstream/review-mark/configuration/review-mark-configuration.yaml +++ b/docs/reqstream/review-mark/configuration/review-mark-configuration.yaml @@ -277,7 +277,7 @@ sections: - id: ReviewMark-Config-PathsConstrainedToNeedsReview title: >- Review set paths resolution shall be constrained to the needs-review file set - when needs-review patterns are present, ensuring review set files are a strict + when needs-review patterns are present, ensuring review set files are a subset of files declared as needing review. justification: >- The needs-review list is the canonical declaration of which files require review. diff --git a/docs/user_guide/introduction.md b/docs/user_guide/introduction.md index 0e1ca5d..34a34fa 100644 --- a/docs/user_guide/introduction.md +++ b/docs/user_guide/introduction.md @@ -488,6 +488,7 @@ least one review set grouping related files. ```yaml needs-review: - "src/**/*.cs" + - "tests/**/*.cs" - "docs/**/*.md" - "!**/obj/**" @@ -636,6 +637,7 @@ With a corresponding definition file: # .reviewmark.yaml needs-review: - "src/**/*.cs" + - "tests/**/*.cs" - "docs/**/*.md" - "!**/obj/**" From 21300f461b140b4fc68d54a2680b379c2cd06c3c Mon Sep 17 00:00:00 2001 From: Malcolm Nixon Date: Tue, 23 Jun 2026 23:23:30 -0400 Subject: [PATCH 3/4] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- .../Configuration/ReviewMarkConfiguration.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/DemaConsulting.ReviewMark/Configuration/ReviewMarkConfiguration.cs b/src/DemaConsulting.ReviewMark/Configuration/ReviewMarkConfiguration.cs index 3ee226a..79eef02 100644 --- a/src/DemaConsulting.ReviewMark/Configuration/ReviewMarkConfiguration.cs +++ b/src/DemaConsulting.ReviewMark/Configuration/ReviewMarkConfiguration.cs @@ -541,7 +541,9 @@ internal ReviewSet(string id, string title, IReadOnlyList paths, IReadOn /// The root directory to search. /// /// When non-null, only files whose relative paths appear in this set are - /// returned. Pass the resolved needs-review file set to enforce that review-set + /// returned. Relative paths must use forward slashes (/), matching the + /// normalization performed by . + /// Pass the resolved needs-review file set to enforce that review-set /// files are a strict subset of files declared as needing review (excluding build /// artifacts and other paths that are excluded from needs-review). /// When null, all glob-matched files are returned without filtering. From e02ac8f4c38817115495edc46898dedcdfc47b53 Mon Sep 17 00:00:00 2001 From: Malcolm Nixon Date: Tue, 23 Jun 2026 23:23:37 -0400 Subject: [PATCH 4/4] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- .../Configuration/ReviewMarkConfiguration.cs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/DemaConsulting.ReviewMark/Configuration/ReviewMarkConfiguration.cs b/src/DemaConsulting.ReviewMark/Configuration/ReviewMarkConfiguration.cs index 79eef02..6aa3754 100644 --- a/src/DemaConsulting.ReviewMark/Configuration/ReviewMarkConfiguration.cs +++ b/src/DemaConsulting.ReviewMark/Configuration/ReviewMarkConfiguration.cs @@ -567,9 +567,10 @@ internal IReadOnlyList GetFiles(string directory, IReadOnlySet? /// The root directory to search. /// /// When non-null, only files whose relative paths appear in this set - /// contribute to the fingerprint. Pass the resolved needs-review file set so - /// that build artifacts excluded from needs-review do not affect the - /// fingerprint even when the review-set paths are broad. + /// contribute to the fingerprint. Relative paths must use forward slashes (/), + /// matching the normalization performed by . + /// Pass the resolved needs-review file set so that build artifacts excluded from + /// needs-review do not affect the fingerprint even when the review-set paths are broad. /// When null, all glob-matched files contribute to the fingerprint. /// ///