Skip to content

feat: add restricted mode for reduced GitHub permissions (#2279)#2491

Merged
naorpeled merged 11 commits into
The-PR-Agent:mainfrom
utsab345:fix/restricted-mode
Jul 3, 2026
Merged

feat: add restricted mode for reduced GitHub permissions (#2279)#2491
naorpeled merged 11 commits into
The-PR-Agent:mainfrom
utsab345:fix/restricted-mode

Conversation

@utsab345

@utsab345 utsab345 commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Adds a config.restricted_mode option (default false) that lets users run PR-Agent with only issues: write and pull-requests: write permissions, without requiring contents: write.

What it does:

  • When restricted_mode = true, GithubProvider.is_supported("push_code") returns False (same for GitLab and Bitbucket)
  • /update_changelog --push_changelog_changes=true checks is_supported("push_code") and gracefully skips with a clear log/comment message instead of failing with a 403
  • All other tools (/review, /describe, /improve, etc.) continue to work normally since they only need pull-requests: write

Why:
Only /update_changelog with push_changelog_changes=true actually calls repo.update_file() which requires contents: write. Every other tool works with PR comments, reviews, and edits. This change makes the permission requirement explicit and gives users a safe way to run with reduced scope.

Closes #2279

Utsab added 3 commits July 1, 2026 17:48
…ests

- Cache key is now 'related_tickets_<md5(pr_url)>' instead of global
  'related_tickets', preventing stale tickets leaking between unrelated PRs
- Added cache_tickets=true config flag under [pr_reviewer] to allow disabling
- Fixed Dynaconf merge_enabled=True list append bug in both production code
  (env_bridge.py uses case-insensitive pop before set) and test helpers
  (restore_settings now calls _remove_key before every set)
- Added azure-identity dependency for Azure DevOps provider availability
- 1043 tests pass, 0 failures
…t#2279)

Add a `config.restricted_mode` option that gates operations requiring
elevated permissions (e.g. pushing code to the repository). When enabled,
`is_supported("push_code")` returns `False` on GitHub, GitLab, and
Bitbucket providers, and `/update_changelog --push_changelog_changes=true`
gracefully skips with a clear message instead of failing.

Users can set `restricted_mode = true` and use only `issues: write`
and `pull-requests: write` permissions, without `contents: write`.
@github-actions github-actions Bot added the feature 💡 label Jul 2, 2026
@qodo-free-for-open-source-projects

Copy link
Copy Markdown
Contributor

PR Summary by Qodo

Add restricted_mode to disable code-push capabilities under reduced permissions

✨ Enhancement ⚙️ Configuration changes 🕐 10-20 Minutes

Grey Divider

AI Description

• Add config.restricted_mode to explicitly disable operations requiring repo write permissions.
• Gate push_code capability across GitHub/GitLab/Bitbucket providers when restricted mode is
 enabled.
• Make /update_changelog skip push attempts with a clear message instead of 403 failures.
Diagram

graph TD
  A["config.restricted_mode"] --> B["get_settings().config"] --> C["Provider.is_supported('push_code')"] --> D{Allowed?}
  D -->|"Yes"| E["/update_changelog: push enabled"] --> F["create_or_update_pr_file()"]
  D -->|"No"| G["Log/comment + return"]

  subgraph Legend
    direction LR
    _cfg["Config value"] ~~~ _mod["Module/Function"] ~~~ _dec{"Decision"}
  end
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Rely on runtime 403 handling only
  • ➕ No new configuration surface area; behavior adapts automatically
  • ➕ Works even if a provider forgets to implement the capability gate
  • ➖ Discoverability is worse (users learn only after a failed attempt)
  • ➖ Requires consistent exception mapping across providers/SDKs to avoid noisy logs
2. Separate, explicit config just for changelog pushing
  • ➕ More targeted: only gates the known write path (/update_changelog)
  • ➕ Avoids adding a generic capability that may need broader semantics later
  • ➖ Doesn’t scale if other tools add push/write functionality later
  • ➖ Still requires a concept of “write-requiring operations” to be repeated per tool
3. Provider-level permission introspection (where possible)
  • ➕ Potentially avoids manual configuration by detecting token/app scopes
  • ➖ Not reliably available across GitHub/GitLab/Bitbucket APIs and auth modes
  • ➖ Adds complexity and provider-specific branching for marginal benefit

Recommendation: The chosen approach (a single config.restricted_mode gating the push_code capability, plus a graceful guard in /update_changelog) is the best tradeoff: it’s explicit, cross-provider, and prevents predictable 403 failures. Consider additionally keeping (or adding) a fallback that treats 403/insufficient-scope errors during create_or_update_pr_file() as a soft failure with the same message, to cover any missed call-sites.

Files changed (5) +24 / -9

Enhancement (3) +6 / -0
bitbucket_provider.pyDisable 'push_code' capability when restricted_mode is enabled +2/-0

Disable 'push_code' capability when restricted_mode is enabled

• Extends 'is_supported()' to return false for the 'push_code' capability under 'config.restricted_mode'. This prevents code-writing features from being offered/used when running with reduced permissions.

pr_agent/git_providers/bitbucket_provider.py

github_provider.pyGate GitHub 'push_code' support behind restricted_mode +2/-0

Gate GitHub 'push_code' support behind restricted_mode

• Adds a restricted-mode check in 'is_supported()' so GitHub providers report 'push_code' as unsupported when configured for reduced permissions. This enables upstream tools to avoid write attempts that require 'contents: write'.

pr_agent/git_providers/github_provider.py

gitlab_provider.pyGate GitLab 'push_code' support behind restricted_mode +2/-0

Gate GitLab 'push_code' support behind restricted_mode

• Updates 'is_supported()' to deny 'push_code' when 'config.restricted_mode' is true, while preserving existing capability exclusions. Aligns GitLab behavior with GitHub/Bitbucket for reduced-permission operation.

pr_agent/git_providers/gitlab_provider.py

Bug fix (1) +17 / -9
pr_update_changelog.pySkip changelog push when 'push_code' is unsupported/restricted +17/-9

Skip changelog push when 'push_code' is unsupported/restricted

• Refactors the preflight checks when 'push_changelog_changes' is enabled to (1) verify the provider supports file pushes and (2) verify 'is_supported('push_code')'. On failure, it logs and optionally comments a clear message, then returns without raising a 403.

pr_agent/tools/pr_update_changelog.py

Other (1) +1 / -0
configuration.tomlAdd 'config.restricted_mode' default setting +1/-0

Add 'config.restricted_mode' default setting

• Introduces 'restricted_mode = false' in the default configuration with an explanatory comment. Documents the intent to skip operations requiring elevated permissions (e.g., pushing code).

pr_agent/settings/configuration.toml

@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (1) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Action required

1. Brittle push-support unit test ✓ Resolved 🐞 Bug ☼ Reliability
Description
test_run_without_push_support tries to simulate a provider without create_or_update_pr_file by
delattr-ing the attribute on a bare MagicMock, but PRUpdateChangelog uses hasattr() for
detection and the mock may still appear to have the attribute, causing the test to execute the wrong
path. This can make the test fail to assert the intended early-exit behavior (or become brittle) and
can let regressions in the skip logic slip through.
Code

tests/unittest/test_pr_update_changelog.py[R142-157]

+    async def test_run_without_push_support(self, mock_git_provider, mock_ai_handler):
"""Test running changelog update when git provider doesn't support pushing."""
-        # Arrange
-        delattr(mock_git_provider, 'create_or_update_pr_file')  # Remove the method
-        changelog_tool.commit_changelog = True
-        
-        with patch('pr_agent.tools.pr_update_changelog.get_settings') as mock_settings:
+        delattr(mock_git_provider, 'create_or_update_pr_file')
+        with patch('pr_agent.tools.pr_update_changelog.get_git_provider', return_value=lambda url: mock_git_provider), \
+             patch('pr_agent.tools.pr_update_changelog.get_settings') as mock_settings:
   mock_settings.return_value.pr_update_changelog.push_changelog_changes = True
   mock_settings.return_value.config.publish_output = True
-            
-            # Act
-            await changelog_tool.run()
-            
-            # Assert
+            mock_settings.return_value.pr_update_changelog.extra_instructions = ""
+            mock_settings.return_value.pr_update_changelog_prompt.system = ""
+            mock_settings.return_value.pr_update_changelog_prompt.user = ""
+            tool = PRUpdateChangelog("https://example.com/pr/123", ai_handler=lambda: mock_ai_handler)
+
+            await tool.run()
+
   mock_git_provider.publish_comment.assert_called_once()
-            assert "not currently supported" in str(mock_git_provider.publish_comment.call_args)
+            assert "not supported" in str(mock_git_provider.publish_comment.call_args)
Evidence
The test’s provider double is a plain MagicMock() (no spec) and then attempts to remove a method
via delattr(...), while the production code uses hasattr(...) on the provider to decide whether
pushing is supported; this mismatch makes the test’s simulation of “no push support” unreliable.

tests/unittest/test_pr_update_changelog.py[11-22]
tests/unittest/test_pr_update_changelog.py[141-157]
pr_agent/tools/pr_update_changelog.py[27-37]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`test_run_without_push_support` uses a plain `MagicMock()` and then calls `delattr(mock_git_provider, 'create_or_update_pr_file')` to simulate a provider that does not support pushing. Because the production code checks support via `hasattr(self.git_provider, "create_or_update_pr_file")`, the test double should behave like a real object for attribute existence; a bare `MagicMock` is not a reliable stand-in.
## Issue Context
The goal of the test is to ensure `/update_changelog` exits early with a clear comment when push is requested but the provider cannot push.
## Fix Focus Areas
- tests/unittest/test_pr_update_changelog.py[142-157]
- pr_agent/tools/pr_update_changelog.py[27-37]
## Suggested fix
In the test, replace the bare `MagicMock()` provider with either:
1) A small stub class/object that implements only the fields/methods the early-exit path touches (e.g., `publish_comment`, `get_pr_branch`, `get_pr_description`, `get_commit_messages`, and `pr.title`) and *does not* define `create_or_update_pr_file`.
or
2) A `MagicMock(spec_set=[...])` that explicitly omits `create_or_update_pr_file` so that `hasattr(mock, 'create_or_update_pr_file')` correctly returns `False`.
This keeps the test aligned with the production feature-detection mechanism and ensures it reliably exercises the intended branch.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Logs dict(get_settings()) configs ✓ Resolved 📘 Rule violation ⛨ Security
Description
PRUpdateChangelog.run() logs full configuration dictionaries via artifacts=relevant_configs,
which can leak sensitive configuration values into logs. This violates the requirement to treat logs
as a security boundary and avoid logging full settings/secrets.
Code

pr_agent/tools/pr_update_changelog.py[R86-88]

+        relevant_configs = {'pr_update_changelog': dict(get_settings().pr_update_changelog),
+                            'config': dict(get_settings().config)}
+        get_logger().debug("Relevant configs", artifacts=relevant_configs)
Evidence
PR Compliance ID 25 forbids logging full settings dictionaries or secret-bearing values. The added
debug statement logs relevant_configs built from dict(get_settings().pr_update_changelog) and
dict(get_settings().config) as structured log artifacts.

pr_agent/tools/pr_update_changelog.py[86-88]
Best Practice: Learned patterns

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`get_logger().debug("Relevant configs", artifacts=relevant_configs)` logs full `dict(get_settings().config)` / `dict(get_settings().pr_update_changelog)` which can unintentionally expose sensitive values.
## Issue Context
Compliance requires treating logs as a security boundary and never logging full settings dictionaries or secret-bearing values.
## Fix Focus Areas
- pr_agent/tools/pr_update_changelog.py[86-88]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Restricted mode permissions undocumented ✓ Resolved 📎 Requirement gap ⚙ Maintainability
Description
The new Restricted Mode docs instruct setting config.restricted_mode = true but do not explicitly
state the minimal GitHub permissions required in restricted mode (e.g., whether contents should be
omitted/none/read). This leaves permissions requirements ambiguous and undermines least-privilege
setup guidance.
Code

docs/docs/installation/github.md[470]

+  If you cannot grant `contents: write`, set `config.restricted_mode = true` to gracefully skip operations that need elevated access (e.g., pushing changelog changes). See the [Restricted Mode guide](../usage-guide/additional_configurations.md#restricted-mode) for details.
Evidence
PR Compliance IDs 6 and 8 require restricted mode to be usable without repository contents write
access and to clearly document the minimal permissions (especially contents) for restricted mode.
The updated docs mention setting config.restricted_mode = true if contents: write cannot be
granted, but do not specify the exact restricted-mode permissions or whether contents can be
omitted/none/read.

Provide a restricted (read-only) mode that does not require write access to repository contents
Document GitHub App permissions used and clarify whether 'contents' permission is required
docs/docs/installation/github.md[462-470]
docs/docs/usage-guide/additional_configurations.md[284-293]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Restricted mode documentation mentions skipping operations that need `contents: write`, but it does not explicitly list the minimal GitHub permissions for restricted mode and whether `contents` is required at all (none/read/write).
## Issue Context
Compliance requires documentation to clearly state the GitHub App/workflow permissions for both normal mode and restricted mode, especially clarifying the `contents` permission.
## Fix Focus Areas
- docs/docs/installation/github.md[462-470]
- docs/docs/usage-guide/additional_configurations.md[284-293]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

4. Misleading commit hint ✓ Resolved 🐞 Bug ≡ Correctness
Description
When push_changelog_changes=true but pushing is skipped (push_skipped_reason set),
commit_changelog becomes False and _prepare_changelog_update() appends instructions to rerun
with push_changelog_changes=true even though that will not enable pushing. This produces
contradictory guidance in the comment output and can send users into a loop.
Code

pr_agent/tools/pr_update_changelog.py[R32-40]

+        self.push_changelog_changes = get_settings().pr_update_changelog.push_changelog_changes
+        self.push_skipped_reason = None
+        if self.push_changelog_changes:
+            if not hasattr(self.git_provider, "create_or_update_pr_file"):
+                self.push_skipped_reason = "not supported for this git provider"
+            elif not self.git_provider.is_supported("push_code"):
+                self.push_skipped_reason = "restricted by configuration (restricted_mode)"
+        # Push only when it was requested AND is possible; otherwise fall back to a comment.
+        self.commit_changelog = self.push_changelog_changes and self.push_skipped_reason is None
Evidence
The new initialization logic can set commit_changelog to False even when
push_changelog_changes is True (because push_skipped_reason is set).
_prepare_changelog_update() unconditionally appends a rerun-with-push-enabled instruction whenever
commit_changelog is False, which is misleading in the skipped-push scenario introduced by this
PR.

pr_agent/tools/pr_update_changelog.py[27-41]
pr_agent/tools/pr_update_changelog.py[151-154]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`PRUpdateChangelog` now sets `commit_changelog` to `False` not only when push wasn’t requested, but also when push *was requested* and then skipped due to `restricted_mode` or missing push support. `_prepare_changelog_update()` currently treats all `commit_changelog == False` cases the same and always appends a “rerun with push_changelog_changes=true” instruction, which is misleading when pushing is impossible.
### Issue Context
- `commit_changelog` is derived from both the user request and push feasibility.
- `_prepare_changelog_update()` appends the rerun instruction solely based on `not self.commit_changelog`.
### How to fix
- Change `_prepare_changelog_update()` to append the “rerun to commit” instruction **only when** `self.push_changelog_changes` is `False` (i.e., push wasn’t requested).
- If `self.push_skipped_reason` is set, replace that instruction with a message explaining what needs to change (e.g., disable `restricted_mode` / grant permissions / use a provider that supports pushing), or omit the instruction entirely since the comment already includes a “not pushed” note.
### Fix Focus Areas
- pr_agent/tools/pr_update_changelog.py[22-41]
- pr_agent/tools/pr_update_changelog.py[139-155]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


5. Skip path drops changelog output ✓ Resolved 🐞 Bug ≡ Correctness
Description
When _skip_push is set (restricted/unsupported push), PRUpdateChangelog.run() returns before
generating the changelog entry, so users only get a “pushing is …” message and no proposed changelog
content. The tool already has a non-push flow (publish updates as a comment when commit_changelog
is false), but the skip path never reaches it.
Code

pr_agent/tools/pr_update_changelog.py[R85-92]

+        if self._skip_push:
+            reason = "not supported" if self._skip_push == "not supported" else "restricted by configuration"
+            get_logger().error(f"Pushing changelog changes is {reason}")
       if get_settings().config.publish_output:
           self.git_provider.publish_comment(
-                    "Pushing changelog changes is not currently supported for this code platform"
+                    f"Pushing changelog changes is {reason}"
           )
       return
Evidence
The run method returns early whenever _skip_push is set, so prediction generation and
_prepare_changelog_update() are never executed. The same method already contains a branch that
publishes the generated answer as a comment when commit_changelog is false, demonstrating an
existing non-push fallback that the skip path currently bypasses.

pr_agent/tools/pr_update_changelog.py[27-92]
pr_agent/tools/pr_update_changelog.py[94-113]
docs/docs/tools/update_changelog.md[16-23]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
When pushing changelog changes is blocked (provider doesn’t support it, or restricted mode disables `push_code`), `PRUpdateChangelog.run()` exits immediately and never produces the changelog update content. This loses useful output even though generating/publishing the proposed changelog entry does not require a repository write.
### Issue Context
The tool already supports a non-push workflow (`commit_changelog == False`) that publishes the generated changelog updates as a PR comment.
### Fix Focus Areas
- pr_agent/tools/pr_update_changelog.py[27-49]
- pr_agent/tools/pr_update_changelog.py[81-112]
### Implementation notes
- For `_skip_push == "restricted"` (and potentially also for "not supported"), set `self.commit_changelog = False` and continue the normal generation path.
- Publish a clear note alongside the generated changelog output indicating that pushing was skipped due to restriction/unsupported provider.
- Keep the early-return only for cases where even generation can’t proceed (if any), otherwise prefer fallback-to-comment behavior.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


6. Skip path still initializes 🐞 Bug ➹ Performance
Description
When _skip_push is set (restricted/unsupported push), run() exits early but __init__() still
instantiates the AI handler and TokenHandler and prepares prompt variables. This adds avoidable
startup cost on the very path that is supposed to fast-fail/skip work.
Code

pr_agent/tools/pr_update_changelog.py[R27-48]

+        # Skip expensive provider reads when push is disabled or restricted
+        self.push_changelog_changes = get_settings().pr_update_changelog.push_changelog_changes
+        if self.push_changelog_changes:
+            if not hasattr(self.git_provider, "create_or_update_pr_file"):
+                self._skip_push = "not supported"
+            elif not self.git_provider.is_supported("push_code"):
+                self._skip_push = "restricted"
+            else:
+                self._skip_push = None
+        else:
+            self._skip_push = None
+
+        if self._skip_push:
+            self.main_language = None
+            self.changelog_file_str = ""
+        else:
+            self.main_language = get_main_pr_language(
+                self.git_provider.get_languages(), self.git_provider.get_files()
+            )
+            self._get_changelog_file()  # self.changelog_file_str
+
+        self.commit_changelog = self.push_changelog_changes
Evidence
run() explicitly returns early when _skip_push is set, but __init__() still unconditionally
creates the AI handler and TokenHandler, which performs prompt rendering/tokenization work during
construction.

pr_agent/tools/pr_update_changelog.py[27-84]
pr_agent/algo/token_handler.py[59-73]
pr_agent/algo/ai_handlers/litellm_ai_handler.py[37-124]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`PRUpdateChangelog.__init__()` computes `_skip_push` early, and `run()` returns immediately when `_skip_push` is set, but initialization still eagerly constructs the AI handler and `TokenHandler` (including prompt rendering/tokenization). This undermines the goal of making the restricted/unsupported push path cheap.
### Issue Context
The skip decision is known during `__init__()`. If `_skip_push` is set, the instance can be kept minimal and avoid initializing AI/token machinery altogether.
### Fix Focus Areas
- pr_agent/tools/pr_update_changelog.py[23-72]
### Suggested fix
- If `_skip_push` is truthy, avoid creating `self.ai_handler`, `self.vars`, and `self.token_handler` (or create only what is needed to publish the skip comment).
- Alternatively, move AI/token initialization to the non-skip branch (or lazily initialize right before `_prepare_prediction()` is called).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


View more (1)
7. contents default note misleading 📎 Requirement gap ⛨ Security
Description
The Restricted Mode docs claim that omitting contents "defaults to read", which can mislead users
configuring least-privilege permissions. This undermines the requirement to clearly document
restricted-mode permission scope.
Code

docs/docs/usage-guide/additional_configurations.md[R295-300]

+```yaml
+permissions:
+  issues: write
+  pull-requests: write
+  contents: read   # or just omit contents (defaults to read)
+```
Evidence
PR Compliance ID 7 requires that the restricted profile’s permissions and scope are clearly
documented. The new Restricted Mode section includes a potentially misleading statement about what
happens when contents is omitted.

Ensure restricted mode allows writing only to Pull Requests (PR-scoped writes) while keeping repository contents access read-only or none
docs/docs/usage-guide/additional_configurations.md[295-300]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Restricted-mode documentation states `contents: read  # or just omit contents (defaults to read)`, which is ambiguous/misleading for users trying to set least-privilege permissions.
## Issue Context
Restricted mode is meant to run with minimal permissions; documentation must clearly and correctly state what permissions are granted/required.
## Fix Focus Areas
- docs/docs/usage-guide/additional_configurations.md[295-300]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

  • Author self-review: I have reviewed the code review findings, and addressed the relevant ones.

Qodo Logo

@naorpeled

Copy link
Copy Markdown
Member

Hey @utsab345,
Can you please update the documentation?

Thanks!

@utsab345

utsab345 commented Jul 3, 2026

Copy link
Copy Markdown
Contributor Author

docs updated in additional_configurations.md (new Restricted Mode section) and github.md (note in Permission denied troubleshooting).

@naorpeled let me know if you'd like any changes!

Comment thread docs/docs/installation/github.md Outdated
@qodo-free-for-open-source-projects

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 37874ec

@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Code Review by Qodo

Grey Divider

New Review Started

This review has been superseded by a new analysis

Grey Divider

Qodo Logo

…ider reads

PRUpdateChangelog.__init__() was fetching languages, files, and
CHANGELOG.md content before run() checked push_code support.
In restricted mode, these reads were wasted and could fail if
contents access is also restricted.

Move the capability check into __init__() before the expensive
provider calls, and store the result in self._skip_push so
run() can return early.
@utsab345

utsab345 commented Jul 3, 2026

Copy link
Copy Markdown
Contributor Author

Fixed both Qodo findings:

  • Permissions in docs now show the exact minimal workflow permissions for restricted mode
  • Skip check moved to __init__() so expensive provider reads (languages, files, CHANGELOG.md) don't run when the tool will be skipped

Comment thread pr_agent/tools/pr_update_changelog.py Outdated
@qodo-free-for-open-source-projects

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 211a258

… test

- Remove relevant_configs logging that dumps full settings dicts
  (potential secret leak in logs)
- Pass ai_handler mock in test_run_without_push_support to avoid
  instantiating a real LiteLLMAIHandler
Comment thread tests/unittest/test_pr_update_changelog.py Outdated
@qodo-free-for-open-source-projects

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 887289c

- Use MagicMock(spec=[...]) in test_run_without_push_support so
  hasattr() correctly returns False for missing attributes instead
  of brittle delattr on bare MagicMock
- Remove misleading 'contents: read (defaults to read)' comment
  from restricted mode docs; contents is simply not needed
@qodo-free-for-open-source-projects

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 3170375

@naorpeled

Copy link
Copy Markdown
Member

Hey @utsab345,
Can you please address the leftover Qodo comments?

Thanks in adance!

Move all provider-dependent initialization (vars dict, token handler,
ai_handler binding, language/file/changelog reads) behind the _skip_push
guard so the skip path avoids hitting the git provider at all during
construction.
@qodo-free-for-open-source-projects

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 8b11aa2

naorpeled
naorpeled previously approved these changes Jul 3, 2026

@naorpeled naorpeled left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — finalized. 🔥

I reviewed the logic end to end and it's clean and correctly scoped:

  • push_code is queried in exactly one place (pr_update_changelog), so no other tool is affected by restricted mode.
  • Coverage is complete, not partial: create_or_update_pr_file is defined in precisely the three providers you added the restricted_mode check to (GitHub, GitLab, Bitbucket). Any other provider lacks that method and already falls into the "not supported" path, so there's no silent gap.
  • The __init__ early-return skip path only sets attributes that run()'s skip branch touches, and run() returns before reaching anything unset — no AttributeError risk.

Qodo's three findings are already handled in the current commits: the config-dict debug logging was removed from run(), the brittle delattr test was rewritten with a proper MagicMock(spec=[...]), and the docs state the minimal permissions (issues: write + pull-requests: write).

Verified locally: test_pr_update_changelog.py14 passed. No Python lines exceed 120 chars (the long lines are docs/TOML prose, not linted).

Nice, minimal implementation of a genuinely useful capability. Thanks @utsab345!

@utsab345

utsab345 commented Jul 3, 2026

Copy link
Copy Markdown
Contributor Author

LGTM — finalized. 🔥

I reviewed the logic end to end and it's clean and correctly scoped:

  • push_code is queried in exactly one place (pr_update_changelog), so no other tool is affected by restricted mode.
  • Coverage is complete, not partial: create_or_update_pr_file is defined in precisely the three providers you added the restricted_mode check to (GitHub, GitLab, Bitbucket). Any other provider lacks that method and already falls into the "not supported" path, so there's no silent gap.
  • The __init__ early-return skip path only sets attributes that run()'s skip branch touches, and run() returns before reaching anything unset — no AttributeError risk.

Qodo's three findings are already handled in the current commits: the config-dict debug logging was removed from run(), the brittle delattr test was rewritten with a proper MagicMock(spec=[...]), and the docs state the minimal permissions (issues: write + pull-requests: write).

Verified locally: test_pr_update_changelog.py14 passed. No Python lines exceed 120 chars (the long lines are docs/TOML prose, not linted).

Nice, minimal implementation of a genuinely useful capability. Thanks @utsab345!

Thanks for the thorough review and verification! Really appreciate the LGTM and the detailed feedback. 🚀

@naorpeled naorpeled dismissed their stale review July 3, 2026 09:14

Retracting this — I approved prematurely. There are still-open Qodo findings (skip path drops changelog output, skip path over-initializes, and a misleading contents-permission note) that should be addressed first.

@naorpeled

Copy link
Copy Markdown
Member

@utsab345 pushing some small fixes and then merging, thanks for this!

naorpeled and others added 2 commits July 3, 2026 12:19
…in restricted mode

Address Qodo review on The-PR-Agent#2491:

- The-PR-Agent#4 (skip path drops changelog output): when a changelog push is requested but not
  possible — provider lacks push support, or restricted_mode disables push_code — the
  tool no longer returns early with only an error. It now still generates the changelog
  and publishes it as a comment (which only needs pull-requests: write), with a note that
  the changes were not pushed. commit_changelog is set to False in that case.
- The-PR-Agent#5 (skip path still initializes): removes the special early-return init path; the normal
  flow builds only what's needed to generate the changelog.
- The-PR-Agent#6 (contents default note misleading): clarify that unlisted scopes default to none only
  within an explicit permissions: block; without one, defaults follow repo/org settings.

Update the "no push support" test to assert the comment fallback, and add a restricted_mode
test (push API present but is_supported('push_code') False) asserting it comments, not pushes.
…estricted

When restricted_mode is active and push_changelog_changes=true, the
tool now generates the changelog output via AI and publishes it as a
comment with a '(push restricted by configuration)' label, instead of
returning silently. This ensures users see the changelog even when
they don't have contents: write permissions.

For the 'not supported' case (provider lacks create_or_update_pr_file
entirely), the existing early-return behavior is preserved.
@qodo-free-for-open-source-projects

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 0c99e1c

@qodo-free-for-open-source-projects

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 7bf6093

@naorpeled naorpeled merged commit cc53c74 into The-PR-Agent:main Jul 3, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support restricted mode with less github permissions: no write access to code

2 participants