Fix #24 and #25#26
Merged
Merged
Conversation
Add W0112 source-level diagnostics for known artifact IDs in reviewable governed prose. Update reviewer agents to rely on govctl check diagnostics instead of rendered output inference. Fixes #24. Refs WI-2026-06-11-001.
Add canonical work edit/get support for verification.required_guards and existing waiver guard/reason fields. Update work help text and cover the paths with edit CLI regression tests. Refs WI-2026-06-11-002.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (14)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughRFC-0000 is bumped to v1.3.3 to require inline ChangesRFC-0000 Policy and Reference Validation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
gov/rfc/RFC-0000/rfc.toml (1)
53-57: 💤 Low valueConsider clarifying the conditional nature of the warning in the changelog.
The changelog entry states that validation "should warn" on bare artifact IDs, but the implementation only emits W0112 for artifacts in reviewable states (Draft RFCs, Proposed ADRs, not-Done Work Items). While this may be acceptable for a high-level changelog entry, consider whether explicitly noting the conditional behavior would help users understand when these warnings appear.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gov/rfc/RFC-0000/rfc.toml` around lines 53 - 57, Update the changelog entry for version "1.3.3" to explicitly note that the warning about bare artifact IDs is conditional — it only emits W0112 for artifacts in reviewable states (Draft RFCs, Proposed ADRs, non-Done Work Items); modify the "notes" or the specific entry in "changed" that currently reads "Project validation should warn on known artifact IDs in governed prose that are not written with inline reference syntax" to clearly state this scope/condition so users know when the warning will appear.src/cli/resources/work.rs (1)
29-40: 💤 Low valueConsider adding a waiver editing example.
The help text mentions both
verification.required_guardsandverification.waiversat line 33, but only provides an example forrequired_guards(line 39). Since waivers are edited by index rather than added (per the schema), consider adding an example to clarify the indexed editing pattern:govctl work get WI-2026-04-06-001 verification.waivers[0].reasonThis would help users understand how to access and edit existing waiver fields.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli/resources/work.rs` around lines 29 - 40, The after_help string for the work command currently lists verification.waivers but lacks a usage example; update the after_help content (the string literal in the #[command(after_help = "...")] attribute) to include an indexed waiver example such as "govctl work get WI-2026-04-06-001 verification.waivers[0].reason" so users see the correct indexed editing pattern for waivers alongside the existing verification.required_guards example.tests/edit_tests/work_tests/verification.rs (1)
64-107: 💤 Low valueConsider testing waiver guard change with a different value.
At line 80, the test sets
waiver.guardto"GUARD-WAIVED", which is the same value it already has (line 71). While the test verifies that the set operation executes successfully, changing it to a different guard ID would more thoroughly validate that the field update actually persists a new value rather than just executing without error.Example improvement:
common::write_guard(temp_dir.path(), "GUARD-WAIVED-ALT", "true")?; // ... later ... work_edit_set_waiver_guard("WI-2026-01-01-001", 0, "GUARD-WAIVED-ALT"),Then verify the output contains
"GUARD-WAIVED-ALT"in the final get command.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/edit_tests/work_tests/verification.rs` around lines 64 - 107, In test_work_edit_existing_waiver_guard_and_reason update the guard-change assertion to actually change the guard value: add a different guard ID via common::write_guard (e.g., "GUARD-WAIVED-ALT"), call work_edit_set_waiver_guard("WI-2026-01-01-001", 0, "GUARD-WAIVED-ALT") instead of re-setting to "GUARD-WAIVED", and update the final work_get_field/assertions (and the "Set ..." expectation) to check for "GUARD-WAIVED-ALT" so the test verifies the new value persisted; keep other checks for the reason unchanged.gov/schema/edit-ops.json (1)
782-835: Clarify SSOT verb support forverification.required_guardsvsverification.waivers
- In
gov/schema/edit-ops.json(work.verification, lines ~782-835),verification.required_guardsallowsverbs: ["get","set","add","remove"]; confirm that permittingseton indexedrequired_guards[i]is intentional (it’s unlike other list roots likenotesthat only exposeadd/remove).verification.waiversallowsverbs: ["get","remove"](noadd), and the only covered edit behavior is setting existingverification.waivers[i].guard/verification.waivers[i].reason—waivers are initially created by pre-populating[[verification.waivers]]in TOML fixtures. If CLI should also support adding new waivers, the SSOT needs anaddverb for the waivers list and tests/docs should be updated; otherwise, align CLI/help/ADR wording to explicitly state “waivers are TOML-first;work editonly edits/removes existing waivers.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gov/schema/edit-ops.json` around lines 782 - 835, The review points out inconsistent SSOT verb support: make an explicit decision and update the schema and docs accordingly — either (A) allow CLI additions by adding "add" to the verbs for verification.waivers (and adjust tests/docs/ADR to cover creation of new waivers via work.edit and ensure waivers' text_key/item handling supports add), or (B) restrict verification.required_guards to not expose indexed "set" (remove "set" from its verbs) and update CLI/help/ADR/test wording to state waivers are TOML-first and work edit only supports get/remove and editing existing verification.waivers[i].guard/reason; change the verbs on verification.required_guards and verification.waivers in gov/schema/edit-ops.json to match the chosen behavior and update any related tests/docs referenced in the review.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@gov/rfc/RFC-0000/rfc.toml`:
- Around line 53-57: Update the changelog entry for version "1.3.3" to
explicitly note that the warning about bare artifact IDs is conditional — it
only emits W0112 for artifacts in reviewable states (Draft RFCs, Proposed ADRs,
non-Done Work Items); modify the "notes" or the specific entry in "changed" that
currently reads "Project validation should warn on known artifact IDs in
governed prose that are not written with inline reference syntax" to clearly
state this scope/condition so users know when the warning will appear.
In `@gov/schema/edit-ops.json`:
- Around line 782-835: The review points out inconsistent SSOT verb support:
make an explicit decision and update the schema and docs accordingly — either
(A) allow CLI additions by adding "add" to the verbs for verification.waivers
(and adjust tests/docs/ADR to cover creation of new waivers via work.edit and
ensure waivers' text_key/item handling supports add), or (B) restrict
verification.required_guards to not expose indexed "set" (remove "set" from its
verbs) and update CLI/help/ADR/test wording to state waivers are TOML-first and
work edit only supports get/remove and editing existing
verification.waivers[i].guard/reason; change the verbs on
verification.required_guards and verification.waivers in
gov/schema/edit-ops.json to match the chosen behavior and update any related
tests/docs referenced in the review.
In `@src/cli/resources/work.rs`:
- Around line 29-40: The after_help string for the work command currently lists
verification.waivers but lacks a usage example; update the after_help content
(the string literal in the #[command(after_help = "...")] attribute) to include
an indexed waiver example such as "govctl work get WI-2026-04-06-001
verification.waivers[0].reason" so users see the correct indexed editing pattern
for waivers alongside the existing verification.required_guards example.
In `@tests/edit_tests/work_tests/verification.rs`:
- Around line 64-107: In test_work_edit_existing_waiver_guard_and_reason update
the guard-change assertion to actually change the guard value: add a different
guard ID via common::write_guard (e.g., "GUARD-WAIVED-ALT"), call
work_edit_set_waiver_guard("WI-2026-01-01-001", 0, "GUARD-WAIVED-ALT") instead
of re-setting to "GUARD-WAIVED", and update the final work_get_field/assertions
(and the "Set ..." expectation) to check for "GUARD-WAIVED-ALT" so the test
verifies the new value persisted; keep other checks for the reason unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3276a6e0-27c1-45ac-b557-a159ae5cc446
⛔ Files ignored due to path filters (1)
tests/snapshots/test_help__work_get_help.snapis excluded by!**/*.snap
📒 Files selected for processing (28)
.claude/agents/adr-reviewer.md.claude/agents/rfc-reviewer.md.claude/agents/wi-reviewer.mddocs/rfc/RFC-0000.mdgov/rfc/RFC-0000/clauses/C-REFERENCE-HIERARCHY.tomlgov/rfc/RFC-0000/rfc.tomlgov/schema/edit-ops.jsongov/work/2026-06-11-expose-work-item-verification-guard-edit-paths.tomlgov/work/2026-06-11-validate-raw-artifact-reference-syntax-for-reviewer-evidence.tomlgov/work/2026-06-12-fix-subdirectory-check-path-handling.tomlsrc/cli/resources/work.rssrc/cmd/check.rssrc/cmd/migrate/mod.rssrc/cmd/new/mod.rssrc/cmd/project_support.rssrc/config/runtime.rssrc/diagnostic/code/metadata.rssrc/diagnostic/code/mod.rssrc/scan.rssrc/validate/bracket_refs.rssrc/validate/mod.rstests/edit_tests/work_tests/mod.rstests/edit_tests/work_tests/verification.rstests/error_tests/rfc_clause_cases/check.rstests/error_tests/schema.rstests/error_tests/work.rstests/test_errors.rstests/test_scan.rs
Resolve check-time local support and source scan paths relative to the project root so subdirectory invocations do not emit false W0111 warnings or skip source refs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests