[18.0][ADD] cetmix_tower_server_monitor: base module add new module#487
Open
marionumza wants to merge 20 commits intocetmix:18.0from
Open
[18.0][ADD] cetmix_tower_server_monitor: base module add new module#487marionumza wants to merge 20 commits intocetmix:18.0from
marionumza wants to merge 20 commits intocetmix:18.0from
Conversation
…ic push agent - Replaced monitor_interval with differentiated pull (minutes) and push (seconds) intervals. - Refactored push agent to run as a persistent systemd service. - Implemented dynamic interval updates: agents now adapt to frequency changes without redeploy. - Updated dashboard JS to support real-time polling (seconds) in push mode. - Fixed UI consistency: all RAM measurements now display in GB. - Improved code quality: fixed linting issues (E501, F821, W8113) and unused variables.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new Odoo module Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
marionumza
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 22
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cetmix_tower_server_monitor/.pre-commit-config.yaml`:
- Line 1: The nested pre-commit config
cetmix_tower_server_monitor/.pre-commit-config.yaml is not discovered by default
because the CI runs pre-commit run --all-files from the repo root; either update
the CI/workflow command to explicitly pass the config (-c
cetmix_tower_server_monitor/.pre-commit-config.yaml) wherever pre-commit is
invoked (e.g., replace pre-commit run --all-files with pre-commit run
--all-files -c cetmix_tower_server_monitor/.pre-commit-config.yaml), or merge
the hooks from cetmix_tower_server_monitor/.pre-commit-config.yaml into the root
.pre-commit-config.yaml so they run without the -c flag and update any project
README/docs to reflect the chosen approach.
- Around line 132-138: The pylint hooks are currently passing bare filenames
(--rcfile=.pylintrc and --rcfile=.pylintrc-mandatory) which pre-commit resolves
relative to the repo root; update the args for the hooks (eg. the entries with
id: pylint and id: pylint_odoo) to use repo-root-relative paths that point to
the addon-local rcfiles instead of the bare filenames so pre-commit loads the
intended local configs.
In `@cetmix_tower_server_monitor/.pylintrc`:
- Around line 103-118: Remove the stray double comma in the messages list (the
token "too-complex,,") and deduplicate the repeated message IDs so each rule
appears only once; specifically remove the empty item caused by "too-complex,,"
and delete duplicate entries for create-user-wo-reset-password,
dangerous-filter-wo-user, file-not-used, missing-newline-extrafiles,
old-api7-method-defined, unnecessary-utf8-coding-comment,
consider-merging-classes-inherited, invalid-commit, missing-readme, and
odoo-addons-relative-import so the re-enable block contains unique message IDs
only.
In `@cetmix_tower_server_monitor/controllers/main.py`:
- Around line 43-50: The server lookup in controllers/main.py currently searches
by reference OR name which can misidentify servers; change the query in the
request.env["cx.tower.server"].sudo().search call to match only
("reference","=", server_ref) so the endpoint resolves by the stable external
identifier, and additionally ensure any code that enables push metrics verifies
the server has a non-empty reference before accepting push (i.e., add a
pre-check that reference is set on the record returned by
request.env["cx.tower.server"] or reject the request).
- Around line 55-57: The token comparison in controllers/main.py using
"server.monitor_token != token" is susceptible to timing attacks; replace it
with a constant-time comparison using hmac.compare_digest by importing hmac and
comparing str(server.monitor_token or "") and str(token or "") (or otherwise
coercing both to the same string type) before logging/returning; keep the same
_logger.warning and error response behavior but perform the secure compare
instead of the plain != check.
- Around line 87-98: The current handler returns raw exception text and swallows
errors; update get_dashboard_metrics to first call server.ensure_one() to guard
against multi-records, then wrap only the get_dashboard_data call in a
try/except that logs the full traceback (e.g. using _logger.exception or
logging.exception with server_id and other context) and returns a generic client
message like {"status":"error","message":"Internal server error"}; do not return
str(e) to the client and avoid a broad silent failure—log the exception and
return the generic error instead.
- Around line 10-80: The route should use HTTP body parsing instead of the
JSON-RPC handler: change the decorator on monitor_push from type="json" to
type="http", remove usage of request.dispatcher.jsonrequest, and parse the raw
request body with the stdlib json loader (e.g. data =
json.loads(request.httprequest.get_data(as_text=True))) to extract server_ref,
token and metrics; keep the rest of the logic (creating cx.tower.server.metrics,
writing last_metrics_id and calling server._check_monitor_alerts) and return the
same dict including server.monitor_interval_push. Ensure you import json and
handle JSON decode errors to return a clear error response.
In `@cetmix_tower_server_monitor/models/cx_tower_server_metrics.py`:
- Around line 9-18: Add a DB index to speed dashboard queries: set index=True on
the server_id field and add a model-level _indexes entry creating a composite
index on (server_id, timestamp) so queries from get_dashboard_data and
_cron_pull_metrics filtered by server_id and ordered by timestamp DESC are
backed by the DB; locate the fields server_id and timestamp in
cx_tower_server_metrics (and add the _indexes dict on the model class) to
implement this.
In `@cetmix_tower_server_monitor/models/cx_tower_server.py`:
- Around line 215-242: The cron uses last_metrics_id.timestamp while
_compute_monitor_push_status uses last_metrics_id.create_date, causing
inconsistent behavior when agent clock skews; update _cron_pull_metrics to use
last_metrics_id.create_date (the Odoo-controlled timestamp) instead of
.timestamp when computing delta so both _cron_pull_metrics and
_compute_monitor_push_status use the same source of truth (create_date) for
deciding re-polling and status computation; change the reference in the loop
inside _cron_pull_metrics that computes delta to use
server.last_metrics_id.create_date and adjust any variable naming/comments
accordingly.
- Around line 564-579: The uninstall currently ignores the exit status of
_run_command_using_ssh in action_uninstall_push_agent and unconditionally sets
is_push_agent_installed = False; change it to capture the result of
_run_command_using_ssh(client, cmd_clean), check its exit status (or truthiness)
and if the command failed raise a UserError with a clear message (mirroring
action_install_push_agent behavior) instead of flipping the flag; only set
is_push_agent_installed = False and return True when the SSH command succeeded.
- Around line 234-238: Multiple routine monitoring messages are logged using
_logger.warning which should be downgraded to avoid noisy WARNING logs; locate
the _logger.warning call that logs "Cron Pull: Found %s servers total, updating
%s based on intervals" and change it to _logger.info (or _logger.debug if you
want per-tick verbosity), and similarly find other uses of _logger.warning that
emit the SSH command echo, SSH result status, parsing progress messages, and the
"successful metrics creation" message and change them to _logger.info or
_logger.debug as appropriate while keeping any true error/parse-failure or
non-zero SSH status logs at WARNING or ERROR; preserve the log text and
interpolation, only change the logging level on the _logger calls.
- Around line 316-319: The code incorrectly references models.ValidationError
(which doesn't exist) and uses % formatting after _(), causing translation
issues; change the raise to use the imported ValidationError from
odoo.exceptions (i.e., raise ValidationError(...)) and move variable
interpolation into the translation call by passing kwargs to _(), e.g. _('DEBUG
MONITOR OUTPUT\n\nStatus:
%(status)s\n\nResponse:\n%(response)s\n\nError:\n%(error)s',
status=res.get('status'), response=res.get('response'), error=res.get('error'));
update the raise site where _("...") and res.get(...) are used to use
ValidationError and keyword interpolation.
- Around line 294-299: Replace the translation call that uses the % operator so
variables are passed as keyword args for extraction: change the
self.message_post call (and similarly any _("...") % ... usage near the
Monitoring Pull Exception handling, referencing self.message_post and
_logger.error) to use _("Monitoring Pull Error: %(err)s", err=str(e)) instead of
_("Monitoring Pull Error: %s") % str(e) so translations extract correctly.
- Around line 356-397: The parsing failure path currently logs missing pieces
but still builds metrics_vals with zeros; change the logic so that if not
all([res_ram, res_disk, res_cpu]) you return early (or raise) immediately after
the _logger.error and do not construct metrics_vals, do not call whatever code
updates last_metrics_id, and do not invoke
_check_monitor_alerts/_cron_pull_metrics for this cycle; ensure only when all
three regex matches exist you proceed to build metrics_vals and continue the
normal flow.
- Around line 474-514: The _get_monitor_agent_script function currently builds
an f-string script with unsafe interpolations, a broken CPU metric and Python
escape warnings; fix it by (1) escaping all user-controlled values (self.name,
self.monitor_token, self.reference or self.name, and the computed url) with
shlex.quote() before interpolation so URL/TOKEN/REF are safely embedded; (2)
change the triple-quoted literal to an rf"""...""" raw f-string to avoid Python
escape warnings for sequences like \s,\K,\d; and (3) replace the single-sample
CPU read (CPU_USAGE=$(grep 'cpu ' /proc/stat ...)) with a two-sample delta: read
/proc/stat into a variable, sleep 1, read again, compute the delta of idle vs
total to produce an instantaneous CPU usage percentage (update the script block
around CPU_USAGE/CPU_CORES accordingly) while keeping the rest of the payload
and NEXT extraction logic intact using the rf string and the quoted values.
In `@cetmix_tower_server_monitor/static/src/css/dashboard.css`:
- Around line 33-35: The .o_cx_monitor_item.is-active rule uses background:
`#00a39b` with color: white which fails contrast for the 0.85rem metric labels;
update the rule (selector: .o_cx_monitor_item.is-active) to use either a darker
background (e.g. pick a darker teal like `#007a70` or similar that meets WCAG AA
contrast with white) or keep the `#00a39b` background and change the foreground to
a darker text color (e.g. `#0b2b2a`) so the metric labels meet WCAG AA contrast
requirements; verify the chosen pair with a contrast checker and apply the
change in dashboard.css.
In `@cetmix_tower_server_monitor/static/src/js/dashboard.js`:
- Around line 98-134: In fetchData remove the two debug console.log calls that
print this.props.record and the RPC result (the lines logging "Fetching
dashboard data for record:" and "Dashboard RPC result:") so the polling loop no
longer floods the console or exposes sensitive payloads; keep the console.warn
for missing serverId and the existing console.error in the catch, and ensure
fetchData still calls rpc(`/cetmix_tower/server/${serverId}/metrics/dashboard`),
updates state, calls startTimer() when monitor_interval or monitoring_mode
changed, and invokes renderChart().
- Around line 66-89: In startTimer(), guard against zero/null/undefined monitor
intervals by coercing this.state.monitor_interval to a Number and applying sane
minima per mode (e.g. minPush = 5000 ms, minPull = 30000 ms) before computing
intervalMs; ensure monitoring_mode ("push"/"pull") drives the multiplier and
clamp intervalMs = Math.max(computedMs, minForMode). Remove the console.log
inside the hot timer path (the start debug log may be kept once outside
high-frequency execution), and keep existing references to startTimer,
this.state.monitor_interval, this.state.monitoring_mode, this.timer,
refreshData, and fetchData when making the change.
In `@cetmix_tower_server_monitor/static/src/xml/dashboard.xml`:
- Around line 15-20: The CPU/RAM/Disk display expressions (e.g.,
state.current.cpu, state.current.cpu_cores, Math.round(state.current.cpu), and
the RAM/Disk used/total calculations) can produce NaN/Infinity when totals are
missing or zero; update each expression to guard denominators and fall back to
backend-safe percentage fields when available (for example use
state.current.cpu_cores || 0 for cores display and render "0" cores if absent,
and compute percentages only if total > 0 otherwise show "0.00%" or use a
provided safe percent like state.current.cpu_percent_safe); apply the same
pattern to state.current.ram_used/state.current.ram_total and
state.current.disk_used/state.current.disk_total and to the Math.round(...)
usages referenced (also update the other occurrences you noted at the blocks
around lines 31-36 and 48-52).
- Around line 88-93: Update the no-data text in the <t t-else=""> block of
dashboard.xml: locate the <h4>No Monitoring Data Available</h4> and the
following <p> element and remove the phrase "Use the button below to trigger a
test." Replace it with a neutral action or guidance (e.g., "Trigger a test from
the monitoring settings" or "Configure monitoring to start collecting metrics")
so the message does not reference a nonexistent button.
- Around line 7-42: The metric selector tiles (elements with class
o_cx_monitor_item) are not keyboard-accessible; update the CPU, RAM and disk
selector blocks to be operable via keyboard by either converting the outer <div>
to a semantic <button> (preferred) or adding role="button" and tabindex="0" plus
a keydown handler that calls the existing selectMetric('cpu'/'ram'/'disk') on
Enter/Space; ensure you keep the t-att-class binding and t-on-click="() =>
this.selectMetric(...)" and add the same handler for keyboard activation in the
component (the selectMetric method) and include visible focus styles so keyboard
users can see focus.
In `@cetmix_tower_server_monitor/views/cx_tower_server_views.xml`:
- Line 133: Remove the no-op empty xpath element that references a parent
<header> node: delete the line containing <xpath expr="//header"
position="inside" /> so the view no longer relies on a brittle //header xpath
with no body; this eliminates the unnecessary dependency and potential breakage
when the parent view lacks a header node.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d9a8a198-a4a4-441c-95e4-fe8ffa4180b5
⛔ Files ignored due to path filters (1)
cetmix_tower_server_monitor/security/ir.model.access.csvis excluded by!**/*.csv
📒 Files selected for processing (21)
cetmix_tower_server_monitor/.editorconfigcetmix_tower_server_monitor/.pre-commit-config.yamlcetmix_tower_server_monitor/.pylintrccetmix_tower_server_monitor/.pylintrc-mandatorycetmix_tower_server_monitor/.ruff.tomlcetmix_tower_server_monitor/README.rstcetmix_tower_server_monitor/__init__.pycetmix_tower_server_monitor/__manifest__.pycetmix_tower_server_monitor/controllers/__init__.pycetmix_tower_server_monitor/controllers/main.pycetmix_tower_server_monitor/data/ir_cron_data.xmlcetmix_tower_server_monitor/models/__init__.pycetmix_tower_server_monitor/models/cx_tower_server.pycetmix_tower_server_monitor/models/cx_tower_server_alert.pycetmix_tower_server_monitor/models/cx_tower_server_metrics.pycetmix_tower_server_monitor/pyproject.tomlcetmix_tower_server_monitor/static/src/css/dashboard.csscetmix_tower_server_monitor/static/src/js/dashboard.jscetmix_tower_server_monitor/static/src/xml/dashboard.xmlcetmix_tower_server_monitor/views/cx_tower_server_metrics_views.xmlcetmix_tower_server_monitor/views/cx_tower_server_views.xml
…ic push agent - Replaced monitor_interval with differentiated pull (minutes) and push (seconds) intervals. - Refactored push agent to run as a persistent systemd service. - Implemented dynamic interval updates: agents now adapt to frequency changes without redeploy. - Updated dashboard JS to support real-time polling (seconds) in push mode. - Fixed UI consistency: all RAM measurements now display in GB. - Improved code quality: fixed linting issues (E501, F821, W8113, W8120) and formatting.
…ic push agent - Replaced monitor_interval with differentiated pull (minutes) and push (seconds) intervals. - Refactored push agent to run as a persistent systemd service with dynamic local loop. - Implemented dynamic interval updates: agents adapt to server frequency changes without redeploy. - Updated dashboard JS to support real-time polling (seconds) in push mode. - Fixed UI consistency: all RAM measurements now display in GB. - Improved code quality: resolved all linting issues (E501, F821, W8113, W8120, W1401) and minor formatting.
…ic push agent - Replaced monitor_interval with differentiated pull (minutes) and push (seconds) intervals. - Refactored push agent to run as a persistent systemd service with dynamic local loop. - Implemented dynamic interval updates: agents adapt to server frequency changes without redeploy. - Updated dashboard JS component to .esm.js and fixed ESLint compliance. - Fixed UI consistency: all RAM measurements now display in GB. - Improved code quality: resolved all linting issues (E501, F821, W8113, W8120, W1401, no-undef).
- Removed the leading slash from .ruff_cache entry to prevent tracking hidden cache files in submodules. - Cleaned up local ruff_cache files.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (10)
cetmix_tower_server_monitor/static/src/js/dashboard.esm.js (2)
100-111:⚠️ Potential issue | 🟡 MinorRemove polling-loop payload logs.
fetchData()runs continuously and currently logsthis.props.recordplus the full dashboard RPC result. That can flood user consoles and expose record payloads unnecessarily.Proposed fix
async fetchData() { try { - console.log("Fetching dashboard data for record:", this.props.record); const serverId = this.props.record.resId || this.props.record.data.id; if (!serverId) { console.warn("No server ID found in record props"); return; } const result = await rpc( `/cetmix_tower/server/${serverId}/metrics/dashboard` ); - console.log("Dashboard RPC result:", result); if (result.status === "success" && result.data) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cetmix_tower_server_monitor/static/src/js/dashboard.esm.js` around lines 100 - 111, fetchData() is logging polling-loop payloads (this.props.record and full RPC result) which can flood consoles and expose data; remove or replace the noisy console.log calls inside fetchData() (the ones that print "Fetching dashboard data for record:" and "Dashboard RPC result:") and instead log minimal, infrequent information such as serverId or aggregate status, or use debug-level logging guarded by a flag; update the fetchData() function and any related error handling around rpc(`/cetmix_tower/server/${serverId}/metrics/dashboard`) to avoid printing the full result payload.
67-90:⚠️ Potential issue | 🟡 MinorClamp invalid polling intervals and remove hot-path debug logging.
this.state.monitor_interval || 1turns0,null, orundefinedinto a 1-second push poll. Clamp to safe per-mode minima before creating the timer, and drop the recurringconsole.loginside the interval callback.Proposed fix
startTimer() { this.stopTimer(); - let intervalMs = this.state.monitor_interval || 1; - if (this.state.monitoring_mode === "push") { - intervalMs *= 1000; - } else { - intervalMs = intervalMs * 60 * 1000; - } - console.log( - `Starting dashboard timer with interval: ${intervalMs}ms (${this.state.monitoring_mode} mode)` - ); + const rawInterval = Number(this.state.monitor_interval) || 0; + const intervalMs = + this.state.monitoring_mode === "push" + ? Math.max(rawInterval, 5) * 1000 + : Math.max(rawInterval, 1) * 60 * 1000; this.timer = setInterval(async () => { - // If in Pull mode and tab is visible, we can trigger a pull to get "Live" feel if ( this.state.monitoring_mode === "pull" && document.visibilityState === "visible" ) { - console.log("Auto-Pulling metrics for Live mode..."); - // Silent refresh await this.refreshData(true); } else { await this.fetchData();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cetmix_tower_server_monitor/static/src/js/dashboard.esm.js` around lines 67 - 90, startTimer computes intervalMs from this.state.monitor_interval but treats 0/null/undefined as 1 and then may set an unsafe interval; validate and coerce monitor_interval to a number, use per-mode minimums (e.g., for "push" enforce at least 1000 ms, for "pull" enforce at least 60_000 ms) via Math.max before calling setInterval, and remove the recurring debug console.log inside the interval callback (leave any one-time startup log if desired). Update the logic in startTimer (referencing startTimer, intervalMs, this.state.monitor_interval, this.state.monitoring_mode, this.timer, refreshData, fetchData) to clamp values and avoid spamming logs.cetmix_tower_server_monitor/controllers/main.py (4)
95-99:⚠️ Potential issue | 🟡 MinorLog dashboard failures and return a generic error.
Returning
str(e)exposes internal ORM/controller details to the browser and loses the traceback in server logs. Log the exception server-side and keep the client response generic.Proposed fix
try: data = server.get_dashboard_data() return {"status": "success", "data": data} - except Exception as e: - return {"status": "error", "message": str(e)} + except Exception: + _logger.exception("Dashboard fetch failed for server %s", server_id) + return {"status": "error", "message": "Internal server error"}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cetmix_tower_server_monitor/controllers/main.py` around lines 95 - 99, The except block around server.get_dashboard_data() should stop returning str(e) to clients and instead log the full exception/traceback server-side and return a generic error message; replace the current except Exception as e: return {"status":"error","message": str(e)} with code that logs the exception (e.g., logger.exception(...) or logging.exception(...)) including the traceback and then returns a generic response like {"status":"error","message":"Failed to retrieve dashboard data"} so internal details are not exposed.
10-35:⚠️ Potential issue | 🔴 CriticalUse an HTTP JSON endpoint for the raw agent payload.
The deployed agent posts raw JSON, but this route is declared as Odoo
type="json"and readsrequest.dispatcher.jsonrequest. Switch totype="http", parse the request body explicitly, and returnrequest.make_json_response(...)so push agents can call the endpoint directly.Odoo 18 http.route type="json" JSON-RPC envelope raw JSON body request.dispatcher.jsonrequestProposed fix
+import json import logging @@ `@http.route`( "/cetmix_tower/monitor/push", - type="json", + type="http", auth="public", methods=["POST"], csrf=False, ) def monitor_push(self, **post): @@ - data = request.dispatcher.jsonrequest + try: + data = json.loads(request.httprequest.get_data(as_text=True) or "{}") + except ValueError: + return request.make_json_response( + {"status": "error", "message": "Invalid JSON"}, + status=400, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cetmix_tower_server_monitor/controllers/main.py` around lines 10 - 35, The route monitor_push is declared type="json" and reads request.dispatcher.jsonrequest which expects a JSON-RPC envelope; change the route decorator to type="http" and keep auth="public", methods=["POST"], csrf=False, then inside monitor_push replace request.dispatcher.jsonrequest with explicit parsing of the raw request body (use request.httprequest.get_data()/request.httprequest.data and json.loads) to obtain the payload, and return the response using request.make_json_response(...) instead of the JSON-RPC return style so external agents can POST raw JSON directly.
43-49:⚠️ Potential issue | 🟠 MajorResolve push servers by stable reference only.
Matching
referenceornameon a public endpoint can silently attach metrics to the wrong server when names collide with references. Use the external reference as the sole identifier and reject requests when it is missing.Proposed fix
server = ( request.env["cx.tower.server"] .sudo() .search( - ["|", ("reference", "=", server_ref), ("name", "=", server_ref)], + [("reference", "=", server_ref)], limit=1, ) )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cetmix_tower_server_monitor/controllers/main.py` around lines 43 - 49, The current server lookup in the controller uses a disjunctive search on reference OR name which can attach metrics to the wrong server; update the lookup in the handler that calls request.env["cx.tower.server"].sudo().search(...) to require a non-empty external reference (reject the request with a 4xx response when server_ref is missing) and change the search filter to only ("reference","=", server_ref) (remove the "|", ("name","=", server_ref) branch); also handle the case where the search returns no record by returning an appropriate 4xx error instead of proceeding.
55-57:⚠️ Potential issue | 🟠 MajorCompare monitor tokens in constant time.
This endpoint is public and unauthenticated apart from the token. Replace the plain string inequality with
hmac.compare_digest(...)to avoid timing leakage.Proposed fix
+import hmac import logging @@ - if server.monitor_token != token: + if not hmac.compare_digest( + str(server.monitor_token or ""), + str(token or ""), + ): _logger.warning("Invalid monitor token for server %s", server_ref) return {"status": "error", "message": "Invalid token"}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cetmix_tower_server_monitor/controllers/main.py` around lines 55 - 57, Replace the plain inequality check "if server.monitor_token != token" with a constant-time comparison using hmac.compare_digest to prevent timing attacks: import the hmac module and use hmac.compare_digest(...) to compare the stored server.monitor_token and the provided token (ensure both are the same type, e.g., convert to str or bytes before comparing). Keep the existing logging and error return path, but base the conditional on the compare_digest result (negated) instead of !=.cetmix_tower_server_monitor/models/cx_tower_server.py (4)
482-525:⚠️ Potential issue | 🔴 CriticalHarden the generated agent script before deploying it to hosts.
Two issues remain here: values such as
self.name,reference, andweb.base.urlare interpolated into shell assignments/JSON without shell or JSON escaping, andCPU_USAGEis calculated from a single cumulative/proc/statsnapshot instead of a delta sample. This can break the agent, allow command injection on the monitored host, and produce misleading CPU metrics.Proposed fix sketch
import base64 +import json import logging import re +import shlex @@ - script = f"""#!/bin/bash + url_q = shlex.quote(url) + token_q = shlex.quote(self.monitor_token or "") + ref_q = shlex.quote(self.reference or self.name or "") + name_comment = (self.name or "").replace("\n", " ").replace("#", "") + + script = f"""#!/bin/bash # Cetmix Tower Monitor Agent -# Server: {self.name} +# Server: {name_comment} -URL="{url}" -TOKEN="{self.monitor_token}" -REF="{self.reference or self.name}" +URL={url_q} +TOKEN={token_q} +REF={ref_q} INTERVAL={self.monitor_interval_push} @@ - CPU_USAGE=$(grep 'cpu ' /proc/stat | \ - awk '{{usage=($2+$4)*100/($2+$4+$5)}} END {{print usage}}') + read -r _ u1 n1 s1 i1 w1 q1 sq1 st1 _ < /proc/stat + sleep 1 + read -r _ u2 n2 s2 i2 w2 q2 sq2 st2 _ < /proc/stat + CPU_USAGE=$(awk -v u1="$u1" -v n1="$n1" -v s1="$s1" -v i1="$i1" \ + -v w1="$w1" -v q1="$q1" -v sq1="$sq1" -v st1="$st1" \ + -v u2="$u2" -v n2="$n2" -v s2="$s2" -v i2="$i2" \ + -v w2="$w2" -v q2="$q2" -v sq2="$sq2" -v st2="$st2" \ + 'BEGIN {{ + idle1=i1+w1; idle2=i2+w2; + total1=u1+n1+s1+i1+w1+q1+sq1+st1; + total2=u2+n2+s2+i2+w2+q2+sq2+st2; + dt=total2-total1; di=idle2-idle1; + print dt > 0 ? 100 * (dt - di) / dt : 0; + }}')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cetmix_tower_server_monitor/models/cx_tower_server.py` around lines 482 - 525, The generated agent script in the script string (in cx_tower_server.py) is unsafe and reports incorrect CPU usage; fix it by (1) shell-escaping and JSON-encoding all interpolated Python values (self.name, self.reference, self.monitor_token, url, self.monitor_interval_push) before embedding into the heredoc so no unescaped quotes or shell metacharacters can cause command injection or broken JSON, and (2) replace the single-snapshot CPU calculation with a delta-based sample (read /proc/stat twice separated by a short sleep and compute usage from the deltas) so CPU percent is accurate; update the payload construction to produce valid JSON (or use a here-doc with jq or printf-encoded strings) and reference the script variable and payload/CPU_USAGE generation when making changes.
292-326:⚠️ Potential issue | 🟡 MinorMove translation interpolation inside
_().These calls still format the translated string with
%after_(). Pass variables as keyword arguments directly to_()so extraction/runtime translation stays correct.Proposed fix
- self.message_post(body=_("Monitoring Pull Error: %s") % str(e)) + self.message_post( + body=_("Monitoring Pull Error: %(err)s", err=str(e)) + ) @@ raise ValidationError( _( "DEBUG MONITOR OUTPUT\n\n" "Status: %(status)s\n\n" "Response:\n%(response)s\n\n" - "Error:\n%(error)s" + "Error:\n%(error)s", + status=res.get("status"), + response=res.get("response"), + error=res.get("error"), ) - % { - "status": res.get("status"), - "response": res.get("response"), - "error": res.get("error"), - } ) @@ - msg = _("Monitoring Alerts triggered: %s") % ( - ", ".join([a["resource"] for a in alerts]) - ) + msg = _( + "Monitoring Alerts triggered: %(resources)s", + resources=", ".join([a["resource"] for a in alerts]), + )Based on learnings: In Python files using Odoo translations, never use the % operator after _() with variables; instead pass variables as keyword arguments.
Also applies to: 466-472
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cetmix_tower_server_monitor/models/cx_tower_server.py` around lines 292 - 326, The translated strings are being formatted after calling _(), which breaks translation extraction/runtime substitution; update the two places (the self.message_post call that currently does body=_("Monitoring Pull Error: %s") % str(e) and the ValidationError where a multi-line _("...") % {...} is used in action_monitor_debug) to pass variables into the translation function as keyword args (e.g. _("Monitoring Pull Error: %(err)s", err=str(e)) and _("DEBUG MONITOR OUTPUT\n\nStatus: %(status)s\n\nResponse:\n%(response)s\n\nError:\n%(error)s", status=res.get("status"), response=res.get("response"), error=res.get("error")) ), and apply the same fix for the duplicate occurrence noted around lines 466-472.
363-405:⚠️ Potential issue | 🟠 MajorReturn early instead of storing fabricated zero metrics.
When RAM, disk, or CPU parsing fails, the method logs the failure but still creates a metrics row with
0values. That corrupts the dashboard, updateslast_metrics_id, and can suppress alerts/retries for the interval.Proposed fix
if not all([res_ram, res_disk, res_cpu]): failed = [] if not res_ram: failed.append("RAM") if not res_disk: failed.append("Disk") if not res_cpu: failed.append("CPU") _logger.error( "Metric parsing failed for server %s. Missing: %s.\n" "RAM: %s\nDisk: %s\nCPU: %s", self.name, ", ".join(failed), ram_out, disk_out, cpu_out, ) + return try: metrics_vals = {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cetmix_tower_server_monitor/models/cx_tower_server.py` around lines 363 - 405, The code logs parsing failures for RAM/Disk/CPU but continues to build and persist a fabricated metrics row (metrics_vals) with zeros; change the flow in the method that constructs metrics_vals (the block using res_ram/res_disk/res_cpu and self.name) to return early immediately after the _logger.error call when not all([res_ram, res_disk, res_cpu]) is true, so no metrics_vals is created or saved and last_metrics_id is not updated; keep the existing warning path for missing CPU idle only if you intend to accept zero CPU percent, otherwise also return early when res_cpu is missing. Ensure callers handle the early return (e.g., returning None) and update any persistence logic so no row is inserted for failed parses.
584-593:⚠️ Potential issue | 🟠 MajorDo not mark the push agent uninstalled when SSH cleanup fails.
_run_command_using_ssh()is ignored, so Odoo flipsis_push_agent_installedtoFalseeven ifsystemctl stop/disableor file removal failed. Mirror install behavior and only update the flag after a successful remote cleanup.Proposed fix
- self._run_command_using_ssh(client, cmd_clean) + res = self._run_command_using_ssh(client, cmd_clean) + if res.get("status") != 0: + raise UserError( + _("Failed to remove push agent: %(err)s", err=res.get("error")) + ) self.is_push_agent_installed = False return True🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cetmix_tower_server_monitor/models/cx_tower_server.py` around lines 584 - 593, The cleanup currently calls _run_command_using_ssh(client, cmd_clean) and unconditionally sets is_push_agent_installed = False; change this so the flag is only cleared after the remote cleanup succeeds: call _run_command_using_ssh and either check its return value or wrap it in try/except to detect failure, and only set self.is_push_agent_installed = False when _run_command_using_ssh indicates success (or no exception); if the SSH call fails, log the error and leave is_push_agent_installed unchanged (mirroring the install path).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cetmix_tower_server_monitor/models/cx_tower_server.py`:
- Around line 34-46: Add validation for monitor_interval_push similar to the
existing monitor_interval_pull check: create a constraint method (e.g.,
_check_monitor_interval_push) decorated with
`@api.constrains`("monitor_interval_push") that iterates records and raises
ValidationError when rec.monitoring_mode == "push" and rec.monitor_interval_push
< 1, using a clear message like "Monitor Interval (PUSH) must be at least 1
minute."; update the corresponding duplicate block (the other occurrence around
the 519-523 logic) so both push-configurable models enforce the same minimum
interval before the agent loop is generated.
In `@cetmix_tower_server_monitor/static/src/js/dashboard.esm.js`:
- Around line 187-192: When updating the chart in the chart update path, also
set the dataset label so tooltips reflect the currently selected metric: update
this.chart.data.datasets[0].label (in the same block where
this.chart.data.labels, datasets[0].data, borderColor and backgroundColor are
changed) to the new metric name provided by selectMetric() or the current metric
variable before calling this.chart.update(); ensure the label string matches
whatever display text is used when initially rendering the chart.
In `@pre_commit_output.txt`:
- Around line 37-38: The file dashboard.js contains ESM import/export but is
being parsed as a non-module script, causing the ESLint parser error; either
remove or rename the stale dashboard.js (replace it with dashboard.esm.js) so no
non-module file contains ESM syntax, or update your linting/module config to
treat this file as an ES module by enabling sourceType: "module" for files
matching static/src/js/dashboard.js (or adjust the glob/overrides in your
ESLint/pre-commit config). Locate references to dashboard.js and the lint config
entries to apply the fix (rename/delete dashboard.js or add an ESLint override
that targets static/src/js/dashboard.js and sets sourceType/module).
---
Duplicate comments:
In `@cetmix_tower_server_monitor/controllers/main.py`:
- Around line 95-99: The except block around server.get_dashboard_data() should
stop returning str(e) to clients and instead log the full exception/traceback
server-side and return a generic error message; replace the current except
Exception as e: return {"status":"error","message": str(e)} with code that logs
the exception (e.g., logger.exception(...) or logging.exception(...)) including
the traceback and then returns a generic response like
{"status":"error","message":"Failed to retrieve dashboard data"} so internal
details are not exposed.
- Around line 10-35: The route monitor_push is declared type="json" and reads
request.dispatcher.jsonrequest which expects a JSON-RPC envelope; change the
route decorator to type="http" and keep auth="public", methods=["POST"],
csrf=False, then inside monitor_push replace request.dispatcher.jsonrequest with
explicit parsing of the raw request body (use
request.httprequest.get_data()/request.httprequest.data and json.loads) to
obtain the payload, and return the response using
request.make_json_response(...) instead of the JSON-RPC return style so external
agents can POST raw JSON directly.
- Around line 43-49: The current server lookup in the controller uses a
disjunctive search on reference OR name which can attach metrics to the wrong
server; update the lookup in the handler that calls
request.env["cx.tower.server"].sudo().search(...) to require a non-empty
external reference (reject the request with a 4xx response when server_ref is
missing) and change the search filter to only ("reference","=", server_ref)
(remove the "|", ("name","=", server_ref) branch); also handle the case where
the search returns no record by returning an appropriate 4xx error instead of
proceeding.
- Around line 55-57: Replace the plain inequality check "if server.monitor_token
!= token" with a constant-time comparison using hmac.compare_digest to prevent
timing attacks: import the hmac module and use hmac.compare_digest(...) to
compare the stored server.monitor_token and the provided token (ensure both are
the same type, e.g., convert to str or bytes before comparing). Keep the
existing logging and error return path, but base the conditional on the
compare_digest result (negated) instead of !=.
In `@cetmix_tower_server_monitor/models/cx_tower_server.py`:
- Around line 482-525: The generated agent script in the script string (in
cx_tower_server.py) is unsafe and reports incorrect CPU usage; fix it by (1)
shell-escaping and JSON-encoding all interpolated Python values (self.name,
self.reference, self.monitor_token, url, self.monitor_interval_push) before
embedding into the heredoc so no unescaped quotes or shell metacharacters can
cause command injection or broken JSON, and (2) replace the single-snapshot CPU
calculation with a delta-based sample (read /proc/stat twice separated by a
short sleep and compute usage from the deltas) so CPU percent is accurate;
update the payload construction to produce valid JSON (or use a here-doc with jq
or printf-encoded strings) and reference the script variable and
payload/CPU_USAGE generation when making changes.
- Around line 292-326: The translated strings are being formatted after calling
_(), which breaks translation extraction/runtime substitution; update the two
places (the self.message_post call that currently does body=_("Monitoring Pull
Error: %s") % str(e) and the ValidationError where a multi-line _("...") % {...}
is used in action_monitor_debug) to pass variables into the translation function
as keyword args (e.g. _("Monitoring Pull Error: %(err)s", err=str(e)) and
_("DEBUG MONITOR OUTPUT\n\nStatus:
%(status)s\n\nResponse:\n%(response)s\n\nError:\n%(error)s",
status=res.get("status"), response=res.get("response"), error=res.get("error"))
), and apply the same fix for the duplicate occurrence noted around lines
466-472.
- Around line 363-405: The code logs parsing failures for RAM/Disk/CPU but
continues to build and persist a fabricated metrics row (metrics_vals) with
zeros; change the flow in the method that constructs metrics_vals (the block
using res_ram/res_disk/res_cpu and self.name) to return early immediately after
the _logger.error call when not all([res_ram, res_disk, res_cpu]) is true, so no
metrics_vals is created or saved and last_metrics_id is not updated; keep the
existing warning path for missing CPU idle only if you intend to accept zero CPU
percent, otherwise also return early when res_cpu is missing. Ensure callers
handle the early return (e.g., returning None) and update any persistence logic
so no row is inserted for failed parses.
- Around line 584-593: The cleanup currently calls
_run_command_using_ssh(client, cmd_clean) and unconditionally sets
is_push_agent_installed = False; change this so the flag is only cleared after
the remote cleanup succeeds: call _run_command_using_ssh and either check its
return value or wrap it in try/except to detect failure, and only set
self.is_push_agent_installed = False when _run_command_using_ssh indicates
success (or no exception); if the SSH call fails, log the error and leave
is_push_agent_installed unchanged (mirroring the install path).
In `@cetmix_tower_server_monitor/static/src/js/dashboard.esm.js`:
- Around line 100-111: fetchData() is logging polling-loop payloads
(this.props.record and full RPC result) which can flood consoles and expose
data; remove or replace the noisy console.log calls inside fetchData() (the ones
that print "Fetching dashboard data for record:" and "Dashboard RPC result:")
and instead log minimal, infrequent information such as serverId or aggregate
status, or use debug-level logging guarded by a flag; update the fetchData()
function and any related error handling around
rpc(`/cetmix_tower/server/${serverId}/metrics/dashboard`) to avoid printing the
full result payload.
- Around line 67-90: startTimer computes intervalMs from
this.state.monitor_interval but treats 0/null/undefined as 1 and then may set an
unsafe interval; validate and coerce monitor_interval to a number, use per-mode
minimums (e.g., for "push" enforce at least 1000 ms, for "pull" enforce at least
60_000 ms) via Math.max before calling setInterval, and remove the recurring
debug console.log inside the interval callback (leave any one-time startup log
if desired). Update the logic in startTimer (referencing startTimer, intervalMs,
this.state.monitor_interval, this.state.monitoring_mode, this.timer,
refreshData, fetchData) to clamp values and avoid spamming logs.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ee548b80-b8f2-4d5a-a1b9-895a4763284a
📒 Files selected for processing (6)
.gitignorecetmix_tower_server_monitor/__manifest__.pycetmix_tower_server_monitor/controllers/main.pycetmix_tower_server_monitor/models/cx_tower_server.pycetmix_tower_server_monitor/static/src/js/dashboard.esm.jspre_commit_output.txt
- Fix monitor_push controller: use type='http' and manual JSON parsing for agent compatibility - Improve server lookup search to prioritize reference over name - Add monitor_interval_push validation (min 10s) and agent script safety guard - Remove redundant module-level pre-commit config - Fix pylint warnings related to regex escaping in agent script
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (8)
cetmix_tower_server_monitor/models/cx_tower_server.py (5)
232-233:⚠️ Potential issue | 🟠 MajorUse the Odoo receipt timestamp consistently.
Line 232 still uses
last_metrics_id.timestamp, while_compute_monitor_push_status()usescreate_date. Remote agent clock skew can make cron freshness disagree with dashboard status. Usecreate_datehere too.🐛 Proposed fix
- delta = (now - server.last_metrics_id.timestamp).total_seconds() + delta = (now - server.last_metrics_id.create_date).total_seconds()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cetmix_tower_server_monitor/models/cx_tower_server.py` around lines 232 - 233, The delta computation is using server.last_metrics_id.timestamp but should use the Odoo receipt timestamp create_date to match _compute_monitor_push_status(); replace references to last_metrics_id.timestamp with last_metrics_id.create_date (or server.last_metrics_id.create_date) in the code where delta is computed and compared against server.monitor_interval_pull so both freshness checks use the same create_date field.
367-421:⚠️ Potential issue | 🟠 MajorReturn early on metric parse failure.
Lines 367-383 log missing RAM/Disk/CPU, but Lines 385-421 still create a metrics record with zeros. That fabricates dashboard drops, suppresses threshold alerts, and updates
last_metrics_idso cron skips a retry.🐛 Proposed fix
_logger.error( "Metric parsing failed for server %s. Missing: %s.\n" "RAM: %s\nDisk: %s\nCPU: %s", self.name, ", ".join(failed), ram_out, disk_out, cpu_out, ) + return try:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cetmix_tower_server_monitor/models/cx_tower_server.py` around lines 367 - 421, The parsing block currently logs missing metrics (res_ram/res_disk/res_cpu) but continues to build metrics_vals, create a metrics record, update self.last_metrics_id and call _check_monitor_alerts, which fabricates data and suppresses retries; modify the flow in the function containing that block so that if not all([res_ram, res_disk, res_cpu]) you log the missing pieces (using the existing _logger.error) and then immediately return (or otherwise skip creating metrics and avoid setting self.last_metrics_id or calling _check_monitor_alerts) so no metrics record is created from incomplete parse results; keep the existing logging for context but ensure the create(metrics_vals), assignment to self.last_metrics_id, and call to self._check_monitor_alerts(metrics) only run when parsing succeeded.
600-602:⚠️ Potential issue | 🟠 MajorCheck the uninstall SSH result before clearing the installed flag.
Line 600 ignores failures from
systemctl/rm, then Line 602 marks the agent as removed even if it is still running remotely. Mirror install behavior and only flip the flag after a zero status.🐛 Proposed fix
- self._run_command_using_ssh(client, cmd_clean) + res = self._run_command_using_ssh(client, cmd_clean) + if res.get("status") != 0: + raise UserError( + _("Failed to remove push agent: %(err)s", err=res.get("error")) + ) self.is_push_agent_installed = False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cetmix_tower_server_monitor/models/cx_tower_server.py` around lines 600 - 602, The uninstall sequence calls self._run_command_using_ssh(client, cmd_clean) but ignores its result before setting self.is_push_agent_installed = False; capture the return value from _run_command_using_ssh and only set is_push_agent_installed = False when the returned status indicates success (e.g., exit code == 0), mirroring the install logic that checks for zero status; if _run_command_using_ssh can return rich result objects, inspect the appropriate field (exit_code/returncode) before flipping the flag.
486-532:⚠️ Potential issue | 🔴 CriticalHarden the generated push agent before deployment.
This script still embeds
self.name,reference, token, and URL directly into shell/JSON, so quotes/newlines/command substitutions can break the agent or execute on the target host. Line 512 also computes CPU from one cumulative/proc/statsample, not interval usage, and Line 524 can block forever without curl timeouts.Use shell-safe quoting for assignments, JSON-safe encoding for payload values, sanitize the comment, compute CPU from two
/proc/statsamples, and add curl timeouts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cetmix_tower_server_monitor/models/cx_tower_server.py` around lines 486 - 532, The generated bash script embeds unescaped server fields and builds JSON unsafely, computes CPU from a single /proc/stat sample, and calls curl without timeouts; fix the script builder in the method that assigns script (the multi-line f-string) by: 1) sanitize/escape the comment and use shell-safe quoted assignments for URL, TOKEN, REF and numeric INTERVAL (avoid raw interpolation of self.name/self.reference/self.monitor_token); 2) JSON-encode or safely escape payload values instead of concatenating raw strings (ensure PAYLOAD is built with properly quoted/escaped keys and values); 3) replace the single-sample CPU_USAGE code with a two-sample /proc/stat measurement separated by sleep to compute real interval CPU percent (update CPU_USAGE and related logic); and 4) add curl timeouts (connect and total) and fail-safes when parsing NEXT from RESPONSE (make NEXT parsing robust and bound-checked before assigning INTERVAL). Ensure you update the PAYLOAD, CPU_USAGE, RESPONSE (curl) and NEXT handling code paths accordingly.
296-301: 🛠️ Refactor suggestion | 🟠 MajorMove interpolation into Odoo translation calls.
These call sites still format translated strings outside
_()or use positional interpolation. Use named placeholders passed directly to_()so extraction/rendering remains correct.♻️ Proposed fix
- self.message_post(body=_("Monitoring Pull Error: %s") % str(e)) + self.message_post( + body=_("Monitoring Pull Error: %(err)s", err=str(e)) + ) @@ raise ValidationError( _( "DEBUG MONITOR OUTPUT\n\n" "Status: %(status)s\n\n" "Response:\n%(response)s\n\n" - "Error:\n%(error)s" - ) - % { - "status": res.get("status"), - "response": res.get("response"), - "error": res.get("error"), - } + "Error:\n%(error)s", + status=res.get("status"), + response=res.get("response"), + error=res.get("error"), + ) ) @@ - msg = _("Monitoring Alerts triggered: %s") % ( - ", ".join([a["resource"] for a in alerts]) + msg = _( + "Monitoring Alerts triggered: %(resources)s", + resources=", ".join([a["resource"] for a in alerts]), ) @@ - raise UserError(_("Failed to deploy script: %s", res.get("error"))) + raise UserError( + _("Failed to deploy script: %(err)s", err=res.get("error")) + ) @@ - raise UserError(_("Failed to setup systemd service: %s", res.get("error"))) + raise UserError( + _("Failed to setup systemd service: %(err)s", err=res.get("error")) + )Based on learnings: In Python files using Odoo translations, never use the
%operator after_()with variables; instead pass variables as keyword arguments.Also applies to: 318-330, 470-476, 556-556, 583-583
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cetmix_tower_server_monitor/models/cx_tower_server.py` around lines 296 - 301, The translation calls currently interpolate variables outside of _(), break gettext extraction, and use positional %; update each message_post translation to use named placeholders inside _() and pass a mapping, e.g. replace body=_("Monitoring Pull Error: %s") % str(e) with body=_("Monitoring Pull Error: %(error)s") % {'error': str(e)}; do the same for the other occurrences reported (around the self.message_post calls at the other ranges) and ensure _logger.error calls use proper logging formatting (e.g. _logger.error("Monitoring Pull Exception for server %s: %s", self.name, e) or keep the message text separate from translation) so no external interpolation is done on _("...") strings.cetmix_tower_server_monitor/controllers/main.py (3)
79-85:⚠️ Potential issue | 🟠 MajorUse constant-time token comparison.
Line 79 compares the public endpoint token with
!=, which leaks comparison timing. Coerce both sides to strings and usehmac.compare_digest; also reject empty client tokens.🔒 Proposed fix
+import hmac import logging @@ - if server.monitor_token != token: + if not token or not hmac.compare_digest( + str(server.monitor_token or ""), str(token or "") + ): _logger.warning("Invalid monitor token for server %s", server_ref) return Response(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cetmix_tower_server_monitor/controllers/main.py` around lines 79 - 85, Replace the direct inequality check on the token with a constant-time comparison: ensure both server.monitor_token and the incoming token are coerced to str, reject empty/blank client tokens before comparing (return the same 403 Response and log warning via _logger), and use hmac.compare_digest(str(server.monitor_token), str(token)) to decide validity; keep the same Response payload and logging flow but perform the empty-token guard and compare_digest check instead of `!=`.
59-70:⚠️ Potential issue | 🟠 MajorRemove the fallback lookup by server name.
The reference-first lookup still falls back to
name, so a public push can silently attach metrics to the wrong server when names collide or a name equals another server’s reference. Use the stable externalreferenceonly, or reject push setup when no reference exists.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cetmix_tower_server_monitor/controllers/main.py` around lines 59 - 70, Remove the fallback lookup by name and require the external stable reference: stop calling request.env["cx.tower.server"].sudo().search([("name","=",server_ref)], ...) when the initial reference search returns no result; instead treat a missing server (variable server) as an error—return or raise a suitable rejection (e.g., HTTP 400/422 or a ValidationError) indicating that the provided server_ref does not match any server. Ensure this change affects the controller logic in controllers/main.py where server and server_ref are used so pushes cannot silently bind to servers by name.
129-133:⚠️ Potential issue | 🟡 MinorLog dashboard failures and return a generic error.
Line 133 returns raw exception text to the browser and Line 132 swallows the traceback. Log the exception server-side and keep client errors generic.
🛡️ Proposed fix
try: data = server.get_dashboard_data() return {"status": "success", "data": data} - except Exception as e: - return {"status": "error", "message": str(e)} + except Exception: + _logger.exception("Dashboard fetch failed for server %s", server_id) + return {"status": "error", "message": "Internal server error"}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cetmix_tower_server_monitor/controllers/main.py` around lines 129 - 133, The except block around server.get_dashboard_data() should log the full exception (including traceback) server-side and return a generic message to the client; update the except Exception as e handler to call the application's logger (e.g., logger.exception(...) or app.logger.exception(...)) with context like "Failed to get dashboard data for server" and the exception, then return {"status": "error", "message": "Internal server error"} instead of str(e); keep the try/except around server.get_dashboard_data() and only change the logging and returned message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cetmix_tower_server_monitor/controllers/main.py`:
- Around line 39-50: After parsing request.httprequest.data into data, ensure
data is a JSON object by checking isinstance(data, dict) and if not return a 400
Response with the same {"status":"error","message":"Invalid JSON"} payload; then
extract server_ref and token and validate metrics by assigning metrics_data =
data.get("metrics", {}) and verifying isinstance(metrics_data, dict) (return 400
if not), so subsequent uses of data.get(...) and metrics_data[...] never raise
AttributeError.
In `@cetmix_tower_server_monitor/models/cx_tower_server.py`:
- Around line 40-50: The constraint _check_monitor_intervals currently only
lists "monitor_interval_pull" and "monitor_interval_push", so changing
monitoring_mode to "push" after previously saving invalid intervals won't
trigger it; update the `@api.constrains` decorator on _check_monitor_intervals to
include "monitoring_mode" along with "monitor_interval_pull" and
"monitor_interval_push" so that any change to monitoring_mode reruns the
validation and raises the ValidationError in _check_monitor_intervals when
intervals are out of bounds.
---
Duplicate comments:
In `@cetmix_tower_server_monitor/controllers/main.py`:
- Around line 79-85: Replace the direct inequality check on the token with a
constant-time comparison: ensure both server.monitor_token and the incoming
token are coerced to str, reject empty/blank client tokens before comparing
(return the same 403 Response and log warning via _logger), and use
hmac.compare_digest(str(server.monitor_token), str(token)) to decide validity;
keep the same Response payload and logging flow but perform the empty-token
guard and compare_digest check instead of `!=`.
- Around line 59-70: Remove the fallback lookup by name and require the external
stable reference: stop calling
request.env["cx.tower.server"].sudo().search([("name","=",server_ref)], ...)
when the initial reference search returns no result; instead treat a missing
server (variable server) as an error—return or raise a suitable rejection (e.g.,
HTTP 400/422 or a ValidationError) indicating that the provided server_ref does
not match any server. Ensure this change affects the controller logic in
controllers/main.py where server and server_ref are used so pushes cannot
silently bind to servers by name.
- Around line 129-133: The except block around server.get_dashboard_data()
should log the full exception (including traceback) server-side and return a
generic message to the client; update the except Exception as e handler to call
the application's logger (e.g., logger.exception(...) or
app.logger.exception(...)) with context like "Failed to get dashboard data for
server" and the exception, then return {"status": "error", "message": "Internal
server error"} instead of str(e); keep the try/except around
server.get_dashboard_data() and only change the logging and returned message.
In `@cetmix_tower_server_monitor/models/cx_tower_server.py`:
- Around line 232-233: The delta computation is using
server.last_metrics_id.timestamp but should use the Odoo receipt timestamp
create_date to match _compute_monitor_push_status(); replace references to
last_metrics_id.timestamp with last_metrics_id.create_date (or
server.last_metrics_id.create_date) in the code where delta is computed and
compared against server.monitor_interval_pull so both freshness checks use the
same create_date field.
- Around line 367-421: The parsing block currently logs missing metrics
(res_ram/res_disk/res_cpu) but continues to build metrics_vals, create a metrics
record, update self.last_metrics_id and call _check_monitor_alerts, which
fabricates data and suppresses retries; modify the flow in the function
containing that block so that if not all([res_ram, res_disk, res_cpu]) you log
the missing pieces (using the existing _logger.error) and then immediately
return (or otherwise skip creating metrics and avoid setting
self.last_metrics_id or calling _check_monitor_alerts) so no metrics record is
created from incomplete parse results; keep the existing logging for context but
ensure the create(metrics_vals), assignment to self.last_metrics_id, and call to
self._check_monitor_alerts(metrics) only run when parsing succeeded.
- Around line 600-602: The uninstall sequence calls
self._run_command_using_ssh(client, cmd_clean) but ignores its result before
setting self.is_push_agent_installed = False; capture the return value from
_run_command_using_ssh and only set is_push_agent_installed = False when the
returned status indicates success (e.g., exit code == 0), mirroring the install
logic that checks for zero status; if _run_command_using_ssh can return rich
result objects, inspect the appropriate field (exit_code/returncode) before
flipping the flag.
- Around line 486-532: The generated bash script embeds unescaped server fields
and builds JSON unsafely, computes CPU from a single /proc/stat sample, and
calls curl without timeouts; fix the script builder in the method that assigns
script (the multi-line f-string) by: 1) sanitize/escape the comment and use
shell-safe quoted assignments for URL, TOKEN, REF and numeric INTERVAL (avoid
raw interpolation of self.name/self.reference/self.monitor_token); 2)
JSON-encode or safely escape payload values instead of concatenating raw strings
(ensure PAYLOAD is built with properly quoted/escaped keys and values); 3)
replace the single-sample CPU_USAGE code with a two-sample /proc/stat
measurement separated by sleep to compute real interval CPU percent (update
CPU_USAGE and related logic); and 4) add curl timeouts (connect and total) and
fail-safes when parsing NEXT from RESPONSE (make NEXT parsing robust and
bound-checked before assigning INTERVAL). Ensure you update the PAYLOAD,
CPU_USAGE, RESPONSE (curl) and NEXT handling code paths accordingly.
- Around line 296-301: The translation calls currently interpolate variables
outside of _(), break gettext extraction, and use positional %; update each
message_post translation to use named placeholders inside _() and pass a
mapping, e.g. replace body=_("Monitoring Pull Error: %s") % str(e) with
body=_("Monitoring Pull Error: %(error)s") % {'error': str(e)}; do the same for
the other occurrences reported (around the self.message_post calls at the other
ranges) and ensure _logger.error calls use proper logging formatting (e.g.
_logger.error("Monitoring Pull Exception for server %s: %s", self.name, e) or
keep the message text separate from translation) so no external interpolation is
done on _("...") strings.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: cd82bccc-a32a-45b0-b370-5f571663b429
📒 Files selected for processing (2)
cetmix_tower_server_monitor/controllers/main.pycetmix_tower_server_monitor/models/cx_tower_server.py
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
cetmix_tower_server_monitor/models/cx_tower_server.py (1)
319-331:⚠️ Potential issue | 🟡 MinorTranslation uses
_("…") % {…}— must pass variables directly to_().Variables must be passed as keyword arguments directly to the
_()function, not via%formatting after it. This ensures proper runtime translation and pluralization support in Odoo.Proposed fix
raise ValidationError( _( "DEBUG MONITOR OUTPUT\n\n" "Status: %(status)s\n\n" "Response:\n%(response)s\n\n" - "Error:\n%(error)s" - ) - % { - "status": res.get("status"), - "response": res.get("response"), - "error": res.get("error"), - } + "Error:\n%(error)s", + status=res.get("status"), + response=res.get("response"), + error=res.get("error"), + ) )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cetmix_tower_server_monitor/models/cx_tower_server.py` around lines 319 - 331, The raise of ValidationError constructs a translatable string using _("...") % {...} which breaks Odoo translation handling; change it to pass the variables as keyword arguments to _() directly (e.g. use _("DEBUG MONITOR OUTPUT\n\nStatus: %(status)s\n\nResponse:\n%(response)s\n\nError:\n%(error)s", status=res.get("status"), response=res.get("response"), error=res.get("error")) ) and then pass that result into ValidationError — update the raise block that references ValidationError, _(), and the res dict accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cetmix_tower_server_monitor/controllers/main.py`:
- Around line 35-37: The file currently imports json and Response inside the
request handler; move these imports to the module top alongside the existing
odoo.http and request imports so they are evaluated once on import rather than
per-request. Update the module-level imports to include "import json" and "from
odoo.http import Response" and remove the in-function imports from the handler
(look for the handler function that currently imports json/Response). Ensure no
name conflicts with existing symbols (http, request) when hoisting the imports.
In `@cetmix_tower_server_monitor/models/cx_tower_server.py`:
- Around line 487-549: The PAYLOAD assembly in the generated bash script (see
PAYLOAD, REF, TOKEN) embeds $REF directly into a JSON string which lets special
characters in self.reference break or inject the HTTP body; instead either
validate/sanitize self.reference to a strict safe charset before writing the
script or (preferred) generate the JSON using a proper JSON-quoting tool in the
script: export REF and TOKEN as before and replace the manual PAYLOAD
concatenation with a jq invocation (e.g. jq -n --arg ref "$REF" --arg token
"$TOKEN" --argjson metrics "{...}"
'{server_ref:$ref,token:$token,metrics:$metrics}') so the reference is safely
quoted, and ensure PAYLOAD is what that tool outputs for the curl POST.
- Around line 473-478: The chatter message currently uses _("…") % (…) which
breaks i18n guidelines; change the msg construction to use a named placeholder
inside _() and pass the substitution as a mapping. Concretely, replace msg =
_("Monitoring Alerts triggered: %s") % (", ".join([a["resource"] for a in
alerts])) with something like msg = _("Monitoring Alerts triggered:
%(resources)s") % {"resources": ", ".join([a["resource"] for a in alerts])},
keeping the surrounding calls to
self.env["cx.tower.server.alert"].create(alerts) and
self.message_post(body=msg).
- Around line 402-411: The else branch checking res_cpu and the fallback
cpu_percent=0.0 is dead code because earlier in the same function (in
cx_tower_server.py) the function returns when res_ram/res_disk/res_cpu is None;
remove the redundant if/else around res_cpu and directly parse idle_str and set
metrics_vals["cpu_percent"] = 100.0 - float(res_cpu.group(1).replace(",", "."));
likewise remove the unnecessary ternary fallbacks for res_ram and res_disk
(e.g., the "if res_ram else 0" / "if res_disk else 0" expressions) and use the
guaranteed match values directly to compute metrics_vals in the same
function/method.
In `@cetmix_tower_server_monitor/static/src/js/dashboard.esm.js`:
- Around line 112-117: The shallow Object.assign(this.state, result.data) can
clobber nested defaults (e.g., this.state.current.cpu_cores set in setup()), so
change the merge to explicitly merge the current object with safe defaults
before assigning: create a safeCurrent default object (cpu, ram, disk, ram_used,
ram_total, disk_used, disk_total, cpu_cores) merged with result.data.current (if
present), then Object.assign(this.state, result.data, { current: safeCurrent })
so nested fields are preserved and NaNs avoided; update the code around
Object.assign(this.state, result.data) and ensure setup() defaults remain
compatible.
---
Duplicate comments:
In `@cetmix_tower_server_monitor/models/cx_tower_server.py`:
- Around line 319-331: The raise of ValidationError constructs a translatable
string using _("...") % {...} which breaks Odoo translation handling; change it
to pass the variables as keyword arguments to _() directly (e.g. use _("DEBUG
MONITOR OUTPUT\n\nStatus:
%(status)s\n\nResponse:\n%(response)s\n\nError:\n%(error)s",
status=res.get("status"), response=res.get("response"), error=res.get("error"))
) and then pass that result into ValidationError — update the raise block that
references ValidationError, _(), and the res dict accordingly.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f4c83c9d-ef26-47aa-b49e-331d4f70f103
📒 Files selected for processing (6)
cetmix_tower_server_monitor/.pylintrccetmix_tower_server_monitor/controllers/main.pycetmix_tower_server_monitor/models/cx_tower_server.pycetmix_tower_server_monitor/static/src/css/dashboard.csscetmix_tower_server_monitor/static/src/js/dashboard.esm.jscetmix_tower_server_monitor/static/src/xml/dashboard.xml
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (8)
cetmix_tower_server_monitor/models/cx_tower_server.py (4)
302-302:⚠️ Potential issue | 🟡 MinorTranslation call still uses
_("…") %operator.- self.message_post(body=_("Monitoring Pull Error: %s") % str(e)) + self.message_post( + body=_("Monitoring Pull Error: %(err)s", err=str(e)) + )Based on learnings: "When using Odoo translation calls, always pass variables as keyword arguments directly to the _() function, e.g. _('Message %(var)s', var=value). Do not apply % string formatting to the translation string outside the _() call."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cetmix_tower_server_monitor/models/cx_tower_server.py` at line 302, Replace the external %-formatted translation call in the self.message_post call by passing the variable into the translation function as a keyword argument: change the body argument from _("Monitoring Pull Error: %s") % str(e) to _("Monitoring Pull Error: %(err)s", err=str(e)) so the translation system receives the variable directly (locate this in the self.message_post call inside the cx_tower_server class/method).
233-233:⚠️ Potential issue | 🟡 MinorCron still compares against
.timestampwhile the push-status compute uses.create_date.
_cron_pull_metricsat line 233 useslast_metrics_id.timestamp(agent wall-clock) while_compute_monitor_push_statusat line 123 useslast_metrics_id.create_date(Odoo wall-clock). For pushed records with clock skew, this produces contradictory behavior (status "active" but cron re-pulls, or vice versa). Usecreate_datein both places.- delta = (now - server.last_metrics_id.timestamp).total_seconds() + delta = (now - server.last_metrics_id.create_date).total_seconds()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cetmix_tower_server_monitor/models/cx_tower_server.py` at line 233, The cron in _cron_pull_metrics currently compares now against last_metrics_id.timestamp (agent wall-clock) causing inconsistencies with _compute_monitor_push_status which uses last_metrics_id.create_date (Odoo wall-clock); update _cron_pull_metrics to use last_metrics_id.create_date instead of last_metrics_id.timestamp so both places use the Odoo server time for age calculations (refer to the last_metrics_id field and the _cron_pull_metrics and _compute_monitor_push_status functions when making the change).
319-331:⚠️ Potential issue | 🟡 Minor
action_monitor_debugstill interpolates with_(...) % {...}after the previous "fix".The prior critical comment asked for kwargs passed directly into
_(); the commit only swappedmodels.ValidationErrorforValidationErrorbut kept the outer%operator. This breaks translation extraction for the placeholders.♻️ Proposed fix
raise ValidationError( _( "DEBUG MONITOR OUTPUT\n\n" "Status: %(status)s\n\n" "Response:\n%(response)s\n\n" - "Error:\n%(error)s" - ) - % { - "status": res.get("status"), - "response": res.get("response"), - "error": res.get("error"), - } + "Error:\n%(error)s", + status=res.get("status"), + response=res.get("response"), + error=res.get("error"), + ) )Based on learnings: "never use the % operator after _() with variables; instead pass variables as keyword arguments: _('text %(var)s', var=value)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cetmix_tower_server_monitor/models/cx_tower_server.py` around lines 319 - 331, The ValidationError raised in action_monitor_debug currently builds the translatable string with _("...") followed by a % {...} interpolation which prevents proper extraction of placeholders; change it to call _() with the format string and pass the values as keyword arguments (e.g., _("DEBUG MONITOR OUTPUT ... Status: %(status)s ...", status=..., response=..., error=...)) so the placeholders are inside the translation call; update the raise ValidationError(...) expression to pass res.get("status"), res.get("response"), and res.get("error") as keyword args to _() rather than using the % operator.
237-241: 🧹 Nitpick | 🔵 TrivialDowngrade routine
_logger.warningmessages in the monitoring hot path.Lines 237, 275, 283, 336, 340, and 406 log per-tick events ("Cron Pull: Found…", "Executing monitor command…", "Parsing monitor output…", "Created metrics…") at WARNING. Running with N servers every minute this saturates the logs and hides real anomalies. Use
_logger.info(ordebugfor the per-tick parsing trace) and reserve WARNING for parse failures and non-zero SSH status.Also applies to: 275-285, 336-340, 406-412
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cetmix_tower_server_monitor/models/cx_tower_server.py` around lines 237 - 241, The per-tick monitoring log calls using _logger.warning (e.g., the "Cron Pull: Found %s servers...", "Executing monitor command…", "Parsing monitor output…", and "Created metrics…" messages in cx_tower_server.py) should be downgraded to reduce noise: change the _logger.warning calls to _logger.info for routine per-tick status messages and to _logger.debug for verbose parsing traces (the "Parsing monitor output…" line), leaving _logger.warning only for actual parse failures or non-zero SSH status/error conditions (errors produced in the monitor command/SSH handling code paths such as the non-zero exit/status checks). Locate and update the specific _logger.warning invocations referenced in the review (around the calls that build messages for len(servers), executing monitor command, parsing output, and created metrics) accordingly.cetmix_tower_server_monitor/controllers/main.py (2)
90-96:⚠️ Potential issue | 🟠 MajorToken comparison is not constant-time on a public endpoint.
server.monitor_token != tokenshort-circuits on the first differing byte, which leaks token length/prefix timing information to an unauthenticated caller. Usehmac.compare_digestwith string coercion.🔒️ Proposed fix
+import hmac import json import logging @@ - if server.monitor_token != token: + if not token or not hmac.compare_digest( + str(server.monitor_token or ""), str(token) + ): _logger.warning("Invalid monitor token for server %s", server_ref) return Response( json.dumps({"status": "error", "message": "Invalid token"}), content_type="application/json", status=403, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cetmix_tower_server_monitor/controllers/main.py` around lines 90 - 96, Replace the direct inequality check between server.monitor_token and token with a constant-time comparison using hmac.compare_digest; specifically, in the handler where server.monitor_token and token are compared (the block that logs "Invalid monitor token for server %s" and returns the 403 Response), coerce both values to the same string/bytes type and use hmac.compare_digest(server.monitor_token, token) (or compare_digest(str(server.monitor_token), str(token))) and invert its result to trigger the warning/403 when they do not match.
140-144:⚠️ Potential issue | 🟡 MinorDon't leak raw exception text to the client, and don't silently swallow errors.
Returning
str(e)exposes internal ORM/DB details to any authenticated user and the bareexcept Exceptiondrops the traceback from server logs, making real issues invisible. Log the exception and return a generic message.🛡️ Proposed fix
try: data = server.get_dashboard_data() return {"status": "success", "data": data} - except Exception as e: - return {"status": "error", "message": str(e)} + except Exception: + _logger.exception( + "Dashboard fetch failed for server %s", server_id + ) + return {"status": "error", "message": "Internal error"}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cetmix_tower_server_monitor/controllers/main.py` around lines 140 - 144, The except block around server.get_dashboard_data() leaks raw exception text and swallows tracebacks; replace it so you log the full exception/traceback (use logger.exception or logging.getLogger(__name__).exception) and return a generic error message to the client (e.g., {"status":"error","message":"Internal server error"}). Update the except in the controller handling get_dashboard_data to call logger.exception(...) with context and avoid returning str(e); do not suppress logging so issues remain visible in server logs.cetmix_tower_server_monitor/static/src/js/dashboard.esm.js (2)
67-91:⚠️ Potential issue | 🟡 MinorDebug
console.logstill present on the hot path; zero-fallback still hammers the server.Lines 75–77 and 84 keep logging every tick, and
this.state.monitor_interval || 1on line 69 still silently falls back to 1 second in push mode (or 1 minute in pull mode) whenever the backend returns0/null/undefined. Remove the debug logs and clamp the interval to a sane minimum per mode (e.g. ≥5 s push, ≥1 min pull).♻️ Proposed fix
startTimer() { this.stopTimer(); - let intervalMs = this.state.monitor_interval || 1; - if (this.state.monitoring_mode === "push") { - intervalMs *= 1000; - } else { - intervalMs = intervalMs * 60 * 1000; - } - console.log( - `Starting dashboard timer with interval: ${intervalMs}ms (${this.state.monitoring_mode} mode)` - ); + const raw = Number(this.state.monitor_interval) || 0; + const intervalMs = + this.state.monitoring_mode === "push" + ? Math.max(raw, 5) * 1000 + : Math.max(raw, 1) * 60 * 1000; this.timer = setInterval(async () => { - // If in Pull mode and tab is visible, we can trigger a pull to get "Live" feel if ( this.state.monitoring_mode === "pull" && document.visibilityState === "visible" ) { - console.log("Auto-Pulling metrics for Live mode..."); - // Silent refresh await this.refreshData(true); } else { await this.fetchData(); } }, intervalMs); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cetmix_tower_server_monitor/static/src/js/dashboard.esm.js` around lines 67 - 91, In startTimer(), remove the debug console.log calls (the startup log and the "Auto-Pulling metrics" log) and stop silently falling back to 1 by coercing and clamping the interval: read the raw value from this.state.monitor_interval (e.g. Number(...) or parseInt), treat 0/null/undefined as missing, then enforce minimums per mode (push: at least 5000 ms; pull: at least 60000 ms) before computing intervalMs and passing it to setInterval; keep the existing branching (this.state.monitoring_mode === "push" vs "pull") and the calls to this.refreshData(true) / this.fetchData() but do not log on every tick.
100-111:⚠️ Potential issue | 🟡 MinorDebug
console.logcalls infetchDatawere re-introduced.Lines 102 and 111 log
this.props.recordand the entire RPCresulton every polling tick. These were flagged and claimed fixed previously but are present again. They flood the console and can expose dashboard payloads/IDs. Keep onlyconsole.errorin the catch.♻️ Proposed fix
async fetchData() { try { - console.log("Fetching dashboard data for record:", this.props.record); const serverId = this.props.record.resId || this.props.record.data.id; if (!serverId) { console.warn("No server ID found in record props"); return; } const result = await rpc( `/cetmix_tower/server/${serverId}/metrics/dashboard` ); - console.log("Dashboard RPC result:", result); if (result.status === "success" && result.data) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cetmix_tower_server_monitor/static/src/js/dashboard.esm.js` around lines 100 - 111, The fetchData method reintroduces verbose console.log calls—remove the two console.log statements that print this.props.record and the RPC result inside fetchData (the logs around the serverId computation and after awaiting rpc(`/cetmix_tower/server/${serverId}/metrics/dashboard`)); leave only the existing console.error in the catch block (or, if you need development-only diagnostics, replace with a guarded console.debug behind a NODE_ENV/development check) so that fetchData no longer floods logs or exposes payloads.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cetmix_tower_server_monitor/models/cx_tower_server.py`:
- Around line 468-472: Replace positional translation placeholders with
named-kwargs in the cx_tower_server translations: change the _("Monitoring
Alerts triggered: %s", ", ".join(...)) call used to build msg (and the
UserError(_("...: %s", res.get("error"))) raises found in the same file) to use
named placeholders like _("Monitoring Alerts triggered: %(alerts)s", alerts=",
".join(...)) and _("...: %(error)s", error=res.get("error")); update the three
affected sites (the msg formation and the raises at the other occurrences) to
pass variables as keyword arguments to _() so translations survive reordering.
---
Duplicate comments:
In `@cetmix_tower_server_monitor/controllers/main.py`:
- Around line 90-96: Replace the direct inequality check between
server.monitor_token and token with a constant-time comparison using
hmac.compare_digest; specifically, in the handler where server.monitor_token and
token are compared (the block that logs "Invalid monitor token for server %s"
and returns the 403 Response), coerce both values to the same string/bytes type
and use hmac.compare_digest(server.monitor_token, token) (or
compare_digest(str(server.monitor_token), str(token))) and invert its result to
trigger the warning/403 when they do not match.
- Around line 140-144: The except block around server.get_dashboard_data() leaks
raw exception text and swallows tracebacks; replace it so you log the full
exception/traceback (use logger.exception or
logging.getLogger(__name__).exception) and return a generic error message to the
client (e.g., {"status":"error","message":"Internal server error"}). Update the
except in the controller handling get_dashboard_data to call
logger.exception(...) with context and avoid returning str(e); do not suppress
logging so issues remain visible in server logs.
In `@cetmix_tower_server_monitor/models/cx_tower_server.py`:
- Line 302: Replace the external %-formatted translation call in the
self.message_post call by passing the variable into the translation function as
a keyword argument: change the body argument from _("Monitoring Pull Error: %s")
% str(e) to _("Monitoring Pull Error: %(err)s", err=str(e)) so the translation
system receives the variable directly (locate this in the self.message_post call
inside the cx_tower_server class/method).
- Line 233: The cron in _cron_pull_metrics currently compares now against
last_metrics_id.timestamp (agent wall-clock) causing inconsistencies with
_compute_monitor_push_status which uses last_metrics_id.create_date (Odoo
wall-clock); update _cron_pull_metrics to use last_metrics_id.create_date
instead of last_metrics_id.timestamp so both places use the Odoo server time for
age calculations (refer to the last_metrics_id field and the _cron_pull_metrics
and _compute_monitor_push_status functions when making the change).
- Around line 319-331: The ValidationError raised in action_monitor_debug
currently builds the translatable string with _("...") followed by a % {...}
interpolation which prevents proper extraction of placeholders; change it to
call _() with the format string and pass the values as keyword arguments (e.g.,
_("DEBUG MONITOR OUTPUT ... Status: %(status)s ...", status=..., response=...,
error=...)) so the placeholders are inside the translation call; update the
raise ValidationError(...) expression to pass res.get("status"),
res.get("response"), and res.get("error") as keyword args to _() rather than
using the % operator.
- Around line 237-241: The per-tick monitoring log calls using _logger.warning
(e.g., the "Cron Pull: Found %s servers...", "Executing monitor command…",
"Parsing monitor output…", and "Created metrics…" messages in
cx_tower_server.py) should be downgraded to reduce noise: change the
_logger.warning calls to _logger.info for routine per-tick status messages and
to _logger.debug for verbose parsing traces (the "Parsing monitor output…"
line), leaving _logger.warning only for actual parse failures or non-zero SSH
status/error conditions (errors produced in the monitor command/SSH handling
code paths such as the non-zero exit/status checks). Locate and update the
specific _logger.warning invocations referenced in the review (around the calls
that build messages for len(servers), executing monitor command, parsing output,
and created metrics) accordingly.
In `@cetmix_tower_server_monitor/static/src/js/dashboard.esm.js`:
- Around line 67-91: In startTimer(), remove the debug console.log calls (the
startup log and the "Auto-Pulling metrics" log) and stop silently falling back
to 1 by coercing and clamping the interval: read the raw value from
this.state.monitor_interval (e.g. Number(...) or parseInt), treat
0/null/undefined as missing, then enforce minimums per mode (push: at least 5000
ms; pull: at least 60000 ms) before computing intervalMs and passing it to
setInterval; keep the existing branching (this.state.monitoring_mode === "push"
vs "pull") and the calls to this.refreshData(true) / this.fetchData() but do not
log on every tick.
- Around line 100-111: The fetchData method reintroduces verbose console.log
calls—remove the two console.log statements that print this.props.record and the
RPC result inside fetchData (the logs around the serverId computation and after
awaiting rpc(`/cetmix_tower/server/${serverId}/metrics/dashboard`)); leave only
the existing console.error in the catch block (or, if you need development-only
diagnostics, replace with a guarded console.debug behind a NODE_ENV/development
check) so that fetchData no longer floods logs or exposes payloads.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 593988c6-e95a-405e-96c5-e963cddd2ff3
📒 Files selected for processing (3)
cetmix_tower_server_monitor/controllers/main.pycetmix_tower_server_monitor/models/cx_tower_server.pycetmix_tower_server_monitor/static/src/js/dashboard.esm.js
Comment on lines
+468
to
+472
| msg = _( | ||
| "Monitoring Alerts triggered: %s", | ||
| ", ".join([a["resource"] for a in alerts]), | ||
| ) | ||
| self.message_post(body=msg) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefer named-kwargs _() calls over positional %s placeholders.
_("…: %s", value) works but relies on positional args, which is discouraged by the repo's translation convention. Use named placeholders for consistency with the rest of the codebase and to survive reorderings during translation.
♻️ Proposed fix (lines 468–471 example)
- msg = _(
- "Monitoring Alerts triggered: %s",
- ", ".join([a["resource"] for a in alerts]),
- )
+ msg = _(
+ "Monitoring Alerts triggered: %(resources)s",
+ resources=", ".join(a["resource"] for a in alerts),
+ )Apply the same pattern to the UserError(_("…: %s", res.get("error"))) raises at lines 600, 627, and 646.
Based on learnings: "In Python files using Odoo translations, never use the % operator after _() with variables; instead pass variables as keyword arguments: _('text %(var)s', var=value). Applies broadly to Python source files in the repo."
Also applies to: 600-600, 627-627, 646-646
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cetmix_tower_server_monitor/models/cx_tower_server.py` around lines 468 -
472, Replace positional translation placeholders with named-kwargs in the
cx_tower_server translations: change the _("Monitoring Alerts triggered: %s", ",
".join(...)) call used to build msg (and the UserError(_("...: %s",
res.get("error"))) raises found in the same file) to use named placeholders like
_("Monitoring Alerts triggered: %(alerts)s", alerts=", ".join(...)) and _("...:
%(error)s", error=res.get("error")); update the three affected sites (the msg
formation and the raises at the other occurrences) to pass variables as keyword
arguments to _() so translations survive reordering.
tendil
reviewed
Apr 24, 2026
Collaborator
tendil
left a comment
tendil
left a comment
There was a problem hiding this comment.
Thank you for your contributions and interesting ideas!
- We need to create a readme/... files;
- All methods must have type definitions and docstrings;
- We need to write tests to cover the new logic;
- Please implement the changes suggested in the comments, refactor the code yourself, and fix any obvious issues.
| @@ -0,0 +1,82 @@ | |||
| forbidden files......................................(no files to check)Skipped | |||
| en.po files cannot exist.............................(no files to check)Skipped | |||
Collaborator
There was a problem hiding this comment.
we don't need this file; please delete it
| *.py[cod] | ||
| /.venv | ||
| /.pytest_cache | ||
| /.ruff_cache |
Collaborator
There was a problem hiding this comment.
this change needs to be justified if there is a valid reason for it
| @@ -0,0 +1,20 @@ | |||
| # Configuration for known file extensions | |||
Collaborator
There was a problem hiding this comment.
you don't need this file; everything is already configured at the repository level
| @@ -0,0 +1,114 @@ | |||
|
|
|||
|
|
|||
| [MASTER] | |||
Collaborator
There was a problem hiding this comment.
you don't need this file; everything is already configured at the repository level
| @@ -0,0 +1,98 @@ | |||
|
|
|||
| [MASTER] | |||
Collaborator
There was a problem hiding this comment.
you don't need this file; everything is already configured at the repository level
|
|
||
| server_id = fields.Many2one( | ||
| comodel_name="cx.tower.server", | ||
| string="Server", |
Collaborator
There was a problem hiding this comment.
you don't need a string, because one will be generated using the variable name
| else: | ||
| rec.monitor_push_status = "active" | ||
|
|
||
| @api.depends("last_metrics_id") |
Collaborator
There was a problem hiding this comment.
please declare the actual dotted dependencies used by this stored compute
ram_usage_pct / disk_usage_pct are computed from fields on last_metrics_id,
but the decorator currently depends only on "last_metrics_id"
for a stored computed field, this means updates on the linked metrics record itself
may not trigger recomputation unless the m2o link changes
| "monitor_interval_push", | ||
| ) | ||
| def _compute_monitor_push_status(self): | ||
| now = datetime.now() |
Collaborator
There was a problem hiding this comment.
please avoid datetime.now() here and use odoo datetime helpers instead
use fields.Datetime.now() (or another odoo-aware helper) for consistency
| ) | ||
| self.message_post(body=_("Monitoring Pull Error: %s") % str(e)) | ||
|
|
||
| def action_monitor_debug(self): |
Collaborator
There was a problem hiding this comment.
These public action methods need an explicit permission check
any public method can be executed through rpc with chosen
parameters, and acl are only automatically enforced on crud operations
please add an explicit group/role check inside the method before executing the action
| script_b64 = base64.b64encode(script.encode()).decode() | ||
| cmd_write = ( | ||
| f"echo '{script_b64}' | base64 -d | " | ||
| "sudo tee /usr/local/bin/cx_monitor.sh > /dev/null && " |
Collaborator
There was a problem hiding this comment.
right now the authentication secret is written in plain text to
/usr/local/bin/cx_monitor.sh
that makes the token part of the deployed executable
artifact on the target host
please move the secret to a dedicated root-readable config/environment file with
restricted permissions instead of baking it into the script body
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…ic push agent
Summary by CodeRabbit
New Features
Documentation
Chores