fix: add compliance rule for MAX_TOKENS model registry#2281
fix: add compliance rule for MAX_TOKENS model registry#2281
Conversation
The Qodo agentic review was flagging new model entries in the MAX_TOKENS dictionary as "hardcoded config" rule violations. This adds a compliance rule clarifying that MAX_TOKENS is the designated model registry, not hardcoded configuration. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Review Summary by QodoAdd compliance rule for MAX_TOKENS model registry pattern
WalkthroughsDescription• Adds compliance rule clarifying MAX_TOKENS dictionary pattern • Prevents false positive violations in model registry entries • Documents expected model token limit registration approach Diagramflowchart LR
A["Compliance Checklist"] -->|"adds new rule"| B["Model Registry Rule"]
B -->|"clarifies"| C["MAX_TOKENS is designated registry"]
C -->|"prevents"| D["False positive violations"]
File Changes1. pr_compliance_checklist.yaml
|
Code Review by Qodo
1. Unsupported config key
|
There was a problem hiding this comment.
Pull request overview
Adds a new PR compliance checklist rule to clarify that MAX_TOKENS entries in pr_agent/algo/__init__.py are the intended model registry, aiming to prevent automated reviews from incorrectly flagging new model additions as “hardcoded config”.
Changes:
- Introduces a compliance checklist item explicitly designating
MAX_TOKENSas the approved model token registry location. - Defines success/failure criteria to guide where model token limits should be added.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pr_compliance_checklist.yaml
Outdated
| - title: "Model Registry Entries Are Not Hardcoded Config" | ||
| compliance_label: false | ||
| objective: "The MAX_TOKENS dictionary in pr_agent/algo/__init__.py is the designated model token registry. Adding new model entries there is the expected pattern, not a hardcoded configuration violation" | ||
| success_criteria: "New model entries follow existing patterns in the MAX_TOKENS dictionary with appropriate comments" |
There was a problem hiding this comment.
The success criteria currently requires "appropriate comments" for new MAX_TOKENS entries, but many existing registry entries in pr_agent/algo/init.py have no inline comment. If this checklist is meant to prevent false positives, consider relaxing this to "follow the existing MAX_TOKENS entry format" (and only require comments when the limit is non-obvious), otherwise the rule may introduce new false positives for otherwise consistent entries.
| success_criteria: "New model entries follow existing patterns in the MAX_TOKENS dictionary with appropriate comments" | |
| success_criteria: "New model entries follow the existing entry format in the MAX_TOKENS dictionary; add inline comments when the token limit is non-obvious" |
Move the MAX_TOKENS registry guidance to review_agent extra_instructions in .pr_agent.toml so the agentic review actually sees and respects it. Remove the compliance checklist entry which doesn't affect review comments. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Code Review by QodoNew Review StartedThis review has been superseded by a new analysisⓘ The new review experience is currently in Beta. Learn more |
The Qodo agentic review cites AGENTS.md rules as evidence for its findings. Clarify that the MAX_TOKENS dictionary is the designated model registry so it stops flagging new entries as rule violations.
|
Persistent review updated to latest commit cf8fb2f |
|
@ofir-frd what's the best approach to avoid Qodo from adding the finding of the hardcoded values (like in #2266 (comment))? |
| [review_agent] | ||
| enabled = true | ||
| publish_output = true | ||
| extra_instructions = "The MAX_TOKENS dictionary in pr_agent/algo/__init__.py is the designated model token registry. Adding new model entries there is the expected pattern, not a hardcoded configuration violation." |
There was a problem hiding this comment.
1. extra_instructions exceeds 120 chars 📘 Rule violation ⚙ Maintainability
The newly added extra_instructions line in .pr_agent.toml exceeds the project’s 120-character line-length convention, creating an avoidable noisy formatting deviation. This violates the requirement to preserve existing formatting and keep 120-character lines.
Agent Prompt
## Issue description
The added `.pr_agent.toml` `extra_instructions` value is on a single line exceeding the repository’s 120-character line-length convention.
## Issue Context
This repository’s formatting guidance expects 120-character lines; long single-line config strings should be wrapped (e.g., via TOML multiline basic strings).
## Fix Focus Areas
- .pr_agent.toml[21-21]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
AGENTS.md
Outdated
| - **Do** match the interpreter requirement declared in `pyproject.toml` (Python ≥ 3.12) and install `requirements.txt` plus `requirements-dev.txt` before running tools. | ||
| - **Do** run tests with `PYTHONPATH=.` set to keep imports functional (for example `PYTHONPATH=. ./.venv/bin/pytest tests/unittest/test_fix_json_escape_char.py -q`). | ||
| - **Do** adjust configuration through `.pr_agent.toml` or files under `pr_agent/settings/` instead of hard-coding values. | ||
| - **Do** adjust configuration through `.pr_agent.toml` or files under `pr_agent/settings/` instead of hard-coding values. The `MAX_TOKENS` dictionary in `pr_agent/algo/__init__.py` is the designated model token registry — new model entries there are expected, not hardcoded configuration. |
There was a problem hiding this comment.
2. agents.md line exceeds 120 📘 Rule violation ⚙ Maintainability
The modified bullet line in AGENTS.md exceeds the project’s 120-character line-length convention, reducing readability and diverging from established formatting. This violates the requirement to preserve existing formatting and keep 120-character lines.
Agent Prompt
## Issue description
The updated `AGENTS.md` bullet exceeds the repository’s 120-character line-length convention.
## Issue Context
Markdown list items should be wrapped onto subsequent indented lines to preserve readability and match the project’s formatting standard.
## Fix Focus Areas
- AGENTS.md[7-7]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This reverts commit cf8fb2f.
|
Persistent review updated to latest commit 7f42313 |
| [review_agent] | ||
| enabled = true | ||
| publish_output = true | ||
| extra_instructions = "The MAX_TOKENS dictionary in pr_agent/algo/__init__.py is the designated model token registry. Adding new model entries there is the expected pattern, not a hardcoded configuration violation." |
There was a problem hiding this comment.
1. Unsupported config key 🐞 Bug ✓ Correctness
.pr_agent.toml adds extra_instructions under [review_agent], but the PR review flow reads get_settings().pr_reviewer.extra_instructions, so this new instruction will not be included in the reviewer prompt. As a result, the intended behavior change (guidance about MAX_TOKENS) will not take effect for /review runs in this repository.
Agent Prompt
## Issue description
The added setting `review_agent.extra_instructions` is not used by the PR review tool implementation in this repo; the reviewer prompt consumes `pr_reviewer.extra_instructions` instead. This makes the new instruction effectively a no-op for `/review`.
## Issue Context
`PRReviewer` constructs prompt variables and explicitly sets `extra_instructions` from `get_settings().pr_reviewer.extra_instructions`.
## Fix Focus Areas
- .pr_agent.toml[18-21]
- pr_agent/tools/pr_reviewer.py[77-102]
- pr_agent/settings/configuration.toml[74-90]
## Proposed fix
Either:
1) Move/duplicate the new `extra_instructions` entry under `[pr_reviewer]` in `.pr_agent.toml` (so it is actually consumed by `PRReviewer`),
or:
2) If `review_agent` is intended to be a real tool/command in this repo, implement consumption of `get_settings().review_agent.extra_instructions` in the relevant execution path and add a corresponding defaults section in `pr_agent/settings/configuration.toml`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
JiwaniZakir
left a comment
There was a problem hiding this comment.
The extra_instructions string in .pr_agent.toml hardcodes a specific file path (pr_agent/algo/__init__.py) to suppress a false-positive compliance flag. If that file is ever renamed or relocated, this instruction silently becomes stale and either stops working or misleads the review agent. It would be worth adding a brief comment nearby (or in a CHANGELOG/DECISIONS doc) explaining why this override was necessary and what the original compliance rule was triggering on, so future maintainers understand the intent rather than treating it as dead config. Additionally, if other token/model registries exist elsewhere in the codebase with similar patterns, this instruction only exempts the one file — consider whether the scope is broad enough or if the underlying compliance rule itself should be tuned to recognize registry patterns more generally.
Summary
MAX_TOKENSdictionary entries inpr_agent/algo/__init__.pyare the designated model registry pattern, not hardcoded configurationTest plan