Prevent password leakage in login reports - #757
Conversation
WalkthroughThe pull request adds multi-worker NATS coordination, shared node lifecycle state, administrator-aware group validation, deferred WireGuard allocation, dynamic subscription variables, dashboard updates, safer login notifications, and workflow maintenance. ChangesMulti-worker NATS coordination
Safe login notifications
Access and WireGuard allocation
Subscription and dashboard updates
Workflow and dependency maintenance
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
There was a problem hiding this comment.
Pull request overview
This PR removes submitted admin passwords from the admin-login notification contract and downstream Telegram/Discord renderers, ensuring passwords remain confined to the authentication boundary while preserving login-attempt reporting and adding targeted test coverage for success/failure/disabled flows.
Changes:
- Updated
notification.admin_loginand all call sites to drop thepasswordparameter (report only username, client IP, and success/failure). - Removed password rendering from Telegram and Discord admin-login message templates and formatting.
- Added focused async tests to assert the submitted password is never forwarded to notification/reporting or included in rendered payloads.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_admin_login_notifications.py | Adds tests covering successful, failed, and disabled admin login attempts to ensure submitted passwords never reach notifications/renderers. |
| app/routers/admin.py | Stops passing the submitted password into notification.admin_login when reporting admin login attempts. |
| app/notification/telegram/messages.py | Removes the password line from the Telegram admin login message template. |
| app/notification/telegram/admin.py | Updates Telegram admin_login signature/formatting to exclude password handling. |
| app/notification/discord/messages.py | Removes password from the Discord admin login embed description template. |
| app/notification/discord/admin.py | Updates Discord admin_login signature/formatting to exclude password handling. |
| app/notification/init.py | Updates the notification boundary admin_login function signature and dispatch to downstream notifiers without password. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
d3d8ca4 to
88669bf
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (5)
app/app_factory.py (1)
80-82: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComplete the truncated comment.
The comment ends with a semicolon and no clause. State why non-node router roles register the ignore handler for
MessageTopic.NODE.🤖 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 `@app/app_factory.py` around lines 80 - 82, Complete the comment above the MessageTopic.NODE registration in the enable_router branch to explain that non-node router roles must ignore worker_sync messages intended for node routers. Keep the existing router.register_handler call and behavior unchanged..github/workflows/test-database-migrations.yml (1)
16-16: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd an explicit
permissionsblock.The workflow declares no
permissions, so every job inherits the repository default token scope. These jobs only check out code and run tests. Restrict the token to read access at the workflow level.🔒 Proposed least-privilege token scope
+permissions: + contents: read + jobs: test-sqlite:🤖 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/test-database-migrations.yml at line 16, Add a workflow-level permissions block alongside jobs in the test-database-migrations workflow, granting only read access needed for checkout and tests (contents: read). Ensure all jobs inherit this least-privilege token scope.Source: Linters/SAST tools
app/nats/leader.py (1)
112-170: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider refreshing the timestamp before the steal write.
nowis captured at Line 127, before thekv.createandkv.getround trips. The steal payload at Line 128 therefore encodesnow + lease_seconds, so the acquired lease is shorter thanlease_secondsby the elapsed round-trip time. The direction is safe, but recomputing the expiry just beforekv.updatekeeps the lease duration accurate.♻️ Proposed refresh of the lease expiry on the steal path
info = _parse(entry.value) if info is None or info[1] <= now: try: - await kv.update(leader_key(), payload, last=entry.revision) + await kv.update(leader_key(), _payload(token, time.time() + lease_seconds), last=entry.revision) _token = token🤖 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 `@app/nats/leader.py` around lines 112 - 170, Refresh the current timestamp immediately before the expired-leader `kv.update` in `try_become_leader`, and build the steal payload with that timestamp plus `lease_seconds`. Keep the initial `now` and payload for the create path, while ensuring the steal write uses the refreshed expiry.app/node/__init__.py (1)
76-82: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winKeep a strong reference to the shutdown task.
asyncio.create_taskreturns a task that the event loop holds only weakly. If no reference is kept, the garbage collector can destroy the task before_shutdown_nodecompletes, and the node is never set toHealth.INVALIDor stopped.handle_node_messageinapp/node/manager_sync.pyawaitsremove_nodeand then immediately callsclear_bridge_memory_for_node, so the shutdown is already concurrent with cleanup.♻️ Proposed change
+_background_tasks: set[asyncio.Task] = set() + async def remove_node(self, id: int, *, remote_stop: bool = True) -> None: async with self._lock.writer_lock: old_node: PasarGuardNode | None = self._nodes.pop(id, None) self._user_sync_locks.pop(id, None) # Do cleanup without holding the lock to avoid slow delete operations. - asyncio.create_task(self._shutdown_node(old_node, remote_stop=remote_stop)) + task = asyncio.create_task(self._shutdown_node(old_node, remote_stop=remote_stop)) + _background_tasks.add(task) + task.add_done_callback(_background_tasks.discard)🤖 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 `@app/node/__init__.py` around lines 76 - 82, Update remove_node to retain a strong reference to the asyncio task created for _shutdown_node, using the node manager’s existing task-tracking mechanism or an equivalent collection. Ensure the reference is removed when the shutdown task completes, while preserving the current lock-free asynchronous cleanup and remote_stop behavior.app/jobs/record_usages.py (1)
769-773: 🚀 Performance & Scalability | 🔵 TrivialConsider keeping a signal for job duration at INFO level.
These two lines were the only INFO-level confirmation that usage recording completed and how long it took. At the default log level operators now see failures (
logger.exceptionat Lines 777 and 876) but no successful-run signal, so a silently stalled job looks the same as a healthy one.The demotion is reasonable for multi-worker noise. If you keep it, export the job duration and record counts as metrics, or log at INFO only when
job_durationexceeds a threshold.Also applies to: 869-872
🤖 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 `@app/jobs/record_usages.py` around lines 769 - 773, The completion log in the usage-recording job should retain an operator-visible health signal. Update the logging around the completion messages in the job’s main and alternate paths to emit INFO for slow runs based on an appropriate duration threshold, or expose job duration and record counts through metrics while keeping DEBUG for routine runs; preserve the existing completion details and avoid leaving successful runs observable only at DEBUG.
🤖 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 `@app/app_factory.py`:
- Around line 124-138: Update _reclaim_leadership_loop to catch exceptions
raised by start_job_leader(), log the failure, and continue the loop so
leadership acquisition is retried after the next heartbeat. Preserve the
existing success path that resumes jobs and returns, and use the module’s
established logging mechanism.
In `@app/db/crud/wireguard.py`:
- Around line 32-33: Refactor the reconciliation flow around
RECONCILE_USER_CHUNK to process users in keyset-ordered pages, deriving
accessible tags and related WireGuard data only for the current page and
applying it before fetching the next page. Avoid materializing all matching tag
rows, peer-IP user IDs, proxy settings, desired allocations, or changed user
IDs; retain only the global allocation state required across pages, and update
the related logic at the indicated reconciliation sections consistently.
In `@app/jobs/node_checker.py`:
- Around line 315-336: Align shutdown registration with the execution guard by
updating the condition around shutdown_nodes in the startup/shutdown
registration flow to skip it only when is_multi_worker() and
server_settings.workers > 1. Preserve the existing
feature_settings.stop_nodes_on_shutdown check and leave shutdown_nodes’ guard
unchanged, so workers > 1 with NATS disabled still register and execute remote
node stopping.
In `@app/node/manager_sync.py`:
- Around line 46-73: Keep all uses of db_node within the corresponding GetDB
async session: move node_manager.update_node(db_node) into the upsert session
block, and perform the connect branch’s update_node and
NodeOperation.connect_node calls before exiting its session while preserving the
existing lookup, status filtering, core/user map retrieval, and exception
handling.
In `@app/node/nats_memory.py`:
- Around line 436-444: Update shutdown_bridge_memory to acquire _init_lock while
closing and clearing all bridge globals, suppressing any exception from
_nc.close() so shutdown continues. Add a shutdown-state flag and have
ensure_bridge_memory check it while holding _init_lock, rejecting late
initialization after shutdown has begun; ensure the flag is set atomically with
shutdown and preserved across the cleanup.
In `@app/operation/node.py`:
- Around line 318-332: Update the 409 handling in the node connection flow
around NodeOperation._attach_if_running so every failed attach returns None
instead of falling through to the error-status result. Preserve the existing
successful attach response and the e.code == -4 behavior, ensuring
_connect_single_node_local and _connect_nodes_bulk_local skip database error
updates and notifications when another worker owns the lifecycle lease.
In `@app/subscription/share.py`:
- Line 312: Update the SNI formatting logic in the subscription generation flow
to catch ValueError and KeyError from sni.format_map(format_variables), using
the same fallback behavior as subscription metadata formatting. Ensure malformed
templates and undefined variables cannot abort subscription generation.
In `@pyproject.toml`:
- Around line 40-41: Update the pasarguard-node-bridge dependency declaration to
use a PyPI-published version that provides the required storage and lifecycle
APIs, including PasarGuardNodeBridge.storage ClaimedUser, LifecycleLease,
LifecycleOperation, LifecycleStatus, NodeLifecycleState, and create_node with
its expected parameters; alternatively, configure an explicit local or private
source guaranteeing those APIs, and regenerate uv.lock accordingly.
In `@tests/api/test_core.py`:
- Around line 62-69: Update the test setup around create_group and create_user
so the user is created without passing starter_group["id"] in group_ids. Keep
starter_group creation for the WireGuard group used later in the test, and
preserve the existing assertion flow.
In `@tests/test_connect_concurrency.py`:
- Around line 40-46: Update the test after _connect_nodes_bulk_local returns to
yield control to the event loop once, allowing scheduled
notification.connect_node tasks to run or drain before assertions. Keep the
existing peak concurrency assertions unchanged.
In `@tests/test_nats_leader_steal.py`:
- Around line 24-25: Add an autouse pytest fixture in the test module that
snapshots leader._is_leader and leader._token before each test and restores both
values in teardown. Apply it to both tests, including the later assignment
block, so try_become_leader and direct mutations cannot leak module state into
other tests.
---
Nitpick comments:
In @.github/workflows/test-database-migrations.yml:
- Line 16: Add a workflow-level permissions block alongside jobs in the
test-database-migrations workflow, granting only read access needed for checkout
and tests (contents: read). Ensure all jobs inherit this least-privilege token
scope.
In `@app/app_factory.py`:
- Around line 80-82: Complete the comment above the MessageTopic.NODE
registration in the enable_router branch to explain that non-node router roles
must ignore worker_sync messages intended for node routers. Keep the existing
router.register_handler call and behavior unchanged.
In `@app/jobs/record_usages.py`:
- Around line 769-773: The completion log in the usage-recording job should
retain an operator-visible health signal. Update the logging around the
completion messages in the job’s main and alternate paths to emit INFO for slow
runs based on an appropriate duration threshold, or expose job duration and
record counts through metrics while keeping DEBUG for routine runs; preserve the
existing completion details and avoid leaving successful runs observable only at
DEBUG.
In `@app/nats/leader.py`:
- Around line 112-170: Refresh the current timestamp immediately before the
expired-leader `kv.update` in `try_become_leader`, and build the steal payload
with that timestamp plus `lease_seconds`. Keep the initial `now` and payload for
the create path, while ensuring the steal write uses the refreshed expiry.
In `@app/node/__init__.py`:
- Around line 76-82: Update remove_node to retain a strong reference to the
asyncio task created for _shutdown_node, using the node manager’s existing
task-tracking mechanism or an equivalent collection. Ensure the reference is
removed when the shutdown task completes, while preserving the current lock-free
asynchronous cleanup and remote_stop behavior.
🪄 Autofix
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: CHILL
Plan: Pro Plus
Run ID: 686a87e0-9472-4c91-bcdf-b3988b4c6fe5
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (51)
.env.example.github/workflows/api-codeql.yml.github/workflows/build-dev.yml.github/workflows/build.yml.github/workflows/frontend-codeql.yml.github/workflows/test-database-migrations.ymlapp/app_factory.pyapp/core/hosts.pyapp/core/manager.pyapp/db/crud/wireguard.pyapp/jobs/node_checker.pyapp/jobs/record_usages.pyapp/nats/__init__.pyapp/nats/kv_cas.pyapp/nats/leader.pyapp/nats/message.pyapp/nats/router.pyapp/node/__init__.pyapp/node/manager_sync.pyapp/node/nats_memory.pyapp/operation/__init__.pyapp/operation/core.pyapp/operation/group.pyapp/operation/node.pyapp/operation/subscription.pyapp/operation/user.pyapp/operation/user_template.pyapp/routers/group.pyapp/subscription/share.pyconfig.pydashboard/public/statics/locales/en.jsondashboard/public/statics/locales/fa.jsondashboard/public/statics/locales/ru.jsondashboard/public/statics/locales/zh.jsondashboard/src/components/ui/variables-popover.tsxdashboard/src/features/hosts/dialogs/host-modal.tsxdashboard/src/features/subscriptions/components/subscription-general-settings-section.tsxdashboard/src/features/subscriptions/components/subscription-settings-schema.tsdashboard/src/features/users/components/action-buttons.tsxdashboard/src/pages/_dashboard.settings.general.tsxpyproject.tomlrole.pytests/api/test_core.pytests/api/test_user.pytests/test_connect_concurrency.pytests/test_create_app_nats_guard.pytests/test_group_access_unit.pytests/test_nats_leader_heartbeat.pytests/test_nats_leader_steal.pytests/test_nats_node_memory.pytests/test_node_manager_sync.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 11
🧹 Nitpick comments (5)
app/app_factory.py (1)
80-82: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComplete the truncated comment.
The comment ends with a semicolon and no clause. State why non-node router roles register the ignore handler for
MessageTopic.NODE.🤖 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 `@app/app_factory.py` around lines 80 - 82, Complete the comment above the MessageTopic.NODE registration in the enable_router branch to explain that non-node router roles must ignore worker_sync messages intended for node routers. Keep the existing router.register_handler call and behavior unchanged..github/workflows/test-database-migrations.yml (1)
16-16: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd an explicit
permissionsblock.The workflow declares no
permissions, so every job inherits the repository default token scope. These jobs only check out code and run tests. Restrict the token to read access at the workflow level.🔒 Proposed least-privilege token scope
+permissions: + contents: read + jobs: test-sqlite:🤖 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/test-database-migrations.yml at line 16, Add a workflow-level permissions block alongside jobs in the test-database-migrations workflow, granting only read access needed for checkout and tests (contents: read). Ensure all jobs inherit this least-privilege token scope.Source: Linters/SAST tools
app/nats/leader.py (1)
112-170: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider refreshing the timestamp before the steal write.
nowis captured at Line 127, before thekv.createandkv.getround trips. The steal payload at Line 128 therefore encodesnow + lease_seconds, so the acquired lease is shorter thanlease_secondsby the elapsed round-trip time. The direction is safe, but recomputing the expiry just beforekv.updatekeeps the lease duration accurate.♻️ Proposed refresh of the lease expiry on the steal path
info = _parse(entry.value) if info is None or info[1] <= now: try: - await kv.update(leader_key(), payload, last=entry.revision) + await kv.update(leader_key(), _payload(token, time.time() + lease_seconds), last=entry.revision) _token = token🤖 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 `@app/nats/leader.py` around lines 112 - 170, Refresh the current timestamp immediately before the expired-leader `kv.update` in `try_become_leader`, and build the steal payload with that timestamp plus `lease_seconds`. Keep the initial `now` and payload for the create path, while ensuring the steal write uses the refreshed expiry.app/node/__init__.py (1)
76-82: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winKeep a strong reference to the shutdown task.
asyncio.create_taskreturns a task that the event loop holds only weakly. If no reference is kept, the garbage collector can destroy the task before_shutdown_nodecompletes, and the node is never set toHealth.INVALIDor stopped.handle_node_messageinapp/node/manager_sync.pyawaitsremove_nodeand then immediately callsclear_bridge_memory_for_node, so the shutdown is already concurrent with cleanup.♻️ Proposed change
+_background_tasks: set[asyncio.Task] = set() + async def remove_node(self, id: int, *, remote_stop: bool = True) -> None: async with self._lock.writer_lock: old_node: PasarGuardNode | None = self._nodes.pop(id, None) self._user_sync_locks.pop(id, None) # Do cleanup without holding the lock to avoid slow delete operations. - asyncio.create_task(self._shutdown_node(old_node, remote_stop=remote_stop)) + task = asyncio.create_task(self._shutdown_node(old_node, remote_stop=remote_stop)) + _background_tasks.add(task) + task.add_done_callback(_background_tasks.discard)🤖 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 `@app/node/__init__.py` around lines 76 - 82, Update remove_node to retain a strong reference to the asyncio task created for _shutdown_node, using the node manager’s existing task-tracking mechanism or an equivalent collection. Ensure the reference is removed when the shutdown task completes, while preserving the current lock-free asynchronous cleanup and remote_stop behavior.app/jobs/record_usages.py (1)
769-773: 🚀 Performance & Scalability | 🔵 TrivialConsider keeping a signal for job duration at INFO level.
These two lines were the only INFO-level confirmation that usage recording completed and how long it took. At the default log level operators now see failures (
logger.exceptionat Lines 777 and 876) but no successful-run signal, so a silently stalled job looks the same as a healthy one.The demotion is reasonable for multi-worker noise. If you keep it, export the job duration and record counts as metrics, or log at INFO only when
job_durationexceeds a threshold.Also applies to: 869-872
🤖 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 `@app/jobs/record_usages.py` around lines 769 - 773, The completion log in the usage-recording job should retain an operator-visible health signal. Update the logging around the completion messages in the job’s main and alternate paths to emit INFO for slow runs based on an appropriate duration threshold, or expose job duration and record counts through metrics while keeping DEBUG for routine runs; preserve the existing completion details and avoid leaving successful runs observable only at DEBUG.
🤖 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 `@app/app_factory.py`:
- Around line 124-138: Update _reclaim_leadership_loop to catch exceptions
raised by start_job_leader(), log the failure, and continue the loop so
leadership acquisition is retried after the next heartbeat. Preserve the
existing success path that resumes jobs and returns, and use the module’s
established logging mechanism.
In `@app/db/crud/wireguard.py`:
- Around line 32-33: Refactor the reconciliation flow around
RECONCILE_USER_CHUNK to process users in keyset-ordered pages, deriving
accessible tags and related WireGuard data only for the current page and
applying it before fetching the next page. Avoid materializing all matching tag
rows, peer-IP user IDs, proxy settings, desired allocations, or changed user
IDs; retain only the global allocation state required across pages, and update
the related logic at the indicated reconciliation sections consistently.
In `@app/jobs/node_checker.py`:
- Around line 315-336: Align shutdown registration with the execution guard by
updating the condition around shutdown_nodes in the startup/shutdown
registration flow to skip it only when is_multi_worker() and
server_settings.workers > 1. Preserve the existing
feature_settings.stop_nodes_on_shutdown check and leave shutdown_nodes’ guard
unchanged, so workers > 1 with NATS disabled still register and execute remote
node stopping.
In `@app/node/manager_sync.py`:
- Around line 46-73: Keep all uses of db_node within the corresponding GetDB
async session: move node_manager.update_node(db_node) into the upsert session
block, and perform the connect branch’s update_node and
NodeOperation.connect_node calls before exiting its session while preserving the
existing lookup, status filtering, core/user map retrieval, and exception
handling.
In `@app/node/nats_memory.py`:
- Around line 436-444: Update shutdown_bridge_memory to acquire _init_lock while
closing and clearing all bridge globals, suppressing any exception from
_nc.close() so shutdown continues. Add a shutdown-state flag and have
ensure_bridge_memory check it while holding _init_lock, rejecting late
initialization after shutdown has begun; ensure the flag is set atomically with
shutdown and preserved across the cleanup.
In `@app/operation/node.py`:
- Around line 318-332: Update the 409 handling in the node connection flow
around NodeOperation._attach_if_running so every failed attach returns None
instead of falling through to the error-status result. Preserve the existing
successful attach response and the e.code == -4 behavior, ensuring
_connect_single_node_local and _connect_nodes_bulk_local skip database error
updates and notifications when another worker owns the lifecycle lease.
In `@app/subscription/share.py`:
- Line 312: Update the SNI formatting logic in the subscription generation flow
to catch ValueError and KeyError from sni.format_map(format_variables), using
the same fallback behavior as subscription metadata formatting. Ensure malformed
templates and undefined variables cannot abort subscription generation.
In `@pyproject.toml`:
- Around line 40-41: Update the pasarguard-node-bridge dependency declaration to
use a PyPI-published version that provides the required storage and lifecycle
APIs, including PasarGuardNodeBridge.storage ClaimedUser, LifecycleLease,
LifecycleOperation, LifecycleStatus, NodeLifecycleState, and create_node with
its expected parameters; alternatively, configure an explicit local or private
source guaranteeing those APIs, and regenerate uv.lock accordingly.
In `@tests/api/test_core.py`:
- Around line 62-69: Update the test setup around create_group and create_user
so the user is created without passing starter_group["id"] in group_ids. Keep
starter_group creation for the WireGuard group used later in the test, and
preserve the existing assertion flow.
In `@tests/test_connect_concurrency.py`:
- Around line 40-46: Update the test after _connect_nodes_bulk_local returns to
yield control to the event loop once, allowing scheduled
notification.connect_node tasks to run or drain before assertions. Keep the
existing peak concurrency assertions unchanged.
In `@tests/test_nats_leader_steal.py`:
- Around line 24-25: Add an autouse pytest fixture in the test module that
snapshots leader._is_leader and leader._token before each test and restores both
values in teardown. Apply it to both tests, including the later assignment
block, so try_become_leader and direct mutations cannot leak module state into
other tests.
---
Nitpick comments:
In @.github/workflows/test-database-migrations.yml:
- Line 16: Add a workflow-level permissions block alongside jobs in the
test-database-migrations workflow, granting only read access needed for checkout
and tests (contents: read). Ensure all jobs inherit this least-privilege token
scope.
In `@app/app_factory.py`:
- Around line 80-82: Complete the comment above the MessageTopic.NODE
registration in the enable_router branch to explain that non-node router roles
must ignore worker_sync messages intended for node routers. Keep the existing
router.register_handler call and behavior unchanged.
In `@app/jobs/record_usages.py`:
- Around line 769-773: The completion log in the usage-recording job should
retain an operator-visible health signal. Update the logging around the
completion messages in the job’s main and alternate paths to emit INFO for slow
runs based on an appropriate duration threshold, or expose job duration and
record counts through metrics while keeping DEBUG for routine runs; preserve the
existing completion details and avoid leaving successful runs observable only at
DEBUG.
In `@app/nats/leader.py`:
- Around line 112-170: Refresh the current timestamp immediately before the
expired-leader `kv.update` in `try_become_leader`, and build the steal payload
with that timestamp plus `lease_seconds`. Keep the initial `now` and payload for
the create path, while ensuring the steal write uses the refreshed expiry.
In `@app/node/__init__.py`:
- Around line 76-82: Update remove_node to retain a strong reference to the
asyncio task created for _shutdown_node, using the node manager’s existing
task-tracking mechanism or an equivalent collection. Ensure the reference is
removed when the shutdown task completes, while preserving the current lock-free
asynchronous cleanup and remote_stop behavior.
🪄 Autofix
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: CHILL
Plan: Pro Plus
Run ID: 686a87e0-9472-4c91-bcdf-b3988b4c6fe5
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (51)
.env.example.github/workflows/api-codeql.yml.github/workflows/build-dev.yml.github/workflows/build.yml.github/workflows/frontend-codeql.yml.github/workflows/test-database-migrations.ymlapp/app_factory.pyapp/core/hosts.pyapp/core/manager.pyapp/db/crud/wireguard.pyapp/jobs/node_checker.pyapp/jobs/record_usages.pyapp/nats/__init__.pyapp/nats/kv_cas.pyapp/nats/leader.pyapp/nats/message.pyapp/nats/router.pyapp/node/__init__.pyapp/node/manager_sync.pyapp/node/nats_memory.pyapp/operation/__init__.pyapp/operation/core.pyapp/operation/group.pyapp/operation/node.pyapp/operation/subscription.pyapp/operation/user.pyapp/operation/user_template.pyapp/routers/group.pyapp/subscription/share.pyconfig.pydashboard/public/statics/locales/en.jsondashboard/public/statics/locales/fa.jsondashboard/public/statics/locales/ru.jsondashboard/public/statics/locales/zh.jsondashboard/src/components/ui/variables-popover.tsxdashboard/src/features/hosts/dialogs/host-modal.tsxdashboard/src/features/subscriptions/components/subscription-general-settings-section.tsxdashboard/src/features/subscriptions/components/subscription-settings-schema.tsdashboard/src/features/users/components/action-buttons.tsxdashboard/src/pages/_dashboard.settings.general.tsxpyproject.tomlrole.pytests/api/test_core.pytests/api/test_user.pytests/test_connect_concurrency.pytests/test_create_app_nats_guard.pytests/test_group_access_unit.pytests/test_nats_leader_heartbeat.pytests/test_nats_leader_steal.pytests/test_nats_node_memory.pytests/test_node_manager_sync.py
🛑 Comments failed to post (11)
app/app_factory.py (1)
124-138: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard the reclaim loop against exceptions.
_reclaim_leadership_loopcallsstart_job_leader()without exception handling.start_job_leaderreaches_ensure_kv, which awaitscreate_nats_client()andget_or_create_kv_bucket. If NATS is unreachable at that moment, the call can raise. The exception then terminates the background task silently, because nothing awaits it before shutdown. After that the worker never retries leadership, so the scheduler stays paused and the notification dispatcher stays stopped for the process lifetime.Wrap the acquisition attempt in a try/except and log the failure so the loop keeps retrying.
🛡️ Proposed fix to keep the reclaim loop alive
async def _reclaim_leadership_loop(): # start_job_leader = try_become_leader + heartbeat restart while True: await asyncio.sleep(HEARTBEAT_INTERVAL) if is_job_leader(): return - if await start_job_leader(): - await _resume_jobs_on_leadership_gained() - return + try: + won = await start_job_leader() + except Exception as exc: + logger.warning("Leadership reclaim attempt failed: %s", exc) + continue + if won: + await _resume_jobs_on_leadership_gained() + return📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.async def _reclaim_leadership_loop(): # start_job_leader = try_become_leader + heartbeat restart while True: await asyncio.sleep(HEARTBEAT_INTERVAL) if is_job_leader(): return try: won = await start_job_leader() except Exception as exc: logger.warning("Leadership reclaim attempt failed: %s", exc) continue if won: await _resume_jobs_on_leadership_gained() return def _ensure_reclaim_task(): task = reclaim_task["task"] if task is not None and not task.done(): return reclaim_task["task"] = asyncio.create_task(_reclaim_leadership_loop())🤖 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 `@app/app_factory.py` around lines 124 - 138, Update _reclaim_leadership_loop to catch exceptions raised by start_job_leader(), log the failure, and continue the loop so leadership acquisition is retried after the next heartbeat. Preserve the existing success path that resumes jobs and returns, and use the module’s established logging mechanism.app/db/crud/wireguard.py (1)
32-33: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Make reconciliation memory-bounded.
RECONCILE_USER_CHUNKonly limits eachINquery. The code still materializes every matching tag row, peer-IP user ID, proxy setting, desired allocation, and changed user ID.A large deployment where most users have WireGuard access can still exhaust worker memory. Use keyset pages for users, derive accessible tags per page, and apply each page before loading the next page. Preserve only the global allocation state that reconciliation requires.
Also applies to: 320-362, 541-546, 597-603
🤖 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 `@app/db/crud/wireguard.py` around lines 32 - 33, Refactor the reconciliation flow around RECONCILE_USER_CHUNK to process users in keyset-ordered pages, deriving accessible tags and related WireGuard data only for the current page and applying it before fetching the next page. Avoid materializing all matching tag rows, peer-IP user IDs, proxy settings, desired allocations, or changed user IDs; retain only the global allocation state required across pages, and update the related logic at the indicated reconciliation sections consistently.app/jobs/node_checker.py (1)
315-336: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Align the two multi-worker conditions for remote node stop.
Line 316 gates registration on
server_settings.workers <= 1. Line 334 gates execution onis_multi_worker() and server_settings.workers > 1. The two conditions disagree whenworkers > 1and NATS is disabled.In that configuration
is_multi_worker()is false, so there is no shared lifecycle coordination and no other worker owns the remote core. The execution guard at Line 334 would allow the stop. But registration at Line 316 already skippedshutdown_nodes, so remote cores keep running after shutdown even thoughfeature_settings.stop_nodes_on_shutdownis enabled.Gate registration on the same predicate that the function uses.
🐛 Proposed fix
# Multi-uvicorn workers must not Stop remote cores / clear shared sync queues on exit. - if feature_settings.stop_nodes_on_shutdown and server_settings.workers <= 1: + if feature_settings.stop_nodes_on_shutdown and not (is_multi_worker() and server_settings.workers > 1): on_shutdown(shutdown_nodes)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.# Multi-uvicorn workers must not Stop remote cores / clear shared sync queues on exit. if feature_settings.stop_nodes_on_shutdown and not (is_multi_worker() and server_settings.workers > 1): on_shutdown(shutdown_nodes) on_shutdown(_stop_node_loops) on_shutdown(shutdown_bridge_memory) async def _stop_node_loops(): for task in _node_loop_tasks: task.cancel() if _node_loop_tasks: await asyncio.gather(*_node_loop_tasks, return_exceptions=True) _node_loop_tasks.clear() async def shutdown_nodes(): if not runtime_settings.role.runs_node: return if is_multi_worker() and server_settings.workers > 1: logger.info("Skipping remote node stop on multi-worker shutdown") return🤖 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 `@app/jobs/node_checker.py` around lines 315 - 336, Align shutdown registration with the execution guard by updating the condition around shutdown_nodes in the startup/shutdown registration flow to skip it only when is_multi_worker() and server_settings.workers > 1. Preserve the existing feature_settings.stop_nodes_on_shutdown check and leave shutdown_nodes’ guard unchanged, so workers > 1 with NATS disabled still register and execute remote node stopping.app/node/manager_sync.py (1)
46-73: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Use
db_nodeinside the session scope.
GetDB.__aexit__closes the session (app/db/base.pyLines 46-65). Both theupsertbranch and theconnectbranch usedb_nodeafter theasync with GetDB()block ends, so the instance is detached at that point.
- Line 51:
node_manager.update_node(db_node)readsaddress,port,api_port,server_ca,api_key,name,default_timeout,internal_timeout,proxy_url,usage_coefficient, andidin_create_node_kwargs.- Lines 68 and 72:
update_nodeandNodeOperation.connect_nodereadstatus,name, andkeep_alive.These column attributes are already loaded, so the code works today. Any future access to an unloaded or expired attribute, or to a lazy relationship such as
core_config, raisesMissingGreenleton a closed async session._connect_single_node_localinapp/operation/node.pykeeps the equivalent work inside the session. Match that pattern.♻️ Proposed change
if action == "upsert": async with GetDB() as db: db_node = await get_node_by_id(db, node_id, load_usage_logs=False) - if db_node is None: - return - await node_manager.update_node(db_node) + if db_node is None: + return + await node_manager.update_node(db_node) return if action == "connect": # Quiet attach/start on siblings — originator already wrote DB status / notifications. from app.operation.node import NodeOperation async with GetDB() as db: db_node = await get_node_by_id(db, node_id, load_usage_logs=False) if db_node is None or db_node.status in (NodeStatus.disabled, NodeStatus.limited): return core_id = db_node.core_config_id or 1 cores_by_id, users_by_core = await NodeOperation._get_core_users_map(db, {core_id}) core = cores_by_id.get(core_id) users = users_by_core.get(core_id, []) - try: - await node_manager.update_node(db_node) - except Exception: - logger.exception("Node sync connect update_node failed for node_id=%s", node_id) - return - await NodeOperation.connect_node(db_node, core, users) + try: + await node_manager.update_node(db_node) + except Exception: + logger.exception("Node sync connect update_node failed for node_id=%s", node_id) + return + await NodeOperation.connect_node(db_node, core, users) return📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if action == "upsert": async with GetDB() as db: db_node = await get_node_by_id(db, node_id, load_usage_logs=False) if db_node is None: return await node_manager.update_node(db_node) return if action == "connect": # Quiet attach/start on siblings — originator already wrote DB status / notifications. from app.operation.node import NodeOperation async with GetDB() as db: db_node = await get_node_by_id(db, node_id, load_usage_logs=False) if db_node is None or db_node.status in (NodeStatus.disabled, NodeStatus.limited): return core_id = db_node.core_config_id or 1 cores_by_id, users_by_core = await NodeOperation._get_core_users_map(db, {core_id}) core = cores_by_id.get(core_id) users = users_by_core.get(core_id, []) try: await node_manager.update_node(db_node) except Exception: logger.exception("Node sync connect update_node failed for node_id=%s", node_id) return await NodeOperation.connect_node(db_node, core, users) return🤖 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 `@app/node/manager_sync.py` around lines 46 - 73, Keep all uses of db_node within the corresponding GetDB async session: move node_manager.update_node(db_node) into the upsert session block, and perform the connect branch’s update_node and NodeOperation.connect_node calls before exiting its session while preserving the existing lookup, status filtering, core/user map retrieval, and exception handling.app/node/nats_memory.py (1)
436-444: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard shutdown with
_init_lockand suppress close errors.
shutdown_bridge_memorymutates the same globals thatensure_bridge_memorywrites, but it does not take_init_lock. Two problems follow.
ensure_bridge_memoryruns on everyNodeManager.update_nodecall (app/node/__init__.pyLine 61). If a node update races with shutdown,ensure_bridge_memorycan create a new NATS client aftershutdown_bridge_memoryhas already cleared the globals. That connection is then never closed.await _nc.close()is not wrapped. The failure path at Lines 418-420 usescontextlib.suppress(Exception)for the same call. Ifclose()raises here, the shutdown hook propagates the exception and later hooks registered throughon_shutdownmay not run.Add a shutdown flag so a late
ensure_bridge_memorycall does not re-initialize.🔒️ Proposed fix
+_shutting_down = False + + async def shutdown_bridge_memory() -> None: - global _nc, _user_sync_kv, _lifecycle_kv, _user_sync_store, _lifecycle_coordinator - if _nc is not None: - await _nc.close() - _nc = None - _user_sync_kv = None - _lifecycle_kv = None - _user_sync_store = None - _lifecycle_coordinator = None + global _nc, _user_sync_kv, _lifecycle_kv, _user_sync_store, _lifecycle_coordinator, _shutting_down + async with _init_lock: + _shutting_down = True + if _nc is not None: + with contextlib.suppress(Exception): + await _nc.close() + _nc = None + _user_sync_kv = None + _lifecycle_kv = None + _user_sync_store = None + _lifecycle_coordinator = NoneThen reject late initialization inside
ensure_bridge_memory:async with _init_lock: + if _shutting_down: + return None, None if _user_sync_store is not None and _lifecycle_coordinator is not None: return _user_sync_store, _lifecycle_coordinator🤖 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 `@app/node/nats_memory.py` around lines 436 - 444, Update shutdown_bridge_memory to acquire _init_lock while closing and clearing all bridge globals, suppressing any exception from _nc.close() so shutdown continues. Add a shutdown-state flag and have ensure_bridge_memory check it while holding _init_lock, rejecting late initialization after shutdown has begun; ensure the flag is set atomically with shutdown and preserved across the cleanup.app/operation/node.py (1)
318-332: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not report an error status when another worker owns the lifecycle lease.
The 409 branch attaches once. If the attach fails, execution falls through to Line 334 and returns a result with
NodeStatus.error._connect_single_node_localand_connect_nodes_bulk_localthen writeerrorto the database and dispatchnotification.error_node.A 409 means another worker holds the lease and is starting the node. The attach can legitimately fail during that window, because
pg_node.info()returns no versions until the remote core is listening. The losing worker then raises a false error alert for a node that is starting correctly. ReturningNoneskips the status write and leaves the owner's result authoritative, which matches the existinge.code == -4handling on Line 319.🐛 Proposed fix
if e.code == 409: # Another worker holds the lifecycle lease; try attach once more. attached = await NodeOperation._attach_if_running(pg_node, db_node.name) if attached is not None: return { "node_id": db_node.id, "status": NodeStatus.connected, "message": "", "xray_version": attached.core_version, "node_version": attached.node_version, "old_status": old_status, } + # The lease owner is still starting the node. Let it report the result. + logger.debug(f'Skipping status update for "{db_node.name}"; lease held by another worker') + return None📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.except NodeAPIError as e: if e.code == -4: return None if e.code == 409: # Another worker holds the lifecycle lease; try attach once more. attached = await NodeOperation._attach_if_running(pg_node, db_node.name) if attached is not None: return { "node_id": db_node.id, "status": NodeStatus.connected, "message": "", "xray_version": attached.core_version, "node_version": attached.node_version, "old_status": old_status, } # The lease owner is still starting the node. Let it report the result. logger.debug(f'Skipping status update for "{db_node.name}"; lease held by another worker') return None🤖 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 `@app/operation/node.py` around lines 318 - 332, Update the 409 handling in the node connection flow around NodeOperation._attach_if_running so every failed attach returns None instead of falling through to the error-status result. Preserve the existing successful attach response and the e.code == -4 behavior, ensuring _connect_single_node_local and _connect_nodes_bulk_local skip database error updates and notifications when another worker owns the lifecycle lease.app/subscription/share.py (1)
312-312: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Handle invalid SNI format strings.
Line 312 calls
format_map()without error handling. A malformed configured SNI can raiseValueErrorand fail subscription generation. An undefined variable can also raiseKeyErrorwhen the caller provides a normal mapping.Catch these errors with the same fallback used by subscription metadata formatting, or reject invalid SNI templates when configuration is saved.
🤖 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 `@app/subscription/share.py` at line 312, Update the SNI formatting logic in the subscription generation flow to catch ValueError and KeyError from sni.format_map(format_variables), using the same fallback behavior as subscription metadata formatting. Ensure malformed templates and undefined variables cannot abort subscription generation.pyproject.toml (1)
40-41: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Check published versions and the bridge storage API surface. set -euo pipefail curl -s https://pypi.org/pypi/typer/json | jq -r '.releases | keys[]' | tail -5 curl -s https://pypi.org/pypi/pasarguard-node-bridge/json | jq -r '.info.version, (.releases | keys[])' | tail -10 pip install "pasarguard-node-bridge==0.9.0" >/dev/null 2>&1 || echo "install failed" python - <<'PY' import inspect try: from PasarGuardNodeBridge import storage, create_node except Exception as exc: print("import failed:", exc) raise SystemExit(0) print([n for n in dir(storage) if not n.startswith("_")]) print(inspect.signature(create_node)) PYRepository: PasarGuard/panel
Length of output: 313
🌐 Web query:
PyPI typer 0.27.1 Release Summary💡 Result:
The Typer 0.27.1 release was published on August 3, 2026 [1][2][3]. This release focused on documentation improvements and minor formatting consistency [1][3]. Key changes in version 0.27.1 include: Features: - Formatting: The epilog in help strings was updated to be consistent with other parts of the help output [1][3]. Documentation: - Added new Library Skills documentation [1][3]. - Updated help documentation [1][3]. - Now recommends using uv projects by default [1][3]. - Included a fix to prevent the page from scrolling to the top when using restart or fast buttons in the documentation [1][3]. Internal: - Upgraded the latest-changes dependency to version 0.7.1 [1][3].
Citations:
- 1: https://newreleases.io/project/pypi/typer/release/0.27.1
- 2: https://newreleases.io/project/github/fastapi/typer/release/0.27.1
- 3: https://typer.tiangolo.com/release-notes/
🌐 Web query:
PyPI pasarguard-node-bridge 0.9.0💡 Result:
The PyPI package pasarguard-node-bridge is an asynchronous Python client library designed for connecting to and interacting with a PasarGuard node via gRPC or REST [1][2]. As of August 9, 2026, the available documentation and history on PyPI reflect version 0.8.1 as a recent version [1][3]. There is no official record or release listing for a version 0.9.0 in the public package history [1]. Users should verify their requirements against the latest available version on the official PyPI project page [1]. The library requires Python 3.12 or higher and provides tools for node management, including: - Unified node API for gRPC and protobuf-over-HTTP transport types [3][4]. - User synchronization (single, batch, and chunked streaming) [3][4]. - Health, version, and log streaming helpers [3][4]. - Node maintenance endpoints [3][4]. The source code for this project is maintained on GitHub under the PasarGuard organization [2][4].
Citations:
- 1: https://pypi.org/project/pasarguard-node-bridge/
- 2: https://github.com/PasarGuard/node_bridge_py
- 3: https://pypi.org/project/pasarguard-node-bridge/0.8.1/
- 4: https://github.com/pasarguard/node_bridge_py
🌐 Web query:
PasarGuardNodeBridge storage ClaimedUser LifecycleLease LifecycleOperation LifecycleStatus NodeLifecycleState create_node node_id user_sync_store lifecycle_coordinator worker_id💡 Result:
The PasarGuardNodeBridge is an asynchronous Python client library used to connect and interact with PasarGuard nodes via gRPC or REST protocols [1][2][3]. The parameters mentioned relate to the configuration and management of node lifecycles and user synchronization, particularly in distributed environments [3][4]: - create_node: This is the primary factory method used to instantiate a node client [1][3]. It accepts connection details (address, port, api_port), authentication (API key, server CA), and optional identifiers like node_id and worker_id [1][3]. - user_sync_store: By default, the bridge uses a process-local, in-memory store to queue user updates [3][4]. For multi-process or multi-host deployments, you can provide a custom user_sync_store implementation (e.g., backed by Redis or SQL) that satisfies the UserSyncStoreProtocol, ensuring all workers claim from the same pending-user queue [3][4]. - lifecycle_coordinator: This component manages node operations such as start, stop, and updates (e.g., core, geofiles) [3][4]. Similar to the user sync store, the default is process-local; providing a shared lifecycle_coordinator in distributed setups ensures that only one worker can execute lifecycle operations for a specific node at any given time [3][4]. - node_id and worker_id: These are identifiers passed during node creation [3]. node_id distinguishes the specific node instance (or group of instances sharing a configuration), while worker_id identifies the specific process or host worker responsible for that node [3][4]. - Lifecycle States (ClaimedUser, LifecycleLease, Operation, LifecycleStatus, NodeLifecycleState): These represent the internal models and states used to coordinate asynchronous tasks across distributed workers, ensuring that operations like user syncing and node maintenance remain consistent and conflict-free [3][4]. In summary, these components enable the PasarGuard node bridge to scale beyond a single process by offloading state management and coordination to external backends [3][4].
Citations:
- 1: https://github.com/PasarGuard/node_bridge_py
- 2: https://pypi.org/project/pasarguard-node-bridge/
- 3: https://github.com/pasarguard/node_bridge_py
- 4: https://github.com/Free-Guy-IR/node_bridge_py
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== PyPI package/release metadata ==" python3 - <<'PY' import json import urllib.request for pkg in ["typer", "pasarguard-node-bridge", "PasarGuardNodeBridge"]: try: with urllib.request.urlopen(f"https://pypi.org/pypi/{pkg}/json", timeout=20) as r: data = json.load(r) info = data.get("info", {}) releases = data.get("releases", {}) print(pkg) print(" info_name=", info.get("name")) print(" latest_version=", info.get("version")) print(" release_count=", len(releases)) target="0.27.1" if pkg=="typer" else "0.9.0" print(f" has_{target}=", target in releases) if target in releases: for url in [u["url"] for u in releases[target]][:5]: print(" ", url) except Exception as exc: print(pkg, "ERROR:", repr(exc)) PY echo echo "== Repository references to bridge storage symbols/signature ==" rg -n "PasarGuardNodeBridge|ClaimedUser|LifecycleLease|LifecycleOperation|LifecycleStatus|NodeLifecycleState|create_node|user_sync_store|lifecycle_coordinator" -S . || trueRepository: PasarGuard/panel
Length of output: 11465
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== candidate dependency files ==" git ls-files | rg '(^|/)(pyproject\.toml|requirements.*\.txt|uv\.lock|poetry\.lock|Pipfile|Pipfile\.lock|setup\.py|setup\.cfg)$' || true echo echo "== pyproject relevant lines ==" if [ -f pyproject.toml ]; then sed -n '1,90p' pyproject.toml | cat -n fi echo echo "== lock/package references ==" rg -n 'typer|pasarguard-node-bridge|PasarGuardNodeBridge' -S . --glob 'pyproject.toml' --glob '*.lock' --glob 'requirements*.txt' --glob 'setup.py' --glob 'setup.cfg' || trueRepository: PasarGuard/panel
Length of output: 4042
Use a publishable
pasarguard-node-bridgelower bound.
typer0.27.1 is published and satisfied byuv.lock.pasarguard-node-bridge0.9.0 is not available on PyPI, butpyproject.tomlanduv.lockstill allow it to resolve as a public lower bound. Raise this to a PyPI-published version that exposesPasarGuardNodeBridge.storage ClaimedUser,LifecycleLease,LifecycleOperation,LifecycleStatus,NodeLifecycleState, andcreate_node(node_id, user_sync_store, lifecycle_coordinator, worker_id), or change the dependency source to a local/private version that guarantees these APIs.🤖 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 `@pyproject.toml` around lines 40 - 41, Update the pasarguard-node-bridge dependency declaration to use a PyPI-published version that provides the required storage and lifecycle APIs, including PasarGuardNodeBridge.storage ClaimedUser, LifecycleLease, LifecycleOperation, LifecycleStatus, NodeLifecycleState, and create_node with its expected parameters; alternatively, configure an explicit local or private source guaranteeing those APIs, and regenerate uv.lock accordingly.tests/api/test_core.py (1)
62-69: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the test independent of existing WireGuard inbounds.
create_groupwithoutinbound_tagsincludes all current inbounds. If the test database already has a WireGuard inbound,starter_groupcan allocate a peer IP during user creation. The assertion on Line 113 then fails because it expects only10.70.0.2/32.Create the user without
starter_group. This test only needs the WireGuard group created on Line 98.Proposed fix
- starter = create_core(access_token, name=unique_name("wg_create_starter")) - starter_group = create_group(access_token, name=unique_name("wg_create_starter_group")) user = create_user( access_token, - group_ids=[starter_group["id"]], payload={"username": unique_name("wg_create_user")}, ) @@ delete_user(access_token, user["username"]) - delete_group(access_token, starter_group["id"]) delete_core(access_token, wg_core["id"]) - delete_core(access_token, starter["id"])Also applies to: 112-120
🤖 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 `@tests/api/test_core.py` around lines 62 - 69, Update the test setup around create_group and create_user so the user is created without passing starter_group["id"] in group_ids. Keep starter_group creation for the WireGuard group used later in the test, and preserve the existing assertion flow.tests/test_connect_concurrency.py (1)
40-46: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Drain the notification tasks before the test ends.
_connect_nodes_bulk_localschedulesasyncio.create_task(notification.connect_node(...))for every connected node (app/operation/node.pyLine 700). All 25 nodes returnNodeStatus.connected, so 25 tasks are pending when_connect_nodes_bulk_localreturns. The test finishes immediately and pytest-asyncio closes the loop, which can produce "Task was destroyed but it is pending" warnings. If the project enablesfilterwarnings = error, the test becomes flaky.Yield to the loop once before asserting.
💚 Proposed change
await op._connect_nodes_bulk_local(MagicMock(), nodes) + await asyncio.sleep(0) assert peak <= CONNECT_CONCURRENCY assert peak > 1📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.nodes = [ SimpleNamespace(id=i, status=NodeStatus.connecting, core_config_id=1, name=f"n{i}") for i in range(25) ] await op._connect_nodes_bulk_local(MagicMock(), nodes) await asyncio.sleep(0) assert peak <= CONNECT_CONCURRENCY assert peak > 1🤖 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 `@tests/test_connect_concurrency.py` around lines 40 - 46, Update the test after _connect_nodes_bulk_local returns to yield control to the event loop once, allowing scheduled notification.connect_node tasks to run or drain before assertions. Keep the existing peak concurrency assertions unchanged.tests/test_nats_leader_steal.py (1)
24-25: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Restore the leader module globals after each test.
Both tests assign
leader._is_leaderandleader._tokendirectly and never reset them.try_become_leadersets_is_leader = Trueon success, so the module keeps_is_leader = Trueand a live_tokenafter these tests finish. Other tests that readleader.is_job_leader()or driveleader._heartbeat_loopthen depend on execution order.Add an autouse fixture that saves and restores the module state.
💚 Proposed isolation fixture
from app.nats import leader from role import Role +@pytest.fixture(autouse=True) +def _reset_leader_state(): + saved = (leader._is_leader, leader._token, leader._kv, leader._nc) + yield + leader._is_leader, leader._token, leader._kv, leader._nc = saved + + `@pytest.mark.asyncio` async def test_try_become_leader_falls_through_to_steal_after_generic_create_error():Also applies to: 54-55
🤖 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 `@tests/test_nats_leader_steal.py` around lines 24 - 25, Add an autouse pytest fixture in the test module that snapshots leader._is_leader and leader._token before each test and restores both values in teardown. Apply it to both tests, including the later assignment block, so try_become_leader and direct mutations cannot leak module state into other tests.
|
We never send a success password in notifications only wrong passwords that is because if a server was under brut force attack the admin can know it |
Summary
Validation
pytest -q tests/test_admin_login_notifications.py(4 passed)The existing API harness currently fails before these tests at its test-admin bootstrap (401/503); the focused tests exercise the affected notification boundary directly.
Summary by CodeRabbit
Security Improvements
New Features
Bug Fixes
Tests