feat: global extension registration via register_job(room='@global')#884
Merged
feat: global extension registration via register_job(room='@global')#884
Conversation
Covers register_job room="@global" support, deprecated public param, deprecated register_extension shim, GLOBAL_ROOM constant, and test plan. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- public=False is now treated same as public=None (no conflict check) - Add explicit warnings.warn for public=True to match existing codebase pattern Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
9 tasks covering GLOBAL_ROOM constant, register_job room="@global", deprecated public param, deprecated register_extension shim, and comprehensive test coverage including cross-room visibility. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…_extension shim Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds client-side global extension registration: introduces Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (ZnDraw)
participant API as register_job()
participant Validator as Validation Logic
participant Registry as Job Registry
participant Server as Server (Auth)
Client->>API: register_job(cls, room="@global")
activate API
API->>Validator: validate public vs room
Validator-->>API: ok / raise ValueError
API->>Validator: is room == "@global"?
Validator-->>API: yes (skip resolution)
API->>Registry: register(cls, room="@global")
activate Registry
Registry->>Server: submit registration request
activate Server
Server->>Server: check admin permission
alt admin
Server-->>Registry: 200 OK
Registry-->>API: success
API-->>Client: registration complete
else not admin
Server-->>Registry: 403 Forbidden
Registry-->>API: propagate PermissionError
API-->>Client: error
end
deactivate Server
deactivate Registry
deactivate API
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
for more information, see https://pre-commit.ci
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/zndraw/client/core.py (1)
623-625: Consider a local sentinel constant instead of repeating"@global"literals.Using a module-level constant (e.g.
_GLOBAL_ROOM) here reduces drift risk betweenregister_job()andregister_extension().Also applies to: 642-642
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/zndraw/client/core.py` around lines 623 - 625, Introduce a module-level sentinel constant (e.g. _GLOBAL_ROOM = "@global") and replace the repeated string literal occurrences with that constant; update usages in methods that reference the global room (including where _resolve_room is called and in register_job and register_extension) so checks and assignments use _GLOBAL_ROOM instead of the string literal to avoid drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/superpowers/plans/2026-03-20-global-extension-registration.md`:
- Line 15: The markdown jumps from a level-1 heading to a level-3 for "### Task
1: Export `GLOBAL_ROOM` constant" (MD001); fix by adding a level-2 heading
(e.g., "## Tasks" or change the task heading to "## Task 1: Export `GLOBAL_ROOM`
constant") so heading levels are incremental—update the "### Task 1: Export
`GLOBAL_ROOM` constant" heading accordingly to maintain proper hierarchy.
In `@src/zndraw/client/core.py`:
- Around line 634-643: register_extension currently swallows unexpected kwargs
and bypasses register_job's conflict check by blindly extracting only
kwargs.get("room"); update register_extension to validate inputs: if public is
True and "room" is in kwargs raise the same ValueError register_job would raise,
and if kwargs contains any keys other than "room" raise a TypeError (or
propagate) so unexpected keyword args aren't ignored; finally call
self.register_job(cls, room=room, **remaining_kwargs) only after validation so
register_job's own checks still apply.
In `@tests/worker/test_register_job_api.py`:
- Around line 23-151: Multiple tests repeat the same pattern of registering Echo
in GLOBAL_ROOM and asserting visibility (test_register_job_global,
test_register_extension_public, test_register_job_public_deprecated,
test_guest_cannot_register_global_via_register_job,
test_admin_can_register_global_via_register_job); refactor by creating
parametric tests using pytest.mark.parametrize that iterate over the different
registration call variants (worker.register_job with room=GLOBAL_ROOM,
worker.register_extension with public=True, worker.register_job with
public=True) and over auth roles (unauthenticated guest vs admin) while keeping
assertions for DeprecationWarning when expected and PermissionError for guests;
locate uses of ZnDraw, register_job, register_extension, get_job_list and the
tests named above and replace duplicate setup/assert blocks with a single
parametrized test function that receives (caller_variant, kwargs,
expected_exception_or_warning_flag) and performs the same
connect/register/get_job_list/name assertion logic.
---
Nitpick comments:
In `@src/zndraw/client/core.py`:
- Around line 623-625: Introduce a module-level sentinel constant (e.g.
_GLOBAL_ROOM = "@global") and replace the repeated string literal occurrences
with that constant; update usages in methods that reference the global room
(including where _resolve_room is called and in register_job and
register_extension) so checks and assignments use _GLOBAL_ROOM instead of the
string literal to avoid drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 463d03a7-5399-47ad-afe0-5d273b9c3d46
📒 Files selected for processing (5)
docs/superpowers/plans/2026-03-20-global-extension-registration.mddocs/superpowers/specs/2026-03-20-global-extension-registration-design.mdsrc/zndraw/__init__.pysrc/zndraw/client/core.pytests/worker/test_register_job_api.py
Comment on lines
+23
to
+151
| def test_register_job_global(server, Echo, get_job_list): | ||
| """register_job(cls, room='@global') registers a global job.""" | ||
| worker = ZnDraw(url=server) | ||
| try: | ||
| worker.register_job(Echo, room=GLOBAL_ROOM) | ||
| jobs = get_job_list(worker, room_id="@global") | ||
| names = {j.name for j in jobs} | ||
| assert "Echo" in names | ||
| finally: | ||
| worker.jobs.disconnect() | ||
| worker.disconnect() | ||
|
|
||
|
|
||
| # ============================================================================= | ||
| # Deprecated register_extension shim | ||
| # ============================================================================= | ||
|
|
||
|
|
||
| def test_register_extension_public(server, Echo, get_job_list): | ||
| """Deprecated register_extension(cls, public=True) registers globally.""" | ||
| worker = ZnDraw(url=server) | ||
| try: | ||
| with warnings.catch_warnings(record=True) as w: | ||
| warnings.simplefilter("always") | ||
| worker.register_extension(Echo, public=True) | ||
| assert any(issubclass(x.category, DeprecationWarning) for x in w) | ||
| jobs = get_job_list(worker, room_id="@global") | ||
| names = {j.name for j in jobs} | ||
| assert "Echo" in names | ||
| finally: | ||
| worker.jobs.disconnect() | ||
| worker.disconnect() | ||
|
|
||
|
|
||
| # ============================================================================= | ||
| # Deprecated public= parameter on register_job | ||
| # ============================================================================= | ||
|
|
||
|
|
||
| def test_register_job_public_deprecated(server, Echo, get_job_list): | ||
| """register_job(cls, public=True) works but emits DeprecationWarning.""" | ||
| worker = ZnDraw(url=server) | ||
| try: | ||
| with warnings.catch_warnings(record=True) as w: | ||
| warnings.simplefilter("always") | ||
| worker.register_job(Echo, public=True) | ||
| dep_warnings = [x for x in w if issubclass(x.category, DeprecationWarning)] | ||
| assert len(dep_warnings) >= 1 | ||
| assert "room='@global'" in str(dep_warnings[0].message) | ||
| jobs = get_job_list(worker, room_id="@global") | ||
| names = {j.name for j in jobs} | ||
| assert "Echo" in names | ||
| finally: | ||
| worker.jobs.disconnect() | ||
| worker.disconnect() | ||
|
|
||
|
|
||
| # ============================================================================= | ||
| # ValueError when both room and public are set | ||
| # ============================================================================= | ||
|
|
||
|
|
||
| def test_register_job_room_and_public_raises(server, Echo): | ||
| """Passing both room= and public=True raises ValueError.""" | ||
| vis = ZnDraw(url=server) | ||
| try: | ||
| with pytest.raises(ValueError, match="Cannot specify both"): | ||
| vis.register_job(Echo, room=vis.room, public=True) | ||
| finally: | ||
| vis.disconnect() | ||
|
|
||
|
|
||
| # ============================================================================= | ||
| # Global extension visible in all rooms | ||
| # ============================================================================= | ||
|
|
||
|
|
||
| def test_global_extension_visible_in_all_rooms(server, Echo, get_job_list): | ||
| """A @global extension registered via register_job is visible from any room.""" | ||
| registrar = ZnDraw(url=server) | ||
| room_a = ZnDraw(url=server) | ||
| room_b = ZnDraw(url=server) | ||
| try: | ||
| registrar.register_job(Echo, room=GLOBAL_ROOM) | ||
|
|
||
| # Verify visible from room_a | ||
| jobs_a = get_job_list(room_a, room_id=room_a.room) | ||
| global_a = [j for j in jobs_a if j.full_name.startswith("@global")] | ||
| names_a = {j.name for j in global_a} | ||
| assert "Echo" in names_a | ||
|
|
||
| # Verify visible from room_b (different room) | ||
| jobs_b = get_job_list(room_b, room_id=room_b.room) | ||
| global_b = [j for j in jobs_b if j.full_name.startswith("@global")] | ||
| names_b = {j.name for j in global_b} | ||
| assert "Echo" in names_b | ||
| finally: | ||
| registrar.jobs.disconnect() | ||
| registrar.disconnect() | ||
| room_a.disconnect() | ||
| room_b.disconnect() | ||
|
|
||
|
|
||
| # ============================================================================= | ||
| # Auth Mode — register_job API | ||
| # ============================================================================= | ||
|
|
||
|
|
||
| def test_guest_cannot_register_global_via_register_job(server_auth, Echo): | ||
| """Guest user gets 403 when using register_job(room='@global').""" | ||
| guest = ZnDraw(url=server_auth) | ||
| try: | ||
| with pytest.raises(PermissionError): | ||
| guest.register_job(Echo, room=GLOBAL_ROOM) | ||
| finally: | ||
| guest.disconnect() | ||
|
|
||
|
|
||
| def test_admin_can_register_global_via_register_job(server_auth, Echo, get_job_list): | ||
| """Admin user can register global jobs via register_job.""" | ||
| admin = ZnDraw(url=server_auth, user="admin@local.test", password="adminpassword") | ||
| try: | ||
| admin.register_job(Echo, room=GLOBAL_ROOM) | ||
| jobs = get_job_list(admin, room_id="@global") | ||
| names = {j.name for j in jobs} | ||
| assert "Echo" in names | ||
| finally: | ||
| admin.jobs.disconnect() | ||
| admin.disconnect() |
There was a problem hiding this comment.
Please parametrize repeated scenarios to match test guidelines.
These tests repeat very similar setup/assert patterns (global registration path variants and auth role variants). Consolidating with pytest.mark.parametrize will reduce duplication and make failures easier to read.
As per coding guidelines "Use pytest.mark.parametrize for tests. Each test must be a function, not a class method" and "Write specific tests that test one thing. Avoid complex setups".
🧰 Tools
🪛 Ruff (0.15.6)
[error] 143-143: Possible hardcoded password assigned to argument: "password"
(S106)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/worker/test_register_job_api.py` around lines 23 - 151, Multiple tests
repeat the same pattern of registering Echo in GLOBAL_ROOM and asserting
visibility (test_register_job_global, test_register_extension_public,
test_register_job_public_deprecated,
test_guest_cannot_register_global_via_register_job,
test_admin_can_register_global_via_register_job); refactor by creating
parametric tests using pytest.mark.parametrize that iterate over the different
registration call variants (worker.register_job with room=GLOBAL_ROOM,
worker.register_extension with public=True, worker.register_job with
public=True) and over auth roles (unauthenticated guest vs admin) while keeping
assertions for DeprecationWarning when expected and PermissionError for guests;
locate uses of ZnDraw, register_job, register_extension, get_job_list and the
tests named above and replace duplicate setup/assert blocks with a single
parametrized test function that receives (caller_variant, kwargs,
expected_exception_or_warning_flag) and performs the same
connect/register/get_job_list/name assertion logic.
Member
Author
There was a problem hiding this comment.
Disagree — each test covers a distinct scenario with different assertions and fixtures:
test_register_job_global: no warning assertionstest_register_extension_public: checks DeprecationWarning existstest_register_job_public_deprecated: checks specific warning message content- Auth tests use
server_authfixture and assertPermissionError
Parametrizing would require conditional branches inside the test body, which conflicts with "test one thing" and "avoid complex setups." Keeping them separate.
There was a problem hiding this comment.
@PythonFZ, that's a fair point — you're right that each test has meaningfully different assertion semantics (no warnings, warning existence, warning message content, PermissionError, different fixtures). Forcing them into a single parametrized function would introduce conditional branches inside the test body, which would genuinely conflict with "test one thing" and "avoid complex setups." I withdraw the suggestion; the current structure is appropriate.
🧠 Learnings used
Learnt from: CR
Repo: zincware/ZnDraw PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-06T10:07:24.874Z
Learning: Applies to tests/**/*.py : Use `pytest.mark.parametrize` for tests. Each test must be a function, not a class method
Learnt from: CR
Repo: zincware/ZnDraw PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-06T10:07:24.874Z
Learning: Applies to tests/**/*.py : Write specific tests that test one thing. Avoid complex setups
Learnt from: CR
Repo: zincware/ZnDraw PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-06T10:07:24.874Z
Learning: Applies to tests/**/*.py : You MUST NEVER use `pytest.mark.xfail` or similar — all tests must pass
Learnt from: PythonFZ
Repo: zincware/ZnDraw PR: 877
File: tests/test_constraints.py:138-140
Timestamp: 2026-03-19T10:02:26.990Z
Learning: In the ZnDraw project, when initializing `FrameStorage` (and the historical `AsebytesStorage`/`asebytes` registry) for the LMDB backend, bare filesystem paths (e.g., `str(tmp_path / "test.lmdb")`) are valid and follow the project convention. Do not require or enforce a URI scheme for these LMDB path arguments, and do not flag bare LMDB paths as “invalid URI”. Only flag URI-scheme issues when the target backend is not expecting bare LMDB paths (i.e., when a scheme is required by the specific storage/registry being used).
- README: fix vis.register() → vis.register_job(), add room="@global" note - python-api.rst: rewrite extensions section with registering, scopes, running, worker lifecycle, and GLOBAL_ROOM constant - SKILL.md: mention room="@global" for global registration Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
docs/source/python-api.rst (1)
1057-1059: UseGLOBAL_ROOMin scope examples to avoid drift.The scope table currently shows
register_job(cls, room="@global"). Preferroom=GLOBAL_ROOMin docs examples for consistency with the public API export and to prevent string-literal mistakes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/source/python-api.rst` around lines 1057 - 1059, Update the docs to use the exported constant instead of the string literal: replace examples using register_job(cls, room="@global") with register_job(cls, room=GLOBAL_ROOM) (and update any other occurrences showing "@global") so the examples reference the public API symbol GLOBAL_ROOM and avoid string-literal drift; keep the explanatory text the same and ensure GLOBAL_ROOM is shown/imported where needed in example snippets.README.md (1)
167-167: PreferGLOBAL_ROOMin docs examples for consistency.This line hardcodes
room="@global". Consider documentingfrom zndraw import GLOBAL_ROOMand usingroom=GLOBAL_ROOMas the primary pattern to reduce sentinel-string typos.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 167, Update the README example to prefer using the GLOBAL_ROOM constant instead of the literal string sentinel by showing import and usage: reference adding "from zndraw import GLOBAL_ROOM" and changing the example call from vis.register_job(cls, room="@global") to vis.register_job(cls, room=GLOBAL_ROOM); mention GLOBAL_ROOM as the recommended pattern to avoid sentinel-string typos and keep the vis.register_job usage consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/source/python-api.rst`:
- Around line 1057-1059: Update the docs to use the exported constant instead of
the string literal: replace examples using register_job(cls, room="@global")
with register_job(cls, room=GLOBAL_ROOM) (and update any other occurrences
showing "@global") so the examples reference the public API symbol GLOBAL_ROOM
and avoid string-literal drift; keep the explanatory text the same and ensure
GLOBAL_ROOM is shown/imported where needed in example snippets.
In `@README.md`:
- Line 167: Update the README example to prefer using the GLOBAL_ROOM constant
instead of the literal string sentinel by showing import and usage: reference
adding "from zndraw import GLOBAL_ROOM" and changing the example call from
vis.register_job(cls, room="@global") to vis.register_job(cls,
room=GLOBAL_ROOM); mention GLOBAL_ROOM as the recommended pattern to avoid
sentinel-string typos and keep the vis.register_job usage consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1f34980f-8a16-4775-b529-3ecc6f1e9420
📒 Files selected for processing (3)
README.mddocs/source/python-api.rstskills/zndraw/SKILL.md
✅ Files skipped from review due to trivial changes (1)
- skills/zndraw/SKILL.md
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #884 +/- ##
==========================================
+ Coverage 91.22% 91.31% +0.08%
==========================================
Files 179 180 +1
Lines 17240 17354 +114
==========================================
+ Hits 15727 15846 +119
+ Misses 1513 1508 -5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Address code review feedback: - register_extension() now rejects unexpected kwargs with TypeError and enforces public+room conflict with ValueError - Fix heading level jump (MD001) in plan doc Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- register_job: use `public is not None` to catch explicit False+room - Parametrize room_and_public_raises test for True and False - Add test for register_extension room-scoped default path - Add test for register_extension unexpected kwargs TypeError - Restore missing smiles2atoms import in README example Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
GLOBAL_ROOMconstant ("@global") exported fromzndrawfor easy global job registrationregister_job()to acceptroom="@global"(admin-only, server enforces 403) and a deprecatedpublicparam for migration from v0.6.0register_extension()shim that delegates toregister_job(), easing migration fromvis.register_extension(cls, public=True)Test Plan
register_job(cls, room='@global')registers a global job (dev mode)register_extension(cls, public=True)emits DeprecationWarning and registers globallyregister_job(cls, public=True)emits DeprecationWarning and registers globallyregister_job(cls, room=r, public=True)raises ValueErrorroom='@global'register_job🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Deprecations
publicparameter deprecated — use room="@global" instead.Documentation
Tests