feat(server): harden deployment and add log management - #195
Conversation
|
Warning Review limit reached
Next review available in: 31 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThe change makes GREMLIN deployment settings mandatory, replaces persistent token signing with an ephemeral key, adds scheduled log rotation and archive limits, introduces an admin-only log viewer, updates bootstrap behavior and documentation, and extracts Gist manifest generation into a standalone tool. ChangesGREMLIN server operations
Gist manifest publishing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Admin
participant LogViewer
participant GREMLINRoutes
participant LogDirectory
Admin->>LogViewer: Open Server Logs
LogViewer->>GREMLINRoutes: Request active log
GREMLINRoutes->>LogDirectory: Validate and stream managed file
LogDirectory-->>GREMLINRoutes: Log chunks
GREMLINRoutes-->>LogViewer: Stream response
Admin->>LogViewer: Expand rotated archives
LogViewer->>GREMLINRoutes: Request archive metadata
GREMLINRoutes->>LogDirectory: List validated ZIP archives
LogDirectory-->>GREMLINRoutes: Grouped archive metadata
GREMLINRoutes-->>LogViewer: Render archive tree and download links
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| Python |  |
Jul 29, 2026 3:06a.m. | Review ↗ |
Important
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| BestPractice | 1 medium |
🟢 Metrics 161 complexity · 1 duplication
Metric Results Complexity 161 Duplication 1
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![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: f8da9f61b1
ℹ️ 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".
| if set(_bootstrap_passwords) != ADMIN_USERS: | ||
| raise RuntimeError( | ||
| "Bootstrap credentials for every ADMIN_USERS entry are required for an empty " | ||
| "user database; start the deployment with restart_pssm_flask.sh" | ||
| ) |
There was a problem hiding this comment.
up
On a fresh installation started with the documented restart_pssm_flask.sh up subcommand or the equivalent direct Docker Compose command, ADMIN_BOOTSTRAP_CREDENTIALS is unset because only cmd_restart generates it. The newly created user database is empty, so this check raises during application import and Gunicorn repeatedly fails to start. Generate bootstrap credentials for every supported first-start path, or reject up before launching when the database is empty.
Useful? React with 👍 / 👎.
| id=f"{self.id}-thresholds", | ||
| replace_existing=True, | ||
| coalesce=True, | ||
| max_instances=self.max_instances, | ||
| **self._threshold_args, |
There was a problem hiding this comment.
When both a scheduled period and a line/size threshold are configured, this registers the threshold invocation under a separate job ID from the cron invocation. APScheduler's max_instances=1 therefore applies independently to each job, so a long threshold rotation can overlap the cron rotation (especially after misfires or while compressing large logs). Both executions can ZIP and truncate the same active file concurrently, producing incomplete archives and discarding writes; guard the rotation method with a shared lock or combine the triggers into one serialized job.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/dev-guide/server.md (1)
390-393: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale "First run" description contradicts the new multi-admin bootstrap.
This step still describes a single default
adminuser, but the PR changes bootstrap to create every configuredADMIN_USERSentry with a distinct, transient password (see README.md's "First run" section andpssm_gremlin.py's bootstrap loop). This section wasn't updated alongside the rest of this file's environment-variable table changes.📝 Suggested fix
3. **Configure authentication**: - - On first run, a default admin user is created automatically (username: `admin`, password auto-generated and displayed by `restart_pssm_flask.sh`). - - Change the admin password immediately via the Profile page. + - On first run, every username listed in `ADMIN_USERS` is created automatically, each with a distinct, transient password printed once by `restart_pssm_flask.sh`. + - Change each admin password immediately via the Profile page. - Optionally enable self-registration with `ENABLE_REGISTER=true` and either SMTP or Resend settings.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/dev-guide/server.md` around lines 390 - 393, Update the “Configure authentication” section to describe the multi-admin bootstrap: every configured ADMIN_USERS entry is created with a distinct transient password, and the generated credentials are displayed by restart_pssm_flask.sh. Remove the outdated single default admin/password wording while retaining the instruction to change credentials and the optional ENABLE_REGISTER guidance.
🧹 Nitpick comments (2)
server/pssm_gremlin_server/pssm_gremlin.py (1)
37-38: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winFlask session secret now reuses auth.py's private token-signing key.
app.secret_keyis set fromauth._SECRET_KEY(imported here as a private, underscore-prefixed symbol), so the same ephemeral secret now signs both Flask's session cookie and the bearer/verification/reset tokens produced byauth._serializer. Previously these were independent secrets.itsdangerous's per-purpose salting mitigates cross-use risk, but merging two distinct security domains onto one secret is worth a second look, and importing a leading-underscore name across module boundaries is fragile.Please confirm this reuse is intentional (and that no other Flask feature relying on
secret_key, e.g. CSRF/flash, assumes an independently-rotatable secret). Consider generatingapp.secret_keyindependently (e.g. its ownsecrets.token_hex(32)at import time) if separation is desired, and exporting a non-underscored accessor fromauth.pyif the shared value is intentional.♻️ Optional: decouple the two secrets
-app.secret_key = app.secret_key or _TOKEN_SIGNING_KEY +app.secret_key = app.secret_key or secrets.token_hex(32)Also applies to: 86-87
🤖 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 `@server/pssm_gremlin_server/pssm_gremlin.py` around lines 37 - 38, Decouple Flask’s session secret from auth.py’s private _SECRET_KEY by generating an independent app.secret_key with the module’s existing secrets support, and remove the cross-module import alias _TOKEN_SIGNING_KEY. Keep auth._SECRET_KEY dedicated to auth._serializer token signing and preserve the existing Flask configuration behavior.server/pssm_gremlin_server/templates/log_viewer.html (1)
32-40: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueIncomplete ARIA tab pattern on the log toolbar.
The toolbar declares
role="tablist"but its children are plain<button>s withoutrole="tab"/aria-selected, and#logOutputlacksrole="tabpanel". Buttons remain keyboard-operable natively, so this is polish rather than a blocker.🤖 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 `@server/pssm_gremlin_server/templates/log_viewer.html` around lines 32 - 40, The log toolbar’s ARIA tab pattern is incomplete. Update the log-select buttons in the log-toolbar to use role="tab" with appropriate aria-selected states, and assign role="tabpanel" to `#logOutput`; keep the refreshLog button outside the tab semantics.
🤖 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 `@Makefile`:
- Around line 84-90: Update the Gist target’s trap command to use the
POSIX-compatible exit signal 0 instead of the environment-specific EXIT,
preserving the existing cleanup command and recipe flow.
- Around line 83-90: Replace the predictable /tmp/manifest.json usage in the
Makefile target with a unique directory created via mktemp -d, ensure it
contains only manifest.json, and generate the manifest at that path. Update the
cleanup trap to remove the temporary directory and pass the generated manifest
file from that directory to gh gist edit in both the existing-file and add-file
branches.
In `@server/pssm_gremlin_server/maintenance/tasks/log_rotation.py`:
- Around line 60-72: Update the log-rotation flow around _prune_oldest_archives
so archives created during the current invocation are excluded from deletion.
Track and pass the newly rotated archive(s) as an exemption when pruning,
allowing the size cap to be temporarily exceeded rather than deleting the only
surviving copy of the log.
- Around line 88-97: Update the size-based rotation logic in the loop over
`directory.glob("*.log")` to re-evaluate `_managed_log_size(directory) >
max_size` for each log instead of relying on the upfront `rotate_for_size`
boolean. Preserve the existing line-count, periodic, and empty-file checks so
size rotation stops once the aggregate total is back under `max_size`.
In `@server/run/restart_pssm_flask.sh`:
- Around line 356-374: Update the ADMIN_USERS bootstrap loop around
_configured_admins and _admin_username to track usernames already encountered
and reject duplicates before generating or printing credentials. On a duplicate,
emit an error and exit nonzero immediately; preserve the existing whitespace
trimming, empty-entry skipping, and credential generation for unique usernames.
In `@server/tests/test_process_isolation.py`:
- Line 149: Remove the unnecessary f-string prefix from the assertion’s literal
error message in the process-isolation test, leaving the expected text and
assertion behavior unchanged.
---
Outside diff comments:
In `@docs/dev-guide/server.md`:
- Around line 390-393: Update the “Configure authentication” section to describe
the multi-admin bootstrap: every configured ADMIN_USERS entry is created with a
distinct transient password, and the generated credentials are displayed by
restart_pssm_flask.sh. Remove the outdated single default admin/password wording
while retaining the instruction to change credentials and the optional
ENABLE_REGISTER guidance.
---
Nitpick comments:
In `@server/pssm_gremlin_server/pssm_gremlin.py`:
- Around line 37-38: Decouple Flask’s session secret from auth.py’s private
_SECRET_KEY by generating an independent app.secret_key with the module’s
existing secrets support, and remove the cross-module import alias
_TOKEN_SIGNING_KEY. Keep auth._SECRET_KEY dedicated to auth._serializer token
signing and preserve the existing Flask configuration behavior.
In `@server/pssm_gremlin_server/templates/log_viewer.html`:
- Around line 32-40: The log toolbar’s ARIA tab pattern is incomplete. Update
the log-select buttons in the log-toolbar to use role="tab" with appropriate
aria-selected states, and assign role="tabpanel" to `#logOutput`; keep the
refreshLog button outside the tab semantics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3b091c15-fdd3-4392-bfb4-1b2a3ec36bf5
📒 Files selected for processing (26)
CHANGELOG.mdMakefiledocs/dev-guide/server.mdserver/.env.exampleserver/README.mdserver/docker-compose.ymlserver/pssm_gremlin_server/auth.pyserver/pssm_gremlin_server/config.pyserver/pssm_gremlin_server/maintenance/manager.pyserver/pssm_gremlin_server/maintenance/tasks/log_rotation.pyserver/pssm_gremlin_server/pssm_gremlin.pyserver/pssm_gremlin_server/routes.pyserver/pssm_gremlin_server/static/css/log-viewer.cssserver/pssm_gremlin_server/static/js/log-viewer.jsserver/pssm_gremlin_server/templates/log_viewer.htmlserver/pssm_gremlin_server/templates/pssm_gremlin_dashboard.htmlserver/run/restart_pssm_flask.shserver/tests/conftest.pyserver/tests/test_admin.pyserver/tests/test_config.pyserver/tests/test_database_backup.pyserver/tests/test_log_rotation.pyserver/tests/test_maintenance_manager.pyserver/tests/test_process_isolation.pyserver/tests/test_security_advanced.pytools/generate_gist_manifest.py
| ) | ||
|
|
||
| assert result.returncode != 0 | ||
| assert f"Missing required setting(s)" in result.stderr |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove extraneous f prefix (Ruff F541).
No placeholders in this f-string.
🧹 Fix
- assert f"Missing required setting(s)" in result.stderr
+ assert "Missing required setting(s)" in result.stderr📝 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.
| assert f"Missing required setting(s)" in result.stderr | |
| assert "Missing required setting(s)" in result.stderr |
🧰 Tools
🪛 Ruff (0.16.0)
[error] 149-149: f-string without any placeholders
Remove extraneous f prefix
(F541)
🤖 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 `@server/tests/test_process_isolation.py` at line 149, Remove the unnecessary
f-string prefix from the assertion’s literal error message in the
process-isolation test, leaving the expected text and assertion behavior
unchanged.
Source: Linters/SAST tools
1 participant
Summary
Why
Deployment previously retained unsafe implicit defaults and configurable signing-secret infrastructure, while server logs could grow without bounds and were not inspectable from the admin web interface. This change fails closed on required deployment settings, keeps bootstrap credentials and signing material transient, and adds bounded, administrator-only log operations.
Operational impact
SERVER_DIR,DB_UNIREF30,DB_UNIREF90, andADMIN_USERSare mandatoryROTATE_LOG_MAX_LINENO,ROTATE_LOG_PERIOD, andMAX_LOG_SIZEare all unsetROTATE_LOG_PERIODaccepts a quoted five-field crontab expression such as"0 0 * * *"Validation
make -C server test— 259 passedgit diff --checkSummary by CodeRabbit
New Features
Bug Fixes