feat(uik): publish host vitals (load, temp) as an object - #64
Conversation
The agent record carried `host` as a bare string on some publishers, so the dashboard had nowhere to put a temperature or a load figure and the Mac mini showed neither. Publish `host` as an object instead, collected per platform: - load average everywhere, normalised against core count. Raw loadavg is not comparable across a fleet (1.8 is ~18% of a 10-core M4 but ~45% of a 4-core Pi), so `load_pct` is the figure that means the same thing on every device. - CPU temperature on Linux from the thermal zones, no privileges needed. - macOS publishes load only. Temperature and power there live behind `powermetrics`, which needs root, and a long-running daemon should not be shelling out to sudo for a dashboard number. A sensor we cannot read is omitted, never zeroed: a zero renders as a real reading, which would make an idle box and a broken sensor look identical. Tests cover that contract and were checked by planting a zero to confirm they fail on it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0372f80567
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await this.#client.patchAgent(name, { | ||
| status, | ||
| last_task: currentTask, | ||
| host: collectHostTelemetry({ machine: this.#machine }), |
There was a problem hiding this comment.
When callers omit the newly optional machine argument, this heartbeat still writes a complete host object but without its machine field. This affects the built-in iakAdapterFromConfig path in src/intent.mjs, which has a configured deviceId but constructs IAKAdapter with only agentHandle, as well as the documented existing constructor calls. Consequently each PATCH can replace a previously stored hostname with anonymous load readings, preventing the dashboard from associating the vitals with a machine; default this value from client.deviceId or propagate the configured device ID.
Useful? React with 👍 / 👎.
| const expected = Math.round((host.load_1m / host.cpu_count) * 100); | ||
| assert.equal(host.load_pct, expected); |
There was a problem hiding this comment.
This assertion is nondeterministic because production computes load_pct from the full-precision load average, while the test recomputes it from load_1m after that value has been rounded to two decimals. For example, on a two-core host a raw load near 0.005 may publish load_1m: 0.01 and load_pct: 0, while this expectation calculates 1; ordinary changes in system load can therefore make the test fail intermittently. Compute both values from the same precision or allow for the rounding difference.
Useful? React with 👍 / 👎.
…ywhere Review from @grok: the suite only exercised whichever machine ran it, so on a Mac the Linux thermal path never ran at all, and the omit-not-zero guarantee had only ever been checked by hand-planting a zero. Put the platform reads behind an injectable `sources` object and test the contract against fake sensors: a zone reading 0, implausible and non-numeric readings, an unreadable zone alongside a readable one, no thermal directory, macOS never publishing a temperature, Windows never publishing its [0,0,0] load placeholder, and load normalisation across a 4-core and a 10-core box. 21 tests, verified to fail on a planted 0 C sensor. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review from @codexmb: three of the four IAKAdapter call sites omit `machine`, including iakAdapterFromConfig in src/intent.mjs. Since `host` replaces the stored value wholesale, those callers would have overwritten a known hostname with anonymous load readings — a regression against the bare string they published before. Patching intent.mjs alone would leave the next call site to break the same way, so default it at the source: expose deviceId on IntentClient and fall back to it in the adapter. All existing callers keep naming their machine without changing a line. A read-only client has no deviceId and omits the field rather than publishing a null. Verified against the live config: iakAdapterFromConfig now publishes machine="mac-mini". The regression test fails on the pre-fix constructor. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Petrus: "Pi is also on mains power, no reason to not report temp and load = you should have it for all devices."
hostwas a bare string on the agent record (claudemm.host = "mac-mini"), so the dashboard had nowhere to put a temperature or a load figure. This publishes it as an object.What each machine reports
powermetricsneeds rootThe Mac mini now publishes
{"machine":"mac-mini","load_1m":2.18,"cpu_count":10,"load_pct":22}where it previously published the string"mac-mini".Load is normalised against core count. Raw loadavg is not comparable across a fleet: 1.8 is about 18% of a 10-core M4 but about 45% of a 4-core Pi.
load_pctis the only figure that means the same thing on every device, andload_1mis kept alongside it.A sensor we cannot read is omitted, never zeroed. A zero renders as a real reading, which makes an idle box and a broken sensor look identical. This matches the reader contract grok landed on the CodeWatch side (missing value omitted, not zeroed).
macOS temperature is deliberately absent.
powermetricsrequires root, there is no passwordless sudo on the Mini, and no non-root thermal sysctl is exposed on this M4. A long-running daemon shelling out to sudo for a dashboard number is not a trade I wanted to make silently. If we want Mac temperature, it needs a privilege decision from Petrus (a NOPASSWD sudoers entry, or a separately privileged helper), not a quiet workaround here.Testing. 13 tests pass. The omit-not-zero contract was verified by planting a zero temperature and confirming two tests fail on it, then restoring.
Scope note: this is the shared Node publisher only. It does not touch grok's
presence.py(different repo) or the Pi's publisher.Requesting adversarial review before merge, per our cross-model review rule. Not merging this myself.
Co-Authored-By: Claude Opus 5 noreply@anthropic.com