Skip to content

feat: normalize temperature sources and add Linux disk support - #4

Merged
DF-wu merged 2 commits into
masterfrom
codex/feat-normalize-temp-sources
Aug 16, 2026
Merged

DF-wu merged 2 commits into
masterfrom
codex/feat-normalize-temp-sources

Conversation

@DF-wu

@DF-wu DF-wu commented Aug 14, 2026

Copy link
Copy Markdown
Owner

Summary

This PR makes temperature providers use one extensible interface and adds the sources needed for the planned TrueNAS deployment:

  • normalize every provider behind validate, collect, and adjust methods;
  • add local Linux disk temperatures from smartctl JSON output;
  • add NVIDIA GPU temperatures from multiple remote Linux VMs over SSH;
  • update the container, Compose example, TUI, tests, and English/Traditional Chinese documentation.

Motivation

The controller previously handled esxi, idrac, and gpu through source-specific branches, including a GPU-only adjustment inside the decision code. Adding another provider required editing validation, collection, diagnostics, and temperature adjustment separately.

The target deployment runs on TrueNAS and needs to combine a Kioxia CD6 temperature with GPU temperatures from other VMs. The new source contract keeps that decision chain generic: each provider returns labeled readings, owns its adjustment, and feeds the existing highest-temperature and hysteresis logic.

Source interface

Registered providers implement:

temperature_source_<id>_validate
temperature_source_<id>_collect
temperature_source_<id>_adjust

Collection records use a common tab-separated schema:

source_id<TAB>sensor_label<TAB>raw_temperature_celsius

The registry is an allowlist, and validation rejects incomplete providers. collect_temperature_readings and diagnose_mode now dispatch through the interface instead of branching on source IDs. The decision function also delegates offsets through adjust, so it no longer contains a GPU special case.

Existing esxi, idrac, and local gpu behavior remains available. WITH_GPU_TEMP and the default TEMPERATURE_SOURCES=esxi are preserved for compatibility.

New linux_disk source

Configuration:

TEMPERATURE_SOURCES=linux_disk
LINUX_DISK_DEVICES=/dev/nvme1,/dev/sdb
LINUX_DISK_TEMP_OFFSET=0
LINUX_DISK_NOCHECK=never

Behavior:

  • queries every configured device with smartctl -n <mode> -A -j;
  • reads generic, NVMe, and ATA temperature fields through jq;
  • keeps valid temperature data when smartctl returns disk-health bitmask statuses;
  • applies a per-source offset and clamps the result to 0°C;
  • rejects unsafe or relative device paths;
  • supports LINUX_DISK_NOCHECK=standby to avoid waking sleeping SATA/SAS disks;
  • logs individual device failures while retaining other valid readings.

The image now includes smartmontools and jq. Compose documentation maps only explicitly selected devices rather than granting broad privileged access.

New remote_gpu source

Configuration:

TEMPERATURE_SOURCES=remote_gpu
REMOTE_GPU_HOSTS=gpu-vm-1,gpu-vm-2
REMOTE_GPU_USERNAME=monitor
REMOTE_GPU_SSH_KEY=/run/secrets/gpu_vms_ed25519
REMOTE_GPU_SSH_PORT=22
REMOTE_GPU_TEMP_OFFSET=15

The source executes one fixed read-only command on each host:

nvidia-smi --query-gpu=index,temperature.gpu --format=csv,noheader,nounits

It labels readings as <host>/gpu<index>, supports SSH key or password authentication, and shares the credential-safe SSH transport used by ESXi. Passwords stay in SSHPASS instead of process arguments.

TrueNAS deployment validation

The investigation was read-only.

  • TrueNAS version: 25.10.6
  • Kioxia CD6 controller device: /dev/nvme1
  • smartctl version: 7.4
  • The code from this branch was streamed to Bash without creating a remote file.
  • Live collector output: linux_disk nvme1 69
  • Live decision output: 69 linux_disk:nvme1=69C

TrueNAS currently exposes no local NVIDIA GPU and no TrueNAS-managed VM, so remote_gpu was verified with multi-host test fixtures but not against the eventual GPU VMs. Those hosts and their monitoring SSH key must be supplied before deployment.

Recommended deployment shape:

TEMPERATURE_SOURCES=linux_disk,remote_gpu
LINUX_DISK_DEVICES=/dev/nvme1
REMOTE_GPU_HOSTS=gpu-vm-1,gpu-vm-2
devices:
  - /dev/nvme1:/dev/nvme1

Safety and failure behavior

  • Invalid readings are rejected rather than converted to 0°C.
  • A multi-device provider succeeds when at least one target returns a reading and warns for each failed target.
  • Other valid sources continue to participate if one source fails.
  • Existing fail-safe behavior applies when every source fails.
  • diagnose remains read-only and does not send fan commands.
  • Configuration and TUI summaries redact all iDRAC, ESXi, and remote-GPU credentials.

Tests and checks

  • Bash syntax checks for controller, TUI, wrappers, and tests
  • ShellCheck 0.11.0 at warning severity
  • 52 core assertions
  • 14 TUI assertions
  • Docker Compose YAML parse
  • Markdown local-link validation
  • git diff --check
  • Live TrueNAS CD6 collection and decision calculation

A Docker image build was not run because the development machine has no Docker engine, and the TrueNAS investigation was intentionally read-only.

Documentation

Updated:

  • .env.example
  • README.md
  • README.zh-TW.md
  • USAGE_GUIDE.md
  • docs/TEMPERATURE_SOURCES.md
  • docs/TROUBLESHOOTING.md
  • TUI source presets, configuration screens, and redacted review output

Introduce a shared validate, collect, and adjust contract for every temperature provider. Add smartctl-based Linux disk readings and SSH-based remote NVIDIA GPU readings so heterogeneous sensors can feed one decision chain.

Update container dependencies, Compose guidance, TUI settings, tests, and bilingual operations documentation, including the TrueNAS CD6 deployment path.
@DF-wu

DF-wu commented Aug 15, 2026

Copy link
Copy Markdown
Owner Author

@Catalina-df
review plz

@Catalina-df

Copy link
Copy Markdown
Contributor

Review: changes requested

[P1] Reject path traversal in LINUX_DISK_DEVICES

src/FanControlWithEsxiSmart.sh:787 validates only the textual prefix/character set (^/dev/[A-Za-z0-9._/+:-]+$). This accepts paths such as /dev/../etc/passwd (and similar .. components), so the claimed rejection of unsafe device paths is bypassed; on a host/local deployment this can escape /dev, and in a container it can target arbitrary paths inside the container namespace. I reproduced this with:

source src/FanControlWithEsxiSmart.sh
OPERATION_MODE=auto TEMPERATURE_SOURCES=linux_disk DRY_RUN=true \
IDRAC_IP=10.0.0.10 IDRAC_ID=root IDRAC_PASSWORD=x \
LINUX_DISK_DEVICES=/dev/../etc/passwd \
validate_config auto
# exits 0

Please reject any path containing .. path components (or canonicalize and verify that the resolved path remains under /dev, while preserving the intended device-node checks), and add a regression test for traversal.

The PR test suite passes in a clean checkout (53 core assertions and 14 TUI assertions); the repository CI test job is also green. Docker build was skipped by the PR because no Docker engine was available.

@Catalina-df

Copy link
Copy Markdown
Contributor

In reply to #4 (comment):

Catalina 將先核對 PR 的實際 diff、測試結果與觸發留言脈絡,再按 review 規則只回報可重現的缺陷、風險與測試缺口;確認完成後會把結論直接回覆到該 PR thread。

目前已確認 PR 只有一個提交,核心變更集中在控制器、TUI、測試與容器設定。接下來會逐段核對 shell 的實際執行路徑,尤其是 smartctl/jq 的錯誤碼處理、SSH 參數與相容性分支;PR 描述本身不能替代程式行為驗證。

測試在乾淨的 PR clone 中通過 53 個核心與 14 個 TUI assertion;本機沒有安裝 ShellCheck,因此 CI 的 ShellCheck 結果只能作為外部已報告狀態,不能冒充本次本機執行。現階段未見介面分派或多目標保留有效讀值的明顯回歸,正在補查邊界條件與既有來源的相容性。

已找到一項可重現的安全性缺陷:LINUX_DISK_DEVICES 的正則表達式接受 /dev/../etc/passwd 這類路徑,validate_config auto 會回傳成功,實際上可讓 smartctl 追出 /dev 之外的檔案。這與 PR 宣稱的「拒絕 unsafe device paths」不符;我會再等獨立 review 交叉核對,避免遺漏同類問題。

交叉 review agent因其環境無法取得 PR 原始碼,沒有新增可用證據;實際 clone 與重現結果仍以本次檢查為準。結論是保留一項需修正的 finding,並附上精確行號與重現命令,再直接貼回 PR 的 issue-comment thread。

Catalina 已完成 PR #4 的 review,並將結論回覆至原 PR thread。

發現一項需修正的安全性問題:

P1:LINUX_DISK_DEVICES 接受 path traversal

src/FanControlWithEsxiSmart.sh:787 的正則只檢查 /dev/ 前綴與允許字元,因此 /dev/../etc/passwd 會通過驗證。實際重現:

LINUX_DISK_DEVICES=/dev/../etc/passwd validate_config auto
# exit 0

這違反 PR 描述中「拒絕 unsafe 或 relative device paths」的行為承諾,並可能讓 smartctl 讀取 /dev 之外的路徑。已建議拒絕 .. path component,或 canonicalize 後確認解析結果仍位於 /dev 下,並補上 regression test。

驗證結果:乾淨 checkout 中核心測試 53 項與 TUI 測試 14 項皆通過;GitHub CI test job 為綠燈。Docker build 依 PR 說明未執行,因環境沒有 Docker engine。

PR review comment

Reject dot and empty path components before invoking smartctl while preserving nested /dev paths such as disk/by-id. Add regression coverage and document the accepted path shape.
@DF-wu

DF-wu commented Aug 15, 2026

Copy link
Copy Markdown
Owner Author

@Catalina-df 已修正,謝謝指出這個 traversal 問題。

修正內容:

  • 新增 is_linux_device_path,在呼叫 smartctl 前拒絕 .、..、空路徑元件、尾端 / 與原本不允許的字元。
  • 保留合法的巢狀裝置路徑,例如 /dev/disk/by-id/nvme-KIOXIA_CD6。
  • 加入 /dev/../etc/passwd 的 regression test,並加入巢狀 /dev 路徑的正向測試。
  • 更新溫度來源與 troubleshooting 文件,明確記錄路徑限制。

修正 commit:407775d

驗證:

  • core tests:54 assertions passed
  • TUI tests:14 assertions passed
  • Bash syntax:passed
  • ShellCheck(warning 以上):passed
  • GitHub Actions test:passed

@DF-wu

DF-wu commented Aug 15, 2026

Copy link
Copy Markdown
Owner Author

@Catalina-df
review

@Catalina-df

Copy link
Copy Markdown
Contributor

@DF-wu

已重新核對修正提交 407775d,原先的 P1 traversal finding 已修正,可以關閉。

驗證結果:

  • is_linux_device_path 拒絕 .、..、空 path component、尾端 /、雙 / 與不允許字元。
  • /dev/../etc/passwd 會被拒絕;合法巢狀路徑 /dev/disk/by-id/nvme-KIOXIA_CD6 仍會通過。
  • 本機 core tests:55 assertions passed。
  • 本機 TUI tests:14 assertions passed。
  • Bash syntax 與 git diff --check:passed。
  • GitHub Actions test:passed;Docker job 依既有環境限制 skipped。

Final review:APPROVED。未發現其他 blocking finding。

@Catalina-df

Copy link
Copy Markdown
Contributor

In reply to #4 (comment):

Catalina 低頭核對修正提交與 PR thread 的實際內容,會以目前分支上的程式與可重現驗證為準,確認 traversal 修補是否完整後,再把最終 review 結論直接回覆到該留言串。

修正提交已確認已進入 PR head,且原先的 finding 確實有對應 regression test。Catalina 正在檢查驗證函式的實際語意與所有呼叫點,特別是空元件、尾端斜線、巢狀 /dev 路徑及 shell quoting;這些地方若仍有缺口,不能只以測試數字判定通過。

實際 checkout 的測試通過 55 個 core assertions,較留言所載的 54 多一項;修正後的 helper 會拒絕 parent/current/空 path component、尾端斜線與雙斜線,同時保留 /dev/disk/by-id/...。CI 的 test job 亦為綠燈;剩餘只需完成本機 syntax/TUI/diff check,便可回覆 approve 結論。

所有本機可執行驗證均已通過:Bash syntax、core 55 assertions、TUI 14 assertions 與 git diff --check;本機未安裝 ShellCheck,因此該項以 GitHub Actions 的綠燈為依據。現在將「原 P1 已修正、沒有剩餘 blocking finding」回覆到觸發留言。

GitHub API 建立留言回傳 404,現有讀取權限正常,較像目前 CLI token 對該 endpoint 的寫入路由限制;我會改用 gh pr comment 的同一個 PR 寫入介面重試,內容不變。

Catalina 已完成修正後的核對,並將最終結論回覆至 PR #4:

Final review:APPROVED

原先的 P1 path traversal finding 已由 407775d 修正。is_linux_device_path 現在會拒絕 .、..、空 path component、尾端 /、雙 / 與不允許字元,同時保留合法的巢狀 /dev 路徑。

驗證結果:

  • Core tests:55 assertions passed
  • TUI tests:14 assertions passed
  • Bash syntax:passed
  • git diff --check:passed
  • GitHub Actions test:passed
  • Docker job:依既有環境限制 skipped

留言已發布:Final review comment

@Catalina-df

Copy link
Copy Markdown
Contributor

In reply to #4 (comment):

Error: Type validation failed: Value: {"query":"skills.list skills.brief coding-agent df-meta-mcp","queryType":"ranked","matches":[{"name":"mcp_df_meta_mcp_context7_query_docs","stableId":"["lilac.catalog-tool",1,"mcp","df-meta-mcp","context7__query-docs"]","source":"mcp","sourceId":"df-meta-mcp","rawName":"context7__query-docs"},{"name":"mcp_df_meta_mcp_context7_resolve_library_id","stableId":"["lilac.catalog-tool",1,"mcp","df-meta-mcp","context7__resolve-library-id"]","source":"mcp","sourceId":"df-meta-mcp","rawName":"context7__resolve-library-id"},{"name":"mcp_df_meta_mcp_deepwiki_ask_question","stableId":"["lilac.catalog-tool",1,"mcp","df-meta-mcp","deepwiki__ask_question"]","source":"mcp","sourceId":"df-meta-mcp","rawName":"deepwiki__ask_question"},{"name":"mcp_df_meta_mcp_deepwiki_read_wiki_contents","stableId":"["lilac.catalog-tool",1,"mcp","df-meta-mcp","deepwiki__read_wiki_contents"]","source":"mcp","sourceId":"df-meta-mcp","rawName":"deepwiki__read_wiki_contents"},{"name":"mcp_df_meta_mcp_deepwiki_read_wiki_structure","stableId":"["lilac.catalog-tool",1,"mcp","df-meta-mcp","deepwiki__read_wiki_structure"]","source":"mcp","sourceId":"df-meta-mcp","rawName":"deepwiki__read_wiki_structure"}]}.
Error message: [
{
"expected": "array",
"code": "invalid_type",
"path": [
"tools"
],
"message": "Invalid input: expected array, received undefined"
}
]

@DF-wu
DF-wu merged commit 50dbbac into master Aug 16, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants