feat: Frozen plugin install/update routes through the ADR 0093 wheel installer instead of refusing#2235
Conversation
|
Warning Review limit reached
Next review available in: 54 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
WalkthroughFrozen desktop plugin installation now installs missing hard dependencies through the managed Python runtime, with coverage for runtime availability and installation failures. Documentation, navigation, ADR statuses, release guidance, configuration references, and roadmap content are updated. ChangesFrozen plugin runtime
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant PluginInstaller as installer.install
participant ManagedRuntime as managed Python runtime
participant PluginStore as plugin directory
PluginInstaller->>ManagedRuntime: Install missing hard dependencies
ManagedRuntime-->>PluginInstaller: Return success or PythonInstallError
PluginInstaller->>PluginStore: Land plugin when dependency installation succeeds
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing Touches🧪 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 |
![protoreview[bot]](https://avatars.githubusercontent.com/in/4233969?s=60&v=4){kind=small}
There was a problem hiding this comment.
QA panel review — FAIL
code-review-structural · head 07d1a49d0a2c · formal
[review-synthesizer completed: workflow code-review-structural:report]
Brief
Overall risk: Low production risk — this is a test-only defect. The implementation code is correct; the new tests are simply not exercising the intended code paths due to a Python import-system gotcha. The PR's functional behavior is unaffected, but the test coverage claims are invalid until the mocks are corrected.
Fix-first item: Redirect the monkeypatches to patch the locally-bound names (infra.python_runtime.managed_python_exe → the local managed_python_exe in the implementation module, and similarly for install_requirements_into_managed_runtime). The cleanest fix is to patch infra.python_runtime.managed_python_exe and runtime.python_install.install_requirements_into_managed_runtime on the modules where they're imported into, not on the source modules themselves.
Panel disagreements: Single finding from the cross-file structural pass; no LLM finders raised this. No disagreement — the verifier confirmed it.
Verification changes: The verifier confirmed the finding with detailed reasoning about why the patch-on-wrong-object defect is real and blocks all five affected tests. No findings were refuted or downgraded.
Gaps: The structural pass was explicitly skipped this round (per task instructions), so this finding came solely from the cross-file finder. No other panel members raised issues with this PR.
[
{
"file": "tests/test_plugin_installer.py",
"line": 337,
"severity": "blocker",
"category": "tests",
"claim": "Tests monkeypatch `infra.python_runtime.managed_python_exe` and `runtime.python_install.install_requirements_into_managed_runtime` on the source modules, but `_frozen_install_missing_deps` imports both names directly (`from infra.python_runtime import managed_python_exe` / `from runtime.python_install import … install_requirements_into_managed_runtime`), so the monkeypatches never reach the call sites and the tests assert against behavior that never executes.",
"evidence": "Diff hunk adding the three tests in `tests/test_plugin_installer.py` lines 337–389: each test does `import infra.python_runtime as pr` / `import runtime.python_install as pi` followed by `monkeypatch.setattr(pr, 'managed_python_exe', …)` / `monkeypatch.setattr(pi, 'install_requirements_into_managed_runtime', …)`. The implementation hunk for `_frozen_install_missing_deps` shows the local imports `from infra.python_runtime import managed_python_exe` and `from runtime.python_install import PythonRuntimeError, install_requirements_into_managed_runtime`, and the call site `if managed_python_exe() is None:` / `install_requirements_into_managed_runtime(to_install)` resolves against those local names, not against `pr.managed_python_exe` / `pi.install_requirements_into_managed_runtime`. Same pattern in `test_plugin_installer_archive.py` lines 154–156 and 210–212 where the archive-path tests patch `pr.managed_python_exe` to `None` to drive the 'runtime absent' branch — the patch is on the wrong object.",
"source": "cross-file finder",
"verdict": "confirmed",
"note": "Re-read the diff: `_frozen_install_missing_deps` uses `from infra.python_runtime import managed_python_exe` and `from runtime.python_install import … install_requirements_into_managed_runtime` (local bindings captured at function-entry import time). All three new tests in `tests/test_plugin_installer.py` (lines 337–389) and both archive tests (`test_plugin_installer_archive.py` lines 154–156, 210–212) patch `pr.managed_python_exe` and/or `pi.install_requirements_into_managed_runtime` on the module objects — these changes never reach the locally-bound names. The patch-on-wrong-object defect is real and blocks all five tests."
}
]
|
Operator adjudication of the FAIL (operator-ruled — the brain's /v1 is dropping turns right now, and the finding is unambiguous): fix-now. The from-imports defeat the tests' monkeypatches (local bindings never see patches on the source modules); switch to the codebase's own lazy convention ( |
…installer instead of refusing
…le attributes Lazy module imports (import runtime.python_install as pi / import infra.python_runtime as pr) inside the helper so test monkeypatches on the source modules bind — from-imports snapshot the callables and defeat the patches. Matches the install_deps test pattern. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@docs/adr/0058-runtime-plugin-install-frozen-app.md`:
- Line 3: Update the accepted ADR summaries to reflect managed-runtime
dependency installation: in docs/adr/0058-runtime-plugin-install-frozen-app.md
at line 3, revise D2 and its consequences to reference ADR 0093 installation and
refusal when the runtime is unavailable or installation fails; in
docs/adr/0059-unified-plugin-manager.md at line 3, qualify the claim that
one-click installation succeeds on every surface; and in docs/adr/index.md at
line 69, replace the requires_pip ⊆ bundled summary with the current conditional
installation model.
In `@docs/guides/releasing.md`:
- Around line 124-126: Update the changelog-check workflow’s PR_ACTOR value to
use github.event.pull_request.user.login rather than github.actor, so the
Dependabot exemption identifies the PR author across reruns and initial runs.
Update the corresponding releasing guide text to describe Dependabot-authored
PRs as exempt, while preserving the existing skip-changelog and release/* escape
hatches.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 239acd8f-9361-4fcc-8e6d-e24a14f66158
⛔ Files ignored due to path filters (4)
CHANGELOG.mdis excluded by!*.mdREADME.mdis excluded by!*.mdROADMAP.mdis excluded by!*.mdTEMPLATE.mdis excluded by!*.md
📒 Files selected for processing (24)
apps/desktop/README.mddocs/.vitepress/config.mtsdocs/adr/0057-command-palette.mddocs/adr/0058-runtime-plugin-install-frozen-app.mddocs/adr/0059-unified-plugin-manager.mddocs/adr/0068-developer-flags-and-panel.mddocs/adr/0076-managed-git-acp-delegates.mddocs/adr/0077-adversarial-code-review-workflow.mddocs/adr/index.mddocs/guides/building-react-plugin-views.mddocs/guides/fleet.mddocs/guides/index.mddocs/guides/plugins.mddocs/guides/python-runtime.mddocs/guides/react-tauri-ui.mddocs/guides/releasing.mddocs/index.mddocs/plans/coding-agent-deterministic-git.mddocs/reference/configuration.mdgraph/plugins/installer.pyplugins/docs/nav.jsonsites/marketing/data/roadmap.jsontests/test_plugin_installer.pytests/test_plugin_installer_archive.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@docs/adr/0058-runtime-plugin-install-frozen-app.md`:
- Line 3: Update the accepted ADR summaries to reflect managed-runtime
dependency installation: in docs/adr/0058-runtime-plugin-install-frozen-app.md
at line 3, revise D2 and its consequences to reference ADR 0093 installation and
refusal when the runtime is unavailable or installation fails; in
docs/adr/0059-unified-plugin-manager.md at line 3, qualify the claim that
one-click installation succeeds on every surface; and in docs/adr/index.md at
line 69, replace the requires_pip ⊆ bundled summary with the current conditional
installation model.
In `@docs/guides/releasing.md`:
- Around line 124-126: Update the changelog-check workflow’s PR_ACTOR value to
use github.event.pull_request.user.login rather than github.actor, so the
Dependabot exemption identifies the PR author across reruns and initial runs.
Update the corresponding releasing guide text to describe Dependabot-authored
PRs as exempt, while preserving the existing skip-changelog and release/* escape
hatches.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 239acd8f-9361-4fcc-8e6d-e24a14f66158
⛔ Files ignored due to path filters (4)
CHANGELOG.mdis excluded by!*.mdREADME.mdis excluded by!*.mdROADMAP.mdis excluded by!*.mdTEMPLATE.mdis excluded by!*.md
📒 Files selected for processing (24)
apps/desktop/README.mddocs/.vitepress/config.mtsdocs/adr/0057-command-palette.mddocs/adr/0058-runtime-plugin-install-frozen-app.mddocs/adr/0059-unified-plugin-manager.mddocs/adr/0068-developer-flags-and-panel.mddocs/adr/0076-managed-git-acp-delegates.mddocs/adr/0077-adversarial-code-review-workflow.mddocs/adr/index.mddocs/guides/building-react-plugin-views.mddocs/guides/fleet.mddocs/guides/index.mddocs/guides/plugins.mddocs/guides/python-runtime.mddocs/guides/react-tauri-ui.mddocs/guides/releasing.mddocs/index.mddocs/plans/coding-agent-deterministic-git.mddocs/reference/configuration.mdgraph/plugins/installer.pyplugins/docs/nav.jsonsites/marketing/data/roadmap.jsontests/test_plugin_installer.pytests/test_plugin_installer_archive.py
🛑 Comments failed to post (2)
docs/adr/0058-runtime-plugin-install-frozen-app.md (1)
3-3: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Update all accepted ADR summaries to reflect managed-runtime dependency installation.
The accepted documentation still describes frozen plugins as refusing missing dependencies or requires every dependency to be bundled, while this PR adds an eligible managed-runtime wheel-installation path.
docs/adr/0058-runtime-plugin-install-frozen-app.md#L3-L3: revise D2 and consequences to describe ADR 0093 installation and its runtime-unavailable/install-failure refusal cases.docs/adr/0059-unified-plugin-manager.md#L3-L3: qualify the claim that one-click installation succeeds on every surface.docs/adr/index.md#L69-L69: replace therequires_pip ⊆ bundledsummary with the current conditional installation model.📍 Affects 3 files
docs/adr/0058-runtime-plugin-install-frozen-app.md#L3-L3(this comment)docs/adr/0059-unified-plugin-manager.md#L3-L3docs/adr/index.md#L69-L69🤖 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 `@docs/adr/0058-runtime-plugin-install-frozen-app.md` at line 3, Update the accepted ADR summaries to reflect managed-runtime dependency installation: in docs/adr/0058-runtime-plugin-install-frozen-app.md at line 3, revise D2 and its consequences to reference ADR 0093 installation and refusal when the runtime is unavailable or installation fails; in docs/adr/0059-unified-plugin-manager.md at line 3, qualify the claim that one-click installation succeeds on every surface; and in docs/adr/index.md at line 69, replace the requires_pip ⊆ bundled summary with the current conditional installation model.docs/guides/releasing.md (1)
124-126: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "Repository files matching relevant names:" git ls-files | rg '(^\.github/workflows/checks\.yml$|^docs/guides/releasing\.md$|CHANGELOG\.md$)' || true echo echo "checks.yml relevant sections:" if [ -f .github/workflows/checks.yml ]; then nl -ba .github/workflows/checks.yml | sed -n '1,220p' fi echo echo "releasing.md relevant section:" if [ -f docs/guides/releasing.md ]; then nl -ba docs/guides/releasing.md | sed -n '110,135p' fiRepository: protoLabsAI/protoAgent
Length of output: 341
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "checks.yml PR_ACTOR occurrences and surrounding workflow:" grep -n -C 8 'PR_ACTOR\|pull_request.*ACTOR\|pull_actor' .github/workflows/checks.yml echo echo "checks.yml workflow triggers relevant:" awk '/^name:/,/^jobs:/{print NR": "$0}' .github/workflows/checks.yml echo echo "releasing.md relevant section:" awk 'NR>=110 && NR<=135{printf "%3d: %s\n", NR, $0}' docs/guides/releasing.md echo echo "CHANGELOG entry check implementation:" grep -n -C 12 'Changelog|changelog|CHANGELOG|skip-changelog|pull_act' .github/workflows/checks.ymlRepository: protoLabsAI/protoAgent
Length of output: 3269
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "Find changelog gate script:" git ls-files | grep 'scripts/changelog_gate' || true echo echo "Read changelog gate script:" if [ -f scripts/changelog_gate.sh ]; then cat -n scripts/changelog_gate.sh fi echo echo "All occurrences of PR_ACTOR/PR_HEAD_REF/dep/dependabot/skip in workflow/scripts:" grep -Rn -E 'PR_(ACTOR|HEAD_REF)|dependabot|skip-changelog|changelog' .github scripts docs || trueRepository: protoLabsAI/protoAgent
Length of output: 11089
🌐 Web query:
GitHub Actions github.actor pull_request rerun by human github actor depends_on user login💡 Result:
In GitHub Actions, the value of github.actor remains the username of the user who triggered the initial workflow run, even if the workflow is re-run [1][2][3]. When a workflow is re-run, GitHub Actions uses the privileges of this original actor for the execution of the workflow [4][1][5]. To identify the specific user who initiated a re-run, you should use the github.triggering_actor context [1][6]. This context provides the username of the user that initiated the current workflow run [2][7]. While github.actor and github.triggering_actor may be the same for the initial run, they will differ if a different user triggers a re-run of that workflow [1][6]. In summary: - github.actor: The user who triggered the initial workflow run. Privileges for the run are always derived from this actor [1][5]. - github.triggering_actor: The user who triggered the current execution (or re-run) of the workflow [2][5].
Citations:
- 1: https://github.com/github/docs/blob/main/content/actions/reference/workflows-and-actions/contexts.md
- 2: https://docs.github.com/en/actions/reference/workflows-and-actions/variables
- 3: https://docs.github.com/en/enterprise-server@3.19/actions/reference/workflows-and-actions/contexts
- 4: https://docs.github.com/en/actions/how-tos/manage-workflow-runs/re-run-workflows-and-jobs
- 5: https://github.blog/changelog/2022-07-19-differentiating-triggering-actor-from-executing-actor/
- 6: https://docs.github.com/en/actions/reference/workflows-and-actions/contexts
- 7: https://docs.rs/github-actions/latest/github_actions/constant.GITHUB_TRIGGERING_ACTOR.html
Use the PR author for the Dependabot changelog escape hatch.
.github/workflows/checks.ymlpasses$\{{ github.actor }}asPR_ACTOR, butPR_ACTO Ris only compared againstdependabot[bot]. A Dependabot PR rerun/initial run can have a humangithub.actor, making a Dependabot-owned PR hit the changelog gate unless it also has a label or release branch. Pass$\{{ github.event.pull_request.user.login }}instead and update the docs to say Dependabot-authored PRs are exempt.🤖 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 `@docs/guides/releasing.md` around lines 124 - 126, Update the changelog-check workflow’s PR_ACTOR value to use github.event.pull_request.user.login rather than github.actor, so the Dependabot exemption identifies the PR author across reruns and initial runs. Update the corresponding releasing guide text to describe Dependabot-authored PRs as exempt, while preserving the existing skip-changelog and release/* escape hatches.
There was a problem hiding this comment.
QA panel review — PASS
code-review-structural · head e4b4b7e475e6 · formal
[review-synthesizer completed: workflow code-review-structural:report]
Brief
Overall risk: Low. PR #2235 contains only documentation updates (CHANGELOG, README, ADR status, VitePress sidebar, templates) with no behavioral code changes.
Fix-first item: None — the findings array is empty.
Panel disagreement: N/A — no findings were submitted for this round.
Verification changes: The verifier confirmed the findings array is empty. The prior request from round 1 referenced a monkeypatching issue in tests/test_plugin_installer.py:337, but this finding is not present in the current findings array to verify. Since the PR contains only documentation updates with no code changes, this monkeypatching issue (if it exists) would be in the existing test code, not in this PR's diff.
Gaps:
- The prior monkeypatch finding from round 1 is not carried forward into this verification pass. If this remains a concern, it should be re-evaluated against the actual test file in a subsequent round.
- No structural pass was performed (as noted by the verifier).
[]This PASS does not lift the standing block. An earlier round of this same panel confirmed a blocker finding that this round neither reports nor explains:
tests/test_plugin_installer.py:337— Tests monkeypatchinfra.python_runtime.managed_python_exeandruntime.python_install.install_requirements_into_managed_runtimeon the source modules, but_frozen_install_missing_depsimports both names directly (from infra.python_runtime import managed_python_exe/from runtime.python_install import … install_requirements_into_managed_runtime), so the monkeypatches never reach the call s
A finding that disappears without being fixed, carried, or refuted is unproven, not resolved — and a clean PASS is exactly the verdict that would clear the merge path (issue #26). Either the fix landed (say so, and the next review will corroborate and lift), or the panel missed it on this draw. A second consecutive clean PASS lifts the block automatically; an operator can also dismiss this review directly.
There was a problem hiding this comment.
QA panel review — PASS
code-review-structural · head c61543249808 · formal
[review-synthesizer completed: workflow code-review-structural:report]
Prose Brief
This is a no-op review: PR #2235 contains no file changes, and the review infrastructure returned an empty findings array. The structural finder's gateway errors from round 3 likely prevented the review from executing. The prior blocker from round 1 (tests/test_plugin_installer.py:337 — monkeypatch targets imported directly, preventing test isolation) was not addressed because no code changed. The panel should investigate whether the review infrastructure actually ran before accepting an empty result. Risk: the standing block from round 1 remains in effect but was never formally lifted.
[{"prior": "tests/test_plugin_installer.py:337", "disposition": "open", "why": "PR diff is empty — no code changed, so the monkeypatch import issue was not fixed"}][]
Superseded — a later head cleared the QA panel (see the newest verdict).
1 participant
Summary
Frozen plugin install/update routes through the ADR 0093 wheel installer instead of refusing (
bd-3l6)See the diff for details.
Fixes #2226
Summary by CodeRabbit
New Features
Documentation