Add task disabling support, add self updater#50
Merged
Conversation
XGBoost's XGBClassifier requires scikit-learn. The drive health prediction uses XGBClassifier to load the trained model, but sklearn was missing from dependencies causing the health check service to fail.
… badges and icons
… docs post implementation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds app-update orchestration and CLI, persistent disabled-timer tracking and restore tooling, schedule enable/disable UI and server routes, installer/uninstaller improvements, alert notifier refactor, many tests, and documentation/UI/CSS/template updates. ChangesApp update & CLI
Schedule disabling, restore, and alerts
Task schedule UI and server routes
Installer / Uninstaller changes
Templates, titles, and small UX updates
Tests and docs
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds two major operational features to SimpleSaferServer: (1) the ability to disable/enable scheduled task timers (without preventing manual runs), including persistence + automatic restore of temporary disables, and (2) an application self-updater that inspects the installed Git checkout and runs a systemd-backed update task. It also moves durable runtime state out of the app checkout into /var/lib/SimpleSaferServer and updates the UI (task pages, dashboard, system updates) accordingly.
Changes:
- Add schedule disable/enable support for tasks (persisted state + restore worker timer + UI controls/badges).
- Add “App Update” scheduled task + system updates UI/API to refresh Git status and trigger update/cleanup-update runs.
- Relocate durable state (telemetry, HDSentinel state, apt staging) out of the Git checkout and adjust installer/uninstaller/tests/docs.
Reviewed changes
Copilot reviewed 89 out of 90 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| uninstall.sh | Uninstalls new updater/restore scripts and removes system Git safe.directory entry. |
| install.sh | Installs new scripts/units, ensures Git safe.directory, uses /var/lib for durable data, rsync --delete for app-owned trees. |
| requirements.txt | Adds scikit-learn>=1.6.0. |
| requirements-legacy-py37.txt | Adds scikit-learn==1.0.2 for the legacy lane. |
| simple_safer_server/services/runtime.py | Moves real-mode data_dir to /var/lib/SimpleSaferServer and telemetry path accordingly; extends fake tasks. |
| simple_safer_server/services/task_service.py | Adds schedule state model, log line clamp/limit, “App Update” task, and disable/enable schedule APIs. |
| simple_safer_server/services/disabled_timers.py | New persisted disabled-timer state + restore logic with retry/alert. |
| simple_safer_server/services/system_utils.py | Generates app_update.* units and restore timer; preserves disabled timers on unit refresh. |
| simple_safer_server/services/app_updates.py | New app update manager: Git status caching + update/cleanup-update workflows. |
| simple_safer_server/adapters/app_update_commands.py | Command adapter for git + installer execution (including journald streaming mode). |
| simple_safer_server/routes/tasks.py | Task detail/logs updated to fixed log window; adds API task status and schedule enable/disable routes. |
| simple_safer_server/routes/system_updates.py | Adds application update status/refresh/update/force-update endpoints. |
| simple_safer_server/adapters/systemd.py | Preserves ANSI in journal output (journalctl -a); adds timer enable/disable helpers. |
| simple_safer_server/adapters/command_runner.py | Adds cwd support for subprocess execution. |
| simple_safer_server/services/system_updates.py | Avoids staging apt config inside repo; uses volatile dir for Ubuntu Pro attach config. |
| simple_safer_server/services/drive_health.py | Refactors alert + email send to shared AlertNotifier. |
| simple_safer_server/services/alert_notifications.py | New shared alert notifier for logging + optional email sending. |
| simple_safer_server/app_factory.py | Wires new services (app updates, disabled timers, notifier) + adds browser_title() template helper. |
| simple_safer_server/services/container.py | Adds app_update_manager to service container typing. |
| scripts/app_update.sh | Root-run helper to execute app update Python entrypoint from installed checkout. |
| scripts/app_update.py | Implements systemd task logic (consume mode, check status, run update/cleanup-update). |
| scripts/restore_disabled_timers.py | Root-run helper to restore expired temporary disables periodically. |
| templates/base.html | Uses browser_title() as default title generator. |
| templates/dashboard.html | Renders schedule-aware next-run label/state styling; adds disable/enable schedule actions + modal. |
| templates/task_detail.html | Adds schedule badge, schedule issue notice, manage schedule dropdown, disable modal, and log window metadata. |
| templates/system_updates.html | Adds “Application” panel (status/refresh/update/force update). |
| templates/login.html | Uses browser_title('Sign in'). |
| templates/users.html | Uses browser_title('Users'). |
| templates/cloud_backup.html | Uses browser_title('Cloud Backup'). |
| templates/ddns.html | Uses browser_title('DDNS'). |
| templates/drive_health.html | Uses browser_title('Drive Health'). |
| templates/network_file_sharing.html | Uses browser_title('File Sharing'). |
| templates/alerts.html | Uses browser_title('Alerts'). |
| static/js/scripts.js | Task detail: unified polling for status+logs, schedule actions modal/menu, ANSI log rendering. |
| static/js/system_updates.js | Renders application update status and triggers update/cleanup update flows. |
| static/js/common.js | Enhances confirmation dialog to accept a DOM body node (structured confirmation). |
| static/css/styles.css | Adds schedule badge styles, app cleanup confirmation styles, task schedule modal styles, ANSI log styling, layout tweaks. |
| docs/system_updates.md | Documents application update/cleanup, state locations, and safe.directory handling. |
| docs/task_detail.md | Documents schedule badges/actions and 500-line log window. |
| docs/dashboard.md | Documents schedule states/labels, context menu actions, and App Update task behavior. |
| docs/setup.md | Notes that unit refresh preserves Disable Schedule records. |
| docs/uninstall.md | Notes removal of schedule restore state and related artifacts. |
| docs/manual_install.md | Adds unattended-upgrades dependency, /var/lib state dir, rsync --delete, safe.directory, and new timers. |
| docs/fake_mode.md | Notes Disable/Enable Schedule behavior in fake mode. |
| AGENTS.md | Removes notes/ workflow requirements, leaving docs/development.md as the canonical standard. |
| notes/* | Removes project note templates and historical notes. |
| tests/test_uninstall.py | Verifies new uninstall cleanup for restore units + helper. |
| tests/test_install_preflight.py | Adds coverage for safe.directory helper and installer copy-loop “same file” behavior. |
| tests/test_task_service.py | Adds schedule-state tests, disable/enable tests, log window limit tests, and compact datetime label tests. |
| tests/test_task_routes.py | New tests for task routes: log window clamp, status endpoint, schedule operations. |
| tests/test_task_schedule_ui_rendering.py | New UI render assertions for schedule controls and JS cleanup. |
| tests/test_systemd_adapter.py | New tests for journalctl -a and timer enable/disable commands. |
| tests/test_system_utils.py | Adds app_update + restore schedule timer generation and preserve-disabled behavior tests. |
| tests/test_system_updates.py | Adjusts runtime repo_root usage and verifies apt staging doesn’t write into data_dir. |
| tests/test_system_updates_routes.py | Adds API tests for application update/force update endpoints. |
| tests/test_runtime.py | Verifies real-mode durable data dir and telemetry path location. |
| tests/test_drive_health.py | Verifies HDSentinel state path uses durable data dir. |
| tests/test_disabled_timers.py | New tests for DisabledTimerService state, restore, retry, alert behavior. |
| tests/test_app_updates.py | New tests for AppUpdateManager Git status logic and update/cleanup flows. |
| tests/test_app_factory_routes.py | Refactors fake app creation helpers; adds assertions for schedule UI and dynamic titles. |
| tests/test_app_directory_cleanliness.py | New tests ensuring normal runtime writes don’t dirty the Git checkout and gitignore behavior stays correct. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+112
to
+114
| def restore_expired(self, *, now: Optional[datetime] = None) -> Dict[str, List[str]]: | ||
| now = now or datetime.utcnow() | ||
| restored = [] # type: List[str] |
Comment on lines
+247
to
+259
| def disable_schedule( | ||
| self, | ||
| task: Task, | ||
| mode: str, | ||
| *, | ||
| hours: Optional[int] = None, | ||
| ) -> None: | ||
| expires_at = None | ||
| if mode == "temporary": | ||
| if hours is None: | ||
| raise ValueError("temporary schedule disable requires hours") | ||
| expires_at = datetime.now() + timedelta(hours=hours) | ||
| self.disabled_timer_service.disable( |
Comment on lines
+166
to
+168
| @tasks.route("/task/<task_name>/disable-schedule", methods=["POST"]) | ||
| @admin_required | ||
| def disable_schedule(task_name): |
Comment on lines
+176
to
+197
| data = request.get_json(silent=True) or request.form | ||
| mode = (data.get("mode") or "").strip() | ||
| hours = data.get("hours") | ||
| try: | ||
| parsed_hours = None | ||
| if hours is not None and hours != "": | ||
| parsed_hours = int(hours) | ||
| task.disable_schedule(mode, hours=parsed_hours) | ||
| if request.accept_mimetypes.best == "application/json": | ||
| summary = _get_services().task_service.task_summary(task) | ||
| return json_data({"task": summary}, message=f"Disabled schedule for {task_name}.") | ||
| return redirect(url_for("task_routes.task_detail", task_name=task_name)) | ||
| except Exception: | ||
| current_app.logger.exception("Failed to disable schedule for %s", task_name) | ||
| if request.accept_mimetypes.best == "application/json": | ||
| return json_problem( | ||
| OperationProblem( | ||
| "Could not disable schedule. Check systemd status.", | ||
| slug="task-operation-failed", | ||
| ) | ||
| ) | ||
| abort(500) |
Comment on lines
217
to
232
| function fetchLogs() { | ||
| const distanceFromBottom = logContainer | ||
| ? logContainer.scrollHeight - logContainer.scrollTop - logContainer.clientHeight | ||
| : 0; | ||
| const stickToBottom = distanceFromBottom < 48; | ||
|
|
||
| return fetch(`/task/${encodeURIComponent(taskName)}/logs`) | ||
| .then((resp) => resp.text()) | ||
| return fetch(`/task/${encodeURIComponent(taskName)}/logs?lines=${encodeURIComponent(logLines)}`) | ||
| .then((resp) => { | ||
| if (!resp.ok) throw new Error(`Log refresh failed with HTTP ${resp.status}`); | ||
| return resp.text(); | ||
| }) | ||
| .then((text) => { | ||
| if (logContainer) logContainer.textContent = text; | ||
| failedFetchCount = 0; | ||
| if (refreshState) refreshState.textContent = ""; | ||
| if (logContainer) logContainer.innerHTML = renderAnsiLog(text); | ||
| if (initialLoad) { |
Comment on lines
15
to
+18
| requests==2.31.0 | ||
| urllib3==2.0.7 | ||
| h11==0.14.0 | ||
| scikit-learn==1.0.2 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
simple_safer_server/services/task_service.py (1)
201-206:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEnforce log-line clamping at the service boundary.
Line 205 uses raw
lines; this bypasses the new clamp helper and allows oversized journal reads from any caller path. Clamp here to keep behavior bounded and consistent.Suggested fix
def get_logs(self, task: Task, lines: int = TASK_LOG_LINE_LIMIT) -> str: + lines = clamp_task_log_lines(lines) if self.runtime.is_fake: return self._require_fake_state().get_task_log(task.name) try: return self.systemd_adapter.journal(task.service_name, lines)🤖 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 `@simple_safer_server/services/task_service.py` around lines 201 - 206, The get_logs method currently passes the raw lines parameter to systemd_adapter.journal, allowing callers to bypass the new clamping behavior; update get_logs to clamp the incoming lines value (using the existing clamp helper, e.g., clamp_log_lines or similar helper used elsewhere) before calling self.systemd_adapter.journal(task.service_name, lines) so the journal call always receives a bounded value; keep the fake runtime branch and the CalledProcessError handling unchanged.
🧹 Nitpick comments (6)
scripts/app_update.sh (1)
1-11: ⚡ Quick winConsider adding comments to explain the design.
The script uses several non-obvious patterns (venv fallback,
execto replace the process). Adding brief comments would help future maintainers understand the intent.💡 Example
#!/bin/bash +# App updater entrypoint - exec's the Python updater with the correct interpreter APP_DIR="/opt/SimpleSaferServer" +# Prefer venv Python to ensure correct dependencies; fallback to system Python PYTHON_BIN="$APP_DIR/venv/bin/python" if [ ! -x "$PYTHON_BIN" ]; then PYTHON_BIN="/usr/bin/python3" fi cd "$APP_DIR" || { echo "Error: Cannot access $APP_DIR" >&2; exit 1; } +# exec replaces this shell process with Python, avoiding an extra process exec "$PYTHON_BIN" "$APP_DIR/scripts/app_update.py"Based on learnings: When writing comments and documentation, avoid assuming the reader has seen previous versions of the code. As per coding guidelines: Always sprinkle comments about things we might forget in a few months, or that might not immediately seem obvious on a first read.
🤖 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 `@scripts/app_update.sh` around lines 1 - 11, Add brief inline comments explaining the design choices in the script: document the APP_DIR and PYTHON_BIN variables, the virtualenv fallback logic that checks [ ! -x "$PYTHON_BIN" ] and sets /usr/bin/python3, the safety of cd "$APP_DIR" || exit 1, and the use of exec to replace the shell with the Python process when calling "$PYTHON_BIN" "$APP_DIR/scripts/app_update.py"; place these comments adjacent to the corresponding symbols (APP_DIR, PYTHON_BIN, the venv fallback check, cd command, and exec invocation) so future maintainers understand intent.scripts/restore_disabled_timers.py (1)
8-17: ⚡ Quick winAdd comments explaining the path discovery logic.
The
_add_app_to_pathfunction uses a specific search order (script parent, then/opt/SimpleSaferServer) that might not be obvious to future maintainers.📝 Proposed addition
def _add_app_to_path(): + """Add SimpleSaferServer to sys.path for imports. + + Tries the script's parent directory first (for development), + then falls back to the production install path. + """ script_path = Path(__file__).resolve() candidates = [ - script_path.parents[1], - Path("/opt/SimpleSaferServer"), + script_path.parents[1], # Development: /path/to/repo + Path("/opt/SimpleSaferServer"), # Production install ] for candidate in candidates: if (candidate / "simple_safer_server").exists(): sys.path.insert(0, str(candidate)) returnBased on learnings: Always sprinkle comments about things we might forget in a few months. As per coding guidelines: Always sprinkle comments about things we might forget in a few months, or that might not immediately seem obvious on a first read.
🤖 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 `@scripts/restore_disabled_timers.py` around lines 8 - 17, Add a short explanatory comment above the _add_app_to_path function describing the path discovery/search order and rationale: that it first checks the script's grandparent directory (script_path.parents[1]) to support running from a checked-out repo during development and then falls back to the production install path Path("/opt/SimpleSaferServer"); also note why we look for the "simple_safer_server" package and that inserting the found candidate into sys.path at position 0 ensures it takes precedence.scripts/app_update.py (1)
5-7: ⚡ Quick winAdd a short maintainer note for the
sys.pathbootstrap.This repo-root path injection is important but non-obvious when debugging import behavior; add a brief comment explaining why this direct-run path setup is needed.
As per coding guidelines: "Always sprinkle comments about things we might forget in a few months, or that might not immediately seem obvious on a first read".
🤖 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 `@scripts/app_update.py` around lines 5 - 7, Add a short maintainer comment above the APP_ROOT / sys.path insertion block explaining that APP_ROOT = Path(__file__).resolve().parents[1] and the subsequent sys.path.insert(0, str(APP_ROOT)) are intentionally injecting the repository root into sys.path so the script can be run directly (without installing the package or setting PYTHONPATH) and to help future debuggers understand import resolution behavior; keep it concise (1–2 lines) and mention it’s for direct-run bootstrap of imports.simple_safer_server/services/alert_notifications.py (1)
26-40: ⚡ Quick winAdd intent comments for alert-email gating paths.
The fake-mode suppression and incomplete-email early-return branches encode policy decisions; add short comments so future edits don’t accidentally change the behavior.
💡 Suggested patch
if self._runtime.is_fake: + # Fake mode keeps alert history realistic while avoiding side-effectful + # outbound notifications during development/testing. self._logger.info("Fake mode: suppressing email for alert '%s'", title) return @@ if not email_address or not from_address: + # Keep the alert log entry, but skip sending email until both addresses + # are configured. self._logger.warning("Skipping email alert because email settings are incomplete.") returnAs per coding guidelines:
Always sprinkle comments about things we might forget in a few months, or that might not immediately seem obvious on a first read.🤖 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 `@simple_safer_server/services/alert_notifications.py` around lines 26 - 40, Add short intent comments explaining the two gating/early-return branches in AlertNotifications: one above the self._runtime.is_fake check to indicate fake mode intentionally suppresses outbound emails (used for tests/dev) and one above the email configuration check (email_address/from_address) to indicate we intentionally skip sending when settings are incomplete to avoid mis-sending; reference the existing symbols self._runtime.is_fake, the log message "Fake mode: suppressing email for alert '%s'", and the variables email_address and from_address so reviewers can locate and preserve these policy branches.tests/test_task_service.py (1)
65-113: ⚡ Quick winAdd a brief note explaining the fake systemd contract.
The expanded
FakeSystemdAdapternow encodes severalsystemctl showassumptions (timer state properties, journal call capture, enable/disable side effects). A short comment here would prevent accidental drift between tests and real adapter behavior.As per coding guidelines, “Always sprinkle comments about things we might forget in a few months, or that might not immediately seem obvious on a first read”.
🤖 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_task_service.py` around lines 65 - 113, Add a short explanatory comment above the FakeSystemdAdapter class describing the testing contract it implements: mention that FakeSystemdAdapter simulates systemctl behaviors used by tests (it records start/stop calls in started/stopped lists, captures journal() calls into journal_calls and returns journal_output, models timer enable/disable via enable_timer_now/disable_timer_now populating enabled_timers/disabled_timers, and provides deterministic outputs for show_property() and show_properties() via the properties and multi_properties dicts which encode expected systemctl show keys like NextElapseUSecRealtime, ExecMainStartTimestamp, UnitFileState, ActiveState and monotonic timestamps); indicate that tests rely on these exact keys/values so changes must be mirrored in tests and the real adapter.simple_safer_server/routes/tasks.py (1)
166-225: ⚡ Quick winAdd a short maintainer note for payload/response branching.
These handlers mix JSON and form payloads, then switch JSON vs redirect by
Accept. A brief inline comment would make this behavior easier to preserve during future edits.As per coding guidelines, “Always sprinkle comments about things we might forget in a few months, or that might not immediately seem obvious on a first read”.
🤖 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 `@simple_safer_server/routes/tasks.py` around lines 166 - 225, Add a short maintainer comment in the disable_schedule and enable_schedule functions explaining that request data can come from JSON payloads or HTML forms (data = request.get_json(...) or request.form) and that response behavior branches based on the Accept header (returning JSON via json_data/json_problem when Accept is application/json, otherwise redirect/abort), so future edits preserve both payload parsing and Accept-based response handling; place the comment near the top of each handler (just after retrieving the task) so it’s obvious when modifying payload or response logic.
🤖 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 `@requirements-legacy-py37.txt`:
- Line 18: Update the scikit-learn pin in requirements-legacy-py37.txt: replace
the line "scikit-learn==1.0.2" with a safe minimum of "scikit-learn>=1.5.0" (or
a specific fixed version like "scikit-learn==1.5.0" or later), then re-run
dependency installation and tests to ensure compatibility with functions that
import scikit-learn.
In `@scripts/app_update.sh`:
- Around line 10-11: The cd failure currently exits silently; update the script
(around the cd "$APP_DIR" || exit 1 line in scripts/app_update.sh) to print a
clear error to stderr including the APP_DIR value (e.g., "Failed to cd to
$APP_DIR") before exiting with a non‑zero status, then proceed to the existing
exec "$PYTHON_BIN" "$APP_DIR/scripts/app_update.py"; ensure the error message is
written to stderr (>&2) and preserves the same exit code behavior.
In `@scripts/restore_disabled_timers.py`:
- Around line 29-40: The main function currently calls service.restore_expired()
with no error handling; wrap the call in a try/except that logs start and
success messages and catches Exception around
DisabledTimerService.restore_expired to record failures (use logging.exception
or the module's logger to capture the traceback and error), return a non‑zero
exit code on failure (e.g., return 1) and 0 on success, and ensure you include a
short comment explaining this is important for scheduled task visibility;
reference the main function and DisabledTimerService.restore_expired, and use a
module logger (logging.getLogger(__name__) or runtime logger if available).
In `@simple_safer_server/routes/tasks.py`:
- Around line 176-197: Validate the incoming mode and hours before calling
task.disable_schedule and return a 400 for client errors instead of letting them
fall into the generic exception handler: check mode = (data.get("mode") or
"").strip() is non-empty (and/or matches the allowed values for this task if you
have a canonical set) and parse hours with int() only after verifying it's
numeric and non-negative; if validation fails return a bad-request response
using json_problem(OperationProblem(...), slug=...) when
request.accept_mimetypes.best == "application/json" or abort(400) for HTML
requests. Move these checks out of the broad try/except that surrounds
task.disable_schedule so that only real server-side failures from
task.disable_schedule are caught by the existing exception block, and keep
referencing task.disable_schedule, parsed_hours/hours, mode,
request.accept_mimetypes.best, json_problem, and OperationProblem to locate the
code to change.
In `@simple_safer_server/services/disabled_timers.py`:
- Line 71: The code records "created_at" with datetime.now() but later compares
using UTC (datetime.utcnow()), causing timezone mismatches; update the timestamp
creation to use a single UTC basis (e.g., datetime.utcnow() or preferably
datetime.now(timezone.utc)) and ensure format_timestamp is fed that UTC-aware
datetime so the restore logic (the comparison in the code around the
restore/check routine) uses the same time basis; locate the "created_at":
format_timestamp(datetime.now()) assignment and replace datetime.now() with a
UTC timestamp generator matching the comparison (datetime.utcnow() or
datetime.now(timezone.utc)) and adjust format_timestamp usage if needed.
- Around line 158-165: The updater function update(records: Dict[str, Dict[str,
Any]]) mutates records assuming it's a dict and will raise if persisted state is
malformed (non-dict); change the function to guard/normalize records to an empty
dict when it's not a dict (mirror other methods): ensure you set current =
records.get(timer_name, record) only after converting records to {} if not
isinstance(records, dict), then update current["restore_attempts"],
["last_restore_error"], ["last_restore_attempt_at"], ["restore_failed"], assign
back to records[timer_name], and return records so the updater never tries to
index into a non-dict.
In `@static/css/styles.css`:
- Around line 1236-1239: The CSS rule for the selector .task-schedule-notice
code uses the deprecated value word-break: break-word; — replace that
declaration with overflow-wrap: anywhere; (or overflow-wrap: break-word;) and
remove or change the word-break property to a non-deprecated value if needed,
keeping white-space: pre-wrap intact; update the .task-schedule-notice code rule
accordingly.
In `@tests/test_app_directory_cleanliness.py`:
- Around line 105-126: The git commands in the ignored and not_ignored
subprocess.run calls can fail in CI due to "dubious ownership"; fix both calls
in tests/test_app_directory_cleanliness.py by adding a git config override so
git ignores ownership checks (e.g. insert "-c", f"safe.directory={repo_root}"
immediately after "git" in the argv lists used to create the ignored and
not_ignored subprocess.run invocations), keeping the rest of the subprocess.run
args the same.
In `@tests/test_install_preflight.py`:
- Around line 287-306: The test invokes ensure_git_safe_directory but may run as
non-root in CI so the helper can skip writes; modify the bash snippet passed to
subprocess.run in the test to force root-path behavior by stubbing id -u to
return 0 (e.g., override id or provide a function/alias that echoes 0) before
calling ensure_git_safe_directory "/opt/SimpleSaferServer" so the helper will
perform the write; keep the rest of the snippet structure and then assert
config_path contents as before (referencing ensure_git_safe_directory and
config_path in the test).
In `@tests/test_task_routes.py`:
- Around line 47-48: The assertions in tests/test_task_routes.py assume
render_template was called with keyword args and access
render.call_args.kwargs["log_lines"], which fails when the view passes
positional context; update the test to check render.call_args.args for the
template context (or handle both args and kwargs) when asserting the values for
TASK_LOG_LINE_LIMIT and the task_summary dict (e.g., inspect
render.call_args.args[1] or fallback to render.call_args.kwargs) so the
assertions for TASK_LOG_LINE_LIMIT and {"schedule": {"state": "active"}} are
robust to positional vs keyword template arguments.
---
Outside diff comments:
In `@simple_safer_server/services/task_service.py`:
- Around line 201-206: The get_logs method currently passes the raw lines
parameter to systemd_adapter.journal, allowing callers to bypass the new
clamping behavior; update get_logs to clamp the incoming lines value (using the
existing clamp helper, e.g., clamp_log_lines or similar helper used elsewhere)
before calling self.systemd_adapter.journal(task.service_name, lines) so the
journal call always receives a bounded value; keep the fake runtime branch and
the CalledProcessError handling unchanged.
---
Nitpick comments:
In `@scripts/app_update.py`:
- Around line 5-7: Add a short maintainer comment above the APP_ROOT / sys.path
insertion block explaining that APP_ROOT = Path(__file__).resolve().parents[1]
and the subsequent sys.path.insert(0, str(APP_ROOT)) are intentionally injecting
the repository root into sys.path so the script can be run directly (without
installing the package or setting PYTHONPATH) and to help future debuggers
understand import resolution behavior; keep it concise (1–2 lines) and mention
it’s for direct-run bootstrap of imports.
In `@scripts/app_update.sh`:
- Around line 1-11: Add brief inline comments explaining the design choices in
the script: document the APP_DIR and PYTHON_BIN variables, the virtualenv
fallback logic that checks [ ! -x "$PYTHON_BIN" ] and sets /usr/bin/python3, the
safety of cd "$APP_DIR" || exit 1, and the use of exec to replace the shell with
the Python process when calling "$PYTHON_BIN" "$APP_DIR/scripts/app_update.py";
place these comments adjacent to the corresponding symbols (APP_DIR, PYTHON_BIN,
the venv fallback check, cd command, and exec invocation) so future maintainers
understand intent.
In `@scripts/restore_disabled_timers.py`:
- Around line 8-17: Add a short explanatory comment above the _add_app_to_path
function describing the path discovery/search order and rationale: that it first
checks the script's grandparent directory (script_path.parents[1]) to support
running from a checked-out repo during development and then falls back to the
production install path Path("/opt/SimpleSaferServer"); also note why we look
for the "simple_safer_server" package and that inserting the found candidate
into sys.path at position 0 ensures it takes precedence.
In `@simple_safer_server/routes/tasks.py`:
- Around line 166-225: Add a short maintainer comment in the disable_schedule
and enable_schedule functions explaining that request data can come from JSON
payloads or HTML forms (data = request.get_json(...) or request.form) and that
response behavior branches based on the Accept header (returning JSON via
json_data/json_problem when Accept is application/json, otherwise
redirect/abort), so future edits preserve both payload parsing and Accept-based
response handling; place the comment near the top of each handler (just after
retrieving the task) so it’s obvious when modifying payload or response logic.
In `@simple_safer_server/services/alert_notifications.py`:
- Around line 26-40: Add short intent comments explaining the two
gating/early-return branches in AlertNotifications: one above the
self._runtime.is_fake check to indicate fake mode intentionally suppresses
outbound emails (used for tests/dev) and one above the email configuration check
(email_address/from_address) to indicate we intentionally skip sending when
settings are incomplete to avoid mis-sending; reference the existing symbols
self._runtime.is_fake, the log message "Fake mode: suppressing email for alert
'%s'", and the variables email_address and from_address so reviewers can locate
and preserve these policy branches.
In `@tests/test_task_service.py`:
- Around line 65-113: Add a short explanatory comment above the
FakeSystemdAdapter class describing the testing contract it implements: mention
that FakeSystemdAdapter simulates systemctl behaviors used by tests (it records
start/stop calls in started/stopped lists, captures journal() calls into
journal_calls and returns journal_output, models timer enable/disable via
enable_timer_now/disable_timer_now populating enabled_timers/disabled_timers,
and provides deterministic outputs for show_property() and show_properties() via
the properties and multi_properties dicts which encode expected systemctl show
keys like NextElapseUSecRealtime, ExecMainStartTimestamp, UnitFileState,
ActiveState and monotonic timestamps); indicate that tests rely on these exact
keys/values so changes must be mirrored in tests and the real adapter.
🪄 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
Run ID: e306b2f7-c67e-4e09-a9ae-1181bca02a3c
📒 Files selected for processing (90)
AGENTS.mddocs/dashboard.mddocs/fake_mode.mddocs/manual_install.mddocs/setup.mddocs/system_updates.mddocs/task_detail.mddocs/uninstall.mdinstall.shnotes/0001-monolith-to-package-refactor.mdnotes/0002-system-command-adapters.mdnotes/0003-pr-ci-review-stabilization.mdnotes/0004-legacy-artifact-cleanup.mdnotes/0005-pr-review-feedback-hardening.mdnotes/0006-review-feedback-verification.mdnotes/0007-backup-share-rollback-snapshot.mdnotes/0008-python-security-baseline.mdnotes/0009-review-feedback-hardening.mdnotes/0010-review-feedback-runtime-hardening.mdnotes/0011-coderabbit-review-hardening.mdnotes/0012-livepatch-uninstall-warning.mdnotes/0013-installer-os-preflight.mdnotes/0014-coderabbit-review-loop.mdnotes/0015-python-accessor-style.mdnotes/0016-api-response-contract-refactor.mdnotes/0017-dev-bootstrap-path-fix.mdnotes/0018-review-followup-hardening.mdnotes/0019-ocr-review-loop.mdnotes/0020-file-persistence-pattern.mdnotes/0021-schedule-time-normalizer.mdnotes/0022-subprocess-timeouts.mdnotes/0023-setup-format-drive-gpt.mdnotes/0024-config-manager-locked-updates.mdnotes/0025-review-feedback-runtime-guards.mdnotes/0026-rclone-installer-exit-code.mdnotes/0027-cloud-backup-setup-service-refactor.mdnotes/0028-server-identity-hostname.mdnotes/README.mdnotes/TEMPLATE.mdrequirements-legacy-py37.txtrequirements.txtscripts/app_update.pyscripts/app_update.shscripts/restore_disabled_timers.pysimple_safer_server/adapters/app_update_commands.pysimple_safer_server/adapters/command_runner.pysimple_safer_server/adapters/systemd.pysimple_safer_server/app_factory.pysimple_safer_server/routes/system_updates.pysimple_safer_server/routes/tasks.pysimple_safer_server/services/alert_notifications.pysimple_safer_server/services/app_updates.pysimple_safer_server/services/container.pysimple_safer_server/services/disabled_timers.pysimple_safer_server/services/drive_health.pysimple_safer_server/services/runtime.pysimple_safer_server/services/system_updates.pysimple_safer_server/services/system_utils.pysimple_safer_server/services/task_service.pystatic/css/styles.cssstatic/js/common.jsstatic/js/scripts.jsstatic/js/system_updates.jstemplates/alerts.htmltemplates/base.htmltemplates/cloud_backup.htmltemplates/dashboard.htmltemplates/ddns.htmltemplates/drive_health.htmltemplates/login.htmltemplates/network_file_sharing.htmltemplates/system_updates.htmltemplates/task_detail.htmltemplates/users.htmltests/test_app_directory_cleanliness.pytests/test_app_factory_routes.pytests/test_app_updates.pytests/test_disabled_timers.pytests/test_drive_health.pytests/test_install_preflight.pytests/test_runtime.pytests/test_system_updates.pytests/test_system_updates_routes.pytests/test_system_utils.pytests/test_systemd_adapter.pytests/test_task_routes.pytests/test_task_schedule_ui_rendering.pytests/test_task_service.pytests/test_uninstall.pyuninstall.sh
💤 Files with no reviewable changes (31)
- notes/0020-file-persistence-pattern.md
- notes/TEMPLATE.md
- notes/0021-schedule-time-normalizer.md
- notes/0027-cloud-backup-setup-service-refactor.md
- notes/0015-python-accessor-style.md
- notes/0017-dev-bootstrap-path-fix.md
- notes/0006-review-feedback-verification.md
- notes/0005-pr-review-feedback-hardening.md
- notes/0004-legacy-artifact-cleanup.md
- notes/0012-livepatch-uninstall-warning.md
- notes/0010-review-feedback-runtime-hardening.md
- notes/0019-ocr-review-loop.md
- notes/0003-pr-ci-review-stabilization.md
- notes/0023-setup-format-drive-gpt.md
- notes/0007-backup-share-rollback-snapshot.md
- notes/0016-api-response-contract-refactor.md
- notes/0013-installer-os-preflight.md
- notes/0024-config-manager-locked-updates.md
- notes/0011-coderabbit-review-hardening.md
- notes/0022-subprocess-timeouts.md
- notes/0002-system-command-adapters.md
- notes/0014-coderabbit-review-loop.md
- notes/0028-server-identity-hostname.md
- notes/0001-monolith-to-package-refactor.md
- notes/0009-review-feedback-hardening.md
- notes/README.md
- notes/0008-python-security-baseline.md
- notes/0026-rclone-installer-exit-code.md
- notes/0018-review-followup-hardening.md
- notes/0025-review-feedback-runtime-guards.md
- AGENTS.md
| requests==2.31.0 | ||
| urllib3==2.0.7 | ||
| h11==0.14.0 | ||
| scikit-learn==1.0.2 |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check scikit-learn 1.0.2 availability and security advisories
# Check PyPI for version availability
echo "=== Checking PyPI for scikit-learn 1.0.2 ==="
curl -s https://pypi.org/pypi/scikit-learn/json | jq -r '.releases | keys[]' | grep '^1\.0\.2$' || echo "Version not found"
# Check for security advisories
echo -e "\n=== Checking for security advisories ==="
gh api graphql -f query='
{
securityVulnerabilities(first: 10, ecosystem: PIP, package: "scikit-learn") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'Repository: chrismin13/SimpleSaferServer
Length of output: 830
Upgrade scikit-learn to 1.5.0 or later to patch a sensitive data leakage vulnerability.
scikit-learn 1.0.2 is vulnerable to a moderate-severity sensitive data leakage vulnerability (published June 2024). The fix requires upgrading to version 1.5.0 or later.
🤖 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 `@requirements-legacy-py37.txt` at line 18, Update the scikit-learn pin in
requirements-legacy-py37.txt: replace the line "scikit-learn==1.0.2" with a safe
minimum of "scikit-learn>=1.5.0" (or a specific fixed version like
"scikit-learn==1.5.0" or later), then re-run dependency installation and tests
to ensure compatibility with functions that import scikit-learn.
Comment on lines
+10
to
+11
| cd "$APP_DIR" || exit 1 | ||
| exec "$PYTHON_BIN" "$APP_DIR/scripts/app_update.py" |
Contributor
There was a problem hiding this comment.
Add error message when directory change fails.
The script exits silently if cd fails. Add an error message to help diagnose issues when the app directory is missing or inaccessible.
💬 Proposed fix
-cd "$APP_DIR" || exit 1
+cd "$APP_DIR" || { echo "Error: Cannot access $APP_DIR" >&2; exit 1; }
exec "$PYTHON_BIN" "$APP_DIR/scripts/app_update.py"📝 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.
Suggested change
| cd "$APP_DIR" || exit 1 | |
| exec "$PYTHON_BIN" "$APP_DIR/scripts/app_update.py" | |
| cd "$APP_DIR" || { echo "Error: Cannot access $APP_DIR" >&2; exit 1; } | |
| exec "$PYTHON_BIN" "$APP_DIR/scripts/app_update.py" |
🤖 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 `@scripts/app_update.sh` around lines 10 - 11, The cd failure currently exits
silently; update the script (around the cd "$APP_DIR" || exit 1 line in
scripts/app_update.sh) to print a clear error to stderr including the APP_DIR
value (e.g., "Failed to cd to $APP_DIR") before exiting with a non‑zero status,
then proceed to the existing exec "$PYTHON_BIN"
"$APP_DIR/scripts/app_update.py"; ensure the error message is written to stderr
(>&2) and preserves the same exit code behavior.
Comment on lines
+1236
to
+1239
| .task-schedule-notice code { | ||
| white-space: pre-wrap; | ||
| word-break: break-word; | ||
| } |
Contributor
There was a problem hiding this comment.
Replace deprecated word-break: break-word usage.
Line 1238 uses a deprecated keyword. Use overflow-wrap: anywhere; (or overflow-wrap: break-word;) and keep word-break for non-deprecated values only.
Suggested fix
.task-schedule-notice code {
white-space: pre-wrap;
- word-break: break-word;
+ overflow-wrap: anywhere;
}📝 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.
Suggested change
| .task-schedule-notice code { | |
| white-space: pre-wrap; | |
| word-break: break-word; | |
| } | |
| .task-schedule-notice code { | |
| white-space: pre-wrap; | |
| overflow-wrap: anywhere; | |
| } |
🧰 Tools
🪛 Stylelint (17.11.0)
[error] 1238-1238: Deprecated keyword "break-word" for property "word-break" (declaration-property-value-keyword-no-deprecated)
(declaration-property-value-keyword-no-deprecated)
🤖 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 `@static/css/styles.css` around lines 1236 - 1239, The CSS rule for the
selector .task-schedule-notice code uses the deprecated value word-break:
break-word; — replace that declaration with overflow-wrap: anywhere; (or
overflow-wrap: break-word;) and remove or change the word-break property to a
non-deprecated value if needed, keeping white-space: pre-wrap intact; update the
.task-schedule-notice code rule accordingly.
Contributor
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/test_system_updates_source_switch_ui.py (1)
1-515:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCI is blocked by formatting in this test module.
ruff format --checkis failing for this file in pipeline; please run formatter before merge.🤖 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_system_updates_source_switch_ui.py` around lines 1 - 515, The test module has styling/formatting issues causing ruff format --check to fail; run the project formatter on this file and commit the changes. Open the test functions (e.g. test_dirty_source_switching_stays_visible_but_disabled, test_application_source_switch_copy_uses_operator_wording, test_branch_source_name_uses_inline_code_style, test_source_switch_static_copy_avoids_jargon) and run ruff format (or your configured formatter / pre-commit hooks) to auto-fix whitespace/formatting problems, then re-run ruff format --check and the test suite before pushing.tests/test_task_schedule_ui_rendering.py (1)
1-344:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCI is blocked by formatting in this test module.
ruff format --checkis failing for this file in pipeline; please run formatter before merge.🤖 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_task_schedule_ui_rendering.py` around lines 1 - 344, The test module’s formatting is failing ruff format --check; run the formatter and commit the fixes so the tests pass CI. Run ruff format on the file (or ruff format .) and re-run ruff format --check, then commit the updated file containing the formatted versions of the tests (e.g. functions like test_task_detail_renders_stable_schedule_toolbar_buttons, test_task_detail_renders_strict_custom_duration_modal, test_dashboard_uses_shared_task_schedule_control_modal, etc.); ensure imports and spacing around those tests are fixed and then push the formatted changes.
🤖 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 `@simple_safer_server/routes/system_updates.py`:
- Around line 110-112: The code queues volatile update intents (e.g.,
app_update_manager.request_cleanup_update()) before ensuring the "App Update"
task has successfully started via _get_services().task_service.get_task("App
Update") and task.start(), which can allow stale requests to be processed if
start fails; modify both places (the request_cleanup_update() call at the first
occurrence and the similar calls around lines 182-184) so that you first
retrieve the task, call task.start(), verify it returned/raised no error (or
that the task is in a running/started state), and only then call
app_update_manager.request_cleanup_update() (or the equivalent branch-switch
request) — if task.start() fails, do not persist the intent and handle/log the
error instead.
In `@simple_safer_server/services/app_updates.py`:
- Around line 452-453: The check that calls
self.list_remote_branches(fetch_remote=True) can raise raw process exceptions;
wrap that call in a try/except and convert any exception into an AppUpdateError
so the switch flow always returns a controlled error. Specifically, replace the
direct membership check using list_remote_branches with a try block that calls
branches = self.list_remote_branches(fetch_remote=True) and an except Exception
as e that raises AppUpdateError with a clear message (e.g., "Failed to fetch
remote branches") and include the original exception text (str(e)) for context
before proceeding to the existing if branch not in branches check.
In `@static/js/scripts.js`:
- Around line 182-190: refreshTaskDetail can be invoked again before prior
fetches finish, causing overlapping requests and stale UI writes; modify
refreshTaskDetail/start to prevent concurrent runs by introducing an in-flight
guard (e.g., boolean isRefreshing) or by switching to a self-scheduling pattern:
have start call an async poll loop that awaits fetchTaskStatus() and fetchLogs()
before scheduling the next poll (or setInterval only after the first run
completes), use the existing function names (refreshTaskDetail, fetchTaskStatus,
fetchLogs, start) and the intervalId variable to ensure only one fetch cycle
runs at a time and to avoid stacking requests.
---
Outside diff comments:
In `@tests/test_system_updates_source_switch_ui.py`:
- Around line 1-515: The test module has styling/formatting issues causing ruff
format --check to fail; run the project formatter on this file and commit the
changes. Open the test functions (e.g.
test_dirty_source_switching_stays_visible_but_disabled,
test_application_source_switch_copy_uses_operator_wording,
test_branch_source_name_uses_inline_code_style,
test_source_switch_static_copy_avoids_jargon) and run ruff format (or your
configured formatter / pre-commit hooks) to auto-fix whitespace/formatting
problems, then re-run ruff format --check and the test suite before pushing.
In `@tests/test_task_schedule_ui_rendering.py`:
- Around line 1-344: The test module’s formatting is failing ruff format
--check; run the formatter and commit the fixes so the tests pass CI. Run ruff
format on the file (or ruff format .) and re-run ruff format --check, then
commit the updated file containing the formatted versions of the tests (e.g.
functions like test_task_detail_renders_stable_schedule_toolbar_buttons,
test_task_detail_renders_strict_custom_duration_modal,
test_dashboard_uses_shared_task_schedule_control_modal, etc.); ensure imports
and spacing around those tests are fixed and then push the formatted changes.
🪄 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
Run ID: bc0b39ea-e19d-420d-9314-7a1599de14ee
📒 Files selected for processing (19)
.gitignoredocs/dashboard.mddocs/system_updates.mdscripts/app_update.pysimple_safer_server/routes/system_updates.pysimple_safer_server/services/app_updates.pystatic/css/styles.cssstatic/js/scripts.jsstatic/js/system_updates.jsstatic/js/task_schedule_control.jstemplates/dashboard.htmltemplates/partials/task_schedule_disable_modal.htmltemplates/system_updates.htmltemplates/task_detail.htmltests/test_app_factory_routes.pytests/test_app_updates.pytests/test_system_updates_routes.pytests/test_system_updates_source_switch_ui.pytests/test_task_schedule_ui_rendering.py
✅ Files skipped from review due to trivial changes (2)
- docs/dashboard.md
- docs/system_updates.md
🚧 Files skipped from review as they are similar to previous changes (4)
- templates/system_updates.html
- scripts/app_update.py
- templates/task_detail.html
- tests/test_app_factory_routes.py
Comment on lines
+452
to
+453
| if branch not in self.list_remote_branches(fetch_remote=True): | ||
| raise AppUpdateError("Branch is no longer available. Refresh and try again.") |
Contributor
There was a problem hiding this comment.
Normalize remote-branch fetch failures to AppUpdateError in switch flow.
If remote fetch/list fails here, the method can leak raw process exceptions instead of returning a controlled AppUpdateError.
Suggested fix
- if branch not in self.list_remote_branches(fetch_remote=True):
+ try:
+ remote_branches = self.list_remote_branches(fetch_remote=True)
+ except CalledProcessError as exc:
+ raise AppUpdateError(self._git_failure_detail(exc))
+ except OSError as exc:
+ raise AppUpdateError(str(exc))
+
+ if branch not in remote_branches:
raise AppUpdateError("Branch is no longer available. Refresh and try again.")🤖 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 `@simple_safer_server/services/app_updates.py` around lines 452 - 453, The
check that calls self.list_remote_branches(fetch_remote=True) can raise raw
process exceptions; wrap that call in a try/except and convert any exception
into an AppUpdateError so the switch flow always returns a controlled error.
Specifically, replace the direct membership check using list_remote_branches
with a try block that calls branches =
self.list_remote_branches(fetch_remote=True) and an except Exception as e that
raises AppUpdateError with a clear message (e.g., "Failed to fetch remote
branches") and include the original exception text (str(e)) for context before
proceeding to the existing if branch not in branches check.
Comment on lines
+182
to
190
| function refreshTaskDetail() { | ||
| fetchTaskStatus().catch((err) => console.error(err)); | ||
| fetchLogs(); | ||
| intervalId = setInterval(fetchLogs, 1000); | ||
| } | ||
|
|
||
| function start() { | ||
| refreshTaskDetail(); | ||
| intervalId = setInterval(refreshTaskDetail, 1000); | ||
| } |
Contributor
There was a problem hiding this comment.
Prevent overlapping poll cycles to avoid request pileups and stale UI writes.
The 1-second interval can fire while the previous refresh is still in flight, which can stack requests and apply older responses after newer ones.
Suggested fix
+ let refreshInFlight = false;
+
- function refreshTaskDetail() {
- fetchTaskStatus().catch((err) => console.error(err));
- fetchLogs();
- }
+ async function refreshTaskDetail() {
+ if (refreshInFlight) return;
+ refreshInFlight = true;
+ try {
+ await Promise.all([
+ fetchTaskStatus().catch((err) => console.error(err)),
+ fetchLogs()
+ ]);
+ } finally {
+ refreshInFlight = false;
+ }
+ }
- function start() {
- refreshTaskDetail();
- intervalId = setInterval(refreshTaskDetail, 1000);
- }
+ function start() {
+ refreshTaskDetail();
+ intervalId = setInterval(refreshTaskDetail, 1000);
+ }📝 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.
Suggested change
| function refreshTaskDetail() { | |
| fetchTaskStatus().catch((err) => console.error(err)); | |
| fetchLogs(); | |
| intervalId = setInterval(fetchLogs, 1000); | |
| } | |
| function start() { | |
| refreshTaskDetail(); | |
| intervalId = setInterval(refreshTaskDetail, 1000); | |
| } | |
| let refreshInFlight = false; | |
| async function refreshTaskDetail() { | |
| if (refreshInFlight) return; | |
| refreshInFlight = true; | |
| try { | |
| await Promise.all([ | |
| fetchTaskStatus().catch((err) => console.error(err)), | |
| fetchLogs() | |
| ]); | |
| } finally { | |
| refreshInFlight = false; | |
| } | |
| } | |
| function start() { | |
| refreshTaskDetail(); | |
| intervalId = setInterval(refreshTaskDetail, 1000); | |
| } |
🤖 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 `@static/js/scripts.js` around lines 182 - 190, refreshTaskDetail can be
invoked again before prior fetches finish, causing overlapping requests and
stale UI writes; modify refreshTaskDetail/start to prevent concurrent runs by
introducing an in-flight guard (e.g., boolean isRefreshing) or by switching to a
self-scheduling pattern: have start call an async poll loop that awaits
fetchTaskStatus() and fetchLogs() before scheduling the next poll (or
setInterval only after the first run completes), use the existing function names
(refreshTaskDetail, fetchTaskStatus, fetchLogs, start) and the intervalId
variable to ensure only one fetch cycle runs at a time and to avoid stacking
requests.
Contributor
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Contributor
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Contributor
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/test_task_routes.py (1)
52-53:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake render context assertion resilient to positional args.
Line 52 and Line 53 assume keyword context, but this route path can pass template context positionally; that’s why CI is failing with
TypeError.Proposed fix
- assert render.call_args.kwargs["log_lines"] == TASK_LOG_LINE_LIMIT - assert render.call_args.kwargs["task_summary"] == {"schedule": {"state": "active"}} + context = ( + render.call_args.args[1] + if len(render.call_args.args) > 1 + else render.call_args.kwargs + ) + assert context["log_lines"] == TASK_LOG_LINE_LIMIT + assert context["task_summary"] == {"schedule": {"state": "active"}}🤖 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_task_routes.py` around lines 52 - 53, Tests assume render() context is passed as kwargs but the view may pass it positionally; make assertions resilient by retrieving the context from render.call_args: if call_args.kwargs is non-empty use it, otherwise search render.call_args.args for the first dict-like argument (the template context) and assign it to a variable (e.g., ctx). Then assert ctx["log_lines"] == TASK_LOG_LINE_LIMIT and ctx["task_summary"] == {"schedule": {"state": "active"}}; reference render.call_args, TASK_LOG_LINE_LIMIT and the task_summary dict when updating the assertions.
🤖 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.
Duplicate comments:
In `@tests/test_task_routes.py`:
- Around line 52-53: Tests assume render() context is passed as kwargs but the
view may pass it positionally; make assertions resilient by retrieving the
context from render.call_args: if call_args.kwargs is non-empty use it,
otherwise search render.call_args.args for the first dict-like argument (the
template context) and assign it to a variable (e.g., ctx). Then assert
ctx["log_lines"] == TASK_LOG_LINE_LIMIT and ctx["task_summary"] == {"schedule":
{"state": "active"}}; reference render.call_args, TASK_LOG_LINE_LIMIT and the
task_summary dict when updating the assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3f2ad16b-0160-4c98-ad59-46d618f935fc
📒 Files selected for processing (11)
docs/system_updates.mdscripts/restore_disabled_timers.pysimple_safer_server/routes/tasks.pysimple_safer_server/services/app_updates.pystatic/js/system_updates.jstemplates/system_updates.htmltests/test_app_updates.pytests/test_restore_disabled_timers.pytests/test_system_updates_source_switch_ui.pytests/test_task_routes.pytests/test_task_schedule_ui_rendering.py
✅ Files skipped from review due to trivial changes (1)
- docs/system_updates.md
🚧 Files skipped from review as they are similar to previous changes (7)
- templates/system_updates.html
- scripts/restore_disabled_timers.py
- tests/test_system_updates_source_switch_ui.py
- static/js/system_updates.js
- simple_safer_server/routes/tasks.py
- tests/test_task_schedule_ui_rendering.py
- simple_safer_server/services/app_updates.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@simple_safer_server/routes/tasks.py`:
- Around line 182-184: The handler currently calls request.get_json(silent=True)
and assumes the result is a dict, causing AttributeError for non-object JSON;
update the logic around data to first capture raw =
request.get_json(silent=True), then set data = raw if isinstance(raw, dict) else
(request.form or {}), and then use data.get("mode") and data.get("hours") so
non-object JSON payloads are rejected via your existing validation path rather
than raising when calling data.get; refer to request.get_json, the data
variable, and the mode/hours extraction in this change.
- Around line 172-173: Routes disable_schedule and enable_schedule currently use
`@api_admin_required` which returns JSON on expired sessions and breaks HTML
redirects; replace the decorator with `@admin_required` on both functions
(disable_schedule and enable_schedule) so the handlers can perform content
negotiation (they already check request.accept_mimetypes.best ==
"application/json" and call redirect() for HTML) and behave like
start_task/stop_task for dashboard UI and API clients.
In `@simple_safer_server/services/task_service.py`:
- Around line 251-268: The disable_schedule method currently accepts arbitrary
mode and non-positive hours; update Task.disable_schedule to validate inputs:
only allow known modes (e.g., "temporary" and "permanent") and raise ValueError
for any other mode, require hours to be provided and >0 when mode == "temporary"
(and ensure hours is None for non-temporary modes), and prevent creating
already-expired disables by computing expires_at = utc_now() +
timedelta(hours=hours) only after validating hours; keep the call to
disabled_timer_service.disable(task.name, task.timer_name, mode=mode,
expires_at=expires_at).
- Around line 205-207: get_logs currently ignores the lines parameter in the
fake runtime branch by returning the full fake buffer from
_require_fake_state().get_task_log(task.name); update the fake branch in
get_logs to honor the lines argument (clamp it to TASK_LOG_LINE_LIMIT) and
either call a get_task_log overload that accepts a lines/count parameter or
post-process the returned log string to return only the last N lines (where N =
min(lines, TASK_LOG_LINE_LIMIT)). Ensure you reference get_logs,
_require_fake_state().get_task_log, TASK_LOG_LINE_LIMIT and the lines parameter
when making the change.
🪄 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
Run ID: b00b0825-406c-4ea3-b5fd-c28372825fd9
📒 Files selected for processing (11)
simple_safer_server/routes/system_updates.pysimple_safer_server/routes/tasks.pysimple_safer_server/services/app_updates.pysimple_safer_server/services/disabled_timers.pysimple_safer_server/services/task_service.pytests/test_app_updates.pytests/test_disabled_timers.pytests/test_restore_disabled_timers.pytests/test_system_updates_routes.pytests/test_task_routes.pytests/test_task_service.py
🚧 Files skipped from review as they are similar to previous changes (9)
- tests/test_restore_disabled_timers.py
- tests/test_disabled_timers.py
- tests/test_system_updates_routes.py
- tests/test_task_routes.py
- simple_safer_server/routes/system_updates.py
- tests/test_app_updates.py
- simple_safer_server/services/disabled_timers.py
- tests/test_task_service.py
- simple_safer_server/services/app_updates.py
Comment on lines
+172
to
+173
| @tasks.route("/task/<task_name>/disable-schedule", methods=["POST"]) | ||
| @api_admin_required |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== admin_required / api_admin_required definitions ==="
fd -i 'user_manager.py' | while read -r file; do
sed -n '1,260p' "$file"
done
echo
echo "=== route decorators and handlers ==="
rg -nC3 'admin_required|api_admin_required|disable-schedule|enable-schedule|start_task|stop_task' simple_safer_server
echo
echo "=== template or client references to the schedule routes ==="
rg -nC2 'disable-schedule|enable-schedule' simple_safer_serverRepository: chrismin13/SimpleSaferServer
Length of output: 50384
🏁 Script executed:
#!/bin/bash
# Extract disable_schedule and enable_schedule function implementations
sed -n '172,220p' simple_safer_server/routes/tasks.py
echo "=== enable_schedule function ==="
sed -n '220,246p' simple_safer_server/routes/tasks.pyRepository: chrismin13/SimpleSaferServer
Length of output: 3504
🏁 Script executed:
#!/bin/bash
# Search for template and JavaScript usage of disable-schedule and enable-schedule
find simple_safer_server -name "*.html" -o -name "*.js" | head -20
# Search for references to the schedule endpoints
rg 'disable-schedule|enable-schedule' simple_safer_server --type html --type js --max-count=20Repository: chrismin13/SimpleSaferServer
Length of output: 54
🏁 Script executed:
#!/bin/bash
# Find template and static files
find simple_safer_server -type f \( -name "*.html" -o -name "*.js" \) 2>/dev/null | head -30
# Search more broadly for schedule endpoint references
rg 'disable-schedule|enable-schedule' --max-count=20 2>/dev/null || echo "No matches found"
# Check if there are template files at all
ls -la simple_safer_server/templates/ 2>/dev/null | head -20Repository: chrismin13/SimpleSaferServer
Length of output: 1925
Use @admin_required for these routes, not @api_admin_required, since they serve both HTML (dashboard UI) and content-negotiated responses.
The functions at lines 172-173 (disable_schedule) and 220-221 (enable_schedule) are called from templates/dashboard.html context menu actions and include redirect() branches for HTML responses. When an admin's session expires, @api_admin_required returns JSON errors instead of redirecting to login, breaking the expected browser flow. The functions already handle content negotiation (request.accept_mimetypes.best == "application/json"), so they can serve both UI and API clients correctly with @admin_required, consistent with start_task and stop_task.
🤖 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 `@simple_safer_server/routes/tasks.py` around lines 172 - 173, Routes
disable_schedule and enable_schedule currently use `@api_admin_required` which
returns JSON on expired sessions and breaks HTML redirects; replace the
decorator with `@admin_required` on both functions (disable_schedule and
enable_schedule) so the handlers can perform content negotiation (they already
check request.accept_mimetypes.best == "application/json" and call redirect()
for HTML) and behave like start_task/stop_task for dashboard UI and API clients.
Comment on lines
+182
to
+184
| data = request.get_json(silent=True) or request.form | ||
| mode = (data.get("mode") or "").strip() | ||
| hours = data.get("hours") |
Contributor
There was a problem hiding this comment.
Reject non-object JSON bodies before calling .get().
request.get_json(silent=True) can return any valid JSON type. Payloads like "temporary" or ["temporary"] are truthy, so data.get(...) raises AttributeError and returns 500 instead of the validation error path.
Proposed fix
- data = request.get_json(silent=True) or request.form
+ json_payload = request.get_json(silent=True)
+ if json_payload is not None:
+ if not isinstance(json_payload, dict):
+ return _bad_disable_schedule_request("Request body must be a JSON object.")
+ data = json_payload
+ else:
+ data = request.form
mode = (data.get("mode") or "").strip()
hours = data.get("hours")🤖 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 `@simple_safer_server/routes/tasks.py` around lines 182 - 184, The handler
currently calls request.get_json(silent=True) and assumes the result is a dict,
causing AttributeError for non-object JSON; update the logic around data to
first capture raw = request.get_json(silent=True), then set data = raw if
isinstance(raw, dict) else (request.form or {}), and then use data.get("mode")
and data.get("hours") so non-object JSON payloads are rejected via your existing
validation path rather than raising when calling data.get; refer to
request.get_json, the data variable, and the mode/hours extraction in this
change.
Comment on lines
+205
to
207
| def get_logs(self, task: Task, lines: int = TASK_LOG_LINE_LIMIT) -> str: | ||
| if self.runtime.is_fake: | ||
| return self._require_fake_state().get_task_log(task.name) |
Contributor
There was a problem hiding this comment.
Honor the lines argument in fake mode too.
task_logs() now clamps callers to TASK_LOG_LINE_LIMIT, but the fake branch still returns the full log buffer. That makes fake mode diverge from production and can turn task detail responses unbounded in dev/tests.
Proposed fix
def get_logs(self, task: Task, lines: int = TASK_LOG_LINE_LIMIT) -> str:
if self.runtime.is_fake:
- return self._require_fake_state().get_task_log(task.name)
+ logs = self._require_fake_state().get_task_log(task.name)
+ return "\n".join(logs.splitlines()[-lines:])
try:
return self.systemd_adapter.journal(task.service_name, lines)📝 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.
Suggested change
| def get_logs(self, task: Task, lines: int = TASK_LOG_LINE_LIMIT) -> str: | |
| if self.runtime.is_fake: | |
| return self._require_fake_state().get_task_log(task.name) | |
| def get_logs(self, task: Task, lines: int = TASK_LOG_LINE_LIMIT) -> str: | |
| if self.runtime.is_fake: | |
| logs = self._require_fake_state().get_task_log(task.name) | |
| return "\n".join(logs.splitlines()[-lines:]) |
🤖 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 `@simple_safer_server/services/task_service.py` around lines 205 - 207,
get_logs currently ignores the lines parameter in the fake runtime branch by
returning the full fake buffer from
_require_fake_state().get_task_log(task.name); update the fake branch in
get_logs to honor the lines argument (clamp it to TASK_LOG_LINE_LIMIT) and
either call a get_task_log overload that accepts a lines/count parameter or
post-process the returned log string to return only the last N lines (where N =
min(lines, TASK_LOG_LINE_LIMIT)). Ensure you reference get_logs,
_require_fake_state().get_task_log, TASK_LOG_LINE_LIMIT and the lines parameter
when making the change.
Comment on lines
+251
to
+268
| def disable_schedule( | ||
| self, | ||
| task: Task, | ||
| mode: str, | ||
| *, | ||
| hours: Optional[int] = None, | ||
| ) -> None: | ||
| expires_at = None | ||
| if mode == "temporary": | ||
| if hours is None: | ||
| raise ValueError("temporary schedule disable requires hours") | ||
| expires_at = utc_now() + timedelta(hours=hours) | ||
| self.disabled_timer_service.disable( | ||
| task.name, | ||
| task.timer_name, | ||
| mode=mode, | ||
| expires_at=expires_at, | ||
| ) |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Validate the disable-schedule contract in the service layer.
Task.disable_schedule() is now a public entry point, but this layer still accepts arbitrary mode values and non-positive hours. One internal caller bypassing the route can create an already-expired or unknown disable state.
Proposed fix
def disable_schedule(
self,
task: Task,
mode: str,
*,
hours: Optional[int] = None,
) -> None:
+ if mode not in {"temporary", "permanent"}:
+ raise ValueError("mode must be 'temporary' or 'permanent'")
expires_at = None
if mode == "temporary":
- if hours is None:
- raise ValueError("temporary schedule disable requires hours")
+ if hours is None or hours <= 0:
+ raise ValueError("temporary schedule disable requires positive hours")
expires_at = utc_now() + timedelta(hours=hours)
self.disabled_timer_service.disable(
task.name,
task.timer_name,
mode=mode,🤖 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 `@simple_safer_server/services/task_service.py` around lines 251 - 268, The
disable_schedule method currently accepts arbitrary mode and non-positive hours;
update Task.disable_schedule to validate inputs: only allow known modes (e.g.,
"temporary" and "permanent") and raise ValueError for any other mode, require
hours to be provided and >0 when mode == "temporary" (and ensure hours is None
for non-temporary modes), and prevent creating already-expired disables by
computing expires_at = utc_now() + timedelta(hours=hours) only after validating
hours; keep the call to disabled_timer_service.disable(task.name,
task.timer_name, mode=mode, expires_at=expires_at).
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
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 by CodeRabbit
New Features
Bug Fixes
Documentation
Tests