ai-review/assessment AI findings; a developer approval after checking them unlocks the merge; a human review request is pending
ai-review/gate build succeeded after 00:14
werkator/server build succeeded after 00:00
werkator build succeeded after 00:13
werkator/werkrevisor-statistics build succeeded after 00:00
Fixes the three findings of the self-review. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
192 lines
8.4 KiB
Markdown
192 lines
8.4 KiB
Markdown
# PR#15: critical paths give ORANGE
|
|
|
|
> **WARNING:** This document describes only the change applied in this PR.
|
|
> It may already be outdated once the next PR is merged.
|
|
> Historic PR documentation is not maintained along with new PRs; treat it as a snapshot.
|
|
|
|
## The Problem
|
|
|
|
werkrevisor 0.14.0 knows only one tier of risky paths: `[hard_yellow] paths` give YELLOW.
|
|
A change of the RBAC code, of the authentication or of a trust boundary gets the same verdict
|
|
as a changed build file or a new dependency.
|
|
In the approval mode, the reviewer cannot see in the verdict which PR needs a deliberate review.
|
|
|
|
## Non-Goals
|
|
|
|
- No review requests to dedicated reviewers per critical path; all ORANGE requests go to `human_reviewers`.
|
|
- No change of `.werkrevisor.toml` of werkrevisor itself;
|
|
the installed werkrevisor 0.14.0 does not know `[hard_orange]` and would give YELLOW for this PR.
|
|
- No new gate behavior: ORANGE from critical paths is unlocked as any other ORANGE.
|
|
|
|
## The Scenarios
|
|
|
|
### Feature: a changed critical path gives ORANGE
|
|
|
|
#### Background
|
|
|
|
- The config sets `[hard_orange] paths`, e.g. `**/auth/**`, `**/rbac/**` and `**/*secret*`.
|
|
- The config sets `[hard_yellow] paths`, e.g. `**/db/**` and `build.gradle*`.
|
|
|
|
#### Scenario#15.01: a changed critical path gives ORANGE with one reason line
|
|
|
|
So that the reviewer sees at once that the PR needs a deliberate review, however many critical files change.
|
|
|
|
- **Given** a PR changes 5 files below `src/auth/`
|
|
- **When** werkrevisor reviews the PR
|
|
- **Then** the verdict is ORANGE
|
|
- **and** one reason names 3 of the files with their glob and the count of the rest, e.g.
|
|
`critical paths changed, review them deliberately: src/auth/A0.java ([hard_orange] **/auth/**), … and 2 more`
|
|
|
|
##### Verified by
|
|
|
|
- [test_rules.HardOrangeTest.test_critical_path_gives_orange_with_one_aggregated_reason](../../tests/test_rules.py)
|
|
|
|
#### Scenario#15.02: documentation never matches a critical path
|
|
|
|
So that a PR document named after a critical topic does not need a deliberate review.
|
|
|
|
- **Given** a PR changes only `<file>`
|
|
- **When** werkrevisor reviews the PR
|
|
- **Then** no reason names critical paths
|
|
|
|
| file |
|
|
|---|
|
|
| `doc/PR/2026-08-17-PR#288-rbac-generator-hardening.md` |
|
|
| `docs/auth/README.md` |
|
|
|
|
##### Verified by
|
|
|
|
- [test_rules.HardOrangeTest.test_documentation_never_matches_critical_paths](../../tests/test_rules.py)
|
|
|
|
#### Scenario#15.03: a renamed file counts with its old path
|
|
|
|
So that a PR cannot move a critical file out of a critical path without a deliberate review.
|
|
|
|
- **Given** a PR renames `src/main/java/net/example/auth/Login.java` to `src/main/java/net/example/Login.java`
|
|
- **When** werkrevisor reviews the PR
|
|
- **Then** the verdict is ORANGE with the reason for critical paths
|
|
|
|
##### Verified by
|
|
|
|
- [test_rules.HardOrangeTest.test_renamed_from_critical_path_counts](../../tests/test_rules.py)
|
|
|
|
#### Scenario#15.04: a mechanical change of a critical path stays ORANGE
|
|
|
|
So that a critical path stays critical, also when the model need not review the change.
|
|
|
|
- **Given** a PR changes only the imports of `src/main/java/net/example/auth/Login.java`
|
|
- **When** werkrevisor reviews the PR
|
|
- **Then** the verdict is ORANGE with the reason for critical paths
|
|
- **and** the model gets no call
|
|
|
|
##### Verified by
|
|
|
|
- [test_mechanical.FlowTest.test_mechanical_change_of_a_critical_path_stays_orange](../../tests/test_mechanical.py)
|
|
|
|
#### Scenario#15.05: a file in both tiers is listed only as critical
|
|
|
|
So that the reasons name each file only once, under the more severe tier.
|
|
|
|
- **Given** the config lists `src/main/java/net/example/auth/**` in `[hard_orange] paths` and in `[hard_yellow] paths`
|
|
- **and** a PR changes `src/main/java/net/example/auth/Login.java`
|
|
- **When** werkrevisor reviews the PR
|
|
- **Then** the reason for critical paths names the file
|
|
- **and** no reason for risky paths names it
|
|
|
|
##### Verified by
|
|
|
|
- [test_rules.HardOrangeTest.test_file_in_both_tiers_is_listed_only_as_critical](../../tests/test_rules.py)
|
|
- [test_rules.HardOrangeTest.test_a_critical_db_schema_path_is_listed_only_as_critical](../../tests/test_rules.py)
|
|
|
|
#### Scenario#15.06: the repository config adds critical paths, but cannot remove them
|
|
|
|
So that a repository can declare its own critical paths, but cannot weaken the host config.
|
|
|
|
- **Given** the host config sets `[hard_orange] paths = ["**/auth/**"]`
|
|
- **and** `.werkrevisor.toml` sets `[hard_orange] paths = <repository paths>`
|
|
- **When** werkrevisor builds the config
|
|
- **Then** `[hard_orange] paths` are `<merged paths>`
|
|
|
|
| repository paths | merged paths |
|
|
|---|---|
|
|
| `["werkrevisor/gitea.py"]` | `["**/auth/**", "werkrevisor/gitea.py"]` |
|
|
| `[]` | `["**/auth/**"]` |
|
|
|
|
##### Verified by
|
|
|
|
- [test_config_layers.LayeringTest.test_hard_orange_paths_are_added](../../tests/test_config_layers.py)
|
|
- [test_config_layers.LayeringTest.test_repository_cannot_remove_hard_orange_paths](../../tests/test_config_layers.py)
|
|
|
|
#### Scenario#15.07: every config needs the section `[hard_orange]`
|
|
|
|
So that an operator who forgot the critical paths notices it at the first run.
|
|
|
|
- **Given** neither the host config nor `.werkrevisor.toml` sets `[hard_orange]`
|
|
- **When** werkrevisor loads the config
|
|
- **Then** werkrevisor stops with a config error that names `[hard_orange]`
|
|
|
|
##### Verified by
|
|
|
|
- [test_config_source_and_isolation.MissingSectionTest.test_a_config_without_hard_orange_is_refused](../../tests/test_config_source_and_isolation.py)
|
|
|
|
#### Scenario#15.08: the comment lists the critical paths before the YELLOW reasons
|
|
|
|
So that the reviewer reads first why the PR needs a deliberate review.
|
|
|
|
- **Given** a PR changes `src/main/java/net/example/auth/Login.java` and `build.gradle.kts`
|
|
- **When** werkrevisor writes the comment
|
|
- **Then** the reason for critical paths comes before the reason for risky paths
|
|
|
|
##### Verified by
|
|
|
|
- [test_flow.ApprovalGateTest.test_critical_paths_reason_comes_before_the_yellow_reasons](../../tests/test_flow.py)
|
|
|
|
#### Scenario#15.09: in the approval mode, a critical path waits for a human approval
|
|
|
|
So that a critical path never merges with the approval of the bot alone.
|
|
|
|
- **Given** the config sets `[gate] yellow_gate = "approval"` and `human_reviewers = ["alice"]`
|
|
- **and** a PR changes `src/main/java/net/example/auth/Login.java`
|
|
- **and** the model reports no finding
|
|
- **When** werkrevisor reviews the PR
|
|
- **Then** the bot submits no review
|
|
- **and** werkrevisor requests a review from alice
|
|
- **and** the status `ai-review/assessment` is `warning`
|
|
- **and** the exit code is 0
|
|
|
|
##### Verified by
|
|
|
|
- [test_flow.ApprovalGateTest.test_critical_path_requests_human_review_without_bot_review](../../tests/test_flow.py)
|
|
|
|
## The Solution
|
|
|
|
- The new section `[hard_orange]` has one key `paths`, a list of globs, as `[hard_yellow] paths`.
|
|
- The rules check each changed file, with its old path for a rename, against `[hard_orange] paths` first.
|
|
A match gives ORANGE with the reason code `critical_paths`;
|
|
the file is then not checked against the YELLOW paths, also not against the built-in ones.
|
|
- Files of the `docs` category never match, and mechanical files count as all other files.
|
|
- The repository layer adds its paths to the host paths, as for `[hard_yellow] paths`.
|
|
- `[hard_orange]` is a required section; an empty list is allowed.
|
|
- `config/hsadmin-ng.toml` moves RBAC, security, permissions and secrets from `[hard_yellow]` to `[hard_orange]`:
|
|
`**/rbac/**`, `src/main/resources/db/changelog/1-rbac/**`, `**/*Security*`, `**/*ApiKey*`, `**/*Auth*`, `**/*auth*`,
|
|
`**/*Jwt*`, `**/*Oidc*`, `**/*Rbac*`, `**/*rbac*`, `**/*Permission*`, `**/*secret*`, `**/*.pem`, `**/*.key`,
|
|
`.env*` and `.tc-environment`.
|
|
Build, CI, deployment, profiles and the DB schema stay in `[hard_yellow]`.
|
|
- `config/werkprobe.toml` moves authorization, RBAC and secrets to `[hard_orange]`.
|
|
- `config/werkrevisor.toml` sets `[hard_orange] paths = []`; the repository part follows in a separate PR.
|
|
- The README and the German operator docs describe `[hard_orange]`;
|
|
the proposal for hsadmin-ng lists the ORANGE and the YELLOW paths separately.
|
|
- werkrevisor 0.15.0.
|
|
Operators add `[hard_orange]` to their host configs before they install it.
|
|
|
|
## Additional Changes
|
|
|
|
- Three tests used an RBAC or authorization path of `config/werkprobe.toml` as a YELLOW path;
|
|
they now use a path that stays in `[hard_yellow]`.
|
|
|
|
## Follow-up PRs
|
|
|
|
- After the deploy of 0.15.0, `.werkrevisor.toml` moves the trust-boundary modules of werkrevisor
|
|
from `[hard_yellow]` to `[hard_orange]`.
|
|
- Review requests to dedicated reviewers per critical path.
|