Repository navigation
Implement project management - #37
Conversation
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis change adds locked, file-backed project and facade-key management across storage, runtime registry, API, CLI, and Web UI. It also updates asynchronous identity resolution, key inventory presentation, documentation, and tests for validation, concurrency, recovery, authorization, and secret handling. ChangesProject and facade-key management
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant ProjectsUI
participant FastAPI
participant ProjectKeyManager
participant FacadeKeyRegistry
Operator->>ProjectsUI: submit project or key action
ProjectsUI->>FastAPI: send same-origin JSON mutation
FastAPI->>ProjectKeyManager: execute locked file-backed mutation
ProjectKeyManager->>FacadeKeyRegistry: refresh runtime snapshot
FacadeKeyRegistry-->>FastAPI: updated project/key state
FastAPI-->>ProjectsUI: return mutation result
ProjectsUI-->>Operator: display fingerprint or one-time secret
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a thread- and process-safe, file-backed project and facade-key management system. It adds new API endpoints and Web UI controls to create and delete projects, as well as create and revoke facade API keys. Additionally, it integrates this management layer into the application container and CLI tool, backed by comprehensive unit tests. The review feedback highlights a potential cross-platform issue on macOS where calling fsync on a directory file descriptor might raise an OSError (such as EINVAL or ENOTSUP), and suggests catching and ignoring these specific errors to ensure compatibility.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| try: | ||
| os.fsync(directory_fd) | ||
| except OSError: | ||
| if os.name != "nt": | ||
| raise |
There was a problem hiding this comment.
On some platforms (such as macOS) or specific filesystems, calling fsync on a directory file descriptor is not supported and raises an OSError with EINVAL (Invalid argument). Since os.name != "nt" is checked, this error will be re-raised, causing all project and key mutation operations to fail on macOS. To ensure cross-platform compatibility, we should catch and ignore EINVAL, ENOTSUP, and EOPNOTSUPP errors during directory fsync.
| try: | |
| os.fsync(directory_fd) | |
| except OSError: | |
| if os.name != "nt": | |
| raise | |
| try: | |
| os.fsync(directory_fd) | |
| except OSError as exc: | |
| import errno | |
| if exc.errno not in {errno.EINVAL, errno.ENOTSUP, errno.EOPNOTSUPP}: | |
| raise |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
smolrouter/templates/project_detail.html (1)
251-256: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated JSON-mutation/error-handling logic across the two project templates. Both files independently re-implement the same fetch →
response.json()→if (!response.ok) throwpattern for calling/api/project-management/*; extracting a shared helper (e.g., a small included<script>partial) would avoid drift as more mutation endpoints are added.
smolrouter/templates/project_detail.html#L251-L256: promote the existingmutate()helper into a shared script partial included by both templates.smolrouter/templates/projects.html#L219-L224: replace the inline fetch/parse/throw block with a call to the sharedmutate()helper.🤖 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 `@smolrouter/templates/project_detail.html` around lines 251 - 256, The JSON mutation and error-handling logic is duplicated across both project templates. Extract the existing mutate helper from smolrouter/templates/project_detail.html lines 251-256 into a shared script partial included by both templates, then update smolrouter/templates/projects.html lines 219-224 to call mutate() instead of reimplementing fetch, JSON parsing, and error handling; preserve the current behavior and signatures.smolrouter/facade_key_cli.py (1)
122-127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHandle
ProjectKeyManagementErrorand sourcesecret_countfrom the actual write.
manager.create_key(...)can raiseProjectKeyConflictError/ProjectKeyNotFoundError/OSError(e.g. concurrent routes change), which would surface as an unhandled traceback instead of the friendly exit-code style used earlier inrun_create. Also,secret_countis printed from the stale in-memoryupdatedcomputed before delegation rather than from what was persisted.(Requires importing `ProjectKeyManagementError` alongside `ProjectKeyManager`.)♻️ Suggested handling
- manager = ProjectKeyManager(routes_path=routes_path, secrets_path=secret_file_path) - generated, _key_id = manager.create_key(project_id, generated) + manager = ProjectKeyManager(routes_path=routes_path, secrets_path=secret_file_path) + try: + generated, _key_id = manager.create_key(project_id, generated) + except ProjectKeyManagementError as exc: + print(f"Failed to create facade key: {exc}") + return 4 print(f"Generated facade key for project: {project_id}") print(f"secret (copy exactly once): {generated}") print(f"wrote facade-key secrets to: {secret_file_path}") - print(f"secret_count: {len(updated.get(project_id, []))}") + print(f"secret_count: {len(manager.list_keys(project_id))}") return 0🤖 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 `@smolrouter/facade_key_cli.py` around lines 122 - 127, Update run_create around ProjectKeyManager.create_key to catch ProjectKeyManagementError and exit using the existing friendly error/exit-code handling pattern, including conflicts, missing projects, and persistence OSError failures. Replace the stale updated-based secret_count output with the count returned or otherwise exposed by the successful persisted write, and import ProjectKeyManagementError alongside ProjectKeyManager.
🤖 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.
Nitpick comments:
In `@smolrouter/facade_key_cli.py`:
- Around line 122-127: Update run_create around ProjectKeyManager.create_key to
catch ProjectKeyManagementError and exit using the existing friendly
error/exit-code handling pattern, including conflicts, missing projects, and
persistence OSError failures. Replace the stale updated-based secret_count
output with the count returned or otherwise exposed by the successful persisted
write, and import ProjectKeyManagementError alongside ProjectKeyManager.
In `@smolrouter/templates/project_detail.html`:
- Around line 251-256: The JSON mutation and error-handling logic is duplicated
across both project templates. Extract the existing mutate helper from
smolrouter/templates/project_detail.html lines 251-256 into a shared script
partial included by both templates, then update
smolrouter/templates/projects.html lines 219-224 to call mutate() instead of
reimplementing fetch, JSON parsing, and error handling; preserve the current
behavior and signatures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c27e6cf3-bdf7-4855-90b2-6af751247409
📒 Files selected for processing (9)
README.mdSTATUS.mdsmolrouter/app.pysmolrouter/container.pysmolrouter/facade_key_cli.pysmolrouter/project_key_manager.pysmolrouter/templates/project_detail.htmlsmolrouter/templates/projects.htmltests/unit/test_project_key_manager.py
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 120239f357
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| raw_projects = routes.get("facade_keys", {}) | ||
| if not isinstance(raw_projects, dict): | ||
| raise ProjectKeyManagementError("routes facade_keys must be a mapping") |
There was a problem hiding this comment.
When the routes file contains an empty placeholder like facade_keys: (YAML parses this as None), this check now raises ProjectKeyManagementError during default container startup because SmolRouterContainer() calls ProjectKeyManager.get_registry(). Previously SmolRouterConfig.__post_init__ normalized None facade keys to {}, so installs with no configured projects but a blank section stop serving until they hand-edit it to facade_keys: {}; treat None the same as a missing key here.
Useful? React with 👍 / 👎.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #37 +/- ##
==========================================
+ Coverage 75.66% 76.29% +0.63%
==========================================
Files 37 38 +1
Lines 10280 10736 +456
Branches 1600 1653 +53
==========================================
+ Hits 7778 8191 +413
- Misses 1913 1943 +30
- Partials 589 602 +13 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Summary by CodeRabbit