[feature] Mass command asynchronous execution pipeline#1395
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces a batch command execution system by adding AbstractBatchCommand to manage organization-scoped command execution across groups of devices, with targeting via manual selection, device group, or location. Links individual Command rows to their parent batch via a nullable batch_command foreign key on AbstractCommand. Implements device resolution, command generation, and status aggregation logic in the batch model. Provides a concrete, swappable BatchCommand model and a database migration. Adds a Celery task launch_batch_command to fetch and execute batch-scheduled tasks. Exposes API serializers for batch creation with organization and device consistency validation, a BatchCommandExecuteView with POST (create) and GET (dry-run/preview) endpoints, and a new /api/v1/controller/batch-command/execute/ route. Updates GitHub Actions CI to include gsoc26-* branch wildcard and adjust Coveralls flag naming. Sequence Diagram(s)sequenceDiagram
participant Admin
participant API
participant DB
participant CeleryWorker
Admin->>API: POST /batch-command/execute/
API->>DB: validate and save BatchCommand
API->>CeleryWorker: transaction.on_commit enqueues launch_batch_command(batch_id)
API-->>Admin: HTTP 201 with batch_id
CeleryWorker->>DB: fetch BatchCommand by batch_id
CeleryWorker->>DB: BatchCommand.create_commands() creates per-device Command rows
CeleryWorker->>DB: each Command.save() triggers BatchCommand.calculate_and_update_status()
DB->>DB: batch status aggregated from child Command statuses
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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 |
CI Failures: Black Formatting and System ChecksHello @dee077,
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
4-16: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd workflow concurrency to prevent duplicate CI runs.
Line 4 defines triggers, but there is no
concurrencyguard. Rapid pushes to the same PR/branch can run overlapping matrices and waste runners.Suggested patch
on: push: branches: - master - "gsoc26-*" pull_request: branches: - master - "1.1" - "1.2" - "gsoc26-*" + +concurrency: + group: ci-${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }} + cancel-in-progress: true🤖 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 @.github/workflows/ci.yml around lines 4 - 16, Add a top-level concurrency stanza to the workflow (next to the existing on: triggers) to prevent duplicate CI runs: add a concurrency key with a group expression that uniquely identifies the branch/PR (e.g. using github.workflow plus github.ref or github.event.pull_request.number when available) and set cancel-in-progress: true so newer pushes cancel older runs; update the workflow around the existing on: block to include this concurrency configuration.Source: Linters/SAST tools
🤖 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 `@openwisp_controller/connection/base/models.py`:
- Around line 817-832: The launch() method sets status="in-progress" and saves
even when resolve_devices() yields no devices, which leaves batches stuck
because no CommandOperation rows will trigger calculate_and_update_status();
update launch() to detect an empty devices queryset: after resolving devices
(resolve_devices()) and before creating CommandOperation instances, if there are
zero devices set status to a terminal state (e.g., "completed" or "no-devices"),
set total_devices=0, persist those fields via save(update_fields=[...]) and
return early so no bulk_create is attempted; keep references to
CommandOperation, total_devices, and calculate_and_update_status() unchanged.
- Around line 822-830: The loop builds a potentially huge in-memory list `ops`
before calling `CommandOperation.objects.bulk_create(ops)`, causing memory
spikes; modify the code that iterates `devices.iterator()` to create and
validate each `CommandOperation` (call `full_clean()`), accumulate them in a
small buffer, and call `CommandOperation.objects.bulk_create(buffer,
batch_size=...)` whenever the buffer reaches a chosen chunk size (then clear the
buffer) and after the loop flush any remaining items; keep references to the
same symbols (`CommandOperation`, `batch_command_operation`,
`devices.iterator()`, `full_clean`, `bulk_create`) so reviewers can find and
verify the chunking change.
- Around line 823-830: The batch launch loop in launch() only creates
CommandOperation rows via CommandOperation.objects.bulk_create(ops) leaving
their command field NULL and never invoking AbstractCommand._schedule_command,
so per-device work is never enqueued; fix by, after creating each
CommandOperation (or immediately after bulk_create), ensuring each operation
gets a scheduled command—either call the scheduling logic
(AbstractCommand._schedule_command or an equivalent method) for each
CommandOperation instance or create and assign the related Command objects and
enqueue them so operations transition out of in-progress; update the code
referencing devices.iterator(), ops, batch_command_operation and
CommandOperation to create/assign commands and invoke the scheduler for every op
so the per-device work is enqueued and can complete.
- Around line 779-805: AbstractBatchCommand.clean currently only checks org
ownership for group and location but neglects to enforce the
organization-specific command allowlist and to validate command_input against
the command schema, allowing invalid or disabled device commands to be stored;
update AbstractBatchCommand.clean to perform the same command validation that
AbstractCommand.clean does: verify that self.command is allowed for
self.organization (apply the same allowlist check), validate/deserialize
self.command_input against the command's schema (raise ValidationError on schema
errors), and ensure disabled commands are rejected — reuse or call the existing
AbstractCommand validation helper/logic rather than duplicating ad-hoc checks so
errors are consistent.
- Line 835: The code uses the hardcoded reverse accessor
self.commandoperation_set in BatchCommandOperation.calculate_and_update_status
which breaks when CommandOperation is swapped; add an explicit related_name on
AbstractCommandOperation.batch_command_operation (e.g.,
related_name="batch_command_operations") and update
BatchCommandOperation.calculate_and_update_status to use that new accessor
(replace self.commandoperation_set with the chosen related_name) ensuring
compatibility with swapper.swappable_setting("connection", "CommandOperation").
---
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 4-16: Add a top-level concurrency stanza to the workflow (next to
the existing on: triggers) to prevent duplicate CI runs: add a concurrency key
with a group expression that uniquely identifies the branch/PR (e.g. using
github.workflow plus github.ref or github.event.pull_request.number when
available) and set cancel-in-progress: true so newer pushes cancel older runs;
update the workflow around the existing on: block to include this concurrency
configuration.
🪄 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
Run ID: 9644ffdf-bf70-4c83-809f-afee03e5b60c
📒 Files selected for processing (3)
.github/workflows/ci.ymlopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/models.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/connection/models.pyopenwisp_controller/connection/base/models.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/connection/models.pyopenwisp_controller/connection/base/models.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/connection/models.pyopenwisp_controller/connection/base/models.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_controller/connection/models.pyopenwisp_controller/connection/base/models.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Mark user-facing strings for translation with Django i18n helpers in Django code
Avoid unnecessary blank lines inside function and method bodies
Be careful with authentication, authorization, queryset filtering, serializers, admin behavior, cache invalidation, signals, Celery tasks, and websocket updates in Django code
Preserve validation around templates, VPN/PKI material, SSH credentials, device commands, uploaded files, URLs, and subnet/IP data
Write comments and docstrings only when they explain why code is shaped a certain way, placing them before the relevant code block instead of scattering them inside it
Files:
openwisp_controller/connection/models.pyopenwisp_controller/connection/base/models.py
🧠 Learnings (5)
📚 Learning: 2026-02-24T16:24:55.443Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1233
File: .github/workflows/backport.yml:22-22
Timestamp: 2026-02-24T16:24:55.443Z
Learning: In repositories within the OpenWISP organization, it is acceptable to reference reusable workflows from other OpenWISP-controlled repos using mutable refs (e.g., master) in .github/workflows. This is permissible due to the shared trust boundary within the organization. If applying this pattern, ensure the target repos are under the same organization and maintain awareness of potential breakages from upstream mutable refs; consider pinning to a tagged version for longer-term stability when appropriate.
Applied to files:
.github/workflows/ci.yml
📚 Learning: 2026-02-24T16:25:20.080Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1233
File: .github/workflows/backport.yml:35-35
Timestamp: 2026-02-24T16:25:20.080Z
Learning: In .github/workflows/backport.yml, enforce that backport-on-comment triggers only for users with author_association MEMBE R or OWNER (COLLABORATOR excluded), reflecting maintainer feedback. Update the trigger condition to check author_association and restrict to MEMBERS/OWNERS; document rationale and PR `#1233` reference in code comments.
Applied to files:
.github/workflows/ci.yml
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/connection/models.pyopenwisp_controller/connection/base/models.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/connection/models.pyopenwisp_controller/connection/base/models.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/connection/models.pyopenwisp_controller/connection/base/models.py
🪛 zizmor (1.25.2)
.github/workflows/ci.yml
[warning] 4-14: insufficient job-level concurrency limits (concurrency-limits): workflow is missing concurrency setting
(concurrency-limits)
🔇 Additional comments (1)
.github/workflows/ci.yml (1)
8-8: LGTM!Also applies to: 14-14, 103-103
Introduced AbstractBatchCommand model with calculate_and_update_status() and launch() methods to support batch command execution on multiple devices, following the pattern of BatchUpgradeOperation in openwisp-firmware-upgrader. Added batch_command FK to the existing Command model to link individual commands to their parent batch. Closes #1344
dee077
force-pushed
the
feature/1344-mass-command-execution-pipeline
branch
from
ac060dd to
cbe8ea6
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@openwisp_controller/connection/base/models.py`:
- Around line 844-845: The parent batch aggregation isn't recalculated after a
child command finishes; update the code so that whenever a child transitions out
of "in-progress" (i.e., at the end of AbstractCommand.execute() where the child
row is saved/finished) you explicitly refresh the parent by invoking its
calculate_and_update_status() (and save the parent if needed) so parent
counters/status reflect the new child state; apply the same change to the other
similar blocks referenced (around the 853-895 region) wherever child completion
only saves the child without touching the parent.
- Around line 738-741: The organization ForeignKey on the model is currently
required but must be nullable to support cross-organization batches; change the
organization field declaration (organization = models.ForeignKey(...)) to allow
null and blank (null=True, blank=True) and update any DB migration. Then update
resolve_devices (and any code that assumes organization is always present) to
load the Device model (Device = load_model("config", "Device")) and only filter
by organization when self.organization_id is truthy (e.g., if
self.organization_id: qs = qs.filter(organization=self.organization)); ensure
any callers/permissions that relied on a non-null organization handle the
nullable case safely.
- Around line 826-851: The launch path currently creates new Command records
every time launch() or launch_async() runs, causing duplicate commands; make it
idempotent by skipping devices that already have a Command tied to this batch.
In launch(), after resolving devices (resolve_devices()) load existing Commands
for this batch
(Command.objects.filter(batch_command=self).values_list('device_id', flat=True))
and only create Command instances for devices whose id is not in that set (or
use Command.objects.get_or_create(...) keyed by device + batch_command + type).
Update total_devices to reflect the count of devices with a Command for this
batch (existing + newly created) and continue to call
calculate_and_update_status(); ensure launch_async() remains unchanged except
for relying on the idempotent launch() behavior.
In `@openwisp_controller/connection/tasks.py`:
- Around line 104-109: The current except ObjectDoesNotExist wraps both the DB
lookup and batch.launch(), hiding errors raised inside BatchCommand.launch();
narrow the exception to only the lookup by enclosing only
BatchCommand.objects.get(pk=batch_id) in the try/except and handling the missing
object with the existing warning; call batch.launch() outside that except block
(and optionally add a separate try/except around batch.launch() to catch and log
unexpected exceptions including ObjectDoesNotExist at error level so failures in
BatchCommand.launch() are not misreported).
🪄 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
Run ID: 35398c9a-4dec-42fc-b505-27427d8d6602
📒 Files selected for processing (3)
openwisp_controller/connection/base/models.pyopenwisp_controller/connection/models.pyopenwisp_controller/connection/tasks.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/connection/models.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/base/models.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/connection/models.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/base/models.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/connection/models.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/base/models.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_controller/connection/models.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/base/models.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Mark user-facing strings for translation with Django i18n helpers in Django code
Avoid unnecessary blank lines inside function and method bodies
Be careful with authentication, authorization, queryset filtering, serializers, admin behavior, cache invalidation, signals, Celery tasks, and websocket updates in Django code
Preserve validation around templates, VPN/PKI material, SSH credentials, device commands, uploaded files, URLs, and subnet/IP data
Write comments and docstrings only when they explain why code is shaped a certain way, placing them before the relevant code block instead of scattering them inside it
Files:
openwisp_controller/connection/models.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/base/models.py
🧠 Learnings (3)
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/connection/models.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/base/models.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/connection/models.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/base/models.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/connection/models.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/base/models.py
Migrations Check FailedHello @dee077, The CI build failed because of a migrations check. The error message indicates that model changes have not been migrated. To fix this, please run the following command in your local environment: ./manage.py makemigrationsThen, commit the generated migration files and push your changes again. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openwisp_controller/connection/base/models.py (1)
733-739:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThe batch status enum no longer matches the feature contract.
The PR objectives call out
partial successandno-target, but this enum cannot represent either. Downstream, mixed outcomes are collapsed intofailed, empty launches fall back toidle, andcancelledis unreachable because child commands never emit a cancelled state.Based on learnings from the PR objectives, batch execution must expose distinct aggregate states for partial success and no-target outcomes.
🤖 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 `@openwisp_controller/connection/base/models.py` around lines 733 - 739, STATUS_CHOICES no longer reflects required aggregate states; add explicit choices for "partial-success" and "no-target" and update labels so they read e.g. ("partial-success", _("partial success")) and ("no-target", _("no target")) alongside existing values in STATUS_CHOICES. After adding the new constants, update any aggregation logic that computes the batch status from child command outcomes (the code that collapses mixed outcomes into "failed" and treats empty launches as "idle") to emit "partial-success" when there are mixed success/fail results and "no-target" when a launch had no target commands, and ensure "cancelled" remains reachable only if child commands can produce cancelled — adjust that aggregator function accordingly.
♻️ Duplicate comments (4)
openwisp_controller/connection/base/models.py (3)
566-567:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThis hook still misses the real completion path.
calculate_and_update_status()only runs when a terminal child is persisted throughsave(), butAbstractCommand.execute()and the failure paths inopenwisp_controller/connection/tasks.pywrite terminal states via_save_without_resurrecting(). The batch therefore stays on its initial aggregate and never reflects completed children.🤖 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 `@openwisp_controller/connection/base/models.py` around lines 566 - 567, The post-save hook only calls batch_command.calculate_and_update_status() when a terminal child is persisted via save(), but terminal states written by AbstractCommand.execute() and the failure paths in openwisp_controller/connection/tasks.py use _save_without_resurrecting(), so the batch status never gets recalculated; update those code paths to invoke batch_command.calculate_and_update_status() after calling _save_without_resurrecting() (or modify _save_without_resurrecting() to trigger the same post-save behavior) so that when a Command model with batch_command_id transitions to a terminal state the batch aggregate is recalculated and persisted.
850-868:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
launch()is still non-idempotent.A second
launch_async()delivery, manuallaunch(), or task retry will create a freshCommandrow for every target again, andCommand.save()schedules execution on create. For device commands, that turns a retry into duplicated commands sent to the same devices.🤖 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 `@openwisp_controller/connection/base/models.py` around lines 850 - 868, The launch() method currently creates a new Command for every device unconditionally, causing duplicate commands on retries; change it to be idempotent by checking for existing Commands tied to this batch before creating new ones: use the Command model to query existing commands with batch_command=self (and optionally matching device, type and input) and skip creation for devices that already have a Command, e.g., build a set of device ids from existing Command objects and only create Command objects for devices not in that set (or use get_or_create per device), then update total_devices to reflect the number of unique target devices (existing + newly created) and call calculate_and_update_status(); reference launch(), Command, resolve_devices(), device, batch_command, type, input, total_devices, calculate_and_update_status() when making the change.
743-744:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftCross-organization batches are only validated against the global allowlist.
When
organizationis null,clean()accepts any command enabled in__all__, butlaunch()later validates each childCommandagainst that device’s organization. A superuser batch spanning orgs with different command policies can therefore create/schedule some commands and then blow up mid-loop on the first disallowed device, leaving a partial batch behind.As per coding guidelines, "Preserve validation around templates, VPN/PKI material, SSH credentials, device commands, uploaded files, URLs, and subnet/IP data".
Also applies to: 817-823
🤖 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 `@openwisp_controller/connection/base/models.py` around lines 743 - 744, The Batch model's clean() currently trusts the global __all__ when organization is null, allowing cross-org batches to pass initial validation but fail in launch(), so update the clean() method to validate child Command entries against the actual device/organization policy before saving: when self.organization is None iterate through the related commands/devices (use the same checks/utility used by launch() — e.g., call Command.full_clean() or the command-permission validator used in launch) and raise ValidationError on any disallowed command so a cross-organization batch cannot be created; apply the same change to the similar validation block referenced around lines 817-823 to ensure both code paths enforce per-device organization command policies.Source: Coding guidelines
openwisp_controller/connection/tasks.py (1)
112-116:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOnly catch the missing-batch lookup here.
The current
except ObjectDoesNotExistalso coversbatch.launch(), so any nestedDoesNotExistraised while resolving targets or creating child commands is misreported as “batch deleted” and the task exits cleanly with stale state.As per coding guidelines, "New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level".
🤖 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 `@openwisp_controller/connection/tasks.py` around lines 112 - 116, The ObjectDoesNotExist except currently wraps both the lookup and batch.launch(), which hides DoesNotExist errors raised during launch; change the flow so only the lookup is guarded: perform BatchCommand.objects.get(pk=batch_id) inside a try/except catching ObjectDoesNotExist and call logger.warning(f"The BatchCommand object with id {batch_id} not found") in that except, then call batch.launch() outside that try block so any DoesNotExist from batch.launch() (or other errors) will propagate and be handled/logged at error level by the task runner.Source: Coding guidelines
🤖 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 `@openwisp_controller/connection/api/views.py`:
- Around line 168-169: The code calls batch.resolve_devices() then builds
device_pks by instantiating full Device objects (resolved =
batch.resolve_devices(); device_pks = [str(d.pk) for d in resolved]) which loads
entire rows unnecessarily; change to obtain only primary keys (e.g., have
resolve_devices() return a queryset or iterable of pks or call values_list('pk',
flat=True) on the queryset) and build device_pks from those pks (or update
batch.resolve_devices to provide a resolve_device_pks() / return pks) so you
never instantiate full Device model instances when only IDs are needed.
- Around line 151-156: The POST flow creates a batch via serializer.save() and
then calls batch.launch_async(); if launch_async() raises, the code currently
neither logs nor returns a controlled error. Wrap the batch.launch_async() call
in a try/except, log the exception at error level with contextual details
(batch.pk, request.user or request.auth) and return a clear API error Response
(e.g., status.HTTP_500_INTERNAL_SERVER_ERROR or HTTP_503) describing that
asynchronous enqueue failed; optionally mark or update the batch state to
"failed" if a setter exists on the Batch model to avoid leaving it in an
indeterminate state. Ensure you reference the post() handler, serializer.save(),
batch.launch_async(), Response and status when making the change.
In
`@openwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.py`:
- Around line 125-127: The migration's frozen model options ("Batch command
operation" / plural) no longer match the model's Meta on AbstractBatchCommand
which now uses "Batch command" / "Batch commands"; update the migration
(0011_batchcommand_command_batch_command.py) to set options to "verbose_name":
"Batch command" and "verbose_name_plural": "Batch commands" (or regenerate the
migration so the frozen Meta matches AbstractBatchCommand.Meta) to avoid an
immediate AlterModelOptions diff.
In `@openwisp_controller/connection/tasks.py`:
- Around line 84-91: Remove the demo print block that prints
command.batch_command_id/devices/status/output to stdout; instead, stop using
print() and (within the same task/function that references
command.batch_command_id) use the project's logger to record only non-sensitive,
minimal info (e.g., log an info-level message that a batch command completed and
a warning/error-level message on failures) and never dump command.output or
device credentials to stdout; reference the existing variables
command.batch_command_id, command.device.name, command.status, and
command.output when implementing the replacement and ensure you follow the
project's logging/error-handling conventions (mask or omit sensitive output and
log detailed errors at error level).
---
Outside diff comments:
In `@openwisp_controller/connection/base/models.py`:
- Around line 733-739: STATUS_CHOICES no longer reflects required aggregate
states; add explicit choices for "partial-success" and "no-target" and update
labels so they read e.g. ("partial-success", _("partial success")) and
("no-target", _("no target")) alongside existing values in STATUS_CHOICES. After
adding the new constants, update any aggregation logic that computes the batch
status from child command outcomes (the code that collapses mixed outcomes into
"failed" and treats empty launches as "idle") to emit "partial-success" when
there are mixed success/fail results and "no-target" when a launch had no target
commands, and ensure "cancelled" remains reachable only if child commands can
produce cancelled — adjust that aggregator function accordingly.
---
Duplicate comments:
In `@openwisp_controller/connection/base/models.py`:
- Around line 566-567: The post-save hook only calls
batch_command.calculate_and_update_status() when a terminal child is persisted
via save(), but terminal states written by AbstractCommand.execute() and the
failure paths in openwisp_controller/connection/tasks.py use
_save_without_resurrecting(), so the batch status never gets recalculated;
update those code paths to invoke batch_command.calculate_and_update_status()
after calling _save_without_resurrecting() (or modify
_save_without_resurrecting() to trigger the same post-save behavior) so that
when a Command model with batch_command_id transitions to a terminal state the
batch aggregate is recalculated and persisted.
- Around line 850-868: The launch() method currently creates a new Command for
every device unconditionally, causing duplicate commands on retries; change it
to be idempotent by checking for existing Commands tied to this batch before
creating new ones: use the Command model to query existing commands with
batch_command=self (and optionally matching device, type and input) and skip
creation for devices that already have a Command, e.g., build a set of device
ids from existing Command objects and only create Command objects for devices
not in that set (or use get_or_create per device), then update total_devices to
reflect the number of unique target devices (existing + newly created) and call
calculate_and_update_status(); reference launch(), Command, resolve_devices(),
device, batch_command, type, input, total_devices, calculate_and_update_status()
when making the change.
- Around line 743-744: The Batch model's clean() currently trusts the global
__all__ when organization is null, allowing cross-org batches to pass initial
validation but fail in launch(), so update the clean() method to validate child
Command entries against the actual device/organization policy before saving:
when self.organization is None iterate through the related commands/devices (use
the same checks/utility used by launch() — e.g., call Command.full_clean() or
the command-permission validator used in launch) and raise ValidationError on
any disallowed command so a cross-organization batch cannot be created; apply
the same change to the similar validation block referenced around lines 817-823
to ensure both code paths enforce per-device organization command policies.
In `@openwisp_controller/connection/tasks.py`:
- Around line 112-116: The ObjectDoesNotExist except currently wraps both the
lookup and batch.launch(), which hides DoesNotExist errors raised during launch;
change the flow so only the lookup is guarded: perform
BatchCommand.objects.get(pk=batch_id) inside a try/except catching
ObjectDoesNotExist and call logger.warning(f"The BatchCommand object with id
{batch_id} not found") in that except, then call batch.launch() outside that try
block so any DoesNotExist from batch.launch() (or other errors) will propagate
and be handled/logged at error level by the task runner.
🪄 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
Run ID: 2a7daccb-35b2-4120-905b-86413be228fe
📒 Files selected for processing (6)
openwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/api/urls.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/tasks.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/connection/api/urls.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/base/models.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/connection/api/urls.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/base/models.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/connection/api/urls.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/base/models.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_controller/connection/api/urls.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/base/models.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Mark user-facing strings for translation with Django i18n helpers in Django code
Avoid unnecessary blank lines inside function and method bodies
Be careful with authentication, authorization, queryset filtering, serializers, admin behavior, cache invalidation, signals, Celery tasks, and websocket updates in Django code
Preserve validation around templates, VPN/PKI material, SSH credentials, device commands, uploaded files, URLs, and subnet/IP data
Write comments and docstrings only when they explain why code is shaped a certain way, placing them before the relevant code block instead of scattering them inside it
Files:
openwisp_controller/connection/api/urls.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/base/models.py
🧠 Learnings (3)
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/connection/api/urls.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/base/models.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/connection/api/urls.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/base/models.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/connection/api/urls.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/base/models.py
🪛 ast-grep (0.43.0)
openwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.py
[info] 55-65: use help_text to document model columns
Context: models.CharField(
choices=[
("idle", "idle"),
("in-progress", "in progress"),
("success", "completed successfully"),
("failed", "completed with some failures"),
("cancelled", "completed with some cancellations"),
],
default="idle",
max_length=12,
)
Note: Security best practice.
(model-help-text)
[info] 69-72: use help_text to document model columns
Context: models.CharField(
choices=openwisp_controller.connection.commands.get_command_choices,
max_length=16,
)
Note: Security best practice.
(model-help-text)
🔇 Additional comments (4)
openwisp_controller/connection/api/serializers.py (2)
15-15: LGTM!Also applies to: 47-50
125-127: PreserveAbstractBatchCommand.clean()auth/schema checks inBatchCommandExecuteSerializer
BatchCommandExecuteSerializerinherits from plainserializers.ModelSerializerandvalidate()returnsdatadirectly; meanwhileAbstractBatchCommand.clean()enforces org/group/location/device consistency plus org-allowedcommand_typeand JSON schema validation forcommand_input. If thoseclean()validations aren’t invoked duringserializer.save()inBatchCommandExecuteView.post(), this serializer path can bypass model-level authorization/schema enforcement (lines 125-173 inopenwisp_controller/connection/api/serializers.py).Suggested fix
class BatchCommandExecuteSerializer( - FilterSerializerByOrgManaged, serializers.ModelSerializer + FilterSerializerByOrgManaged, ValidatedModelSerializer ): @@ def validate(self, data): if ( not data.get("organization") and not self.context["request"].user.is_superuser ): raise serializers.ValidationError( _("Only superusers can execute batch commands without an organization.") ) if devices := data.get("devices"): org = data.get("organization") for device in devices: if org and device.organization_id != org.id: raise serializers.ValidationError( { "devices": _( "All devices must belong to the same organization." ) } ) - return data + return super().validate(data)openwisp_controller/connection/api/views.py (1)
194-194: LGTM!openwisp_controller/connection/api/urls.py (1)
43-47: LGTM!
| # Todo: Remove once demo is completed | ||
| if command.batch_command_id: | ||
| print("****************************") | ||
| print(f"Device: {command.device.name}") | ||
| print(f"Status: {command.status}") | ||
| print("") | ||
| print(command.output) | ||
| print("****************************") |
There was a problem hiding this comment.
Remove the demo print() block before merge.
This sends raw command output, device names, and execution status to worker stdout. Batch command output can contain sensitive device data, and print() bypasses the project’s logging/error-handling conventions.
As per coding guidelines, "New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level" and "Preserve validation around templates, VPN/PKI material, SSH credentials, device commands, uploaded files, URLs, and subnet/IP data".
🤖 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 `@openwisp_controller/connection/tasks.py` around lines 84 - 91, Remove the
demo print block that prints command.batch_command_id/devices/status/output to
stdout; instead, stop using print() and (within the same task/function that
references command.batch_command_id) use the project's logger to record only
non-sensitive, minimal info (e.g., log an info-level message that a batch
command completed and a warning/error-level message on failures) and never dump
command.output or device credentials to stdout; reference the existing variables
command.batch_command_id, command.device.name, command.status, and
command.output when implementing the replacement and ensure you follow the
project's logging/error-handling conventions (mask or omit sensitive output and
log detailed errors at error level).
Source: Coding guidelines
There was a problem hiding this comment.
@pandafy, noted! The demo print() block is flagged for removal. When you're ready to address it, you can replace it with a structured log call — something like:
if command.batch_command_id:
logger.info(
f"Batch command {command.batch_command_id}: device '{command.device.name}' "
f"finished with status '{command.status}'"
)This keeps only non-sensitive metadata (no command.output) and follows the project's logging conventions.
🧠 Learnings used
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Flake8, Migrations, and Test FailuresHello @dee077, There are three distinct failures in the CI logs:
|
…nd execution - Removed counter DB fields - Added computed properties via aggregation - Added execute_all boolean field - Renamed launch() to create_commands() - Added execute() and dry_run() classmethods - Updated calculate_and_update_status() - Made organization FK nullable - Updated views with execute/dry_run - Updated serializer with execute_all and type/input aliases - Updated migration and celery task
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
openwisp_controller/connection/tasks.py (2)
84-91:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove the demo stdout dump before merge.
This prints device names and raw
command.outputfor batch runs straight to worker stdout. That can leak sensitive device data and bypasses the project's logging/error-handling conventions.As per coding guidelines, "New code must handle errors properly..." and "Preserve validation around ... SSH credentials, device commands...".
🤖 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 `@openwisp_controller/connection/tasks.py` around lines 84 - 91, Remove the entire debug print block that outputs sensitive device data to stdout. Delete the conditional block starting with the comment "Todo: Remove once demo is completed" that checks command.batch_command_id and all subsequent print statements that write device names and command output. This debug code bypasses proper logging conventions and leaks sensitive information, so it must be completely removed before merging.Source: Coding guidelines
112-117:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOnly treat lookup failures as “batch deleted”.
except ObjectDoesNotExistcurrently wraps both Line 113 and Line 114. Ifcreate_commands()raises its ownObjectDoesNotExist, the worker logs that the batch was deleted and exits cleanly, which hides the real failure and leaves the batch state stale.As per coding guidelines, "New code must handle errors properly: log errors that cannot be resolved by the user with error level...".
In Django, is ObjectDoesNotExist the base class for model-specific DoesNotExist exceptions, and would `except ObjectDoesNotExist` also catch `DoesNotExist` raised inside a later method call in the same try block?🤖 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 `@openwisp_controller/connection/tasks.py` around lines 112 - 117, The except ObjectDoesNotExist handler currently catches exceptions from both the BatchCommand.objects.get() call and the batch.create_commands() call. If create_commands() raises ObjectDoesNotExist for a reason unrelated to the batch lookup, it gets misidentified as a deleted batch and hides the real failure. Restructure the exception handling so that only the get() call on line 113 is protected by the except ObjectDoesNotExist clause, while the create_commands() call on line 114 is either moved outside the try block or handled separately with proper error-level logging according to coding guidelines. Also fix the typo "foound" to "found" in the warning message.Source: Coding guidelines
openwisp_controller/connection/base/models.py (1)
890-904:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
create_commands()still duplicates work on rerun.Every invocation creates a fresh
Commandfor every resolved device, and Line 903 immediately schedules execution viaCommand.save(). A Celery redelivery or a second manual call will enqueue duplicate commands for the same(batch_command, device)pair unless existing rows are filtered out first.Based on the batch orchestration flow in this PR, retries need to be idempotent before child commands are created.
🤖 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 `@openwisp_controller/connection/base/models.py` around lines 890 - 904, The create_commands() method creates duplicate Command objects on rerun because it does not check for existing commands before creating new ones for each device. Before creating a Command in the loop over self.resolve_devices().iterator(), query the Command model to check if a command already exists for the current (batch_command, device) pair. Only instantiate and save a new Command if one does not already exist for that combination. This ensures the method is idempotent and safe for Celery redeliveries or manual reruns without generating duplicates.
🤖 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 `@openwisp_controller/connection/api/views.py`:
- Around line 166-173: The dry-run endpoint in the BatchCommand.dry_run response
handling is materializing and returning the entire list of resolved device UUIDs
without any bounds, which creates performance issues for mass targets like
execute_all or org-wide previews. Instead of returning all devices in the
data["devices"] field, modify the response to include a device count and a
capped or paginated sample of device UUIDs. This prevents unbounded payload
sizes while still providing sufficient preview information to the user.
- Around line 157-160: The ValidationError exception handler is only returning
the first error message by accessing e.messages[0], which loses field names and
additional validation errors from the full_clean() call. This creates an
inconsistent 400 response contract compared to serializer validation. Instead of
flattening to a single error string, return the complete validation error
payload that preserves all field names and error details. This fix should be
applied to both the exception handler in the execute method (lines 157-160) and
the corresponding exception handler in the dry_run method (lines 168-171) to
ensure consistent error responses across both code paths.
In `@openwisp_controller/connection/base/models.py`:
- Around line 894-905: The issue is that when command.full_clean() raises a
ValidationError, the command is never saved, causing the device to be dropped
from batch aggregates including total_devices, successful, failed, and
calculate_and_update_status(). This violates the requirement to preserve
validation around device commands and expose accurate batch status aggregated
from all targeted devices. Instead of skipping the device entirely, modify the
exception handling in the loop over self.resolve_devices().iterator() to persist
the command with an appropriate status (such as failed or invalid) even when
full_clean() fails, ensuring all targeted devices are represented in the batch
aggregates and the batch status accurately reflects the execution outcome across
all intended targets.
- Around line 868-888: The `execute()` and `dry_run()` classmethod
implementations have an authorization gap where explicitly provided devices
bypass organization validation. The `devices_list` parameter is validated after
`full_clean()` is called, but the organization validation in the `clean()`
method requires `self.pk` to exist, allowing mismatched devices to bypass
checks. Before calling `full_clean()` in both methods, add explicit validation
of the `devices_list` parameter (if provided) to ensure all devices belong to
the same organization as the batch. This ensures authorization checks are
performed on user-provided devices before any instance is persisted.
---
Duplicate comments:
In `@openwisp_controller/connection/base/models.py`:
- Around line 890-904: The create_commands() method creates duplicate Command
objects on rerun because it does not check for existing commands before creating
new ones for each device. Before creating a Command in the loop over
self.resolve_devices().iterator(), query the Command model to check if a command
already exists for the current (batch_command, device) pair. Only instantiate
and save a new Command if one does not already exist for that combination. This
ensures the method is idempotent and safe for Celery redeliveries or manual
reruns without generating duplicates.
In `@openwisp_controller/connection/tasks.py`:
- Around line 84-91: Remove the entire debug print block that outputs sensitive
device data to stdout. Delete the conditional block starting with the comment
"Todo: Remove once demo is completed" that checks command.batch_command_id and
all subsequent print statements that write device names and command output. This
debug code bypasses proper logging conventions and leaks sensitive information,
so it must be completely removed before merging.
- Around line 112-117: The except ObjectDoesNotExist handler currently catches
exceptions from both the BatchCommand.objects.get() call and the
batch.create_commands() call. If create_commands() raises ObjectDoesNotExist for
a reason unrelated to the batch lookup, it gets misidentified as a deleted batch
and hides the real failure. Restructure the exception handling so that only the
get() call on line 113 is protected by the except ObjectDoesNotExist clause,
while the create_commands() call on line 114 is either moved outside the try
block or handled separately with proper error-level logging according to coding
guidelines. Also fix the typo "foound" to "found" in the warning message.
🪄 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
Run ID: 50afe5cb-bf73-45c0-b66d-723c6793776e
📒 Files selected for processing (5)
openwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/tasks.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/connection/tasks.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/base/models.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/connection/tasks.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/base/models.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/connection/tasks.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/base/models.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_controller/connection/tasks.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/base/models.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Mark user-facing strings for translation with Django i18n helpers in Django code
Avoid unnecessary blank lines inside function and method bodies
Be careful with authentication, authorization, queryset filtering, serializers, admin behavior, cache invalidation, signals, Celery tasks, and websocket updates in Django code
Preserve validation around templates, VPN/PKI material, SSH credentials, device commands, uploaded files, URLs, and subnet/IP data
Write comments and docstrings only when they explain why code is shaped a certain way, placing them before the relevant code block instead of scattering them inside it
Files:
openwisp_controller/connection/tasks.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/base/models.py
🧠 Learnings (3)
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/connection/tasks.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/base/models.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/connection/tasks.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/base/models.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/connection/tasks.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/base/models.py
🔇 Additional comments (2)
openwisp_controller/connection/api/views.py (1)
155-161: Enqueue failures are still unhandled in the POST path.
BatchCommand.execute()persists the batch before scheduling the Celery dispatch, so a broker/on-commit failure here still bubbles as a 500 after the batch has been created.openwisp_controller/connection/api/serializers.py (1)
139-183: LGTM!
CI Failures: Code Style and TestsHello @dee077,
|
…dempotency guard - Added full_clean() to dry_run() for model-level validation - Create failed Command records instead of skipping on validation error - Added idempotency guard to create_commands() via Command existence check - Narrowed ObjectDoesNotExist handler in launch_batch_command task - Return full message_dict instead of first message on ValidationError
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
openwisp_controller/connection/base/models.py (1)
891-893:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftStrengthen idempotency to handle retries and partial runs safely.
Line 892 returns as soon as any child
Commandexists. That makes retries skip missing devices after partial failures, and concurrent workers can still pass this pre-check and create duplicates (TOCTOU). Deduplicate per device within a transaction/lock instead of short-circuiting the entire batch.Suggested fix
def create_commands(self): Command = load_model("connection", "Command") - if Command.objects.filter(batch_command=self).exists(): - return + with transaction.atomic(): + batch = self.__class__.objects.select_for_update().get(pk=self.pk) + existing_device_ids = set( + Command.objects.filter(batch_command=batch).values_list( + "device_id", flat=True + ) + ) - self.status = "in-progress" - self.save() - for device in self.resolve_devices().iterator(): + batch.status = "in-progress" + batch.save(update_fields=["status"]) + for device in self.resolve_devices().exclude(pk__in=existing_device_ids).iterator(): command = Command( device=device, type=self.command_type, input=self.command_input, batch_command=self, )🤖 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 `@openwisp_controller/connection/base/models.py` around lines 891 - 893, The early return in the batch_command method (when checking if any Command exists) is not properly idempotent and allows race conditions where concurrent workers or retries after partial failures can create duplicates. Instead of returning early when any child Command exists, implement per-device deduplication within a transaction or lock. This ensures that if some Commands already exist for certain devices, they won't be recreated on retry, while missing Commands for other devices will be created, and concurrent workers cannot bypass the check and create duplicates.
🤖 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 `@openwisp_controller/connection/base/models.py`:
- Around line 907-910: The code directly serializes the exception into both the
command output and log message, which can expose sensitive validation payloads.
Replace the raw exception string in the command.output assignment (the `str(e)`
part) with a generic failure message like "Command validation failed".
Similarly, modify the logger.warning call to exclude the raw exception `{e}` and
instead log only non-sensitive identifiers (such as the device.pk and batch
self.pk that are already included). Extract only field names or error types from
the ValidationError if needed for debugging, but never include the actual
submitted values or detailed validation messages that could contain sensitive
command data.
---
Duplicate comments:
In `@openwisp_controller/connection/base/models.py`:
- Around line 891-893: The early return in the batch_command method (when
checking if any Command exists) is not properly idempotent and allows race
conditions where concurrent workers or retries after partial failures can create
duplicates. Instead of returning early when any child Command exists, implement
per-device deduplication within a transaction or lock. This ensures that if some
Commands already exist for certain devices, they won't be recreated on retry,
while missing Commands for other devices will be created, and concurrent workers
cannot bypass the check and create duplicates.
🪄 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
Run ID: 4fea12c9-1fda-4a74-94d0-e2c93a2be9aa
📒 Files selected for processing (4)
openwisp_controller/connection/api/views.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/tasks.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/connection/tasks.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/api/views.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/connection/tasks.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/api/views.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/connection/tasks.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/api/views.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_controller/connection/tasks.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/api/views.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Mark user-facing strings for translation with Django i18n helpers in Django code
Avoid unnecessary blank lines inside function and method bodies
Be careful with authentication, authorization, queryset filtering, serializers, admin behavior, cache invalidation, signals, Celery tasks, and websocket updates in Django code
Preserve validation around templates, VPN/PKI material, SSH credentials, device commands, uploaded files, URLs, and subnet/IP data
Write comments and docstrings only when they explain why code is shaped a certain way, placing them before the relevant code block instead of scattering them inside it
Files:
openwisp_controller/connection/tasks.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/api/views.py
🧠 Learnings (3)
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/connection/tasks.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/api/views.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/connection/tasks.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/api/views.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/connection/tasks.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/api/views.py
🔇 Additional comments (3)
openwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.py (1)
3-4: LGTM!Also applies to: 12-13
openwisp_controller/connection/tasks.py (1)
114-117: LGTM!openwisp_controller/connection/api/views.py (1)
159-160: LGTM!Also applies to: 171-172
| command.status = "failed" | ||
| command.output = str(e) | ||
| models.Model.save(command) | ||
| logger.warning(f"Device {device.pk} failed for batch {self.pk}: {e}") |
There was a problem hiding this comment.
Avoid leaking raw validation payloads in batch failure logs/output.
Lines 908-910 serialize and log ValidationError directly. Schema validation errors can echo submitted command values, so this risks exposing sensitive command input. Persist a generic failure message and log only non-sensitive identifiers/field names.
Suggested fix
except ValidationError as e:
command.status = "failed"
- command.output = str(e)
+ command.output = _("Command validation failed.")
models.Model.save(command)
- logger.warning(f"Device {device.pk} failed for batch {self.pk}: {e}")
+ fields = list(getattr(e, "message_dict", {}).keys()) or ["__all__"]
+ logger.warning(
+ "Command validation failed for batch=%s device=%s fields=%s",
+ self.pk,
+ device.pk,
+ ",".join(fields),
+ )As per coding guidelines, preserve validation around device commands and avoid exposing sensitive command data in logs.
🤖 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 `@openwisp_controller/connection/base/models.py` around lines 907 - 910, The
code directly serializes the exception into both the command output and log
message, which can expose sensitive validation payloads. Replace the raw
exception string in the command.output assignment (the `str(e)` part) with a
generic failure message like "Command validation failed". Similarly, modify the
logger.warning call to exclude the raw exception `{e}` and instead log only
non-sensitive identifiers (such as the device.pk and batch self.pk that are
already included). Extract only field names or error types from the
ValidationError if needed for debugging, but never include the actual submitted
values or detailed validation messages that could contain sensitive command
data.
Source: Coding guidelines
Flake8 Linting FailuresHello @dee077, There is a
|
- Fixed migration swappable dependency for config/geo apps - Log only field names (not values) in create_commands error handler - Added batch_command field to expected websocket response - Added sample_connection BatchCommand model, migration, view, and settings - Updated geo test query count assertions
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
Test Failures in CIHello @dee077, There are test failures in the CI pipeline.
|
…o true - Make type optional on GET requests via serializer __init__ - Skip full_clean() in dry_run when command_type is not provided - Default execute_all to True for both GET and POST
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (2/3). |
pandafy
left a comment
pandafy
left a comment
There was a problem hiding this comment.
I believe the current state of PR is no up-to-date with your development. I'd some time today so went ahead to review the model and API definitions.
Some of these comments might be already outdated, which is fine. I added them so keep a track of the decisions. Also, coderabbit will help us flag if anything gets missed when the PR becomes huge.
| STATUS_CHOICES = ( | ||
| ("in-progress", _("in progress")), | ||
| ("success", _("success")), | ||
| ("failed", _("failed")), | ||
| ) |
There was a problem hiding this comment.
I think we shall add a new status error for command objects which raised ValidationError while creating.
| allowed = dict( | ||
| AbstractCommand.get_org_allowed_commands( | ||
| organization_id=self.organization_id | ||
| ) | ||
| ) |
There was a problem hiding this comment.
A superuser can create a batch command without selecting an organization.
Consider the following configuration:
OPENWISP_CONTROLLER_ORGANIZATION_ENABLED_COMMANDS = {
"__all__": "*",
# my organization
"111111100-ff46-456c-9036-413a260a7831": ("reboot",),
}What happens if a superuser launches a custom or password reset batch command
without specifying an organization?
In particular, is the command schema used when rendering the underlying
Command objects? If so, we should verify that commands created without an
organization do not cause errors or incomplete/obscured rendering when viewed
later by organization administrators in the device's Recent Commands tab.
It would be good to add a test covering this scenario to ensure that
organization-specific command restrictions do not affect the display of
existing commands.
| raise ValidationError( | ||
| { | ||
| "command_type": _( | ||
| '"{command}" command is not available ' "for this organization" |
There was a problem hiding this comment.
| '"{command}" command is not available ' "for this organization" | |
| '"{command}" command is not available for this organization' |
nitpick
| except SchemaError as e: | ||
| raise ValidationError({"command_input": e.message}) | ||
|
|
||
| def resolve_devices(self): |
There was a problem hiding this comment.
Let's add a docstring here explaining the role of this method. We shall opt for user iterator() instead of all().
| blank=True, | ||
| verbose_name=_("devices"), | ||
| ) | ||
| execute_all = models.BooleanField(default=False) |
There was a problem hiding this comment.
@dee077 are we still continuing with the execute_all flag? In our previous discussion we concluded that the organization, location and group fields acts like a filter parameter for devices. If these fields are absent, then it automatically means that the user want to execute the command on all devices.
Can you explain the duty served by this flag?
There was a problem hiding this comment.
@nemesifier do you agree with above conclusion, or do you want to keep the explicit execute_all flag in the model?
| l1 = self._create_location() | ||
| path = reverse("geo_api:detail_location", args=[l1.pk]) | ||
| with self.assertNumQueries(5): | ||
| with self.assertNumQueries(6): |
There was a problem hiding this comment.
Question: Why the number of queries increased here?
| if request and request.method == "GET": | ||
| self.fields["type"].required = False |
| if not org and not self.context["request"].user.is_superuser: | ||
| raise serializers.ValidationError( | ||
| _("Only superusers can execute batch commands without an organization.") | ||
| ) |
There was a problem hiding this comment.
Can you check if we have existing mixins in openwisp-users which can perform this operation?
| if devices: | ||
| for device in devices: | ||
| if org and device.organization_id != org.id: | ||
| raise serializers.ValidationError( | ||
| { | ||
| "devices": _( | ||
| "All devices must belong to the same organization." | ||
| ) | ||
| } | ||
| ) | ||
| return data |
There was a problem hiding this comment.
I see the Model also performs this validation. And, it does so more elegantly.
| DeviceConnenctionListCreateView = DeviceConnectionListCreateView | ||
|
|
||
|
|
||
| class BatchCommandExecuteView(ProtectedAPIMixin, GenericAPIView): |
3 participants
Checklist
Reference to Existing Issue
Closes #1344.
Description of Changes
BatchCommand+CommandOperationmodels: parent-child batch execution pipeline mirroring the firmware upgrader'sBatchUpgradeOperation/UpgradeOperationpattern, with organization scoping, group/location targeting, and status aggregation from child operationsToDo: