feat(policy): fold config.anti_policies into the review prompt (#46) - #59
DYNOSuprovo wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
@DYNOSuprovo Really appreciate the contribution, especially since this is one of the first contributions to marginal! 🙌
I went through the changes and left a few comments around consistency and documentation. Nothing major; once those are addressed, I’d be happy to get this merged.
Thanks again for taking the time to contribute and help shape the project from the early days!
| finding: Finding | None = None | ||
| if reviewer_model is not None: | ||
| policies = load_policies(path, config.policies) | ||
| anti_policies = load_policies(path, config.anti_policies) |
There was a problem hiding this comment.
I was wondering if we could also add a matching scaffold for anti_policies here. Currently, marginal init creates .marginal/policies/ via POLICIES_DIR, but there’s no equivalent directory for anti-policies.
Without it, users running marginal init from scratch may not realize this feature exists unless they happen to check the CHANGELOG or source code. Having something like .marginal/anti-policies/ would give both features a similar onboarding experience. What do you think?
| the content of any `config.policies` and `config.anti_policies` files | ||
| (loaded via `marginal.policy.load_policies`, relative to `path`; a | ||
| missing file is skipped with a warning rather than failing the review), | ||
| then runs it through `marginal.review.filter_findings`: dropped | ||
| outright if its self-reported `confidence` is below | ||
| `config.review.confidence_threshold`, otherwise capped alongside any | ||
| others at `config.review.max_comments` (highest-confidence first). A | ||
| finding that survives appends to the printed summary as a | ||
| severity+confidence badge (e.g. `🔴 **Critical** · 92% confidence`) | ||
| followed by its message. Without `models.reviewer`, or if the finding | ||
| gets filtered out, the summary stays metadata-only, same as if nothing | ||
| was generated. |
There was a problem hiding this comment.
I think it would be good to document anti_policies alongside policies here as well. The Configuration and “What it reviews for” sections currently explain .marginal/policies/ in detail, but don’t mention anti_policies.
Since this is a user-facing configuration option, giving it the same documentation treatment would make it much easier to discover, especially for users who aren’t looking through the source code.
| def _build_finding_prompt( | ||
| files: list[dict[str, object]], | ||
| policies: list[tuple[str, str]], | ||
| anti_policies: list[tuple[str, str]] | None = None, | ||
| ) -> str: |
There was a problem hiding this comment.
I noticed that anti_policies defaults to None in _generate_finding / _build_finding_prompt (marginal/cli/review.py), while policies is required and has no default. Since the two are conceptually parallel, it feels a little odd to have them behave differently here.
I understand that the current default keeps the existing 2-arg test call working, but I think it might be cleaner either to give policies the same default for consistency.
ad5694f to
d7dc47a
Compare
Signed-off-by: DYNOSuprovo <DYNOSuprovo@users.noreply.github.com>
d7dc47a to
6596815
Compare
|
@exactml Thanks for the thoughtful review and warm welcome! 🙌 I've addressed all the feedback and rebased onto the latest
The PR is now clean and |
2 participants
Summary
Closes #46.
This PR adds support for
config.anti_policiesin.marginal/config.yaml. Anti-policies allow repositories to document deliberate, accepted tradeoffs or intentional patterns that the AI reviewer must never flag as issues.Changes
marginal/config/schema.py):anti_policies: list[str] = Field(default_factory=list)toMarginalConfig.marginal/cli/review.py):run_review, loadsconfig.anti_policiesviamarginal.policy.load_policies(path, config.anti_policies)alongsideconfig.policies, reusing the existing skip-and-warn handling for missing files._build_finding_prompt, folds anti-policy content into the prompt under a distinct "do not flag" instruction framing:tests/test_config.py: verified defaultanti_policies == []and valid parsing of configuredanti_policies.tests/test_cli.py:policiesandanti_policiesfold together in expected order ahead of the diff.marginal reviewwith configured anti-policy reaches the LLM prompt.CHANGELOG.md):### New Featuresfor ISSUE-46.Verification
pytest -v: All 112 tests passed in 21.96sruff check .: All checks passedruff format --check .: 37 files already formatted