Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions docs/design/review-mark.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
62 changes: 50 additions & 12 deletions docs/design/review-mark/configuration/review-mark-configuration.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ compliance documents.

| Property | Type | Description |
| -------- | ---- | ----------- |
| `NeedsReviewPatterns` | `IReadOnlyList<string>` | 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<ReviewSet>` | Ordered list of review-set definitions |
| `GlobalContext` | `IReadOnlyList<string>` | Ordered list of glob patterns identifying global context files (empty when omitted from YAML) |
Expand All @@ -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` |
Expand All @@ -49,6 +50,15 @@ compliance documents.

#### Key Methods

**`ReviewMarkConfiguration.GetNeedsReviewFiles(string directory)`** → `IReadOnlyList<string>` (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
Expand Down Expand Up @@ -80,25 +90,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<string>` 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<string>` 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<string>? constraint = null)`**
→ `IReadOnlyList<string>` (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<string>? 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`)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
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
19 changes: 19 additions & 0 deletions docs/user_guide/introduction.md
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -471,6 +488,7 @@ least one review set grouping related files.
```yaml
needs-review:
- "src/**/*.cs"
- "tests/**/*.cs"
- "docs/**/*.md"
- "!**/obj/**"

Expand Down Expand Up @@ -619,6 +637,7 @@ With a corresponding definition file:
# .reviewmark.yaml
needs-review:
- "src/**/*.cs"
- "tests/**/*.cs"
- "docs/**/*.md"
- "!**/obj/**"

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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**:
Expand Down Expand Up @@ -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
Loading
Loading