Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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